On Tue, 26 Mar 2024 23:10:44 GMT, Magnus Ihse Bursie <i...@openjdk.org> wrote:
>> This took me a while to decode. The old code will unconditionally override >> `LCMS_CFLAGS` which means whatever value it gets in configure is >> overwritten. That certainly seems like a bug. >> >> Your current patch clears the variable when building with an external >> liblcms. I agree this is equivalent behavior for the external liblcms case. >> If we can assume that `LCMS_CFLAGS` from configure is always empty when >> building liblcms from source, then it's also equivalent when building from >> source. I assume that `LCMS_CFLAGS` is only ever populated with `-I` paths >> pointing to the header files of an external liblcms. >> >> Am I understanding correctly that the fix you are proposing is to stop >> clearing `LCMS_CFLAGS` and just trust that configure sets it with the >> correct values if needed? I suppose doing it in a followup is probably >> better. > > The code, prior to this PR, includes these lines: > > # The fast floor code loses precision. > LCMS_CFLAGS=-DCMS_DONT_USE_FAST_FLOOR -DCMS_NO_HALF_SUPPORT > > > This will overwrite the value of `LCMS_CFLAGS` as given in `spec.gmk` by > configure. It is normally empty, but if you run `configure > --with-lcms=system`, then the configure script will query `pkg-config` about > the value needed for `CFLAGS` for LCMS. On my test system it was still empty, > but there is no guarantee that it will be so. (And if we truly believed that > would be the case, we wouldn't export LCMS_CFLAGS in spec.gmk...) > > Hence, it is a bug to replace this value. I am pretty certain that the > intention here was to add `-DCMS_DONT_USE_FAST_FLOOR -DCMS_NO_HALF_SUPPORT` > to the compilation, in addition to any value LCMS_CFLAGS would have from > configure. > If we can assume that LCMS_CFLAGS from configure is always empty when > building liblcms from source It is. It is only set in configure if we set USE_EXTERNAL_LCMS to true. > I assume that LCMS_CFLAGS is only ever populated with -I paths pointing to > the header files of an external liblcms. Presumably; we don't even set it ourselves, but rely on pkg-config. In theory it could also include e.g. -D values. In practice, it was empty in my test machine. > Am I understanding correctly that the fix you are proposing is to stop > clearing LCMS_CFLAGS and just trust that configure sets it with the correct > values if needed? I suppose doing it in a followup is probably better. Yes, that is correctly understood. I'll do it in a separate followup then. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/18486#discussion_r1540237249
