Re: [PATCH] t/perf/run: Don't forget to copy config.mak.autogen & friends to build area

Subsystems: the rest

2 messages, 2 authors, 2016-09-13 · open the first message on its own page

Re: [PATCH] t/perf/run: Don't forget to copy config.mak.autogen & friends to build area

From: Junio C Hamano <hidden>
Date: 2016-09-12 23:10:18

Junio C Hamano [off-list ref] writes:
In other words, something along this line, perhaps.
...
Not quite.  There is no guanratee that the user is using autoconf at
all.  It should be more like this, I think.

 t/perf/run | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/t/perf/run b/t/perf/run
index aa383c2..7ec3734 100755
--- a/t/perf/run
+++ b/t/perf/run
@@ -30,7 +30,13 @@ unpack_git_rev () {
 }
 build_git_rev () {
 	rev=$1
-	cp -t build/$rev ../../{config.mak,config.mak.autogen,config.status}
+	for config in config.mak config.mak.autogen config.status
+	do
+		if test -f "../../$config"
+		then
+			cp "../../$config" "build/$rev/"
+		fi
+	done
 	(cd build/$rev && make $GIT_PERF_MAKE_OPTS) ||
 	die "failed to build revision '$mydir'"
 }

Re: [PATCH] t/perf/run: Don't forget to copy config.mak.autogen & friends to build area

From: Kirill Smelkov <hidden>
Date: 2016-09-13 06:58:14

On Mon, Sep 12, 2016 at 04:10:09PM -0700, Junio C Hamano wrote:
quoted hunk
Junio C Hamano [off-list ref] writes:
quoted
In other words, something along this line, perhaps.
...
Not quite.  There is no guanratee that the user is using autoconf at
all.  It should be more like this, I think.

 t/perf/run | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/t/perf/run b/t/perf/run
index aa383c2..7ec3734 100755
--- a/t/perf/run
+++ b/t/perf/run
@@ -30,7 +30,13 @@ unpack_git_rev () {
 }
 build_git_rev () {
 	rev=$1
-	cp -t build/$rev ../../{config.mak,config.mak.autogen,config.status}
+	for config in config.mak config.mak.autogen config.status
+	do
+		if test -f "../../$config"
+		then
+			cp "../../$config" "build/$rev/"
+		fi
+	done
 	(cd build/$rev && make $GIT_PERF_MAKE_OPTS) ||
 	die "failed to build revision '$mydir'"
 }
Junio, thanks for encouraging feedback and for catching the *-isms. What
you propose is good (and we also automatically fix error when there was
no config.mak - it was working but cp was giving an error to stderr but
script was continuing normally).

I would amend your squash the following way:

* `test -f` -> `test -e`, because -f tests whether a file exists _and_
  is regular file. Some people might have config.mak as a symlink for
  example. We don't want to miss them too.

Please find updated patch below:

---- 8< ----
From: Kirill Smelkov <redacted>
Subject: [PATCH] t/perf/run: Don't forget to copy config.mak.autogen & friends
 to build area

Otherwise for people who use autotools-based configure in main worktree,
the performance testing results will be inconsistent as work and build
trees could be using e.g. different optimization levels.

See e.g.

	http://public-inbox.org/git/20160818175222.bmm3ivjheokf2qzl@sigill.intra.peff.net/

for example.

NOTE config.status has to be copied because otherwise without it the build
would want to run reconfigure this way loosing just copied config.mak.autogen.

Signed-off-by: Kirill Smelkov <redacted>
---
 t/perf/run | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/t/perf/run b/t/perf/run
index cfd7012..e8adeda 100755
--- a/t/perf/run
+++ b/t/perf/run
@@ -30,7 +30,13 @@ unpack_git_rev () {
 }
 build_git_rev () {
 	rev=$1
-	cp ../../config.mak build/$rev/config.mak
+	for config in config.mak config.mak.autogen config.status
+	do
+		if test -e "../../$config"
+		then
+			cp "../../$config" "build/$rev/"
+		fi
+	done
 	(cd build/$rev && make $GIT_PERF_MAKE_OPTS) ||
 	die "failed to build revision '$mydir'"
 }
-- 
2.9.2.701.gf965a18.dirty
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help