Re: git: 0c3627f44d49 - main - bsdinstall avoid subdir depending on parent
Date: Thu, 07 Dec 2023 01:04:10 UTC
On 7 Dec 2023, at 00:46, Simon J. Gerraty <sjg@juniper.net> wrote:
>
> Jessica Clarke <jrtc27@freebsd.org> wrote:
>> On 21 Apr 2023, at 06:14, Jessica Clarke <jrtc27@FreeBSD.org> wrote:
>
> Sorry, I didn't your mail back in April.
>
> I can move the generation of the header to its own subdir.
> Does usr.sbin/bsdinstall/osname sound reasonable?
> other common options would include gen/ include/ inc/ ?
include sounds ok to me. osname seems a bit too specific.
Thanks,
Jess
>>> On 21 Apr 2023, at 06:01, Simon J. Gerraty <sjg@FreeBSD.org> wrote:
>>>>
>>>> The branch main has been updated by sjg:
>>>>
>>>> URL: https://urldefense.com/v3/__https://cgit.FreeBSD.org/src/commit/?id=0c3627f44d49b460d5b9156145dec9d4a91beb2c__;!!NEt6yMaO-gk!CduM38y2LXW3snhYZYZdtUNWv4VGMwaMwEOIOUMDYGZMRJwin7i46yHWwp5pG_pq3GGTzT-7Czg7AA$
>>>>
>>>> commit 0c3627f44d49b460d5b9156145dec9d4a91beb2c
>>>> Author: Simon J. Gerraty <sjg@FreeBSD.org>
>>>> AuthorDate: 2023-04-21 05:00:40 +0000
>>>> Commit: Simon J. Gerraty <sjg@FreeBSD.org>
>>>> CommitDate: 2023-04-21 05:00:40 +0000
>>>>
>>>> bsdinstall avoid subdir depending on parent
>>>>
>>>> When not doing tree walks, it is bad for sub-dirs to depend on
>>>> parents. Move the generation of opt_osname.h to distextract
>>>> and have others that need that depend on it.
>>>>
>>>> In usr.sbin/bsdinstall use SUBDIR_DEPEND_ so tree walking still works.
>>>>
>>>> Reviewed by: obrien
>>>> Differential Revision: https://urldefense.com/v3/__https://reviews.freebsd.org/D39742__;!!NEt6yMaO-gk!CduM38y2LXW3snhYZYZdtUNWv4VGMwaMwEOIOUMDYGZMRJwin7i46yHWwp5pG_pq3GGTzT_oIs2qhw$
>>>> ---
>>>> usr.sbin/bsdinstall/Makefile | 9 ++-------
>>>> usr.sbin/bsdinstall/distextract/Makefile | 11 ++++++++++-
>>>> usr.sbin/bsdinstall/distfetch/Makefile | 2 +-
>>>> usr.sbin/bsdinstall/partedit/Makefile | 2 +-
>>>> 4 files changed, 14 insertions(+), 10 deletions(-)
>>>>
>>>> diff --git a/usr.sbin/bsdinstall/Makefile b/usr.sbin/bsdinstall/Makefile
>>>> index e71cae726536..aaa006694222 100644
>>>> --- a/usr.sbin/bsdinstall/Makefile
>>>> +++ b/usr.sbin/bsdinstall/Makefile
>>>> @@ -3,19 +3,14 @@
>>>> OSNAME?= FreeBSD
>>>> SUBDIR= distextract distfetch partedit runconsoles scripts
>>>> SUBDIR_PARALLEL=
>>>> +SUBDIR_DEPEND_distfetch = distextract
>>>> +SUBDIR_DEPEND_partedit = distextract
>>>> SCRIPTS= bsdinstall
>>>> MAN= bsdinstall.8
>>>> PACKAGE= bsdinstall
>>>> -GENHDRS= opt_osname.h
>>>> -SRCS+= ${GENHDRS}
>>>> -CLEANFILES+= ${GENHDRS}
>>>>
>>>> SCRIPTS+= startbsdinstall
>>>> SCRIPTSDIR_startbsdinstall= ${LIBEXECDIR}/bsdinstall
>>>>
>>>> -opt_osname.h: .PHONY
>>>> - if ! grep -q "^#define OSNAME \"${OSNAME}\"$"" ${.TARGET}; then \
>>>> - echo "#define OSNAME \"${OSNAME}\"" > ${.TARGET}; \
>>>> - fi
>>>>
>>>> .include <bsd.prog.mk>
>>>> diff --git a/usr.sbin/bsdinstall/distextract/Makefile b/usr.sbin/bsdinstall/distextract/Makefile
>>>> index 6ae9bb65e8fb..0292c01e78f4 100644
>>>> --- a/usr.sbin/bsdinstall/distextract/Makefile
>>>> +++ b/usr.sbin/bsdinstall/distextract/Makefile
>>>> @@ -2,9 +2,18 @@
>>>>
>>>> BINDIR= ${LIBEXECDIR}/bsdinstall
>>>> PROG= distextract
>>>> -CFLAGS+= -I${SRCTOP}/contrib/bsddialog/lib -I${.OBJDIR}/..
>>>> +CFLAGS+= -I${SRCTOP}/contrib/bsddialog/lib -I.
>>>> LIBADD= archive bsddialog m
>>>> +SRCS= distextract.c
>>>>
>>>> MAN=
>>>> +GENHDRS= opt_osname.h
>>>> +SRCS+= ${GENHDRS}
>>>> +CLEANFILES+= ${GENHDRS}
>>>> +
>>>> +opt_osname.h: .PHONY
>>>> + if ! grep -q "^#define OSNAME \"${OSNAME}\"$"" ${.TARGET}; then \
>>>> + echo "#define OSNAME \"${OSNAME}\"" > ${.TARGET}; \
>>>> + fi
>>>>
>>>> .include <bsd.prog.mk>
>>>> diff --git a/usr.sbin/bsdinstall/distfetch/Makefile b/usr.sbin/bsdinstall/distfetch/Makefile
>>>> index 0104df0e3aec..1555719dd15d 100644
>>>> --- a/usr.sbin/bsdinstall/distfetch/Makefile
>>>> +++ b/usr.sbin/bsdinstall/distfetch/Makefile
>>>> @@ -2,7 +2,7 @@
>>>>
>>>> BINDIR= ${LIBEXECDIR}/bsdinstall
>>>> PROG= distfetch
>>>> -CFLAGS+= -I${SRCTOP}/contrib/bsddialog/lib -I${.OBJDIR}/..
>>>> +CFLAGS+= -I${SRCTOP}/contrib/bsddialog/lib -I${.OBJDIR}/../distextract
>>>> LIBADD= fetch bsddialog
>>>>
>>>> MAN=
>>>> diff --git a/usr.sbin/bsdinstall/partedit/Makefile b/usr.sbin/bsdinstall/partedit/Makefile
>>>> index 96c4ddb53961..df17028eab2a 100644
>>>> --- a/usr.sbin/bsdinstall/partedit/Makefile
>>>> +++ b/usr.sbin/bsdinstall/partedit/Makefile
>>>> @@ -5,7 +5,7 @@ PROG= partedit
>>>> LINKS= ${BINDIR}/partedit ${BINDIR}/autopart \
>>>> ${BINDIR}/partedit ${BINDIR}/scriptedpart
>>>> SYMLINKS= ../libexec/bsdinstall/partedit /usr/sbin/sade
>>>> -CFLAGS+= -I${SRCTOP}/contrib/bsddialog/lib -I${.OBJDIR}/..
>>>> +CFLAGS+= -I${SRCTOP}/contrib/bsddialog/lib -I${.OBJDIR}/../distextract
>>>
>>> Surely this is a sign that this is a worse solution? The header isn’t a
>>> part of distextract any more than partedit, so this is entirely
>>> arbitrary. It also blocks the ability to do the subdirectories in
>>> parallel with each other.
>
>>>
>>> I would much rather this reverted; this feels like a regression to me,
>>> with the only justification being that it “is bad”, according to your
>>> commit message, but so is this, and I would argue it’s worse.
>>>
>>> Or go put it in its own common directory.
>>
>> This was never addressed. Moreover, the current code is in fact broken;
>> OSNAME is not defined within distextract’s Makefile, only the parent’s,
>> so opt_osname.h ends up with #define OSNAME "" in it. I guess I’m the
>> first to notice that the top left of the screen says " Installer"
>> during 14.0’s distextract.
>>
>> I am therefore once again asking for this commit to be reverted, but
>> this time because it doesn’t work, not just because I disagree with the
>> design.