From 2ea1d8b55634f538ae87e9d431fe608246ba60e9 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Tue, 18 Oct 2022 10:59:01 +0000 Subject: [PATCH 1/5] 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 Signed-off-by: Junio C Hamano --- 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 1b23f2440d..4aee1e2434 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 6a83b5f081086d3a3d1c3cb8b6a8aad660b705ab Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Tue, 18 Oct 2022 10:59:02 +0000 Subject: [PATCH 2/5] 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 Signed-off-by: Junio C Hamano --- 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 4aee1e2434..fe606c179f 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 79d266223ab7c253c45ded9552694ca8a4b1f010 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Tue, 18 Oct 2022 10:59:03 +0000 Subject: [PATCH 3/5] 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 Signed-off-by: Junio C Hamano --- add-patch.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/add-patch.c b/add-patch.c index 509ca04456..3524555e2b 100644 --- a/add-patch.c +++ b/add-patch.c @@ -1547,7 +1547,7 @@ soft_increment: 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 ee9e66e4e762075f882ec18ed51270099723cfc9 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Tue, 18 Oct 2022 10:59:04 +0000 Subject: [PATCH 4/5] 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 Signed-off-by: Junio C Hamano --- .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 a452215764..b72ddf0934 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 04d0fd1fe6..9347ed90da 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 fe606c179f..29d7e236ae 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 8cabb4d10f..a26a2c6147 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 c858750b41ca9ea1c614dcc8d0c50f702d1e13b4 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Tue, 18 Oct 2022 10:59:05 +0000 Subject: [PATCH 5/5] 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 Signed-off-by: Junio C Hamano --- contrib/buildsystems/CMakeLists.txt | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt index 29d7e236ae..b1306f9525 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