LinuxLists.cc - [PATCH 0/5] Improvements to incremental builds

2022-12-02 05:16:37

Subject: [PATCH 0/5] Improvements to incremental builds

Switching to using install_headers caused incremental builds to always
rebuild most targets. This was caused by the headers always being
reinstalled and then getting new timestamps causing dependencies to be
rebuilt. Follow the convention in libbpf where the install targets are
separated and trigger when the target isn't present or is out-of-date.

Further, fix an issue in the perf build with libpython where
python/perf.so was also regenerated as the target name was incorrect.

Ian Rogers (5):
tools lib api: Add dependency test to install_headers
tools lib perf: Add dependency test to install_headers
tools lib subcmd: Add dependency test to install_headers
tools lib symbol: Add dependency test to install_headers
perf build: Fix python/perf.so library's name

tools/lib/api/Makefile | 38 ++++++++++++++++++++++-----------
tools/lib/perf/Makefile | 43 +++++++++++++++++++-------------------
tools/lib/subcmd/Makefile | 23 +++++++++++---------
tools/lib/symbol/Makefile | 21 ++++++++++++-------
tools/perf/Makefile.config | 4 +++-
tools/perf/Makefile.perf | 2 +-
6 files changed, 79 insertions(+), 52 deletions(-)

--
2.39.0.rc0.267.gcb52ba06e7-goog

2022-12-02 05:20:47

by Ian Rogers

[permalink] [raw]

Subject: [PATCH 2/5] tools lib perf: Add dependency test to install_headers

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 <[email protected]>
---
tools/lib/perf/Makefile | 43 +++++++++++++++++++++--------------------
1 file changed, 22 insertions(+), 21 deletions(-)

diff --git a/tools/lib/perf/Makefile b/tools/lib/perf/Makefile
index a90fb8c6bed4..30b7f91e7147 100644
--- a/tools/lib/perf/Makefile
+++ b/tools/lib/perf/Makefile
@@ -176,10 +176,10 @@ define do_install_mkdir
endef

define do_install
- if [ ! -d '$(DESTDIR_SQ)$2' ]; then \
- $(INSTALL) -d -m 755 '$(DESTDIR_SQ)$2'; \
- fi; \
- $(INSTALL) $1 $(if $3,-m $3,) '$(DESTDIR_SQ)$2'
+ if [ ! -d '$2' ]; then \
+ $(INSTALL) -d -m 755 '$2'; \
+ fi; \
+ $(INSTALL) $1 $(if $3,-m $3,) '$2'
endef

install_lib: libs
@@ -187,23 +187,24 @@ install_lib: libs
$(call do_install_mkdir,$(libdir_SQ)); \
cp -fpR $(LIBPERF_ALL) $(DESTDIR)$(libdir_SQ)

-install_headers:
- $(call QUIET_INSTALL, libperf_headers) \
- $(call do_install,include/perf/bpf_perf.h,$(prefix)/include/perf,644); \
- $(call do_install,include/perf/core.h,$(prefix)/include/perf,644); \
- $(call do_install,include/perf/cpumap.h,$(prefix)/include/perf,644); \
- $(call do_install,include/perf/threadmap.h,$(prefix)/include/perf,644); \
- $(call do_install,include/perf/evlist.h,$(prefix)/include/perf,644); \
- $(call do_install,include/perf/evsel.h,$(prefix)/include/perf,644); \
- $(call do_install,include/perf/event.h,$(prefix)/include/perf,644); \
- $(call do_install,include/perf/mmap.h,$(prefix)/include/perf,644); \
- $(call do_install,include/internal/cpumap.h,$(prefix)/include/internal,644); \
- $(call do_install,include/internal/evlist.h,$(prefix)/include/internal,644); \
- $(call do_install,include/internal/evsel.h,$(prefix)/include/internal,644); \
- $(call do_install,include/internal/lib.h,$(prefix)/include/internal,644); \
- $(call do_install,include/internal/mmap.h,$(prefix)/include/internal,644); \
- $(call do_install,include/internal/threadmap.h,$(prefix)/include/internal,644); \
- $(call do_install,include/internal/xyarray.h,$(prefix)/include/internal,644);
+HDRS := bpf_perf.h core.h cpumap.h threadmap.h evlist.h evsel.h event.h mmap.h
+INTERNAL_HDRS := cpumap.h evlist.h evsel.h lib.h mmap.h threadmap.h xyarray.h
+
+INSTALL_HDRS_PFX := $(DESTDIR)$(prefix)/include/perf
+INSTALL_HDRS := $(addprefix $(INSTALL_HDRS_PFX)/, $(HDRS))
+INSTALL_INTERNAL_HDRS_PFX := $(DESTDIR)$(prefix)/include/internal
+INSTALL_INTERNAL_HDRS := $(addprefix $(INSTALL_INTERNAL_HDRS_PFX)/, $(INTERNAL_HDRS))
+
+$(INSTALL_HDRS): $(INSTALL_HDRS_PFX)/%.h: include/perf/%.h
+ $(call QUIET_INSTALL, $@) \
+ $(call do_install,$<,$(INSTALL_HDRS_PFX)/,644)
+
+$(INSTALL_INTERNAL_HDRS): $(INSTALL_INTERNAL_HDRS_PFX)/%.h: include/internal/%.h
+ $(call QUIET_INSTALL, $@) \
+ $(call do_install,$<,$(INSTALL_INTERNAL_HDRS_PFX)/,644)
+
+install_headers: $(INSTALL_HDRS) $(INSTALL_INTERNAL_HDRS)
+ $(call QUIET_INSTALL, libperf_headers)

