| Message ID | 20190506191611.16770-1-avarab@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | perf-lib.sh: make "./run <revisions>" use the correct gits | expand |
On Mon, May 06, 2019 at 09:16:11PM +0200, Ævar Arnfjörð Bjarmason wrote: > Perhaps there's some better way to fix this, but it seems to me that > the best solution is to just make this behavior less magical. We know > in run_dirs_helper() that we're about to run performance tests on a > given <revision>, so let's just set GIT_TEST_INSTALLED to an absolute > path there, and then make getting logging target from a previously > relative path less magical, we'll just explicitly pass down the > relative path as a variable. > > This makes e.g. these cases all work: > > ./run . $PWD/../../ origin/master origin/next HEAD -- <tests> > > As well as just a plain one-off: > > ./run <tests> Doing this naively would break anybody doing: GIT_TEST_INSTALLED=some-relative-path ./p1234-foo.sh but I doubt that actually matters in practice (notably this already does not work with non-perf tests, as test-lib.sh does not do any normalization). I don't think your patch does, because it leaves the extra absolutizing in perf-lib.sh. But then it doesn't feel like it's really simplified anything. ;) > diff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh > index 169f92eae3..b15ee1d262 100644 > --- a/t/perf/perf-lib.sh > +++ b/t/perf/perf-lib.sh > @@ -32,6 +32,10 @@ TEST_NO_MALLOC_CHECK=t > if test -z "$GIT_TEST_INSTALLED"; then > perf_results_prefix= > else > + if test -n "$GIT_PERF_DIR_MYDIR_REL" > + then > + GIT_TEST_INSTALLED=$GIT_PERF_DIR_MYDIR_REL > + fi > perf_results_prefix=$(printf "%s" "${GIT_TEST_INSTALLED%/bin-wrappers}" | tr -c "[a-zA-Z0-9]" "[_*]")"." > GIT_TEST_INSTALLED=$ABSOLUTE_GIT_TEST_INSTALLED > fi So we reset GIT_TEST_INSTALLED to the relative path here (ignoring what's in it!), and then afterwards set it to the absolute path. That still seems rather magical. :) What if instead we: - taught test-lib.sh to make GIT_TEST_INSTALLED absolute (since after all it is the one who is planning to chdir and wreck the relative path) - let callers pass in $GIT_PERF_RESULTS_PREFIX instead of guessing at it ourselves from the path name. Then the "run" script could quite reasonably just pass in the tree oid it already has instead of us trying to decode it. And nobody would care about whether $GIT_TEST_INSTALLED has been mangled. I thought about going this route for my original patch, but I wanted to fix the regression (which I agree is quite serious and embarrassing) as quickly and simply as possible. -Peff
diff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh index 169f92eae3..b15ee1d262 100644 --- a/t/perf/perf-lib.sh +++ b/t/perf/perf-lib.sh @@ -32,6 +32,10 @@ TEST_NO_MALLOC_CHECK=t if test -z "$GIT_TEST_INSTALLED"; then perf_results_prefix= else + if test -n "$GIT_PERF_DIR_MYDIR_REL" + then + GIT_TEST_INSTALLED=$GIT_PERF_DIR_MYDIR_REL + fi perf_results_prefix=$(printf "%s" "${GIT_TEST_INSTALLED%/bin-wrappers}" | tr -c "[a-zA-Z0-9]" "[_*]")"." GIT_TEST_INSTALLED=$ABSOLUTE_GIT_TEST_INSTALLED fi diff --git a/t/perf/run b/t/perf/run index 9aaa733c77..0a7c8744ab 100755 --- a/t/perf/run +++ b/t/perf/run @@ -91,10 +91,14 @@ run_dirs_helper () { if test "$mydir" = .; then unset GIT_TEST_INSTALLED else - GIT_TEST_INSTALLED="$mydir/bin-wrappers" + GIT_PERF_DIR_MYDIR_REL=$mydir + GIT_PERF_DIR_MYDIR_ABS=$(cd $mydir && pwd) + export GIT_PERF_DIR_MYDIR_REL GIT_PERF_DIR_MYDIR_ABS + + GIT_TEST_INSTALLED="$GIT_PERF_DIR_MYDIR_ABS/bin-wrappers" # Older versions of git lacked bin-wrappers; fallback to the # files in the root. - test -d "$GIT_TEST_INSTALLED" || GIT_TEST_INSTALLED=$mydir + test -d "$GIT_TEST_INSTALLED" || GIT_TEST_INSTALLED=$GIT_PERF_DIR_MYDIR_ABS export GIT_TEST_INSTALLED fi run_one_dir "$@"
Fix a really bad regression in 0baf78e7bc ("perf-lib.sh: rely on test-lib.sh for --tee handling", 2019-03-15). Since that change all runs of different <revisions> of git have used the git found in the user's $PATH, e.g. /usr/bin/git instead of the <revision> we just built and wanted to performance test. The problem starts with GIT_TEST_INSTALLED not working like our non-perf tests with the "run" script. I.e. you can't run performance tests against a given installed git. Instead we expect to use it ourselves to point GIT_TEST_INSTALLED to the <revision> we just built. However, we had been relying on '$(cd "$GIT_TEST_INSTALLED" && pwd)' to resolve that relative $GIT_TEST_INSTALLED to an absolute path *before* test-lib.sh was loaded, in cases where it was e.g. "build/<rev>/bin-wrappers" and we wanted "<abs_path>build/...". Perhaps there's some better way to fix this, but it seems to me that the best solution is to just make this behavior less magical. We know in run_dirs_helper() that we're about to run performance tests on a given <revision>, so let's just set GIT_TEST_INSTALLED to an absolute path there, and then make getting logging target from a previously relative path less magical, we'll just explicitly pass down the relative path as a variable. This makes e.g. these cases all work: ./run . $PWD/../../ origin/master origin/next HEAD -- <tests> As well as just a plain one-off: ./run <tests> And, since we're passing down the new GIT_PERF_DIR_MYDIR_REL we make sure the bug relating to aggregate.perl not finding our files as described in 0baf78e7bc doesn't happen again. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- On Fri, May 03 2019, Jeff King wrote: > On Thu, May 02, 2019 at 05:45:09PM -0400, Jeff King wrote: > >> Here's what I came up with. Note that there's a bug in 'master' right >> now which causes perf to produce nonsense results. It's due to my >> 0baf78e7bc (perf-lib.sh: rely on test-lib.sh for --tee handling, >> 2019-03-15). I'll fix that separately (the timing below is done with >> that commit reverted). > > And here's the fix for that. It's rather subtle, so I hope I explained > it sufficiently. I didn't notice it while working on the original > because everything _appears_ to run fine, but you just get timings from > the wrong version of Git. Which is only noticeable if you're literally > testing two versions that you expect to differ. I ran into this today and it took me an embarrasingly long time to figure out why my code wasn't making things faster. So I wrote this up before seeing your patch, since it wasn't queued in "pu" and my naïve ML search didn't include inline patches (again, *sigh*). Anyway, I wonder if something closer to this patch, or some sort of merge of the two (e.g. to still get rid of the ABSOLUTE_GIT_TEST_INSTALLED variable) is better. I.e. why try to magically detect all of this in perf-lib.sh itself, we know we're going to invoke it like this in the "run" script, so we can just set the appropriate variables there instead of this hard-to-explain magic of $GIT_TEST_INSTALLED being one value the first time, but another one the second time around. t/perf/perf-lib.sh | 4 ++++ t/perf/run | 8 ++++++-- 2 files changed, 10 insertions(+), 2 deletions(-)