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

[Patrick Steinhardt <ps@pks.im>]

On Sun, Apr 20, 2025 at 12:00:08PM +0200, Christian Couder wrote:
> 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.

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