install_pkgconfig: $(LIBPERF_PC)
$(call QUIET_INSTALL, $(LIBPERF_PC)) \
--
2.39.0.rc0.267.gcb52ba06e7-goog

2022-12-02 05:25:35

by Ian Rogers

[permalink] [raw]

Subject: [PATCH 1/5] tools lib api: Add dependency test to install_headers

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 <[email protected]>
---
tools/lib/api/Makefile | 38 ++++++++++++++++++++++++++------------
1 file changed, 26 insertions(+), 12 deletions(-)

diff --git a/tools/lib/api/Makefile b/tools/lib/api/Makefile
index 3649c7f7ea65..044860ac1ed1 100644
--- a/tools/lib/api/Makefile
+++ b/tools/lib/api/Makefile
@@ -88,10 +88,10 @@ define do_install_mkdir
endef

define do_install
- if [ ! -d '$(DESTDIR_SQ)$2' ]; then \
- $(INSTALL) -d -m 755 '$(DESTDIR_SQ)$2'; \
- fi; \
- $(INSTALL) $1 $(if $3,-m $3,) '$(DESTDIR_SQ)$2'
+ if [ ! -d '$2' ]; then \
+ $(INSTALL) -d -m 755 '$2'; \
+ fi; \
+ $(INSTALL) $1 $(if $3,-m $3,) '$2'
endef

install_lib: $(LIBFILE)
@@ -99,14 +99,28 @@ install_lib: $(LIBFILE)
$(call do_install_mkdir,$(libdir_SQ)); \
cp -fpR $(LIBFILE) $(DESTDIR)$(libdir_SQ)

-install_headers:
- $(call QUIET_INSTALL, libapi_headers) \
- $(call do_install,cpu.h,$(prefix)/include/api,644); \
- $(call do_install,debug.h,$(prefix)/include/api,644); \
- $(call do_install,io.h,$(prefix)/include/api,644); \
- $(call do_install,fd/array.h,$(prefix)/include/api/fd,644); \
- $(call do_install,fs/fs.h,$(prefix)/include/api/fs,644); \
- $(call do_install,fs/tracing_path.h,$(prefix)/include/api/fs,644);
+HDRS := cpu.h debug.h io.h
+FD_HDRS := fd/array.h
+FS_HDRS := fs/fs.h fs/tracing_path.h
+INSTALL_HDRS_PFX := $(DESTDIR)$(prefix)/include/api
+INSTALL_HDRS := $(addprefix $(INSTALL_HDRS_PFX)/, $(HDRS))
+INSTALL_FD_HDRS := $(addprefix $(INSTALL_HDRS_PFX)/, $(FD_HDRS))
+INSTALL_FS_HDRS := $(addprefix $(INSTALL_HDRS_PFX)/, $(FS_HDRS))
+
+$(INSTALL_HDRS): $(INSTALL_HDRS_PFX)/%.h: %.h
+ $(call QUIET_INSTALL, $@) \
+ $(call do_install,$<,$(INSTALL_HDRS_PFX)/,644)
+
+$(INSTALL_FD_HDRS): $(INSTALL_HDRS_PFX)/fd/%.h: fd/%.h
+ $(call QUIET_INSTALL, $@) \
+ $(call do_install,$<,$(INSTALL_HDRS_PFX)/fd/,644)
+
+$(INSTALL_FS_HDRS): $(INSTALL_HDRS_PFX)/fs/%.h: fs/%.h
+ $(call QUIET_INSTALL, $@) \
+ $(call do_install,$<,$(INSTALL_HDRS_PFX)/fs/,644)
+
+install_headers: $(INSTALL_HDRS) $(INSTALL_FD_HDRS) $(INSTALL_FS_HDRS)
+ $(call QUIET_INSTALL, libapi_headers)

