diff mbox series

[18/36] scrub: fix Makefile targets which depend on builddefs

Message ID 155259754139.31886.15219987362146132076.stgit@magnolia (mailing list archive)
State Superseded
Headers show
Series xfsprogs-5.0: fix various problems | expand

Commit Message

Darrick J. Wong March 14, 2019, 9:05 p.m. UTC 
From: Darrick J. Wong <darrick.wong@oracle.com>

Add Makefile dependencies for targets that require variables set in
builddefs.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 scrub/Makefile |   11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

Comments

Eric Sandeen March 20, 2019, 8:23 p.m. UTC | #1 
On 3/14/19 4:05 PM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Add Makefile dependencies for targets that require variables set in
> builddefs.

<wonders why this is unique to scrub/ and whether we should make
scrub/ special?>

Ok, Darrick points out that scrub is the only(?) subdir that actually
substitutes builddefs variables in the /build/ process, so it is unique in
that regard.

However, builddefs also defines all the HAVE_FOO and even (as Darrick
points out) CC= so I'm still feeling like this shouldn't be a one-off
for the scrub subdir.  Thoughts?

-Eric

> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  scrub/Makefile |   11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> 
> diff --git a/scrub/Makefile b/scrub/Makefile
> index 6e155c2c..bbcfe338 100644
> --- a/scrub/Makefile
> +++ b/scrub/Makefile
> @@ -3,7 +3,8 @@
>  #
>  
>  TOPDIR = ..
> -include $(TOPDIR)/include/builddefs
> +builddefs=$(TOPDIR)/include/builddefs
> +include $(builddefs)
>  
>  # On linux we get fsmap from the system or define it ourselves
>  # so include this based on platform type.  If this reverts to only
> @@ -103,27 +104,27 @@ LDIRT = $(XFS_SCRUB_ALL_PROG) *.service *.cron
>  
>  default: depend $(LTCOMMAND) $(XFS_SCRUB_ALL_PROG) $(OPTIONAL_TARGETS)
>  
> -xfs_scrub_all: xfs_scrub_all.in
> +xfs_scrub_all: xfs_scrub_all.in $(builddefs)
>  	@echo "    [SED]    $@"
>  	$(Q)$(SED) -e "s|@sbindir@|$(PKG_SBIN_DIR)|g" \
>  		   -e "s|@pkg_version@|$(PKG_VERSION)|g" \
>  		   -e "s|@scrub_args@|$(XFS_SCRUB_ARGS)|g" < $< > $@
>  	$(Q)chmod a+x $@
>  
> -phase5.o unicrash.o xfs.o: $(TOPDIR)/include/builddefs
> +phase5.o unicrash.o xfs.o: $(builddefs)
>  
>  include $(BUILDRULES)
>  
>  install: $(INSTALL_SCRUB)
>  
> -%.service: %.service.in
> +%.service: %.service.in $(builddefs)
>  	@echo "    [SED]    $@"
>  	$(Q)$(SED) -e "s|@sbindir@|$(PKG_SBIN_DIR)|g" \
>  		   -e "s|@scrub_args@|$(XFS_SCRUB_ARGS)|g" \
>  		   -e "s|@pkg_lib_dir@|$(PKG_LIB_DIR)|g" \
>  		   -e "s|@pkg_name@|$(PKG_NAME)|g" < $< > $@
>  
> -%.cron: %.cron.in
> +%.cron: %.cron.in $(builddefs)
>  	@echo "    [SED]    $@"
>  	$(Q)$(SED) -e "s|@sbindir@|$(PKG_SBIN_DIR)|g" < $< > $@
>  
>
Darrick J. Wong March 21, 2019, 8:39 p.m. UTC | #2 
On Wed, Mar 20, 2019 at 03:23:29PM -0500, Eric Sandeen wrote:
> On 3/14/19 4:05 PM, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > Add Makefile dependencies for targets that require variables set in
> > builddefs.
> 
> <wonders why this is unique to scrub/ and whether we should make
> scrub/ special?>
> 
> Ok, Darrick points out that scrub is the only(?) subdir that actually
> substitutes builddefs variables in the /build/ process, so it is unique in
> that regard.
> 
> However, builddefs also defines all the HAVE_FOO and even (as Darrick
> points out) CC= so I'm still feeling like this shouldn't be a one-off
> for the scrub subdir.  Thoughts?

[Summarizing IRC chatter last night]

Dave suggested using AC_CONFIG_FILES to replace the sed-using targets in
scrub/Makefile, but it turns out that AC_CONFIG_FILES only does a single
level of substitution (because that's all that's needed for Makefiles)
so if we have the statement:

@sbindir@/xfs_scrub -foo

and variables:

exec_prefix="/usr"
sbindir="${exec_prefix}/sbin"

Then AC_CONFIG_FILES emits:

${exec_prefix}/sbin/xfs_scrub -foo

Not the fully variable-expanded absolute path we were expecting:

/usr/sbin/xfs_scrub -foo

like what systemd service files and cronjob filess are supposed to have.
That's why we need the sed commands in the Makefile and the explicit
dependency on builddefs.

