All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/5] Improvements to incremental builds
@ 2022-12-02  4:57 Ian Rogers
  2022-12-02  4:57 ` [PATCH 1/5] tools lib api: Add dependency test to install_headers Ian Rogers
                   ` (6 more replies)
  0 siblings, 7 replies; 16+ messages in thread
From: Ian Rogers @ 2022-12-02  4:57 UTC (permalink / raw)
  To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim,
	Josh Poimboeuf, Nathan Chancellor, Nick Desaulniers, Tom Rix,
	Masahiro Yamada, Nicolas Schier, linux-kernel, linux-perf-users,
	bpf, llvm
  Cc: Stephane Eranian, Ian Rogers

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


^ permalink raw reply	[flat|nested] 16+ messages in thread
* [PATCH 1/5] tools lib api: Add dependency test to install_headers
  2022-12-02  4:57 [PATCH 0/5] Improvements to incremental builds Ian Rogers
@ 2022-12-02  4:57 ` Ian Rogers
  2022-12-02  4:57 ` [PATCH 2/5] tools lib perf: " Ian Rogers
                   ` (5 subsequent siblings)
  6 siblings, 0 replies; 16+ messages in thread
From: Ian Rogers @ 2022-12-02  4:57 UTC (permalink / raw)
  To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim,
	Josh Poimboeuf, Nathan Chancellor, Nick Desaulniers, Tom Rix,
	Masahiro Yamada, Nicolas Schier, linux-kernel, linux-perf-users,
	bpf, llvm
  Cc: Stephane Eranian, Ian Rogers

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


^ permalink raw reply related	[flat|nested] 16+ messages in thread
* [PATCH 2/5] tools lib perf: Add dependency test to install_headers
  2022-12-02  4:57 [PATCH 0/5] Improvements to incremental builds Ian Rogers
  2022-12-02  4:57 ` [PATCH 1/5] tools lib api: Add dependency test to install_headers Ian Rogers
@ 2022-12-02  4:57 ` Ian Rogers
  2022-12-16  9:44   ` Alexander Gordeev
  2022-12-02  4:57 ` [PATCH 3/5] tools lib subcmd: Add dependency test to install_headers Ian Rogers
                   ` (4 subsequent siblings)
  6 siblings, 1 reply; 16+ messages in thread
From: Ian Rogers @ 2022-12-02  4:57 UTC (permalink / raw)
  To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim,
	Josh Poimboeuf, Nathan Chancellor, Nick Desaulniers, Tom Rix,
	Masahiro Yamada, Nicolas Schier, linux-kernel, linux-perf-users,
	bpf, llvm
  Cc: Stephane Eranian, Ian Rogers

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


^ permalink raw reply related	[flat|nested] 16+ messages in thread
* [PATCH 3/5] tools lib subcmd: Add dependency test to install_headers
  2022-12-02  4:57 [PATCH 0/5] Improvements to incremental builds Ian Rogers
  2022-12-02  4:57 ` [PATCH 1/5] tools lib api: Add dependency test to install_headers Ian Rogers
  2022-12-02  4:57 ` [PATCH 2/5] tools lib perf: " Ian Rogers
@ 2022-12-02  4:57 ` Ian Rogers
  2022-12-12 20:57   ` Nicolas Schier
  2022-12-20  7:33   ` Nicolas Schier
  2022-12-02  4:57 ` [PATCH 4/5] tools lib symbol: " Ian Rogers
                   ` (3 subsequent siblings)
  6 siblings, 2 replies; 16+ messages in thread
From: Ian Rogers @ 2022-12-02  4:57 UTC (permalink / raw)
  To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim,
	Josh Poimboeuf, Nathan Chancellor, Nick Desaulniers, Tom Rix,
	Masahiro Yamada, Nicolas Schier, linux-kernel, linux-perf-users,
	bpf, llvm
  Cc: Stephane Eranian, Ian Rogers

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


^ permalink raw reply related	[flat|nested] 16+ messages in thread
* [PATCH 4/5] tools lib symbol: Add dependency test to install_headers
  2022-12-02  4:57 [PATCH 0/5] Improvements to incremental builds Ian Rogers
                   ` (2 preceding siblings ...)
  2022-12-02  4:57 ` [PATCH 3/5] tools lib subcmd: Add dependency test to install_headers Ian Rogers
@ 2022-12-02  4:57 ` Ian Rogers
  2022-12-02  4:57 ` [PATCH 5/5] perf build: Fix python/perf.so library's name Ian Rogers
                   ` (2 subsequent siblings)
  6 siblings, 0 replies; 16+ messages in thread
