Re: [PATCH] perf: fix when running with TEST_OUTPUT_DIRECTORY
From: Ævar Arnfjörð Bjarmason <hidden>
Date: 2021-06-16 11:36:59
On Wed, Jun 16 2021, Patrick Steinhardt wrote:
When the TEST_OUTPUT_DIRECTORY is defined, then all test data will be written in that directory instead of the default directory located in
Is the timing of this patch a coincidence, or did you run into this related to the other patches related to this variable now, i.e. https://lore.kernel.org/git/20210609170520.67014-1-felipe.contreras@gmail.com/ (local) and related.
Fix the issue by adding a `--results-dir` parameter to "aggregate.perl" which identifies the directory where results are and by making the "run" script awake of the TEST_OUTPUT_DIRECTORY variable.
Makes sense.
quoted hunk ↗ jump to hunk
[...] my (@dirs, %dirnames, %dirabbrevs, %prefixes, @tests, - $codespeed, $sortby, $subsection, $reponame); + $codespeed, $sortby, $subsection, $reponame, $resultsdir); Getopt::Long::Configure qw/ require_order /; my $rc = GetOptions("codespeed" => \$codespeed, "reponame=s" => \$reponame, + "results-dir=s" => \$resultsdir, "sort-by=s" => \$sortby, "subsection=s" => \$subsection); usage() unless $rc;@@ -137,7 +139,9 @@ sub sane_backticks { @tests = glob "p????-*.sh"; } -my $resultsdir = "test-results"; +if (not $resultsdir) { + $resultsdir = "test-results"; +}
Works, but FWIW in git.git's perl scripts it's usual to do: my $resultsdir = "test-results"; GetOptions(...); Which serves the same purpose. You can also inline this in the GetOptions call: GetOptions(..., "results-dir=s" => \(my $resultsdir = "test-results"), ); But maybe that's too obscurely Perl-ish, i.e. declaring a variable for our scope inline in another function's argument list, and we should just use the first idiom we use in most other places.
if test -z "$GIT_PERF_AGGREGATING_LATER"; then - ( cd "$TEST_DIRECTORY"/perf && ./aggregate.perl $(basename "$0") ) + ( cd "$TEST_DIRECTORY"/perf && ./aggregate.perl --results-dir="$TEST_RESULTS_DIR" $(basename "$0") ) fi }
Makes sense to split up the overly long line at this point probably...
[...] -mkdir -p test-results -get_subsections "perf" >test-results/run_subsections.names +if test -n "$TEST_OUTPUT_DIRECTORY" +then + TEST_RESULTS_DIR="$TEST_OUTPUT_DIRECTORY/test-results" +else + TEST_RESULTS_DIR=test-results +fi
Indending with spaces?