From cb4fab684c7a7ab5e06a26438b4220285e848942 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Tue, 9 Aug 2022 21:50:20 +0200 Subject: [PATCH 1/7] cmake: make it easier to diagnose regressions in CTest runs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When a test script fails in Git's test suite, the usual course of action is to re-run it using options to increase the verbosity of the output, e.g. `-v` and `-x`. Like in Git's CI runs, when running the tests in Visual Studio via the CTest route, it is cumbersome or at least requires a very unintuitive approach to pass options to the test scripts: the CMakeLists.txt file would have to be modified, passing the desired options to _all_ test scripts, and then the CMake Cache would have to be reconfigured before running the test in question individually. Unintuitive at best, and opposite to the niceties IDE users expect. So let's just pass those options by default: This will not clutter any output window but the log that is written to a log file will have information necessary to figure out test failures. While at it, also imitate what the Windows jobs in Git's CI runs do to accelerate running the test scripts: pass the `--no-bin-wrappers` and `--no-chain-lint` options. This makes the test runs noticeably faster because the `bin-wrappers/` scripts as well as the `chain-lint` code make heavy use of POSIX shell scripting, which is really, really slow on Windows due to the need to emulate POSIX behavior via the MSYS2 runtime. In a test by Eric Sunshine, it added two minutes (!) just to perform the chain-lint task. The idea of adding a CMake config option (รก la `GIT_TEST_OPTS`) was considered during the development of this patch, but then dropped: such a setting is global, across _all_ tests, where e.g. `--run=...` would not make sense. Users wishing to override these new defaults are better advised running the test script manually, in a Git Bash, with full control over the command line. Signed-off-by: Johannes Schindelin --- contrib/buildsystems/CMakeLists.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt index 1b23f2440d8e45..4aee1e24342d9a 100644 --- a/contrib/buildsystems/CMakeLists.txt +++ b/contrib/buildsystems/CMakeLists.txt @@ -1088,7 +1088,7 @@ 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 ${SH_EXE} ${tsh} --no-bin-wrappers --no-chain-lint -vx WORKING_DIRECTORY ${CMAKE_SOURCE_DIR}/t) endforeach() From cf3fb084601af239ad58d2607aa054a8eee308db Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Tue, 9 Aug 2022 21:54:07 +0200 Subject: [PATCH 2/7] cmake: copy the merge tools for testing Even when running the tests via CTest, t7609 and t7610 rely on more than only a few mergetools to be copied to the build directory. Let's make it so. Signed-off-by: Johannes Schindelin --- contrib/buildsystems/CMakeLists.txt | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt index 4aee1e24342d9a..fe606c179f7db5 100644 --- a/contrib/buildsystems/CMakeLists.txt +++ b/contrib/buildsystems/CMakeLists.txt @@ -1078,7 +1078,8 @@ if(NOT ${CMAKE_BINARY_DIR}/CMakeCache.txt STREQUAL ${CACHE_PATH}) #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/) - file(COPY ${CMAKE_SOURCE_DIR}/mergetools/tkdiff DESTINATION ${CMAKE_BINARY_DIR}/mergetools/) + file(GLOB mergetools "${CMAKE_SOURCE_DIR}/mergetools/*") + file(COPY ${mergetools} DESTINATION ${CMAKE_BINARY_DIR}/mergetools/) file(COPY ${CMAKE_SOURCE_DIR}/contrib/completion/git-prompt.sh DESTINATION ${CMAKE_BINARY_DIR}/contrib/completion/) file(COPY ${CMAKE_SOURCE_DIR}/contrib/completion/git-completion.bash DESTINATION ${CMAKE_BINARY_DIR}/contrib/completion/) endif() From 21d9f41054fedb797a4788e3e62ebb92f13e38c8 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Tue, 9 Aug 2022 22:15:53 +0200 Subject: [PATCH 3/7] add -p: avoid ambiguous signed/unsigned comparison In the interactive `add` operation, users can choose to jump to specific hunks, and Git will present the hunk list in that case. To avoid showing too many lines at once, only a maximum of 21 hunks are shown, skipping the "mode change" pseudo hunk. The comparison performed to skip the "mode change" pseudo hunk (if any) compares a signed integer `i` to the unsigned value `mode_change` (which can be 0 or 1 because it is a 1-bit type). According to section 6.3.1.8 of the C99 standard (see e.g. https://www.open-std.org/jtc1/sc22/WG14/www/docs/n1256.pdf), what should happen is an automatic conversion of the "lesser" type to the "greater" type, but since the types differ in signedness, it is ill-defined what is the correct "usual arithmetic conversion". Which means that Visual C's behavior can (and does) differ from GCC's: When compiling Git using the latter, `add -p`'s `goto` command shows no hunks by default because it casts a negative start offset to a pretty large unsigned value, breaking the "goto hunk" test case in `t3701-add-interactive.sh`. Let's avoid that by converting the unsigned bit explicitly to a signed integer. Note: This is a long-standing bug in the Visual C build of Git, but it has never been caught because t3701 is skipped when `NO_PERL` is set, which is the case in the `vs-test` jobs of Git's CI runs. Signed-off-by: Johannes Schindelin --- add-patch.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/add-patch.c b/add-patch.c index 509ca04456bd39..3524555e2b034b 100644 --- a/add-patch.c +++ b/add-patch.c @@ -1547,7 +1547,7 @@ static int patch_update_file(struct add_p_state *s, strbuf_remove(&s->answer, 0, 1); strbuf_trim(&s->answer); i = hunk_index - DISPLAY_HUNKS_LINES / 2; - if (i < file_diff->mode_change) + if (i < (int)file_diff->mode_change) i = file_diff->mode_change; while (s->answer.len == 0) { i = display_hunks(s, file_diff, i); From 856472a7e424bb034b68b1ef0eb98765ae861c96 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Fri, 28 Oct 2022 15:22:13 +0200 Subject: [PATCH 4/7] cmake: avoid editing t/test-lib.sh 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). Signed-off-by: Johannes Schindelin --- .gitignore | 1 + Makefile | 1 + contrib/buildsystems/CMakeLists.txt | 7 +------ t/test-lib.sh | 10 ++++++++++ 4 files changed, 13 insertions(+), 6 deletions(-) diff --git a/.gitignore b/.gitignore index a45221576418e7..b72ddf093469bb 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 04d0fd1fe60702..9347ed90da7f5a 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 fe606c179f7db5..29d7e236ae1eb7 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 55857af601b43f..7b57f55c376893 100644 --- a/t/test-lib.sh +++ b/t/test-lib.sh @@ -47,6 +47,16 @@ 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 + # 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 # Prepend a string to a VAR using an arbitrary ":" delimiter, not # adding the delimiter if VAR or VALUE is empty. I.e. a generalized: From d4d90265537a503b2d39c8f2a7b31dc0334ba461 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Mon, 22 Aug 2022 10:48:27 +0200 Subject: [PATCH 5/7] cmake: increase time-out for a long-running test As suggested in https://github.com/git-for-windows/git/issues/3966#issuecomment-1221264238, t7112 can run for well over one hour, which seems to be the default maximum run time at least when running CTest-based tests in Visual Studio. Let's increase the time-out as a stop gap to unblock developers wishing to run Git's test suite in Visual Studio. Note: The actual run time is highly dependent on the circumstances. For example, in Git's CI runs, the Windows-based tests typically take a bit over 5 minutes to run. CI runs have the added benefit that Windows Defender (the common anti-malware scanner on Windows) is turned off, something many developers are not at liberty to do on their work stations. When Defender is turned on, even on this developer's high-end Ryzen system, t7112 takes over 15 minutes to run. Signed-off-by: Johannes Schindelin --- contrib/buildsystems/CMakeLists.txt | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt index 29d7e236ae1eb7..b1306f95256857 100644 --- a/contrib/buildsystems/CMakeLists.txt +++ b/contrib/buildsystems/CMakeLists.txt @@ -1088,4 +1088,8 @@ foreach(tsh ${test_scipts}) WORKING_DIRECTORY ${CMAKE_SOURCE_DIR}/t) endforeach() +# This test script takes an extremely long time and is known to time out even +# on fast machines because it requires in excess of one hour to run +set_tests_properties("${CMAKE_SOURCE_DIR}/t/t7112-reset-submodule.sh" PROPERTIES TIMEOUT 4000) + endif()#BUILD_TESTING From 0b44f03d119d4a5597e5d048501a32336180a0ba Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Wed, 10 Aug 2022 14:58:10 +0200 Subject: [PATCH 6/7] cmake: avoid editing t/test-lib.sh 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). Signed-off-by: Johannes Schindelin --- t/test-lib.sh | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/t/test-lib.sh b/t/test-lib.sh index 7b57f55c376893..84329eefb0db14 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 From 474668ca9d683646ea8beac7630628c734270229 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Tue, 9 Aug 2022 21:55:26 +0200 Subject: [PATCH 7/7] fixup! mingw: allow `git.exe` to be used instead of the "Git wrapper" When building Git via CMake on Windows, the dynamic libraries (`.dll` files) on which Git's executables depend are copied into the build directory, because the `.exe` files would not even load otherwise. The "'MSYSTEM/PATH is adjusted if necessary'" test case of `t0060-path-utils.sh` wants to copy said `.exe` files, though, and expects them to run. Therefore, we need to copy the `.dll` files, too, if there are any. Signed-off-by: Johannes Schindelin --- t/t0060-path-utils.sh | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh index a62c8ab1efc255..48433abca1cd98 100755 --- a/t/t0060-path-utils.sh +++ b/t/t0060-path-utils.sh @@ -566,6 +566,14 @@ test_expect_success MINGW 'MSYSTEM/PATH is adjusted if necessary' ' pretend/mingw64/libexec/git-core pretend/usr/bin && cp "$GIT_EXEC_PATH"/git.exe pretend/mingw64/bin/ && cp "$GIT_EXEC_PATH"/git.exe pretend/mingw64/libexec/git-core/ && + # copy the .dll files, if any (happens when building via CMake) + case "$GIT_EXEC_PATH"/*.dll in + */"*.dll") ;; # no `.dll` files to be copied + *) + cp "$GIT_EXEC_PATH"/*.dll pretend/mingw64/bin/ && + cp "$GIT_EXEC_PATH"/*.dll pretend/mingw64/libexec/git-core/ + ;; + esac && echo "env | grep MSYSTEM=" | write_script "$HOME"/bin/git-test-home && echo "echo mingw64" | write_script pretend/mingw64/bin/git-test-bin && echo "echo usr" | write_script pretend/usr/bin/git-test-bin2 &&