From: Ian Rogers @ 2022-12-02  4:57 UTC (permalink / raw)
  To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim,
	Josh Poimboeuf, Nathan Chancellor, Nick Desaulniers, Tom Rix,
	Masahiro Yamada, Nicolas Schier, linux-kernel, linux-perf-users,
	bpf, llvm
  Cc: Stephane Eranian, Ian Rogers

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/symbol/Makefile | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/tools/lib/symbol/Makefile b/tools/lib/symbol/Makefile
index ea8707b3442a..13d43c6f92b4 100644
--- a/tools/lib/symbol/Makefile
+++ b/tools/lib/symbol/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'; \
-	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)
@@ -100,9 +100,16 @@ install_lib: $(LIBFILE)
 		$(call do_install_mkdir,$(libdir_SQ)); \
 		cp -fpR $(LIBFILE) $(DESTDIR)$(libdir_SQ)
 
-install_headers:
-	$(call QUIET_INSTALL, libsymbol_headers) \
-		$(call do_install,kallsyms.h,$(prefix)/include/symbol,644);
+HDRS := kallsyms.h
+INSTALL_HDRS_PFX := $(DESTDIR)$(prefix)/include/symbol
+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, libsymbol_headers)
 
 install: install_lib install_headers
 
-- 
2.39.0.rc0.267.gcb52ba06e7-goog


^ permalink raw reply related	[flat|nested] 16+ messages in thread
* [PATCH 5/5] perf build: Fix python/perf.so library's name
  2022-12-02  4:57 [PATCH 0/5] Improvements to incremental builds Ian Rogers
                   ` (3 preceding siblings ...)
  2022-12-02  4:57 ` [PATCH 4/5] tools lib symbol: " Ian Rogers
