Re: git: 0c3627f44d49 - main - bsdinstall avoid subdir depending on parent

On 21 Apr 2023, at 06:14, Jessica Clarke <jrtc27@FreeBSD.org> wrote:
> 
> On 21 Apr 2023, at 06:01, Simon J. Gerraty <sjg@FreeBSD.org> wrote:
>> 
>> The branch main has been updated by sjg:
>> 
>> URL: https://cgit.FreeBSD.org/src/commit/?id=0c3627f44d49b460d5b9156145dec9d4a91beb2c
>> 
>> 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://reviews.freebsd.org/D39742
>> ---
>> 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.

Jess