install: install_lib install_headers

--
2.39.0.rc0.267.gcb52ba06e7-goog

2022-12-02 05:35:10

by Ian Rogers

[permalink] [raw]

Subject: [PATCH 4/5] tools lib symbol: Add dependency test to install_headers

2022-12-02 05:58:43

by Ian Rogers

[permalink] [raw]

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

2022-12-05 13:18:04

by Arnaldo Carvalho de Melo

[permalink] [raw]

Subject: Re: [PATCH 0/5] Improvements to incremental builds

Em Thu, Dec 01, 2022 at 08:57:38PM -0800, Ian Rogers escreveu:
> Switching to using install_headers caused incremental builds to always
> rebuild most targets. This was caused by the headers always being
> reinstalled and then getting new timestamps causing dependencies to be
> rebuilt. Follow the convention in libbpf where the install targets are
> separated and trigger when the target isn't present or is out-of-date.
>
> Further, fix an issue in the perf build with libpython where
> python/perf.so was also regenerated as the target name was incorrect.
>
> Ian Rogers (5):
> tools lib api: Add dependency test to install_headers
> tools lib perf: Add dependency test to install_headers
> tools lib subcmd: Add dependency test to install_headers
> tools lib symbol: Add dependency test to install_headers
> perf build: Fix python/perf.so library's name

The last one isn't applying:

Applying: perf build: Fix python/perf.so library's name
error: patch failed: tools/perf/Makefile.perf:642
error: tools/perf/Makefile.perf: patch does not apply
Patch failed at 0005 perf build: Fix python/perf.so library's name
hint: Use 'git am --show-current-patch=diff' to see the failed patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
⬢[acme@toolbox perf]$

I'll have the first 4 applied to make progress, later I'll check what
went wrong and to fix it.

- Arnaldo

2022-12-12 20:59:26

by Nicolas Schier

[permalink] [raw]

Subject: Re: [PATCH 0/5] Improvements to incremental builds

On Thu, Dec 01, 2022 at 08:57:38PM -0800 Ian Rogers wrote:
> Switching to using install_headers caused incremental builds to always
> rebuild most targets. This was caused by the headers always being
> reinstalled and then getting new timestamps causing dependencies to be
> rebuilt. Follow the convention in libbpf where the install targets are
> separated and trigger when the target isn't present or is out-of-date.
>
> Further, fix an issue in the perf build with libpython where
> python/perf.so was also regenerated as the target name was incorrect.
>
> Ian Rogers (5):
> tools lib api: Add dependency test to install_headers
> tools lib perf: Add dependency test to install_headers
> tools lib subcmd: Add dependency test to install_headers
> tools lib symbol: Add dependency test to install_headers
> perf build: Fix python/perf.so library's name
>
> tools/lib/api/Makefile | 38 ++++++++++++++++++++++-----------
> tools/lib/perf/Makefile | 43 +++++++++++++++++++-------------------
> tools/lib/subcmd/Makefile | 23 +++++++++++---------
> tools/lib/symbol/Makefile | 21 ++++++++++++-------
> tools/perf/Makefile.config | 4 +++-
> tools/perf/Makefile.perf | 2 +-
> 6 files changed, 79 insertions(+), 52 deletions(-)
>
> --
> 2.39.0.rc0.267.gcb52ba06e7-goog

Hi Ian,

which tree is your patch set based on? At least it doesn't apply on the
current kbuild trees.

Kind regards,
Nicolas

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

2022-12-12 21:17:28

by Nicolas Schier

[permalink] [raw]

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

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 <[email protected]>
> ---
> 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: [email protected] irc://oftc.net/nsc
↳ gpg: 18ed 52db e34f 860e e9fb c82b 7d97 0932 55a0 ce7f
-- frykten for herren er opphav til kunnskap --

2022-12-13 21:45:18

by Ian Rogers

[permalink] [raw]

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