@ 2022-12-02  4:57 ` Ian Rogers
  2022-12-05 12:50 ` [PATCH 0/5] Improvements to incremental builds Arnaldo Carvalho de Melo
  2022-12-12 20:31 ` Nicolas Schier
  6 siblings, 0 replies; 16+ messages in thread
From: Ian Rogers @ 2022-12-02  4:57 UTC (permalink / raw)
  To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim,
	Josh Poimboeuf, Nathan Chancellor, Nick Desaulniers, Tom Rix,
	Masahiro Yamada, Nicolas Schier, linux-kernel, linux-perf-users,
	bpf, llvm
  Cc: Stephane Eranian, Ian Rogers

Since Python 3.3 extensions have a suffix encoding platform and
version information. For example, the perf extension was previously
perf.so but now maybe perf.cpython-310-x86_64-linux-gnu.so. Compute
the extension using Python and then use this in the target name. Doing
this avoids the "perf.so" target always being rebuilt.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/Makefile.config | 4 +++-
 tools/perf/Makefile.perf   | 2 +-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/tools/perf/Makefile.config b/tools/perf/Makefile.config
index b34288cb1900..ede04e07e9cb 100644
--- a/tools/perf/Makefile.config
+++ b/tools/perf/Makefile.config
@@ -871,6 +871,7 @@ define disable-python_code
   NO_LIBPYTHON := 1
 endef
 
+PYTHON_EXTENSION_SUFFIX := '.so'
 ifdef NO_LIBPYTHON
   $(call disable-python,Python support disabled by user)
 else
@@ -889,7 +890,8 @@ else
       else
          LDFLAGS += $(PYTHON_EMBED_LDFLAGS)
          EXTLIBS += $(PYTHON_EMBED_LIBADD)
-         LANG_BINDINGS += $(obj-perf)python/perf.so
+         PYTHON_EXTENSION_SUFFIX := $(shell $(PYTHON) -c 'from importlib import machinery; print(machinery.EXTENSION_SUFFIXES[0])')
+         LANG_BINDINGS += $(obj-perf)python/perf$(PYTHON_EXTENSION_SUFFIX)
          CFLAGS += -DHAVE_LIBPYTHON_SUPPORT
          $(call detected,CONFIG_LIBPYTHON)
       endif
diff --git a/tools/perf/Makefile.perf b/tools/perf/Makefile.perf
index f0e4daeef812..869856bdfdc9 100644
--- a/tools/perf/Makefile.perf
+++ b/tools/perf/Makefile.perf
@@ -642,7 +642,7 @@ all: shell_compatibility_test $(ALL_PROGRAMS) $(LANG_BINDINGS) $(OTHER_PROGRAMS)
 # Create python binding output directory if not already present
 _dummy := $(shell [ -d '$(OUTPUT)python' ] || mkdir -p '$(OUTPUT)python')
 
-$(OUTPUT)python/perf.so: $(PYTHON_EXT_SRCS) $(PYTHON_EXT_DEPS) $(LIBPERF)
+$(OUTPUT)python/perf$(PYTHON_EXTENSION_SUFFIX): $(PYTHON_EXT_SRCS) $(PYTHON_EXT_DEPS) $(LIBPERF)
 	$(QUIET_GEN)LDSHARED="$(CC) -pthread -shared" \
         CFLAGS='$(CFLAGS)' LDFLAGS='$(LDFLAGS)' \
 	  $(PYTHON_WORD) util/setup.py \
-- 
2.39.0.rc0.267.gcb52ba06e7-goog


^ permalink raw reply related	[flat|nested] 16+ messages in thread
* Re: [PATCH 0/5] Improvements to incremental builds
  2022-12-02  4:57 [PATCH 0/5] Improvements to incremental builds Ian Rogers
                   ` (4 preceding siblings ...)
  2022-12-02  4:57 ` [PATCH 5/5] perf build: Fix python/perf.so library's name Ian Rogers
@ 2022-12-05 12:50 ` Arnaldo Carvalho de Melo
  2022-12-12 20:31 ` Nicolas Schier
  6 siblings, 0 replies; 16+ messages in thread
From: Arnaldo Carvalho de Melo @ 2022-12-05 12:50 UTC (permalink / raw)
  To: Ian Rogers
  Cc: Peter Zijlstra, Ingo Molnar, Mark Rutland, Alexander Shishkin,
	Jiri Olsa, Namhyung Kim, Josh Poimboeuf, Nathan Chancellor,
	Nick Desaulniers, Tom Rix, Masahiro Yamada, Nicolas Schier,
	linux-kernel, linux-perf-users, bpf, llvm, Stephane Eranian

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

^ permalink raw reply	[flat|nested] 16+ messages in thread
* Re: [PATCH 0/5] Improvements to incremental builds
  2022-12-02  4:57 [PATCH 0/5] Improvements to incremental builds Ian Rogers
                   ` (5 preceding siblings ...)
  2022-12-05 12:50 ` [PATCH 0/5] Improvements to incremental builds Arnaldo Carvalho de Melo
@ 2022-12-12 20:31 ` Nicolas Schier
  2022-12-13 21:31   ` Ian Rogers
  6 siblings, 1 reply; 16+ messages in thread
From: Nicolas Schier @ 2022-12-12 20:31 UTC (permalink / raw)
  To: Ian Rogers
  Cc: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim,
	Josh Poimboeuf, Nathan Chancellor, Nick Desaulniers, Tom Rix,
	Masahiro Yamada, linux-kernel, linux-perf-users, bpf, llvm,
	Stephane Eranian

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

^ permalink raw reply	[flat|nested] 16+ messages in thread
* Re: [PATCH 3/5] tools lib subcmd: Add dependency test to install_headers
  2022-12-02  4:57 ` [PATCH 3/5] tools lib subcmd: Add dependency test to install_headers Ian Rogers
