Re: git: 84bf2bbbecc3 - main - stand: constrain zlib/gzip CFLAGS better

On 7/20/22 11:20 AM, Warner Losh wrote:
> On Wed, Jul 20, 2022 at 12:12 PM Warner Losh <imp@bsdimp.com> wrote:
> 
>>
>>
>> On Wed, Jul 20, 2022 at 12:06 PM Warner Losh <imp@bsdimp.com> wrote:
>>
>>>
>>>
>>> On Wed, Jul 20, 2022 at 11:44 AM Toomas Soome <tsoome@me.com> wrote:
>>>
>>>>
>>>>
>>>>> On 20. Jul 2022, at 20:24, Dmitry Chagin <dchagin@heemeyer.club>
>>>> wrote:
>>>>>
>>>>> On Fri, Jul 08, 2022 at 05:50:05PM +0000, Warner Losh wrote:
>>>>>> The branch main has been updated by imp:
>>>>>>
>>>>>> URL:
>>>> https://cgit.FreeBSD.org/src/commit/?id=84bf2bbbecc369cea6095bed7a738674b27f8d13
>>>>>>
>>>>>> commit 84bf2bbbecc369cea6095bed7a738674b27f8d13
>>>>>> Author:     Warner Losh <imp@FreeBSD.org>
>>>>>> AuthorDate: 2022-07-08 16:29:25 +0000
>>>>>> Commit:     Warner Losh <imp@FreeBSD.org>
>>>>>> CommitDate: 2022-07-08 17:47:37 +0000
>>>>>>
>>>>>>     stand: constrain zlib/gzip CFLAGS better
>>>>>>
>>>>>>     Define ZLIB_CFLAGS and use it only for the sources that are in
>>>> ZLIB or
>>>>>>     that include it.
>>>>>>
>>>>>>     Sponsored by:           Netflix
>>>>>> ---
>>>>>> stand/libsa/Makefile | 13 +++++++------
>>>>>> 1 file changed, 7 insertions(+), 6 deletions(-)
>>>>>>
>>>>>> diff --git a/stand/libsa/Makefile b/stand/libsa/Makefile
>>>>>> index b5d800c26295..09637bd5e9d4 100644
>>>>>> --- a/stand/libsa/Makefile
>>>>>> +++ b/stand/libsa/Makefile
>>>>>> @@ -96,9 +96,11 @@ SRCS+=${i}
>>>>>>
>>>>>> # decompression functionality from zlib
>>>>>> .PATH: ${SRCTOP}/sys/contrib/zlib
>>>>>> -CFLAGS+=-DHAVE_MEMCPY -I${SRCTOP}/sys/contrib/zlib
>>>>>> -SRCS+=      adler32.c crc32.c
>>>>>> -SRCS+=      infback.c inffast.c inflate.c inftrees.c zutil.c
>>>>>> +ZLIB_CFLAGS=-DHAVE_MEMCPY -I${SRCTOP}/sys/contrib/zlib
>>>>>> +.for i in adler32.c crc32.c infback.c inffast.c inflate.c inftrees.c
>>>> zutil.c
>>>>>> +CFLAGS.${i}+=${ZLIB_CFLAGS}
>>>>>> +SRCS+=      ${i}
>>>>>> +.endfor
>>>>>>
>>>>>> # lz4 decompression functionality
>>>>>> .PATH: ${SRCTOP}/sys/cddl/contrib/opensolaris/common/lz4
>>>>>> @@ -168,9 +170,8 @@ SRCS+=   time.c
>>>>>> .PATH: ${SRCTOP}/sys/ufs/ffs
>>>>>> SRCS+=ffs_subr.c ffs_tables.c
>>>>>>
>>>>>> -CFLAGS.dosfs.c+= -I${LDRSRC}
>>>>>> -CFLAGS.tftp.c+= -I${LDRSRC}
>>>>>> -CFLAGS.ufs.c+= -I${LDRSRC}
>>>>> ^^^^^^^^^^^^ is this correct? at least it breaks builds with
>>>>> WITHOUT_LOADER_ZFS and WITHOUT_BOOT probably, see PR/260083
>>>>>
>>>>>
>>>>
>>>> No, it is not correct.
>>>>
>>>
>>> My change is correct, theoretically. However, there's a layering
>>> violation that means they are needed so it was premature.
>>>
>>> I'll fix a bandaide and do it better when I return from vacation.
>>>
>>
>> Doh! I don't have the right keys loaded in my ssh-agent, so I can't push
>> the change because the port forwarding on my router is broken and I can't
>> remotely login :(
>>
>> If someone could commit the change I suggested in
>> https://reviews.freebsd.org/D35860 that would be great!
>>
> 
> The changes were needed, btw, to limit the scope of CFLAGS for the OpenZFS
> blake3.c support. However, this one scope limiting shouldn't break that.

The thing that doesn't make sense to me at all though is that what happens
in practice is that ZFS unconditionally adds -ILDRSRC to the global CFLAGS.

Given that the goal is to limit the proliferation of global options in
CFLAGS, it seems odd to reject changes that add -ILDRSRC to specific files.

My second review for GELI does replace a global CFLAGS change you removed
with instead adding it to specific files which seems to be a step in the
right direction.

Further fixing ZFS to not set any global CFLAGS would seem to be a further
improvement that would expose this current breakage even when ZFS is
enabled.  However, I don't really see why we shouldn't add the needed
include just to specific files for now rather than the hack in defs.mk.

If we later fix libsa to not need headers from the loader (which I agree
with, and might mean that we just need to move some headers or their
contents over to libsa) can remove targeted CFLAGS.foo.c bits for -ILDRSRC
as part of that commit.

-- 
John Baldwin