On Mon, Dec 12, 2022 at 12:57 PM Nicolas Schier <[email protected]> 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 <[email protected]>
> > ---
> > 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: [email protected] irc://oftc.net/nsc
> ↳ gpg: 18ed 52db e34f 860e e9fb c82b 7d97 0932 55a0 ce7f
> -- frykten for herren er opphav til kunnskap --

2022-12-13 21:46:21

by Ian Rogers

[permalink] [raw]

Subject: Re: [PATCH 0/5] Improvements to incremental builds

On Mon, Dec 12, 2022 at 12:31 PM Nicolas Schier <[email protected]> wrote:
>
> On Thu, Dec 01, 2022 at 08:57:38PM -0800 Ian Rogers wrote:
> > Switching to using install_headers caused incremental builds to always
> > rebuild most targets. This was caused by the headers always being
> > reinstalled and then getting new timestamps causing dependencies to be
> > rebuilt. Follow the convention in libbpf where the install targets are
> > separated and trigger when the target isn't present or is out-of-date.
> >
> > Further, fix an issue in the perf build with libpython where
> > python/perf.so was also regenerated as the target name was incorrect.
> >
> > Ian Rogers (5):
> > tools lib api: Add dependency test to install_headers
> > tools lib perf: Add dependency test to install_headers
> > tools lib subcmd: Add dependency test to install_headers
> > tools lib symbol: Add dependency test to install_headers
> > perf build: Fix python/perf.so library's name
> >
> > tools/lib/api/Makefile | 38 ++++++++++++++++++++++-----------
> > tools/lib/perf/Makefile | 43 +++++++++++++++++++-------------------
> > tools/lib/subcmd/Makefile | 23 +++++++++++---------
> > tools/lib/symbol/Makefile | 21 ++++++++++++-------
> > tools/perf/Makefile.config | 4 +++-
> > tools/perf/Makefile.perf | 2 +-
> > 6 files changed, 79 insertions(+), 52 deletions(-)
> >
> > --
> > 2.39.0.rc0.267.gcb52ba06e7-goog
>
> Hi Ian,
>
> which tree is your patch set based on? At least it doesn't apply on the
> current kbuild trees.
>
> Kind regards,
> Nicolas

Hi Nicolas,

for the perf tool the branch to follow is Arnaldo's perf/core one:
https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git/?h=perf%2Fcore

I may have done this work on Arnaldo's tmp.perf/core branch, which is
used for work-in-progress merges.

Thanks,
Ian

> --
> epost|xmpp: [email protected] irc://oftc.net/nsc
> ↳ gpg: 18ed 52db e34f 860e e9fb c82b 7d97 0932 55a0 ce7f
> -- frykten for herren er opphav til kunnskap --

2022-12-16 10:23:33

by Alexander Gordeev

[permalink] [raw]

Subject: Re: [PATCH 2/5] tools lib perf: Add dependency test to install_headers

On Thu, Dec 01, 2022 at 08:57:40PM -0800, Ian Rogers wrote:

Hi Ian,

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

[...]

This fix causes the below error in linux-next:

install: cannot create regular file '/usr/lib64/pkgconfig/libperf.pc': Permission denied
make: *** [Makefile:210: install_pkgconfig] Error 1

I am posting a follow-up fix. To me the end result does not look
nice, as do_install and do_install_mkdir are inconsistent, but
the above error goes away.

Thanks!

2022-12-16 10:42:30

by Alexander Gordeev

[permalink] [raw]

Subject: [PATCH] tools lib perf: fix install_pkgconfig target

