tools/perf/Makefile.perf | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)
It seems it cannot be assumed .git/ORIG_HEAD exists if .git/HEAD is
there.
Indeed, recently our public CI reported[1] the following error when
compiling Perf tool:
$ cd tools/perf
$ make -j4 -l4 O=/tmp/(...)/perf DESTDIR=/usr install
(...)
make[2]: *** No rule to make target '../../.git/ORIG_HEAD', needed by '/tmp/(...)/perf/PERF-VERSION-FILE'.
This is because apparently[2] Cirrus Ci uses a Git client implemented
purely in Go[3] to perform a clone. Most likely, this tool doesn't
create any .git/ORIG_HEAD file but .git/HEAD is there. The error can
indeed be reproduced by renaming this .git/ORIG_HEAD file while keeping
.git/HEAD. In other words, it means it is not enough to check the
presence of .git/HEAD to assume .git/ORIG_HEAD exists as well.
With the modification here, the version file is always regenerated if
.git/ORIG_HEAD file is not present. But that is not an issue as kernel
devs probably don't use such tools in their dev env and it can be
assumed for them that they have both .git/HEAD and .git/ORIG_HEAD files.
[1] https://cirrus-ci.com/task/5673396527693824?logs=test#L5026
[2] https://cirrus-ci.org/guide/tips-and-tricks/
[3] https://github.com/go-git/go-git
Fixes: 4e666cdb06ee ("perf tools: Fix dependency for version file creation")
Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net>
---
tools/perf/Makefile.perf | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/tools/perf/Makefile.perf b/tools/perf/Makefile.perf
index 9c935f86d172..2f64e4904bbb 100644
--- a/tools/perf/Makefile.perf
+++ b/tools/perf/Makefile.perf
@@ -1143,10 +1143,12 @@ endif
# Trick: if ../../.git does not exist - we are building out of tree for example,
# then force version regeneration:
#
+GIT-HEAD-PHONY =
ifeq ($(wildcard ../../.git/HEAD),)
- GIT-HEAD-PHONY = ../../.git/HEAD ../../.git/ORIG_HEAD
-else
- GIT-HEAD-PHONY =
+ GIT-HEAD-PHONY += ../../.git/HEAD
+endif
+ifeq ($(wildcard ../../.git/ORIG_HEAD),)
+ GIT-HEAD-PHONY += ../../.git/ORIG_HEAD
endif
FORCE:
base-commit: ab0809af0bee88b689ba289ec8c40aa2be3a17ec
--
2.34.1
Hi Matthieu, Thank you for your modifications, that's great! Our CI did some validations and here is its report: - KVM Validation: normal: - Success! ✅: - Task: https://cirrus-ci.com/task/5186485547696128 - Summary: https://api.cirrus-ci.com/v1/artifact/task/5186485547696128/summary/summary.txt - {"code":404,"message": - "HTTP 404 Not Found"}: - Task: https://cirrus-ci.com/task/6312385454538752 - Summary: https://api.cirrus-ci.com/v1/artifact/task/6312385454538752/summary/summary.txt Initiator: Patchew Applier Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/d2e7106ae0cc Please note that despite all the efforts that have been already done to have a stable tests suite when executed on a public CI like here, it is possible some reported issues are not due to your modifications. Still, do not hesitate to help us improve that ;-) Cheers, MPTCP GH Action bot Bot operated by Matthieu Baerts (Tessares)
Hi, On 2022-03-29 11:31:20 +0200, Matthieu Baerts wrote: > It seems it cannot be assumed .git/ORIG_HEAD exists if .git/HEAD is > there. > > Indeed, recently our public CI reported[1] the following error when > compiling Perf tool: > > $ cd tools/perf > $ make -j4 -l4 O=/tmp/(...)/perf DESTDIR=/usr install > (...) > make[2]: *** No rule to make target '../../.git/ORIG_HEAD', needed by '/tmp/(...)/perf/PERF-VERSION-FILE'. > > This is because apparently[2] Cirrus Ci uses a Git client implemented > purely in Go[3] to perform a clone. Most likely, this tool doesn't > create any .git/ORIG_HEAD file but .git/HEAD is there. The error can > indeed be reproduced by renaming this .git/ORIG_HEAD file while keeping > .git/HEAD. In other words, it means it is not enough to check the > presence of .git/HEAD to assume .git/ORIG_HEAD exists as well. FWIW, It's not just custom git implementations, stock git doesn't ensure it's there either. I build a nightly VM image with Linus' kernel for postgres testing, and as part of that I do a minimal clone: git clone --single-branch --depth 1 git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git /usr/src/linux and then build the kernel. The build recently started failing like this: https://cirrus-ci.com/task/4648999113195520?logs=build_image#L3121 It's not a question of "--single-branch --depth 1" - ORIG_HEAD just isn't there in a new clone. Which makes sense, because there's no previous value for HEAD. Greetings, Andres Freund
On March 31, 2022 2:33:44 PM GMT-03:00, Andres Freund <andres@anarazel.de> wrote: >Hi, > >On 2022-03-29 11:31:20 +0200, Matthieu Baerts wrote: >> It seems it cannot be assumed .git/ORIG_HEAD exists if .git/HEAD is >> there. >> >> Indeed, recently our public CI reported[1] the following error when >> compiling Perf tool: >> >> $ cd tools/perf >> $ make -j4 -l4 O=/tmp/(...)/perf DESTDIR=/usr install >> (...) >> make[2]: *** No rule to make target '../../.git/ORIG_HEAD', needed by '/tmp/(...)/perf/PERF-VERSION-FILE'. >> >> This is because apparently[2] Cirrus Ci uses a Git client implemented >> purely in Go[3] to perform a clone. Most likely, this tool doesn't >> create any .git/ORIG_HEAD file but .git/HEAD is there. The error can >> indeed be reproduced by renaming this .git/ORIG_HEAD file while keeping >> .git/HEAD. In other words, it means it is not enough to check the >> presence of .git/HEAD to assume .git/ORIG_HEAD exists as well. > >FWIW, It's not just custom git implementations, stock git doesn't ensure it's >there either. I build a nightly VM image with Linus' kernel for postgres >testing, and as part of that I do a minimal clone: > git clone --single-branch --depth 1 git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git /usr/src/linux >and then build the kernel. The build recently started failing like this: >https://cirrus-ci.com/task/4648999113195520?logs=build_image#L3121 > >It's not a question of "--single-branch --depth 1" - ORIG_HEAD just isn't >there in a new clone. Which makes sense, because there's no previous value for >HEAD. I merged the patch that stops looking for .git files, as soon as I fix a libperl feature test with CC=clang I'll send a pull req to Linus to get rid of this .git perf --version related regression. - Arnaldo > >Greetings, > >Andres Freund
On 29/03/2022 10:31, Matthieu Baerts wrote: Hi Matthieu, Sorry for the breakage. About 4e666cdb06ee, I have since discussed this topic with the git community and they think it is unreliable to use any git internal file as a depenency, as mentioned here: https://lore.kernel.org/linux-perf-users/85891cbd-e53c-89ac-6da2-40b5d56cd316@huawei.com/T/#t So I am not sure if it's better to fix that or just effectively back it out with a (hopefully) proper fix. > It seems it cannot be assumed .git/ORIG_HEAD exists if .git/HEAD is > there. > > Indeed, recently our public CI reported[1] the following error when > compiling Perf tool: > > $ cd tools/perf > $ make -j4 -l4 O=/tmp/(...)/perf DESTDIR=/usr install > (...) > make[2]: *** No rule to make target '../../.git/ORIG_HEAD', needed by '/tmp/(...)/perf/PERF-VERSION-FILE'. > > This is because apparently[2] Cirrus Ci uses a Git client implemented > purely in Go[3] to perform a clone. Most likely, this tool doesn't > create any .git/ORIG_HEAD file but .git/HEAD is there. The error can > indeed be reproduced by renaming this .git/ORIG_HEAD file while keeping > .git/HEAD. In other words, it means it is not enough to check the > presence of .git/HEAD to assume .git/ORIG_HEAD exists as well. > > With the modification here, the version file is always regenerated if > .git/ORIG_HEAD file is not present. But that is not an issue as kernel > devs probably don't use such tools in their dev env and it can be > assumed for them that they have both .git/HEAD and .git/ORIG_HEAD files. right Could you please try this: ---->8------ From 694964709a7fc2b46c995bb7b1967cc6b129def8 Mon Sep 17 00:00:00 2001 From: John Garry <john.garry@huawei.com> Date: Tue, 29 Mar 2022 11:06:46 +0100 Subject: [PATCH] perf: Stop depending on .git internal files for building PERF-VERSION-FILE This essentially reverts commit c72e3f04b45fb2e50cdd81a50c3778c6a57251d8 and commit 4e666cdb06eede2069a7b1a96a1359d1c441a3eb. In commit c72e3f04b45f ("tools/perf/build: Speed up git-version test on re-make"), a makefile dependency on .git/HEAD was added. The background is that running PERF-VERSION-FILE is relatively slow, and commands like "git describe" are particularly slow. In commit 4e666cdb06ee ("perf tools: Fix dependency for version file creation"), an additional dependency on .git/ORIG_HEAD was added, as .git/HEAD may not change for "git reset --hard HEAD^" command. However, as discussed with the git community in [0], using git internal files for dependencies is not reliable. Commit 4e666cdb06ee also breaks some build scenarios [1]. As mentioned, c72e3f04b45f was added to speed up the build. However in commit 7572733b8499 ("perf tools: Fix version kernel tag") we removed the call to "git describe", so just revert Makefile.perf back to same as pre c72e3f04b45f and the build should not be so slow, as below: Pre 7572733b8499: $> time util/PERF-VERSION-GEN PERF_VERSION = 5.17.rc8.g4e666cdb06ee real 0m0.110s user 0m0.091s sys 0m0.019s Post 7572733b8499: $> time util/PERF-VERSION-GEN PERF_VERSION = 5.17.rc8.g7572733b8499 real 0m0.039s user 0m0.036s sys 0m0.007s [0] https://lore.kernel.org/git/87wngkpddp.fsf@igel.home/T/#m4a4dd6de52fdbe21179306cd57b3761eb07f45f8 [1] https://lore.kernel.org/linux-perf-users/20220329093120.4173283-1-matthieu.baerts@tessares.net/T/#u Signed-off-by: John Garry <john.garry@huawei.com> diff --git a/tools/perf/Makefile.perf b/tools/perf/Makefile.perf index 9c935f86d172..ddd03b21bda2 100644 --- a/tools/perf/Makefile.perf +++ b/tools/perf/Makefile.perf @@ -691,9 +691,8 @@ $(OUTPUT)common-cmds.h: $(wildcard Documentation/perf-*.txt) $(SCRIPTS) : % : %.sh $(QUIET_GEN)$(INSTALL) '$@.sh' '$(OUTPUT)$@' -$(OUTPUT)PERF-VERSION-FILE: ../../.git/HEAD ../../.git/ORIG_HEAD +$(OUTPUT)PERF-VERSION-FILE: .FORCE-PERF-VERSION-FILE $(Q)$(SHELL_PATH) util/PERF-VERSION-GEN $(OUTPUT) - $(Q)touch $(OUTPUT)PERF-VERSION-FILE # These can record PERF_VERSION perf.spec $(SCRIPTS) \ @@ -1139,21 +1138,12 @@ else @echo "FEATURE-DUMP file available in $(OUTPUT)FEATURE-DUMP" endif -# -# Trick: if ../../.git does not exist - we are building out of tree for example, -# then force version regeneration: -# -ifeq ($(wildcard ../../.git/HEAD),) - GIT-HEAD-PHONY = ../../.git/HEAD ../../.git/ORIG_HEAD -else - GIT-HEAD-PHONY = -endif FORCE: .PHONY: all install clean config-clean strip install-gtk .PHONY: shell_compatibility_test please_set_SHELL_PATH_to_a_more_modern_shell -.PHONY: $(GIT-HEAD-PHONY) TAGS tags cscope FORCE prepare +.PHONY: .FORCE-PERF-VERSION-FILE TAGS tags cscope FORCE prepare .PHONY: libtraceevent_plugins archheaders endif # force_fixdep -- 2.26.2 -----8<------ I need to test it more as I just wrote it up now. Thanks, John > > [1] https://cirrus-ci.com/task/5673396527693824?logs=test#L5026 > [2] https://cirrus-ci.org/guide/tips-and-tricks/ > [3] https://github.com/go-git/go-git > > Fixes: 4e666cdb06ee ("perf tools: Fix dependency for version file creation") > Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net> > --- > tools/perf/Makefile.perf | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) > > diff --git a/tools/perf/Makefile.perf b/tools/perf/Makefile.perf > index 9c935f86d172..2f64e4904bbb 100644 > --- a/tools/perf/Makefile.perf > +++ b/tools/perf/Makefile.perf > @@ -1143,10 +1143,12 @@ endif > # Trick: if ../../.git does not exist - we are building out of tree for example, > # then force version regeneration: > # > +GIT-HEAD-PHONY = > ifeq ($(wildcard ../../.git/HEAD),) > - GIT-HEAD-PHONY = ../../.git/HEAD ../../.git/ORIG_HEAD > -else > - GIT-HEAD-PHONY = > + GIT-HEAD-PHONY += ../../.git/HEAD > +endif > +ifeq ($(wildcard ../../.git/ORIG_HEAD),) > + GIT-HEAD-PHONY += ../../.git/ORIG_HEAD > endif > > FORCE: > > base-commit: ab0809af0bee88b689ba289ec8c40aa2be3a17ec
Hi John, On 29/03/2022 12:32, John Garry wrote: > On 29/03/2022 10:31, Matthieu Baerts wrote: > > Hi Matthieu, > > Sorry for the breakage. That's alright, it happens. (...) > Could you please try this: > > ---->8------ > > > From 694964709a7fc2b46c995bb7b1967cc6b129def8 Mon Sep 17 00:00:00 2001 > From: John Garry <john.garry@huawei.com> > Date: Tue, 29 Mar 2022 11:06:46 +0100 > Subject: [PATCH] perf: Stop depending on .git internal files for > building PERF-VERSION-FILE > > This essentially reverts commit c72e3f04b45fb2e50cdd81a50c3778c6a57251d8 > and commit 4e666cdb06eede2069a7b1a96a1359d1c441a3eb. > > In commit c72e3f04b45f ("tools/perf/build: Speed up git-version test on > re-make"), a makefile dependency on .git/HEAD was added. The background > is that running PERF-VERSION-FILE is relatively slow, and commands like > "git describe" are particularly slow. > > In commit 4e666cdb06ee ("perf tools: Fix dependency for version file > creation"), an additional dependency on .git/ORIG_HEAD was added, as > .git/HEAD may not change for "git reset --hard HEAD^" command. > > However, as discussed with the git community in [0], using git internal > files for dependencies is not reliable. Commit 4e666cdb06ee also breaks > some build scenarios [1]. > > As mentioned, c72e3f04b45f was added to speed up the build. However in > commit 7572733b8499 ("perf tools: Fix version kernel tag") we removed > the call to "git describe", so just revert Makefile.perf back to same as > pre c72e3f04b45f and the build should not be so slow, as below: > > Pre 7572733b8499: > $> time util/PERF-VERSION-GEN > PERF_VERSION = 5.17.rc8.g4e666cdb06ee > > real 0m0.110s > user 0m0.091s > sys 0m0.019s > > Post 7572733b8499: > $> time util/PERF-VERSION-GEN > PERF_VERSION = 5.17.rc8.g7572733b8499 > > real 0m0.039s > user 0m0.036s > sys 0m0.007s Probably the optimisation to avoid calling this script is indeed no longer needed but it is not me to judge. What I know is that indeed, 'git describe' can be slow while just returning the current SHA is supposed to be always quick. (...) > diff --git a/tools/perf/Makefile.perf b/tools/perf/Makefile.perf > index 9c935f86d172..ddd03b21bda2 100644 > --- a/tools/perf/Makefile.perf > +++ b/tools/perf/Makefile.perf > @@ -691,9 +691,8 @@ $(OUTPUT)common-cmds.h: $(wildcard > Documentation/perf-*.txt) > $(SCRIPTS) : % : %.sh > $(QUIET_GEN)$(INSTALL) '$@.sh' '$(OUTPUT)$@' > > -$(OUTPUT)PERF-VERSION-FILE: ../../.git/HEAD ../../.git/ORIG_HEAD > +$(OUTPUT)PERF-VERSION-FILE: .FORCE-PERF-VERSION-FILE Regarding my issue, removing the dependence to .git/ORIG_HEAD file fixes the issue. So I'm fine to drop my patch and revert the two commits you mentioned. If you send this patch, please do not forget to add a Fixes tag: Fixes: 4e666cdb06ee ("perf tools: Fix dependency for version file creation") Cheers, Matt -- Tessares | Belgium | Hybrid Access Solutions www.tessares.net
© 2016 - 2024 Red Hat, Inc.