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

Hey John,

On Wed, Jul 20, 2022 at 4:13 PM John Baldwin <jhb@freebsd.org> wrote:

> 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.
>

Yes. That's wrong. I missed that. I have another round planned to fix that.

I'd rather add one more global kludge, marked as such that I can more easily
remove in the next round once the issues are cleared up.


> 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.
>

Yea, these shouldn't be added to these files, at all. Only due to the
layering
violations are they needed at all.


> 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.
>
Yea.


> 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.
>

I'm fine either way. I can undo more stuff later. I just expressed my
preference
since it would be easy to undo later.


> 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.
>

True, but that's more work later and more work now. Since I'm out of pocket
for a while, go ahead and commit them... I'd have already committed the
kludge-for-now
fix...

Warner