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

Reply: Jessica Clarke : "Re: git: 0c3627f44d49 - main - bsdinstall avoid subdir depending on parent"
In reply to: Jessica Clarke : "Re: git: 0c3627f44d49 - main - bsdinstall avoid subdir depending on parent"
Go to: [ bottom of page ] [ top of archives ] [ this month ]
From: Simon J. Gerraty <sjg_at_juniper.net>
Date: Thu, 07 Dec 2023 00:46:40 UTC
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/ ?

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