mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Jeff King <>
To: Christian Couder <>
Cc: git <>, "Junio C Hamano" <>,
	"Thomas Rast" <>,
	"Ævar Arnfjörð Bjarmason" <>,
	"Christian Couder" <>
Subject: Re: [PATCH v1 0/4] Teach 'run' perf script to read config files
Date: Wed, 26 Jul 2017 12:54:28 -0400	[thread overview]
Message-ID: <> (raw)
In-Reply-To: <>

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 which sources

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.


      reply	other threads:[~2017-07-26 16:54 UTC|newest]

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-07-13  6:50 [PATCH v1 0/4] Teach 'run' perf script to read config files Christian Couder
2017-07-13  6:50 ` [PATCH v1 1/4] perf/run: add '--config' option to the 'run' script Christian Couder
2017-07-13  6:50 ` [PATCH v1 2/4] perf/run: add get_var_from_env_or_config() Christian Couder
2017-07-13  6:50 ` [PATCH v1 3/4] perf/run: add GIT_PERF_DIRS_OR_REVS Christian Couder
2017-07-13  6:50 ` [PATCH v1 4/4] perf/run: add calls to get_var_from_env_or_config() Christian Couder
2017-07-13 16:58 ` [PATCH v1 0/4] Teach 'run' perf script to read config files Jeff King
2017-07-13 18:29   ` Junio C Hamano
2017-07-13 18:40     ` Jeff King
2017-07-13 19:21       ` Junio C Hamano
2017-07-13 19:45       ` Christian Couder
2017-07-13 18:57   ` Christian Couder
2017-07-13 20:55     ` Jeff King
2017-07-14  6:27       ` Christian Couder
2017-07-14  8:05         ` Jeff King
2017-07-26 15:58         ` Christian Couder
2017-07-26 16:54           ` Jeff King [this message]

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:

  List information:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \ \ \ \ \ \ \ \ \

* 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.
Code repositories for project(s) associated with this public inbox

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 read-only IMAP folder(s) and NNTP newsgroup(s).