Improvements to incremental builds

[PATCH 3/5] tools lib subcmd: Add dependency test to install_headers

Posted by Ian Rogers 2 years, 9 months ago

Compute the headers to be installed from their source headers and make
each have its own build target to install it. Using dependencies
avoids headers being reinstalled and getting a new timestamp which
then causes files that depend on the header to be rebuilt.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/lib/subcmd/Makefile | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/tools/lib/subcmd/Makefile b/tools/lib/subcmd/Makefile
index 9a316d8b89df..b87213263a5e 100644
--- a/tools/lib/subcmd/Makefile
+++ b/tools/lib/subcmd/Makefile
@@ -89,10 +89,10 @@ define do_install_mkdir
 endef
 
 define do_install
-	if [ ! -d '$(DESTDIR_SQ)$2' ]; then             \
-		$(INSTALL) -d -m 755 '$(DESTDIR_SQ)$2'; \
+	if [ ! -d '$2' ]; then             \
+		$(INSTALL) -d -m 755 '$2'; \
 	fi;                                             \
-	$(INSTALL) $1 $(if $3,-m $3,) '$(DESTDIR_SQ)$2'
+	$(INSTALL) $1 $(if $3,-m $3,) '$2'
 endef
 
 install_lib: $(LIBFILE)
@@ -100,13 +100,16 @@ install_lib: $(LIBFILE)
 		$(call do_install_mkdir,$(libdir_SQ)); \
 		cp -fpR $(LIBFILE) $(DESTDIR)$(libdir_SQ)
 
-install_headers:
-	$(call QUIET_INSTALL, libsubcmd_headers) \
-		$(call do_install,exec-cmd.h,$(prefix)/include/subcmd,644); \
-		$(call do_install,help.h,$(prefix)/include/subcmd,644); \
-		$(call do_install,pager.h,$(prefix)/include/subcmd,644); \
-		$(call do_install,parse-options.h,$(prefix)/include/subcmd,644); \
-		$(call do_install,run-command.h,$(prefix)/include/subcmd,644);
+HDRS := exec-cmd.h help.h pager.h parse-options.h run-command.h
+INSTALL_HDRS_PFX := $(DESTDIR)$(prefix)/include/subcmd
+INSTALL_HDRS := $(addprefix $(INSTALL_HDRS_PFX)/, $(HDRS))
+
+$(INSTALL_HDRS): $(INSTALL_HDRS_PFX)/%.h: %.h
+	$(call QUIET_INSTALL, $@) \
+		$(call do_install,$<,$(INSTALL_HDRS_PFX)/,644)
+
+install_headers: $(INSTALL_HDRS)
+	$(call QUIET_INSTALL, libsubcmd_headers)
 
 install: install_lib install_headers
 
-- 
2.39.0.rc0.267.gcb52ba06e7-goog

Re: [PATCH 3/5] tools lib subcmd: Add dependency test to install_headers

Posted by Nicolas Schier 2 years, 9 months ago

On Thu, Dec 01, 2022 at 08:57:41PM -0800 Ian Rogers wrote:
> Compute the headers to be installed from their source headers and make
> each have its own build target to install it. Using dependencies
> avoids headers being reinstalled and getting a new timestamp which
> then causes files that depend on the header to be rebuilt.
> 
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  tools/lib/subcmd/Makefile | 23 +++++++++++++----------
>  1 file changed, 13 insertions(+), 10 deletions(-)
> 
> diff --git a/tools/lib/subcmd/Makefile b/tools/lib/subcmd/Makefile
> index 9a316d8b89df..b87213263a5e 100644
> --- a/tools/lib/subcmd/Makefile
> +++ b/tools/lib/subcmd/Makefile
> @@ -89,10 +89,10 @@ define do_install_mkdir
>  endef
>  
>  define do_install
> -	if [ ! -d '$(DESTDIR_SQ)$2' ]; then             \
> -		$(INSTALL) -d -m 755 '$(DESTDIR_SQ)$2'; \
> +	if [ ! -d '$2' ]; then             \
> +		$(INSTALL) -d -m 755 '$2'; \
>  	fi;                                             \
> -	$(INSTALL) $1 $(if $3,-m $3,) '$(DESTDIR_SQ)$2'
> +	$(INSTALL) $1 $(if $3,-m $3,) '$2'

What about using '$(INSTALL) -D ...' instead of the if-mkdir-block above?
(E.g. as in tools/debugging/Makefile.)

Kind regards,
Nicolas

