Re: [PATCH] perf: fix when running with TEST_OUTPUT_DIRECTORY

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Jun 16, 2021, 11:33 UTC
Message-ID: <87mtrqxcq1.fsf@evledraar.gmail.com>
In-Reply-To: <005cc9a695f7f9b17190293821369763e9bae662.1623834515.git.ps@pks.im>

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

Show 21 quoted lines

> [...]
>  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.

Show 5 quoted lines

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

Show 9 quoted lines

>[...]
> -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?