@ 2022-12-12 20:57   ` Nicolas Schier
  2022-12-13 21:28     ` Ian Rogers
  2022-12-20  7:33   ` Nicolas Schier
  1 sibling, 1 reply; 16+ messages in thread
From: Nicolas Schier @ 2022-12-12 20:57 UTC (permalink / raw)
  To: Ian Rogers
  Cc: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim,
	Josh Poimboeuf, Nathan Chancellor, Nick Desaulniers, Tom Rix,
	Masahiro Yamada, linux-kernel, linux-perf-users, bpf, llvm,
	Stephane Eranian

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

^ permalink raw reply	[flat|nested] 16+ messages in thread
* Re: [PATCH 3/5] tools lib subcmd: Add dependency test to install_headers
  2022-12-12 20:57   ` Nicolas Schier
@ 2022-12-13 21:28     ` Ian Rogers
  2022-12-19 14:44       ` Nicolas Schier
  0 siblings, 1 reply; 16+ messages in thread
From: Ian Rogers @ 2022-12-13 21:28 UTC (permalink / raw)
  To: Nicolas Schier
  Cc: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim,
	Josh Poimboeuf, Nathan Chancellor, Nick Desaulniers, Tom Rix,
	Masahiro Yamada, linux-kernel, linux-perf-users, bpf, llvm,
	Stephane Eranian

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

^ permalink raw reply	[flat|nested] 16+ messages in thread
* Re: [PATCH 0/5] Improvements to incremental builds
  2022-12-12 20:31 ` Nicolas Schier
@ 2022-12-13 21:31   ` Ian Rogers
  0 siblings, 0 replies; 16+ messages in thread
From: Ian Rogers @ 2022-12-13 21:31 UTC (permalink / raw)
  To: Nicolas Schier
  Cc: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim,
	Josh Poimboeuf, Nathan Chancellor, Nick Desaulniers, Tom Rix,
	Masahiro Yamada, linux-kernel, linux-perf-users, bpf, llvm,
	Stephane Eranian

On Mon, Dec 12, 2022 at 12:31 PM Nicolas Schier <nicolas@fjasle.eu> 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: 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 --

^ permalink raw reply	[flat|nested] 16+ messages in thread
* Re: [PATCH 2/5] tools lib perf: Add dependency test to install_headers
  2022-12-02  4:57 ` [PATCH 2/5] tools lib perf: " Ian Rogers
@ 2022-12-16  9:44   ` Alexander Gordeev
  2022-12-16  9:50     ` [PATCH] tools lib perf: fix install_pkgconfig target Alexander Gordeev
  0 siblings, 1 reply; 16+ messages in thread
From: Alexander Gordeev @ 2022-12-16  9:44 UTC (permalink / raw)
  To: Ian Rogers
  Cc: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim,
	Josh Poimboeuf, Nathan Chancellor, Nick Desaulniers, Tom Rix,
	Masahiro Yamada, Nicolas Schier, linux-kernel, linux-perf-users,
	bpf, llvm, Stephane Eranian

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!

^ permalink raw reply	[flat|nested] 16+ messages in thread
* [PATCH] tools lib perf: fix install_pkgconfig target
  2022-12-16  9:44   ` Alexander Gordeev
@ 2022-12-16  9:50     ` Alexander Gordeev
  2022-12-16 13:02       ` Arnaldo Carvalho de Melo
  0 siblings, 1 reply; 16+ messages in thread
From: Alexander Gordeev @ 2022-12-16  9:50 UTC (permalink / raw)
  To: Ian Rogers
  Cc: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim,
	Josh Poimboeuf, Nathan Chancellor, Nick Desaulniers, Tom Rix,
	Masahiro Yamada, Nicolas Schier, linux-kernel, linux-perf-users,
	bpf, llvm, Stephane Eranian

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 <agordeev@linux.ibm.com>
---
 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


^ permalink raw reply related	[flat|nested] 16+ messages in thread
* Re: [PATCH] tools lib perf: fix install_pkgconfig target
  2022-12-16  9:50     ` [PATCH] tools lib perf: fix install_pkgconfig target Alexander Gordeev
@ 2022-12-16 13:02       ` Arnaldo Carvalho de Melo
  0 siblings, 0 replies; 16+ messages in thread
