Re: [PATCH v2 4/5] cmake: avoid editing t/test-lib.sh

> Johannes Schindelin via GitGitGadget wrote:
>> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>>
>> In 7f5397a07c6c (cmake: support for testing git when building out of the
>> source tree, 2020-06-26), we implemented support for running Git's test
>> scripts even after building Git in a different directory than the source
>> directory.
>>
>> The way we did this was to edit the file `t/test-lib.sh` to override
>> `GIT_BUILD_DIR` to point somewhere else than the parent of the `t/`
>> directory.
>>
>> This is unideal because it always leaves a tracked file marked as
>> modified, and it is all too easy to commit that change by mistake.
>>
>> Let's change the strategy by teaching `t/test-lib.sh` to detect the
>> presence of a file called `GIT-BUILD-DIR` in the source directory. If it
>> exists, the contents are interpreted as the location to the _actual_
>> build directory. We then write this file as part of the CTest
>> definition.
>>
>> To support building Git via a regular `make` invocation after building
>> it using CMake, we ensure that the `GIT-BUILD-DIR` file is deleted (for
>> convenience, this is done as part of the Makefile rule that is already
>> run with every `make` invocation to ensure that `GIT-BUILD-OPTIONS` is
>> up to date).
> 
> While I like that this removes a user error case, it sacrifices some of the
> separation between contrib/ and the main Git tree by adding logic to
> 'test-lib.sh' that only really benefits the CMake build.
> 
> To your point in [1]:

> 
>> Can we maybe agree that the proposed patch is a net improvement over the
>> status quo, and think about a better solution independently (without
>> blocking this here patch)?
> 
> I don't think it does more harm than good, but I wouldn't go so far as to
> call it a definitive "net improvement." I'd personally very much prefer a
> solution that didn't involve adding 'GIT-BUILD-DIR' just for the sake of
> CMake. Unfortunately, after spending a pretty substantial amount of time
> looking for an alternative, I couldn't think of anything that didn't either
> 1) change how users ran tests or 2) change where CMake builds wrote Git's
> binary files. 
> 
> So, I could go either way on this patch - if others feel strongly that it
> should be dropped, I'll defer to that. Otherwise, I'm fine keeping it unless
> someone can think of a better alternative.
> 

>> diff --git a/t/test-lib.sh b/t/test-lib.sh
>> index 55857af601b..4468ac51f25 100644
>> --- a/t/test-lib.sh
>> +++ b/t/test-lib.sh
>> @@ -42,7 +42,16 @@ then
>>  	TEST_OUTPUT_DIRECTORY=$TEST_DIRECTORY
>>  fi
>>  GIT_BUILD_DIR="${TEST_DIRECTORY%/t}"
>> -if test "$TEST_DIRECTORY" = "$GIT_BUILD_DIR"
>> +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
>> +elif test "$TEST_DIRECTORY" = "$GIT_BUILD_DIR"
>>  then
>>  	echo "PANIC: Running in a $TEST_DIRECTORY that doesn't end in '/t'?" >&2
>>  	exit 1
> Referring to Ævar's review in [1] - while I'm not overly concerned about the

> "switching between make & CMake" file staleness (if I'm not mistaken, the
> same thing can happen now with the modified 'test-lib.sh', so this patch
> doesn't really make anything worse), I do think the changes to 'test-lib.sh'
> should be rearranged to preserve the "PANIC" check:
> 
> ----------------->8----------------->8----------------->8-----------------
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 4468ac51f2..7b57f55c37 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -42,6 +42,11 @@ then
>  	TEST_OUTPUT_DIRECTORY=$TEST_DIRECTORY
>  fi
>  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
>  if test -f "$GIT_BUILD_DIR/GIT-BUILD-DIR"
>  then
>  	GIT_BUILD_DIR="$(cat "$GIT_BUILD_DIR/GIT-BUILD-DIR")" || exit 1
> @@ -51,10 +56,6 @@ then
>  		GIT_BUILD_DIR="$(cygpath -au "$GIT_BUILD_DIR")"
>  		;;
>  	esac
> -elif test "$TEST_DIRECTORY" = "$GIT_BUILD_DIR"
> -then
> -	echo "PANIC: Running in a $TEST_DIRECTORY that doesn't end in '/t'?" >&2
> -	exit 1
>  fi
>  
>  # Prepend a string to a VAR using an arbitrary ":" delimiter, not
> -----------------8<-----------------8<-----------------8<-----------------
> 
> Otherwise, a user could run the tests from outside a 't/' directory if they
> built Git with CMake, which doesn't appear to be part of the intended
> behavior of this patch.