Re: [PATCH v2 3/5] t/perf: fix benchmarks with out-of-tree builds

From: Patrick Steinhardt <ps@pks.im>
Date: Apr 22, 2025, 06:51 UTC
Message-ID: <aAc8f52rJ8ATncVc@pks.im>
In-Reply-To: <CAP8UFD3HcAVs351sabU2qd70PmMzTtGbS7P7eKYyDyUQZ4nQ2Q@mail.gmail.com>

On Sun, Apr 20, 2025 at 12:00:08PM +0200, Christian Couder wrote:

Show 29 quoted lines

> On Mon, Apr 14, 2025 at 8:51 AM Patrick Steinhardt <ps@pks.im> wrote:
> 
> > --- a/t/perf/perf-lib.sh
> > +++ b/t/perf/perf-lib.sh
> > @@ -25,7 +25,29 @@ TEST_OUTPUT_DIRECTORY=$(pwd)
> >  TEST_NO_CREATE_REPO=t
> >  TEST_NO_MALLOC_CHECK=t
> >
> > -. ../test-lib.sh
> > +# While test-lib.sh computes the build directory for us, we also have to do the
> > +# same thing in order to locate the script via GIT-BUILD-OPTIONS in the first
> > +# place.
> > +GIT_BUILD_DIR="${GIT_BUILD_DIR:-$TEST_DIRECTORY/..}"
> 
> Right now on 'master' there is:
> 
> GIT_BUILD_DIR="${GIT_BUILD_DIR:-${TEST_DIRECTORY%/t}}"
> if test "$TEST_DIRECTORY" = "$GIT_BUILD_DIR"
> then
>        echo "PANIC: Running in a $TEST_DIRECTORY that doesn't end in '/t'?" >&2
>        exit 1
> fi
> 
> so it's not exactly the same thing, even if it still probably works well.
> 
> Future readers might wonder if this discrepancy results from changes
> that were made to only one of the files or if we really wanted to get
> rid of the "/t" check here. In case we do want to get rid of the "/t"
> check, I think it might be worth saying it clearly in the comment.

The "/.." is intentional here due to the way that `TEST_DIRECTORY` is constructed. If you extend the context of this patch a bit, you can see that `TEST_DIRECTORY=$(pwd)/..`. So stripping "/t" from the suffix wouldn't do anything because it never has that suffix in the first place. And neither do we want to strip "/..", because then we'd end up in "t/perf". So the easiest fix is to just append another "/.." to end up where we want to.

I'll try to paraphrase this in the commit message.

Show 19 quoted lines

> > +if test -f "$GIT_BUILD_DIR/GIT-BUILD-DIR"
> > +then
> > +       GIT_BUILD_DIR="$(cat "$GIT_BUILD_DIR/GIT-BUILD-DIR")" || exit 1
> > +       # On Windows, we must convert Windows paths lest they contain a colon
> > +       case "$(uname -s)" in
> > +       *MINGW*)
> > +               GIT_BUILD_DIR="$(cygpath -au "$GIT_BUILD_DIR")"
> > +               ;;
> > +       esac
> > +fi
> > +
> > +if test ! -f "$GIT_BUILD_DIR"/GIT-BUILD-OPTIONS
> > +then
> > +       echo >&2 'error: GIT-BUILD-OPTIONS missing (has Git been built?).'
> 
> Maybe something like the following could help debug this:
> 
>        echo >&2 "error: GIT-BUILD-OPTIONS file missing from '$GIT_BUILD_DIR'"
>        echo >&2 'error: (has Git been built?).'

I'd rather want to keep this as-is for now as we have the same error message in "test-lib.sh". If we want to change it we should change both errors, but that feels outside of the scope of this patch series.

Patrick