Re: [PATCH v1 0/4] Teach 'run' perf script to read config files

[Jeff King <peff@peff.net>]

On Wed, Jul 26, 2017 at 05:58:09PM +0200, Christian Couder wrote:

> Actually after taking another look at that, it looks like the following happens:
> 
> 1) the run script sources the original GIT-BUILD-OPTIONS file from
> ../.. relative to its location
> 2) a git version is built in "build/$rev" using GIT_PERF_MAKE_OPTS
> which generates a new GIT-BUILD-OPTIONS file in "build/$rev/"
> 3) when the actual perf scripts are run they source the original
> GIT-BUILD-OPTIONS file (through perf-lib.sh which sources test-lib.sh)

Right, the perf scripts are run in the context of the "outer"
repository, and get their options from that one. I think that's
intentional, and does the right thing for GIT_PERF_* options. It's
possibly confusing if the tests really do want to know about the build
options for a particular test-build (like asking if it was built with
NO_PERL, for example).

I think in practice it works out OK, because we tend to do test-builds
that are similar to what's in the outer repo (because we copy
config.mak, and don't tend to add a lot of command-line options). But if
you put exotic options into your GIT_PERF_MAKE_OPTS, they won't be
reflected in the config that the test scripts see.

> I wonder how useful 1) is, as the variables sourced from original
> GIT-BUILD-OPTIONS are not used inside the "run" script and not
> available to its child processes as they are not exported.
> Is it just so that if people add GIT_PERF_* variables to their
> config.mak before building they can then have those variables used by
> the run script?

Exactly. I put GIT_PERF_MAKE_OPTS in my config.mak, and they end up
respected for each run without me having to specify them manually.

> I also wonder if it would be better at step 3) to source the
> GIT-BUILD-OPTIONS file generated at step 2) instead of the original
> one, because they can be different as the options in
> $GIT_PERF_MAKE_OPTS will be baked into the new GIT-BUILD-OPTIONS file.
> (Of course if $GIT_PERF_MAKE_OPTS was added to config.mak before
> building, then they will be in the original one too. But
> $GIT_PERF_MAKE_OPTS should work without that.)

I think that would make some cases work (build options for the tested
build), but I fear that it would break others (perf variables that
probably should be coming from the "outer" layer). Remember that test
builds may not be current versions and may not be forwarding those
variables via GIT-BUILD-OPTIONS. I think it's important that the bundle
of t/perf scripts act as a single unit that is driven primarily by the
currently checked-out version (and it's up to those scripts to handle
inconsistencies in old versions; see the $MODERN_GIT stuff I added a few
months ago.

Right now I don't think it has been a big problem, because the build
config tends to be the same.  But if we introduce more "properties" that
the user can tweak for a certain test run, this distinction is probably
going to cause more bugs. I'd almost say that the perf scripts should be
a project outside of git.git entirely, to eliminate confusion.

-Peff