FWIW the AC_CONFIG_FILES documentation implies it's only supposed to be
used for generating Makefiles, not actual build targets.

--D

> -Eric
> 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> >  scrub/Makefile |   11 ++++++-----
> >  1 file changed, 6 insertions(+), 5 deletions(-)
> > 
> > 
> > diff --git a/scrub/Makefile b/scrub/Makefile
> > index 6e155c2c..bbcfe338 100644
> > --- a/scrub/Makefile
> > +++ b/scrub/Makefile
> > @@ -3,7 +3,8 @@
> >  #
> >  
> >  TOPDIR = ..
> > -include $(TOPDIR)/include/builddefs
> > +builddefs=$(TOPDIR)/include/builddefs
> > +include $(builddefs)
> >  
> >  # On linux we get fsmap from the system or define it ourselves
> >  # so include this based on platform type.  If this reverts to only
> > @@ -103,27 +104,27 @@ LDIRT = $(XFS_SCRUB_ALL_PROG) *.service *.cron
> >  
> >  default: depend $(LTCOMMAND) $(XFS_SCRUB_ALL_PROG) $(OPTIONAL_TARGETS)
> >  
> > -xfs_scrub_all: xfs_scrub_all.in
> > +xfs_scrub_all: xfs_scrub_all.in $(builddefs)
> >  	@echo "    [SED]    $@"
> >  	$(Q)$(SED) -e "s|@sbindir@|$(PKG_SBIN_DIR)|g" \
> >  		   -e "s|@pkg_version@|$(PKG_VERSION)|g" \
> >  		   -e "s|@scrub_args@|$(XFS_SCRUB_ARGS)|g" < $< > $@
> >  	$(Q)chmod a+x $@
> >  
> > -phase5.o unicrash.o xfs.o: $(TOPDIR)/include/builddefs
> > +phase5.o unicrash.o xfs.o: $(builddefs)
> >  
> >  include $(BUILDRULES)
> >  
> >  install: $(INSTALL_SCRUB)
> >  
> > -%.service: %.service.in
> > +%.service: %.service.in $(builddefs)
> >  	@echo "    [SED]    $@"
> >  	$(Q)$(SED) -e "s|@sbindir@|$(PKG_SBIN_DIR)|g" \
> >  		   -e "s|@scrub_args@|$(XFS_SCRUB_ARGS)|g" \
> >  		   -e "s|@pkg_lib_dir@|$(PKG_LIB_DIR)|g" \
> >  		   -e "s|@pkg_name@|$(PKG_NAME)|g" < $< > $@
> >  
> > -%.cron: %.cron.in
> > +%.cron: %.cron.in $(builddefs)
> >  	@echo "    [SED]    $@"
> >  	$(Q)$(SED) -e "s|@sbindir@|$(PKG_SBIN_DIR)|g" < $< > $@
> >  
> >
diff mbox series

Patch

diff --git a/scrub/Makefile b/scrub/Makefile
index 6e155c2c..bbcfe338 100644
--- a/scrub/Makefile
+++ b/scrub/Makefile
@@ -3,7 +3,8 @@ 
 #
 
 TOPDIR = ..
-include $(TOPDIR)/include/builddefs
+builddefs=$(TOPDIR)/include/builddefs
+include $(builddefs)
 
 # On linux we get fsmap from the system or define it ourselves
 # so include this based on platform type.  If this reverts to only
@@ -103,27 +104,27 @@  LDIRT = $(XFS_SCRUB_ALL_PROG) *.service *.cron
 
 default: depend $(LTCOMMAND) $(XFS_SCRUB_ALL_PROG) $(OPTIONAL_TARGETS)
 
-xfs_scrub_all: xfs_scrub_all.in
+xfs_scrub_all: xfs_scrub_all.in $(builddefs)
 	@echo "    [SED]    $@"
 	$(Q)$(SED) -e "s|@sbindir@|$(PKG_SBIN_DIR)|g" \
 		   -e "s|@pkg_version@|$(PKG_VERSION)|g" \
 		   -e "s|@scrub_args@|$(XFS_SCRUB_ARGS)|g" < $< > $@
 	$(Q)chmod a+x $@
 
-phase5.o unicrash.o xfs.o: $(TOPDIR)/include/builddefs
+phase5.o unicrash.o xfs.o: $(builddefs)
 
 include $(BUILDRULES)
 
 install: $(INSTALL_SCRUB)
 
-%.service: %.service.in
+%.service: %.service.in $(builddefs)
 	@echo "    [SED]    $@"
 	$(Q)$(SED) -e "s|@sbindir@|$(PKG_SBIN_DIR)|g" \
 		   -e "s|@scrub_args@|$(XFS_SCRUB_ARGS)|g" \
 		   -e "s|@pkg_lib_dir@|$(PKG_LIB_DIR)|g" \
 		   -e "s|@pkg_name@|$(PKG_NAME)|g" < $< > $@
 
-%.cron: %.cron.in
+%.cron: %.cron.in $(builddefs)
 	@echo "    [SED]    $@"
 	$(Q)$(SED) -e "s|@sbindir@|$(PKG_SBIN_DIR)|g" < $< > $@