From: Arnaldo Carvalho de Melo @ 2022-12-16 13:02 UTC (permalink / raw)
  To: Alexander Gordeev
  Cc: Ian Rogers, Peter Zijlstra, Ingo Molnar, Mark Rutland,
	Alexander Shishkin, Jiri Olsa, Namhyung Kim, Josh Poimboeuf,
	Nathan Chancellor, Nick Desaulniers, Tom Rix, Masahiro Yamada,
	Nicolas Schier, linux-kernel, linux-perf-users, bpf, llvm,
	Stephane Eranian

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 <agordeev@linux.ibm.com>
> ---
>  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

^ permalink raw reply	[flat|nested] 16+ messages in thread
* Re: [PATCH 3/5] tools lib subcmd: Add dependency test to install_headers
  2022-12-13 21:28     ` Ian Rogers
@ 2022-12-19 14:44       ` Nicolas Schier
  0 siblings, 0 replies; 16+ messages in thread
From: Nicolas Schier @ 2022-12-19 14:44 UTC (permalink / raw)
  To: Ian Rogers
  Cc: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim,
	Josh Poimboeuf, Nathan Chancellor, Nick Desaulniers, Tom Rix,
	Masahiro Yamada, linux-kernel, linux-perf-users, bpf, llvm,
	Stephane Eranian

[-- Attachment #1: Type: text/plain, Size: 2457 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 16+ messages in thread
* Re: [PATCH 3/5] tools lib subcmd: Add dependency test to install_headers
  2022-12-02  4:57 ` [PATCH 3/5] tools lib subcmd: Add dependency test to install_headers Ian Rogers
  2022-12-12 20:57   ` Nicolas Schier
@ 2022-12-20  7:33   ` Nicolas Schier
  1 sibling, 0 replies; 16+ messages in thread
From: Nicolas Schier @ 2022-12-20  7:33 UTC (permalink / raw)
  To: Ian Rogers
  Cc: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim,
	Josh Poimboeuf, Nathan Chancellor, Nick Desaulniers, Tom Rix,
	Masahiro Yamada, linux-kernel, linux-perf-users, bpf, llvm,
	Stephane Eranian

[-- Attachment #1: Type: text/plain, Size: 5614 bytes --]

On Mon 19 Dec 2022 14:48:59 GMT, Ian Rogers wrote:
> On Mon, Dec 19, 2022 at 6:44 AM Nicolas Schier <nicolas@fjasle.eu> wrote:
> 
> > 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
> >
> 
> 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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 16+ messages in thread
end of thread, other threads:[~2022-12-20  7:35 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-12-02  4:57 [PATCH 0/5] Improvements to incremental builds Ian Rogers
2022-12-02  4:57 ` [PATCH 1/5] tools lib api: Add dependency test to install_headers Ian Rogers
2022-12-02  4:57 ` [PATCH 2/5] tools lib perf: " Ian Rogers
2022-12-16  9:44   ` Alexander Gordeev
2022-12-16  9:50     ` [PATCH] tools lib perf: fix install_pkgconfig target Alexander Gordeev
2022-12-16 13:02       ` Arnaldo Carvalho de Melo
2022-12-02  4:57 ` [PATCH 3/5] tools lib subcmd: Add dependency test to install_headers Ian Rogers
2022-12-12 20:57   ` Nicolas Schier
2022-12-13 21:28     ` Ian Rogers
2022-12-19 14:44       ` Nicolas Schier
2022-12-20  7:33   ` Nicolas Schier
2022-12-02  4:57 ` [PATCH 4/5] tools lib symbol: " Ian Rogers
2022-12-02  4:57 ` [PATCH 5/5] perf build: Fix python/perf.so library's name Ian Rogers
2022-12-05 12:50 ` [PATCH 0/5] Improvements to incremental builds Arnaldo Carvalho de Melo
2022-12-12 20:31 ` Nicolas Schier
2022-12-13 21:31   ` Ian Rogers

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.