From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: Patrick Steinhardt <ps@pks.im>
Cc: git@vger.kernel.org
Subject: Re: [PATCH] perf: fix when running with TEST_OUTPUT_DIRECTORY
Date: Wed, 16 Jun 2021 13:33:32 +0200 [thread overview]
Message-ID: <87mtrqxcq1.fsf@evledraar.gmail.com> (raw)
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.
> [...]
> 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?
next prev parent reply other threads:[~2021-06-16 11:36 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-06-16 9:12 [PATCH] perf: fix when running with TEST_OUTPUT_DIRECTORY Patrick Steinhardt
2021-06-16 11:33 ` Ævar Arnfjörð Bjarmason [this message]
2021-06-16 12:13 ` Patrick Steinhardt
2021-06-18 13:56 ` [PATCH v2] " Patrick Steinhardt
2021-06-29 1:12 ` Junio C Hamano
2021-07-19 8:25 ` Jeff King
2021-07-19 10:52 ` Patrick Steinhardt
2021-07-19 10:53 ` Jeff King
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87mtrqxcq1.fsf@evledraar.gmail.com \
--to=avarab@gmail.com \
--cc=git@vger.kernel.org \
--cc=ps@pks.im \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).