Re: [PATCH 6/9] cmake: use GIT_TEST_BUILD_DIR instead of editing hack

[Phillip Wood <phillip.wood123@gmail.com>]

Hi Ævar

On 21/10/2022 10:44, Ævar Arnfjörð Bjarmason wrote:
> When cmake builds git in "contrib/buildsystems/out" it will create a
> "t/" directory there containing only the "t/helper/test-tool", but for
> running the tests with "cmake" it cd's to the "real" t/ directory, and
> runs the tests from there.
> 
> To get the test-lib.sh to locate "git" and other binaries in
> "../contrib/buildsystems/out/" rather than "../" we have since [1]
> been editing the "GIT_BUILD_DIR" assignment in test-lib.sh in-place.
> 
> This has meant that when testing cmake we've had to "git reset --hard"
> before running "make" again.
> 
> What this build infrastructure really wanted was some feature like the
> "GIT_TEST_BUILD_DIR" variable added in the preceding commit, so let's
> make use of it.

Lets squash that commit into this one, so we can see how it is used when 
it is added.

> Even though "ctest" will work with this approach, one advantage of the
> previous arrangement was that we could:
> 
> 	A. Build with cmake
> 	B. cd t
> 	C. Run a test
> 
> And have the test itself know to locate and use the cmake binaries,
> this workflow was documented in [2]. The "t/test-lib".sh modification
> here is so that we can support this use-case.
> 
> As [3] notes "contrib/buildsystems/out" isn't just the directory that
> happens to be documented in "contrib/buildsystems/CMakeLists.txt", but
> the one that VS will use when building git.

That may be the directory that VS uses when building git, but it is 
possible to specify a different build directory when running cmake.

> To make it clear what's happening we emit a "setup: " from the
> test-lib.sh to note that we used this fallback method:
> 
> 	$ ./t0001-init.sh
> 	setup: had no ../git, but found & used cmake built git in ../contrib/buildsystems/out/git
> 	ok 1 - plain
> 	[...]
> 
> Note: the "On Windows[...]" part of this is lifted from [4].
> 
> 1. 7f5397a07c6 (cmake: support for testing git when building out of
>     the source tree, 2020-06-26)
> 2. f2f1250c47f (cmake (Windows): recommend using Visual Studio's
>     built-in CMake support, 2020-09-30)
> 3. 3eccc7b99d4 (cmake: ignore files generated by CMake as run in
>     Visual Studio, 2020-09-25)
> 4. https://lore.kernel.org/git/5b0c2a150e9fce1ca0284d65628b42ed5a7aad9a.1666090745.git.gitgitgadget@gmail.com/
> 
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>   contrib/buildsystems/CMakeLists.txt | 15 +--------------
>   t/test-lib.sh                       | 19 +++++++++++++++++++
>   2 files changed, 20 insertions(+), 14 deletions(-)
> 
> diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt
> index 725b3f2ac82..91b7009f4fd 100644
> --- a/contrib/buildsystems/CMakeLists.txt
> +++ b/contrib/buildsystems/CMakeLists.txt
> @@ -1080,25 +1080,12 @@ if(USE_VCPKG)
>   	file(APPEND ${CMAKE_BINARY_DIR}/GIT-BUILD-OPTIONS "PATH=\"$PATH:$TEST_DIRECTORY/../compat/vcbuild/vcpkg/installed/x64-windows/bin\"\n")
>   endif()
>   
> -#Make the tests work when building out of the source tree
> -get_filename_component(CACHE_PATH ${CMAKE_CURRENT_LIST_DIR}/../../CMakeCache.txt ABSOLUTE)
> -if(NOT ${CMAKE_BINARY_DIR}/CMakeCache.txt STREQUAL ${CACHE_PATH})
> -	file(RELATIVE_PATH BUILD_DIR_RELATIVE ${CMAKE_SOURCE_DIR} ${CMAKE_BINARY_DIR}/CMakeCache.txt)
> -	string(REPLACE "/CMakeCache.txt" "" BUILD_DIR_RELATIVE ${BUILD_DIR_RELATIVE})
> -	#Setting the build directory in test-lib.sh before running tests
> -	file(WRITE ${CMAKE_BINARY_DIR}/CTestCustom.cmake
> -		"file(STRINGS ${CMAKE_SOURCE_DIR}/t/test-lib.sh GIT_BUILD_DIR_REPL REGEX \"GIT_BUILD_DIR=(.*)\")\n"
> -		"file(STRINGS ${CMAKE_SOURCE_DIR}/t/test-lib.sh content NEWLINE_CONSUME)\n"
> -		"string(REPLACE \"\${GIT_BUILD_DIR_REPL}\" \"GIT_BUILD_DIR=\\\"$TEST_DIRECTORY/../${BUILD_DIR_RELATIVE}\\\"\" content \"\${content}\")\n"
> -		"file(WRITE ${CMAKE_SOURCE_DIR}/t/test-lib.sh \${content})")
> -endif()
> -
>   file(GLOB test_scipts "${CMAKE_SOURCE_DIR}/t/t[0-9]*.sh")
>   
>   #test
>   foreach(tsh ${test_scipts})
>   	add_test(NAME ${tsh}
> -		COMMAND ${SH_EXE} ${tsh}
> +		COMMAND env GIT_TEST_BUILD_DIR=${CMAKE_BINARY_DIR} ${SH_EXE} ${tsh}

I'm not sure about using env on windows, can we use ${SH_EXE} -c instead 
to avoid creating an extra process?

	COMMAND ${SH_EXE} -c [[GIT_TEST_BUILD_DIR="$1"; . "$2"]] "${tsh}" 
"${CMAKE_BINARY_DIR}" "${tsh}"

>   		WORKING_DIRECTORY ${CMAKE_SOURCE_DIR}/t)
>   endforeach()
>   
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 41b1ddc96ff..284b619708a 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -47,9 +47,21 @@ fi
>   # its build directory.
>   GIT_SOURCE_DIR="${TEST_DIRECTORY%/t}"
>   GIT_BUILD_DIR="$GIT_SOURCE_DIR"
> +GIT_AUTO_CONTRIB_BUILDSYSTEMS_OUT=
>   if test -n "$GIT_TEST_BUILD_DIR"
>   then
>   	GIT_BUILD_DIR="$GIT_TEST_BUILD_DIR"
> +elif ! test -x "$GIT_BUILD_DIR/git" &&
> +     test -x "$GIT_BUILD_DIR/contrib/buildsystems/out/git"

I'm really not keen on hard coding the CMAKE_BINARY_DIR. One of the 
positive things about dscho's approach is that it does not hard code the 
build directory. I'm not convinced this approach is an improvement.

Best Wishes

Phillip

> +then
> +	GIT_BUILD_DIR="$GIT_SOURCE_DIR/contrib/buildsystems/out"
> +	GIT_AUTO_CONTRIB_BUILDSYSTEMS_OUT=t
> +	# 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 "$TEST_DIRECTORY" = "$GIT_BUILD_DIR"
> @@ -1630,6 +1642,13 @@ remove_trash_directory "$TRASH_DIRECTORY" || {
>   	BAIL_OUT 'cannot prepare test area'
>   }
>   
> +# Emitting this now because earlier we didn't have "say", but not in
> +# anything using lib-subtest.sh
> +if test -n "$GIT_AUTO_CONTRIB_BUILDSYSTEMS_OUT" && test -t 1
> +then
> +	say "setup: had no ../git, but found & used cmake built git in ../contrib/buildsystems/out/git"
> +fi
> +
>   remove_trash=t
>   if test -z "$TEST_NO_CREATE_REPO"
>   then