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: Simon J. Gerraty: "git: 0c3627f44d49 - main - bsdinstall avoid subdir depending on parent"
Go to: [ bottom of page ] [ top of archives ] [ this month ]
From: Jessica Clarke <jrtc27_at_freebsd.org>
Date: Fri, 21 Apr 2023 05:14:19 UTC
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.

Jess

> LIBADD+=	geom util bsddialog
> 
> PARTEDIT_ARCH= ${MACHINE}