git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Victoria Dye <vdye@github.com>
Date
Sep 8, 2022, 23:37 UTC
Message-ID
<531620e1-de4c-74aa-c840-c12ce81f8740@github.com>
In-Reply-To
<22473d6b8f3d4e4c482c27a4fb3b58705d4c93ca.1661243463.git.gitgitgadget@gmail.com>
Johannes Schindelin via GitGitGadget wrote:
Show 25 quoted lines
> 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.

Show 75 quoted lines
> 
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  .gitignore                          |  1 +
>  Makefile                            |  1 +
>  contrib/buildsystems/CMakeLists.txt |  7 +------
>  t/test-lib.sh                       | 11 ++++++++++-
>  4 files changed, 13 insertions(+), 7 deletions(-)
> 
> diff --git a/.gitignore b/.gitignore
> index a4522157641..b72ddf09346 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -2,6 +2,7 @@
>  /fuzz_corpora
>  /fuzz-pack-headers
>  /fuzz-pack-idx
> +/GIT-BUILD-DIR
>  /GIT-BUILD-OPTIONS
>  /GIT-CFLAGS
>  /GIT-LDFLAGS
> diff --git a/Makefile b/Makefile
> index 04d0fd1fe60..9347ed90da7 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -3028,6 +3028,7 @@ else
>  	@echo RUNTIME_PREFIX=\'false\' >>$@+
>  endif
>  	@if cmp $@+ $@ >/dev/null 2>&1; then $(RM) $@+; else mv $@+ $@; fi
> +	@if test -f GIT-BUILD-DIR; then rm GIT-BUILD-DIR; fi
>  
>  ### Detect Python interpreter path changes
>  ifndef NO_PYTHON
> diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt
> index fe606c179f7..29d7e236ae1 100644
> --- a/contrib/buildsystems/CMakeLists.txt
> +++ b/contrib/buildsystems/CMakeLists.txt
> @@ -1067,14 +1067,9 @@ 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})")
> +		"file(WRITE ${CMAKE_SOURCE_DIR}/GIT-BUILD-DIR \"${CMAKE_BINARY_DIR}\")")
>  	#misc copies
>  	file(COPY ${CMAKE_SOURCE_DIR}/t/chainlint.sed DESTINATION ${CMAKE_BINARY_DIR}/t/)
>  	file(COPY ${CMAKE_SOURCE_DIR}/po/is.po DESTINATION ${CMAKE_BINARY_DIR}/po/)
> 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.
Previous: Ævar Arnfjörð BjarmasonNext: Victoria Dye
Message 33 of 58 in “Some fixes and an improvement for using CTest on Windows”
  1. 0/5 Some fixes and an improvement for using CTest on WindowsJohannes Schindelin via GitGitGadget, Aug 10, 2022
  2. 1/5 cmake: align CTest definition with Git's CI runsJohannes Schindelin via GitGitGadget, Aug 10, 2022
  3. Junio C HamanoAug 10, 2022
  4. Johannes SchindelinAug 16, 2022
  5. Junio C HamanoAug 16, 2022
  6. Johannes SchindelinAug 19, 2022
  7. Ævar Arnfjörð BjarmasonAug 11, 2022
  8. 2/5 cmake: copy the merge tools for testingJohannes Schindelin via GitGitGadget, Aug 10, 2022
  9. 3/5 tests: explicitly skip `chmod` calls on WindowsJohannes Schindelin via GitGitGadget, Aug 10, 2022
  10. Ævar Arnfjörð BjarmasonAug 11, 2022
  11. Johannes SchindelinAug 22, 2022
  12. Johannes SchindelinAug 23, 2022
  13. 4/5 add -p: avoid ambiguous signed/unsigned comparisonJohannes Schindelin via GitGitGadget, Aug 10, 2022
  14. Junio C HamanoAug 10, 2022
  15. Johannes SchindelinAug 16, 2022
  16. Junio C HamanoAug 16, 2022
  17. Johannes SchindelinAug 19, 2022
  18. Phillip WoodAug 11, 2022
  19. Johannes SchindelinAug 16, 2022
  20. Phillip WoodAug 16, 2022
  21. Johannes SchindelinAug 19, 2022
  22. 5/5 cmake: avoid editing t/test-lib.shJohannes Schindelin via GitGitGadget, Aug 10, 2022
  23. Ævar Arnfjörð BjarmasonAug 11, 2022
  24. Johannes SchindelinOct 18, 2022
  25. Phillip WoodAug 11, 2022
  26. Johannes SchindelinAug 16, 2022
  27. Phillip WoodAug 16, 2022
  28. 0/5 Some fixes and an improvement for using CTest on WindowsJohannes Schindelin via GitGitGadget, Aug 23, 2022
  29. 4/5 cmake: avoid editing t/test-lib.shJohannes Schindelin via GitGitGadget, Aug 23, 2022
  30. Ævar Arnfjörð BjarmasonSep 8, 2022
  31. Johannes SchindelinOct 18, 2022
  32. Ævar Arnfjörð BjarmasonOct 18, 2022
  33. Victoria DyeSep 8, 2022
  34. Victoria DyeSep 8, 2022
  35. Junio C HamanoSep 8, 2022
  36. Johannes SchindelinOct 18, 2022
  37. 5/5 cmake: increase time-out for a long-running testJohannes Schindelin via GitGitGadget, Aug 23, 2022
  38. Ævar Arnfjörð BjarmasonSep 8, 2022
  39. Victoria DyeSep 8, 2022
  40. 2/5 cmake: copy the merge tools for testingJohannes Schindelin via GitGitGadget, Aug 23, 2022
  41. 1/5 cmake: make it easier to diagnose regressions in CTest runsJohannes Schindelin via GitGitGadget, Aug 23, 2022
  42. Victoria DyeSep 7, 2022
  43. Johannes SchindelinOct 18, 2022
  44. Ævar Arnfjörð BjarmasonSep 8, 2022
  45. Eric SunshineSep 28, 2022
  46. 3/5 add -p: avoid ambiguous signed/unsigned comparisonJohannes Schindelin via GitGitGadget, Aug 23, 2022
  47. Victoria DyeSep 8, 2022
  48. 0/5 Some fixes and an improvement for using CTest on WindowsJohannes Schindelin via GitGitGadget, Oct 18, 2022
  49. 1/5 cmake: make it easier to diagnose regressions in CTest runsJohannes Schindelin via GitGitGadget, Oct 18, 2022
  50. Ævar Arnfjörð BjarmasonOct 18, 2022
  51. 2/5 cmake: copy the merge tools for testingJohannes Schindelin via GitGitGadget, Oct 18, 2022
  52. Ævar Arnfjörð BjarmasonOct 18, 2022
  53. 3/5 add -p: avoid ambiguous signed/unsigned comparisonJohannes Schindelin via GitGitGadget, Oct 18, 2022
  54. Ævar Arnfjörð BjarmasonOct 18, 2022
  55. 4/5 cmake: avoid editing t/test-lib.shJohannes Schindelin via GitGitGadget, Oct 18, 2022
  56. Ævar Arnfjörð BjarmasonOct 18, 2022
  57. Johannes SchindelinOct 18, 2022
  58. 5/5 cmake: increase time-out for a long-running testJohannes Schindelin via GitGitGadget, Oct 18, 2022

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.