Re: [PATCH 2/2] Support Out-Of-Tree Valgrind Tests

[Thomas Rast <trast@inf.ethz.ch>]

dag@cray.com (David A. Greene) writes:

> Thomas Rast <trast@inf.ethz.ch> writes:
>
>> greened@obbligato.org writes:
>>
>>> +# GIT_VALGRIND_TOOLS is the location of tools like valgrind.sh.
>>> +if test -z "$GIT_VALGRIND_TOOLS"
>>> +then
>>> +	# We allow tests to override this, in case they want to run tests
>>> +	# outside of t/.
>>> + 
>>> +        # For in-tree test scripts, this is in TEST_DIRECTORY/valgrind
>>> +        # (t/valgrind), but a test script that lives outside t/ can
>>> +        # set this variable to point at the right place so that it can
>>> +        # find t/valgrind directory that house test helpers like
>>> +        # valgrind.sh.
>>> +       GIT_VALGRIND_TOOLS="$TEST_DIRECTORY"/valgrind
>>> +fi
>>
>> I'm a bit curious: why isn't it enough to spell that path
>> $GIT_BUILD_DIR/t/valgrind instead of making it fully configurable?
>
> For the same reason that TEST_DIRECTORY is different and unrelated from
> GIT_BUILD_DIR.  It's my understanding that GIT_BUILD_DIR could end up
> being somewhere compeltely unrelated to where TOP_SRC/t/valgrind is.
> At least that's why I introduced a new parameter.

I'm just worried that for such a fringe use-case, the maintainer of the
out-of-tree tests will never notice that he missed to customize *this*
particular parameter.  So I'd rather have it spelled in terms of the
existing two (?).

Don't we, right now, get stuff as follows:

  item                   path
  --------------------------------------------
  test-lib.sh            $TEST_DIRECTORY
  git                    $GIT_BUILD_DIR/bin-wrappers
  valgrind.sh            $TEST_DIRECTORY/valgrind
  git (with --valgrind)  $TEST_DIRECTORY/valgrind/bin

You are saying this must change to an entirely new path

  valgrind.sh            $GIT_VALGRIND_TOOLS
  git (with --valgrind)  $GIT_VALGRIND_TOOLS/bin

but what's wrong with simply

  valgrind.sh            $GIT_BUILD_DIR/t/valgrind
  git (with --valgrind)  $TEST_DIRECTORY/valgrind/bin

In the common case of t/, these just map to what we had before.  In the
out-of-tree case, we'd create valgrind/bin in the test directory for the
*temporary* stuff, and still look for the wrapping valgrind.sh in the
git tree.

-- 
Thomas Rast
trast@{inf,student}.ethz.ch