>  endef
>  
>  install_lib: $(LIBFILE)
> @@ -100,13 +100,16 @@ install_lib: $(LIBFILE)
>  		$(call do_install_mkdir,$(libdir_SQ)); \
>  		cp -fpR $(LIBFILE) $(DESTDIR)$(libdir_SQ)
>  
> -install_headers:
> -	$(call QUIET_INSTALL, libsubcmd_headers) \
> -		$(call do_install,exec-cmd.h,$(prefix)/include/subcmd,644); \
> -		$(call do_install,help.h,$(prefix)/include/subcmd,644); \
> -		$(call do_install,pager.h,$(prefix)/include/subcmd,644); \
> -		$(call do_install,parse-options.h,$(prefix)/include/subcmd,644); \
> -		$(call do_install,run-command.h,$(prefix)/include/subcmd,644);
> +HDRS := exec-cmd.h help.h pager.h parse-options.h run-command.h
> +INSTALL_HDRS_PFX := $(DESTDIR)$(prefix)/include/subcmd
> +INSTALL_HDRS := $(addprefix $(INSTALL_HDRS_PFX)/, $(HDRS))
> +
> +$(INSTALL_HDRS): $(INSTALL_HDRS_PFX)/%.h: %.h
> +	$(call QUIET_INSTALL, $@) \
> +		$(call do_install,$<,$(INSTALL_HDRS_PFX)/,644)
> +
> +install_headers: $(INSTALL_HDRS)
> +	$(call QUIET_INSTALL, libsubcmd_headers)
>  
>  install: install_lib install_headers
>  
> -- 
> 2.39.0.rc0.267.gcb52ba06e7-goog

-- 
epost|xmpp: nicolas@fjasle.eu          irc://oftc.net/nsc
↳ gpg: 18ed 52db e34f 860e e9fb  c82b 7d97 0932 55a0 ce7f
     -- frykten for herren er opphav til kunnskap --

Re: [PATCH 3/5] tools lib subcmd: Add dependency test to install_headers

Posted by Ian Rogers 2 years, 9 months ago

On Mon, Dec 12, 2022 at 12:57 PM Nicolas Schier <nicolas@fjasle.eu> wrote:
>
> On Thu, Dec 01, 2022 at 08:57:41PM -0800 Ian Rogers wrote:
> > Compute the headers to be installed from their source headers and make
> > each have its own build target to install it. Using dependencies
> > avoids headers being reinstalled and getting a new timestamp which
> > then causes files that depend on the header to be rebuilt.
> >
> > Signed-off-by: Ian Rogers <irogers@google.com>
> > ---
> >  tools/lib/subcmd/Makefile | 23 +++++++++++++----------
> >  1 file changed, 13 insertions(+), 10 deletions(-)
> >
> > diff --git a/tools/lib/subcmd/Makefile b/tools/lib/subcmd/Makefile
> > index 9a316d8b89df..b87213263a5e 100644
> > --- a/tools/lib/subcmd/Makefile
> > +++ b/tools/lib/subcmd/Makefile
> > @@ -89,10 +89,10 @@ define do_install_mkdir
> >  endef
> >
> >  define do_install
> > -     if [ ! -d '$(DESTDIR_SQ)$2' ]; then             \
> > -             $(INSTALL) -d -m 755 '$(DESTDIR_SQ)$2'; \
> > +     if [ ! -d '$2' ]; then             \
> > +             $(INSTALL) -d -m 755 '$2'; \
> >       fi;                                             \
> > -     $(INSTALL) $1 $(if $3,-m $3,) '$(DESTDIR_SQ)$2'
> > +     $(INSTALL) $1 $(if $3,-m $3,) '$2'
>
> What about using '$(INSTALL) -D ...' instead of the if-mkdir-block above?
> (E.g. as in tools/debugging/Makefile.)
>
> Kind regards,
> Nicolas

Thanks Nicolas, the reason was to keep the code consistent. That's not
to say this is the best approach. For example, here is the same thing
for tools/lib/api:
https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git/tree/tools/lib/api/Makefile?h=perf/core&id=f43368371888694a2eceaaad8f5e9775c092009a#n84

If you'd like to improve this in all the versions and send patches I'd
be happy to take a look.

Thanks,
Ian

