Date: Tue, 5 Dec 2023 22:39:18 +0000 From: Jessica Clarke <jrtc27@freebsd.org> To: "Simon J. Gerraty" <sjg@FreeBSD.org> Cc: "src-committers@freebsd.org" <src-committers@FreeBSD.org>, "dev-commits-src-all@freebsd.org" <dev-commits-src-all@FreeBSD.org>, "dev-commits-src-main@freebsd.org" <dev-commits-src-main@FreeBSD.org> Subject: Re: git: 0c3627f44d49 - main - bsdinstall avoid subdir depending on parent Message-ID: <107720F5-1196-4E6C-AABE-48285D7B18B2@freebsd.org> In-Reply-To: <09DDC25F-63F8-440A-A674-31F190C087B4@freebsd.org> References: <202304210501.33L51PBT011707@gitrepo.freebsd.org> <09DDC25F-63F8-440A-A674-31F190C087B4@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On 21 Apr 2023, at 06:14, Jessica Clarke <jrtc27@FreeBSD.org> wrote: >=20 > On 21 Apr 2023, at 06:01, Simon J. Gerraty <sjg@FreeBSD.org> wrote: >>=20 >> The branch main has been updated by sjg: >>=20 >> URL: = https://cgit.FreeBSD.org/src/commit/?id=3D0c3627f44d49b460d5b9156145dec9d4= a91beb2c >>=20 >> 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 >>=20 >> bsdinstall avoid subdir depending on parent >>=20 >> 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. >>=20 >> In usr.sbin/bsdinstall use SUBDIR_DEPEND_ so tree walking still = works. >>=20 >> 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(-) >>=20 >> 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?=3D FreeBSD >> SUBDIR=3D distextract distfetch partedit runconsoles scripts >> SUBDIR_PARALLEL=3D >> +SUBDIR_DEPEND_distfetch =3D distextract >> +SUBDIR_DEPEND_partedit =3D distextract >> SCRIPTS=3D bsdinstall >> MAN=3D bsdinstall.8 >> PACKAGE=3D bsdinstall >> -GENHDRS=3D opt_osname.h >> -SRCS+=3D ${GENHDRS} >> -CLEANFILES+=3D ${GENHDRS} >>=20 >> SCRIPTS+=3D startbsdinstall >> SCRIPTSDIR_startbsdinstall=3D ${LIBEXECDIR}/bsdinstall >>=20 >> -opt_osname.h: .PHONY >> - if ! grep -q "^#define OSNAME \"${OSNAME}\"$"" ${.TARGET}; then \ >> - echo "#define OSNAME \"${OSNAME}\"" > ${.TARGET}; \ >> - fi >>=20 >> .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 @@ >>=20 >> BINDIR=3D ${LIBEXECDIR}/bsdinstall >> PROG=3D distextract >> -CFLAGS+=3D -I${SRCTOP}/contrib/bsddialog/lib -I${.OBJDIR}/.. >> +CFLAGS+=3D -I${SRCTOP}/contrib/bsddialog/lib -I. >> LIBADD=3D archive bsddialog m >> +SRCS=3D distextract.c >>=20 >> MAN=3D >> +GENHDRS=3D opt_osname.h >> +SRCS+=3D ${GENHDRS} >> +CLEANFILES+=3D ${GENHDRS} >> + >> +opt_osname.h: .PHONY >> + if ! grep -q "^#define OSNAME \"${OSNAME}\"$"" ${.TARGET}; then \ >> + echo "#define OSNAME \"${OSNAME}\"" > ${.TARGET}; \ >> + fi >>=20 >> .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 @@ >>=20 >> BINDIR=3D ${LIBEXECDIR}/bsdinstall >> PROG=3D distfetch >> -CFLAGS+=3D -I${SRCTOP}/contrib/bsddialog/lib -I${.OBJDIR}/.. >> +CFLAGS+=3D -I${SRCTOP}/contrib/bsddialog/lib = -I${.OBJDIR}/../distextract >> LIBADD=3D fetch bsddialog >>=20 >> MAN=3D >> 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=3D partedit >> LINKS=3D ${BINDIR}/partedit ${BINDIR}/autopart \ >> ${BINDIR}/partedit ${BINDIR}/scriptedpart >> SYMLINKS=3D ../libexec/bsdinstall/partedit /usr/sbin/sade >> -CFLAGS+=3D -I${SRCTOP}/contrib/bsddialog/lib -I${.OBJDIR}/.. >> +CFLAGS+=3D -I${SRCTOP}/contrib/bsddialog/lib = -I${.OBJDIR}/../distextract >=20 > Surely this is a sign that this is a worse solution? The header = isn=E2=80=99t 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. >=20 > I would much rather this reverted; this feels like a regression to me, > with the only justification being that it =E2=80=9Cis bad=E2=80=9D, = according to your > commit message, but so is this, and I would argue it=E2=80=99s worse. >=20 > 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=E2=80=99s Makefile, only the = parent=E2=80=99s, so opt_osname.h ends up with #define OSNAME "" in it. I guess I=E2=80=99m = the first to notice that the top left of the screen says " Installer" during 14.0=E2=80=99s distextract. I am therefore once again asking for this commit to be reverted, but this time because it doesn=E2=80=99t work, not just because I disagree = with the design. Jess
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?107720F5-1196-4E6C-AABE-48285D7B18B2>