diff mbox series

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

Message ID patch-6.9-45f1a4e6f93-20221021T091013Z-avarab@gmail.com (mailing list archive)
State New, archived
Headers show
Series cmake: fix *nix & general issues, no test-lib.sh editing, ctest in CI | expand

Commit Message

Ævar Arnfjörð Bjarmason Oct. 21, 2022, 9:44 a.m. UTC 
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.

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.

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

Comments

Phillip Wood Oct. 21, 2022, 2:18 p.m. UTC | #1 
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
Ævar Arnfjörð Bjarmason Oct. 21, 2022, 2:43 p.m. UTC | #2 
On Fri, Oct 21 2022, Phillip Wood wrote:

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

Heh, I did that to begin with, but found that the commit message &
change was too long & trying to explain two different things.

Since GIT_TEST_BUILD_DIR (like GIT_TEST_INSTALLED) is something you can
use stand-alone I prefer to keep it this way. The commit message shows
how you can use it without anything to do with cmake, and then later
(here) we can use it for cmake...

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

Possible, but important enough to care? It's what it does by default,
and we have /contrib/buildsystems/out in the top-level .gitignore.

I could bring it back, basically the GIT-BUILD-DIR again, but is the
edge case worth worrying about? This DWIM behavior of "build with cmake,
cd and run the the test" is something I think we can safely assume is OK
to restrict to the defaults.

Once you're off the defaults and e.g. want a dir in /run/user or
whatever it's also easy to set GIT_TEST_BUILD_DIR to that. The
"contrib/buildsystem/out" assumption is just so it works by default
without tweaking.

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

In general, if it didn't work a lot of our test suite would fail, so
it's definitely supported, and since this is only used to run tests it
should be OK with portability.

But I don't have a Windows dev environment other than the CI, are you
able to test this & check if it works?

> 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}"

Neat, so that "[[...]]" syntax makes sure we don't have any quoting
issues?

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

See above.

Thanks for the careful review!
Phillip Wood Oct. 25, 2022, 1:29 p.m. UTC | #3 
On 21/10/2022 15:43, Ævar Arnfjörð Bjarmason wrote:
> 
> On Fri, Oct 21 2022, Phillip Wood wrote:
>> Lets squash that commit into this one, so we can see how it is used
>> when it is added.
> 
> Heh, I did that to begin with, but found that the commit message &
> change was too long & trying to explain two different things.
> 
> Since GIT_TEST_BUILD_DIR (like GIT_TEST_INSTALLED) is something you can
> use stand-alone I prefer to keep it this way. The commit message shows
> how you can use it without anything to do with cmake, and then later
> (here) we can use it for cmake...

We're only adding it because we want to use it with cmake though and the 
only way to see if the previous patch is correct is to start using it.

>>>    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,
> 
> In general, if it didn't work a lot of our test suite would fail, so
> it's definitely supported, and since this is only used to run tests it
> should be OK with portability.
> 
> But I don't have a Windows dev environment other than the CI, are you
> able to test this & check if it works?

I keep meaning to set up a git build on windows but I haven't got round 
to it. Using "sh -c" also gives us more flexibility in later patches.

>> 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}"
> 
> Neat, so that "[[...]]" syntax makes sure we don't have any quoting
> issues?

Yes, it works like lua. 
https://cmake.org/cmake/help/latest/manual/cmake-language.7.html#bracket-argument

Best Wishes

Phillip
diff mbox series

Patch

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