Commit 47e02b94a4c9 ("tools lib perf: Add dependency test
to install_headers") misses the notion of $(DESTDIR_SQ)
for install_pkgconfig target, which leads to error:

install: cannot create regular file '/usr/lib64/pkgconfig/libperf.pc': Permission denied
make: *** [Makefile:210: install_pkgconfig] Error 1

Signed-off-by: Alexander Gordeev <[email protected]>
---
tools/lib/perf/Makefile | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/lib/perf/Makefile b/tools/lib/perf/Makefile
index 30b7f91e7147..d8cad124e4c5 100644
--- a/tools/lib/perf/Makefile
+++ b/tools/lib/perf/Makefile
@@ -208,7 +208,7 @@ install_headers: $(INSTALL_HDRS) $(INSTALL_INTERNAL_HDRS)

install_pkgconfig: $(LIBPERF_PC)
$(call QUIET_INSTALL, $(LIBPERF_PC)) \
- $(call do_install,$(LIBPERF_PC),$(libdir_SQ)/pkgconfig,644)
+ $(call do_install,$(LIBPERF_PC),$(DESTDIR_SQ)$(libdir_SQ)/pkgconfig,644)

install_doc:
$(Q)$(MAKE) -C Documentation install-man install-html install-examples
--
2.34.1

2022-12-16 13:33:47

by Arnaldo Carvalho de Melo

[permalink] [raw]

Subject: Re: [PATCH] tools lib perf: fix install_pkgconfig target

Em Fri, Dec 16, 2022 at 10:50:41AM +0100, Alexander Gordeev escreveu:
> Commit 47e02b94a4c9 ("tools lib perf: Add dependency test
> to install_headers") misses the notion of $(DESTDIR_SQ)
> for install_pkgconfig target, which leads to error:
>
> install: cannot create regular file '/usr/lib64/pkgconfig/libperf.pc': Permission denied
> make: *** [Makefile:210: install_pkgconfig] Error 1

Thanks, applied.

- Arnaldo

> Signed-off-by: Alexander Gordeev <[email protected]>
> ---
> tools/lib/perf/Makefile | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tools/lib/perf/Makefile b/tools/lib/perf/Makefile
> index 30b7f91e7147..d8cad124e4c5 100644
> --- a/tools/lib/perf/Makefile
> +++ b/tools/lib/perf/Makefile
> @@ -208,7 +208,7 @@ install_headers: $(INSTALL_HDRS) $(INSTALL_INTERNAL_HDRS)
>
> install_pkgconfig: $(LIBPERF_PC)
> $(call QUIET_INSTALL, $(LIBPERF_PC)) \
> - $(call do_install,$(LIBPERF_PC),$(libdir_SQ)/pkgconfig,644)
> + $(call do_install,$(LIBPERF_PC),$(DESTDIR_SQ)$(libdir_SQ)/pkgconfig,644)
>
> install_doc:
> $(Q)$(MAKE) -C Documentation install-man install-html install-examples
> --
> 2.34.1

--

- Arnaldo

2022-12-19 16:06:04

by Nicolas Schier

[permalink] [raw]

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

On Tue 13 Dec 2022 13:28:21 GMT, Ian Rogers wrote:
> On Mon, Dec 12, 2022 at 12:57 PM Nicolas Schier <[email protected]> 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 <[email protected]>
> > > ---
> > > 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

Attachments:

(No filename) (2.40 kB)
signature.asc (849.00 B)
Download all attachments

2022-12-20 08:25:07

by Nicolas Schier

[permalink] [raw]

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

On Mon 19 Dec 2022 14:48:59 GMT, Ian Rogers wrote:
> On Mon, Dec 19, 2022 at 6:44 AM Nicolas Schier <[email protected]> wrote:
>
> > On Tue 13 Dec 2022 13:28:21 GMT, Ian Rogers wrote:
> > > On Mon, Dec 12, 2022 at 12:57 PM Nicolas Schier <[email protected]>
> > 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 <[email protected]>
> > > > > ---
> > > > > 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
> >
>
> Hi Nicolas,
>
> Simplicity in the files SGTM, my own shell script norms are to be
> cautious/defensive around the interpretation of spaces. The SQ code was
> cargo culted and so may or may not be necessary. The installation rules are
> dealing with user paths which may contain spaces, so I'd encourage some
> caution. We should be able to come up with some command lines that test all
> cases to determine if anything suffers from the changes and whether to care.
>
> Thanks,
> Ian

Hi Ian,

looking at some of the tools/lib/*/Makefiles and your patch above, the
use of DESTDIR vs. DESTDIR_SQ seems to be quite inconsistent already:

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

Here, the single-quoted DESTDIR_SQ is removed from do_install (which I
think is a good thing)...

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

... and a plain $(DESTDIR) (via $(INSTALL_HDRS_PFX)) is forwarded to
do_install. Doesn't that mean, that we end up with

$(INSTALL) $< -m 644 '$(DESTDIR)$(prefix)/include/subcmd'

where neither $(DESTDIR) nor $(prefix) has the special single-quote
handling? If we would remove the single-quoting and _SQ redefinitions,
it _should_ be possible (again) to have destination paths with spaces
(and single quotes), iff users escape properly e.g.
DESTDIR="'/name with space'". Perhaps a hint about that in the
Documentation might then be helpful.

Or do I get something completely wrong?

If nobody complains, I am going to prepare a patch for removing the
single-quote special handling.

Kind regards,
Nicolas

Attachments:

(No filename) (5.48 kB)
signature.asc (849.00 B)
Download all attachments