> >  endef
> >
> >  install_lib: $(LIBFILE)
> > @@ -100,13 +100,16 @@ install_lib: $(LIBFILE)
> >               $(call do_install_mkdir,$(libdir_SQ)); \
> >               cp -fpR $(LIBFILE) $(DESTDIR)$(libdir_SQ)
> >
> > -install_headers:
> > -     $(call QUIET_INSTALL, libsubcmd_headers) \
> > -             $(call do_install,exec-cmd.h,$(prefix)/include/subcmd,644); \
> > -             $(call do_install,help.h,$(prefix)/include/subcmd,644); \
> > -             $(call do_install,pager.h,$(prefix)/include/subcmd,644); \
> > -             $(call do_install,parse-options.h,$(prefix)/include/subcmd,644); \
> > -             $(call do_install,run-command.h,$(prefix)/include/subcmd,644);
> > +HDRS := exec-cmd.h help.h pager.h parse-options.h run-command.h
> > +INSTALL_HDRS_PFX := $(DESTDIR)$(prefix)/include/subcmd
> > +INSTALL_HDRS := $(addprefix $(INSTALL_HDRS_PFX)/, $(HDRS))
> > +
> > +$(INSTALL_HDRS): $(INSTALL_HDRS_PFX)/%.h: %.h
> > +     $(call QUIET_INSTALL, $@) \
> > +             $(call do_install,$<,$(INSTALL_HDRS_PFX)/,644)
> > +
> > +install_headers: $(INSTALL_HDRS)
> > +     $(call QUIET_INSTALL, libsubcmd_headers)
> >
> >  install: install_lib install_headers
> >
> > --
> > 2.39.0.rc0.267.gcb52ba06e7-goog
>
> --
> epost|xmpp: nicolas@fjasle.eu          irc://oftc.net/nsc
> ↳ gpg: 18ed 52db e34f 860e e9fb  c82b 7d97 0932 55a0 ce7f
>      -- frykten for herren er opphav til kunnskap --

Re: [PATCH 3/5] tools lib subcmd: Add dependency test to install_headers

Posted by Nicolas Schier 2 years, 9 months ago

On Tue 13 Dec 2022 13:28:21 GMT, Ian Rogers wrote:
> On Mon, Dec 12, 2022 at 12:57 PM Nicolas Schier <nicolas@fjasle.eu> wrote:
> >
> > On Thu, Dec 01, 2022 at 08:57:41PM -0800 Ian Rogers wrote:
> > > Compute the headers to be installed from their source headers and make
> > > each have its own build target to install it. Using dependencies
> > > avoids headers being reinstalled and getting a new timestamp which
> > > then causes files that depend on the header to be rebuilt.
> > >
> > > Signed-off-by: Ian Rogers <irogers@google.com>
> > > ---
> > >  tools/lib/subcmd/Makefile | 23 +++++++++++++----------
> > >  1 file changed, 13 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/tools/lib/subcmd/Makefile b/tools/lib/subcmd/Makefile
> > > index 9a316d8b89df..b87213263a5e 100644
> > > --- a/tools/lib/subcmd/Makefile
> > > +++ b/tools/lib/subcmd/Makefile
> > > @@ -89,10 +89,10 @@ define do_install_mkdir
> > >  endef
> > >
> > >  define do_install
> > > -     if [ ! -d '$(DESTDIR_SQ)$2' ]; then             \
> > > -             $(INSTALL) -d -m 755 '$(DESTDIR_SQ)$2'; \
> > > +     if [ ! -d '$2' ]; then             \
> > > +             $(INSTALL) -d -m 755 '$2'; \
> > >       fi;                                             \
> > > -     $(INSTALL) $1 $(if $3,-m $3,) '$(DESTDIR_SQ)$2'
> > > +     $(INSTALL) $1 $(if $3,-m $3,) '$2'
> >
> > What about using '$(INSTALL) -D ...' instead of the if-mkdir-block above?
> > (E.g. as in tools/debugging/Makefile.)
> >
> > Kind regards,
> > Nicolas
> 
> Thanks Nicolas, the reason was to keep the code consistent. That's not
> to say this is the best approach. For example, here is the same thing
> for tools/lib/api:
> https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git/tree/tools/lib/api/Makefile?h=perf/core&id=f43368371888694a2eceaaad8f5e9775c092009a#n84
> 
> If you'd like to improve this in all the versions and send patches I'd
> be happy to take a look.
> 
> Thanks,
> Ian

Ian, while watching at tools/lib/*/Makefile I stumple across the 
special single-quote handling (e.g. 'DESTDIR_SQ') several times.  

Top-level Makefile and kbuild are not designed to work with file or 
directory names containing spaces.  Do you know whether this is needed 
for the installation rules in tools/lib/?  I'd like to remove support 
for path names with spaces for a increasing simplicity and consistency.

Kind regards,
Nicolas