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

From: David A. Greene <greened@obbligato.org>
Date: Mar 6, 2012, 14:40 UTC
Message-ID: <87mx7tiyhh.fsf@smith.obbligato.org>
In-Reply-To: <878vje86cy.fsf@thomas.inf.ethz.ch>

Thomas Rast <trast@inf.ethz.ch> writes:

Show 12 quoted lines

>>> 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 (?).

I understand your concern. Perhaps it could be mitigated with some "HOWTO" comments at the top of the script. I'm nervous about basing the value on other variables because that's what limited the script to such a narrow scope in the first place.

Show 5 quoted lines

> Don't we, right now, get stuff as follows:
>
>   item                   path
>   --------------------------------------------
>   test-lib.sh            $TEST_DIRECTORY

Right now, yes, but it breaks for out-of-tree tests. In the out-of-tree case, TEST_DIRECTORY doesn't contain test-lib.sh. For exmaple, in t7900-subtree.sh, I do this:

. ../../../t/test-lib.sh

because TEST_DIRECTORY is set to some directory under contrib/subtree.

>   git                    $GIT_BUILD_DIR/bin-wrappers

I think so.

>   valgrind.sh            $TEST_DIRECTORY/valgrind

That's what it is now and it's wrong for out-of-tree tests.

>   git (with --valgrind)  $TEST_DIRECTORY/valgrind/bin

Yep.  This is ok.

> You are saying this must change to an entirely new path
>
>   valgrind.sh            $GIT_VALGRIND_TOOLS
>   git (with --valgrind)  $GIT_VALGRIND_TOOLS/bin

The first, yes.  The second, no.  We can leave that alone.

> but what's wrong with simply
>
>   valgrind.sh            $GIT_BUILD_DIR/t/valgrind
>   git (with --valgrind)  $TEST_DIRECTORY/valgrind/bin

These are two separate issues.

GIT_BUILD_DIR may not be anywhere within the source tree, right? If so, t/valgrind may not have any relation whatsoever to GIT_BUILD_DIR. Hence GIT_VALGRIND_TOOLS.

The second part is correct. test-lib.sh sets it up that way (~line 983) and it works fine for out-of-tree tests.

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

Putting valgrind/bin in the test directory is fine. There's no change there. It's this looking for the wrapping valgrind.sh that fails in the current scheme. We cannot rely on GIT_BUILD_DIR to find it as noted above.

                              -Dave