Some fixes and an improvement for using CTest on Windows
cover Some fixes and an improvement for using CTest on Windows
As part of the CMake definition, Git enjoy support for running the test suite via CTest.
In https://github.com/git-for-windows/git/issues/3966, it has been reported that this does not work out of the box, though, but causes a couple of test failures instead. These problems are not caught by Git's CI runs because the vs-tests jobs actually use prove to run the test suite, not CTest.
In addition to fixing these problems, this patch series also addresses a long-standing gripe I have with the way Git's CMake definition supports
CTest: It edits t/test-lib.sh, which leaves this file eternally modified (but these modification should never be committed, they refer to a local-only, configuration-dependent directory).
Johannes Schindelin (5):
cmake: align CTest definition with Git's CI runs
cmake: copy the merge tools for testing
tests: explicitly skip chmod calls on Windows add -p: avoid ambiguous signed/unsigned comparison
cmake: avoid editing t/test-lib.sh
.gitignore | 1 + Makefile | 1 + add-patch.c | 2 +- contrib/buildsystems/CMakeLists.txt | 12 ++++-------- t/test-lib-functions.sh | 10 ++++++++-- t/test-lib.sh | 11 ++++++++++- 6 files changed, 25 insertions(+), 12 deletions(-)
base-commit: bbea4dcf42b28eb7ce64a6306cdde875ae5d09ca
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1320%2Fdscho%2Fctest-on-windows-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1320/dscho/ctest-on-windows-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1320
Signature · 1 line
gitgitgadget
PATCH 1/5 cmake: align CTest definition with Git's CI runs
From: Johannes Schindelin <[email protected]>
In Git's CI runs, the Windows tests are run with --no-bin-wrappers and --no-chain-lint, mainly to win back some time caused by the serious performance penalty paid for the tests relying so heavily on POSIX shell scripting, which only works by using a POSIX emulation layer.
Let's do the same when running the tests, say, in Visual Studio.
While at it, enable the command trace via -x and verbose output via -v, otherwise it would be near impossible to diagnose any problems.
--- contrib/buildsystems/CMakeLists.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
contrib/buildsystems/CMakeLists.txt +1 -1
@@ -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()
Signature · 1 line
gitgitgadget
Re: [PATCH 1/5] cmake: align CTest definition with Git's CI runs
"Johannes Schindelin via GitGitGadget" <[email protected]>
Quoted 2 lines · writes:
While at it, enable the command trace via
-xand verbose output via-v, otherwise it would be near impossible to diagnose any problems.
This sounds like a completely unrelated change. Do the tests behave so differently here, compared to how they are run in t/Makefile, where -vx is not the default?
The only plausible reason I can think of that this change "while at it" is justifiable as part of the other change is if this is only ever used in the CI environment and no interactive human would be sitting in front of the session that runs tests here. If that is the case, I do agree it would make sense, since it would be much less convenient to go back to a failing test and rerun it with -v or -x or whatever. But that needs to be explained in the proposed log message.
Thanks.
Quoted 17 lines
Signed-off-by Johannes Schindelin <[email protected]>--- contrib/buildsystems/CMakeLists.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)contrib/buildsystems/CMakeLists.txt +1
-1@@ -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()
Re: [PATCH 1/5] cmake: align CTest definition with Git's CI runs
Hi Junio,
Quoted 7 lines · Junio C Hamano
"Johannes Schindelin via GitGitGadget" <[email protected]>
Quoted 2 lines · writes:
While at it, enable the command trace via
-xand verbose output via-v, otherwise it would be near impossible to diagnose any problems.This sounds like a completely unrelated change.
It may sound like it, but it is not. In order to make sense of the broken tests, I needed access to more verbose output than our test scripts provide by default.
When running the test suite on the command-line, it is easy to tell the user "oh, if you need more information, just call the test script with these here options: ...".
This is not an option when running the tests within Visual Studio.
Does this clarify the intention and validity of the proposed patch? If so, I will gladly try my best to improve the commit message to explain that intention better.
Ciao,
Dscho
Re: [PATCH 1/5] cmake: align CTest definition with Git's CI runs
Quoted 16 lines · Johannes Schindelin
Hi Junio,
Quoted 7 lines · Junio C Hamano
"Johannes Schindelin via GitGitGadget" <[email protected]>
Quoted 2 lines · writes:
While at it, enable the command trace via
-xand verbose output via-v, otherwise it would be near impossible to diagnose any problems.This sounds like a completely unrelated change.
It may sound like it, but it is not. In order to make sense of the broken tests, I needed access to more verbose output than our test scripts provide by default.
Quoted 5 lines
When running the test suite on the command-line, it is easy to tell the user "oh, if you need more information, just call the test script with these here options: ...".
This is not an option when running the tests within Visual Studio.
I gave an example of "CI environment it is cumbersome to go back and run only a single one" in the review you are responding to that may explain why such a change is needed, and you gave us exactly the context that was lacking here. The environment does not let users run the tests as anything but a single monolithic ball of wax.
Quoted 3 lines
Does this clarify the intention and validity of the proposed patch? If so, I will gladly try my best to improve the commit message to explain that intention better.
It explains why -x -v is needed and needs to be a part of VS+CMake topic, and it will help readers to have it in the explanation. It still does not justify why it has to be a part of a step to omit bin-warppers and skip chain-lint that has nothing to do it, though.
Thanks.
Re: [PATCH 1/5] cmake: align CTest definition with Git's CI runs
Hi Junio,
Quoted 20 lines · Junio C Hamano
Quoted 18 lines · Johannes Schindelin
Quoted 7 lines · Junio C Hamano
"Johannes Schindelin via GitGitGadget" <[email protected]>
Quoted 2 lines · writes:
While at it, enable the command trace via
-xand verbose output via-v, otherwise it would be near impossible to diagnose any problems.This sounds like a completely unrelated change.
It may sound like it, but it is not. In order to make sense of the broken tests, I needed access to more verbose output than our test scripts provide by default.
When running the test suite on the command-line, it is easy to tell the user "oh, if you need more information, just call the test script with these here options: ...".
Quoted 7 lines
Quoted 1 line
This is not an option when running the tests within Visual Studio.
I gave an example of "CI environment it is cumbersome to go back and run only a single one" in the review you are responding to that may explain why such a change is needed, and you gave us exactly the context that was lacking here. The environment does not let users run the tests as anything but a single monolithic ball of wax.
Good, I will amend the commit message.
Quoted 8 lines
Quoted 3 lines
Does this clarify the intention and validity of the proposed patch? If so, I will gladly try my best to improve the commit message to explain that intention better.
It explains why -x -v is needed and needs to be a part of VS+CMake topic, and it will help readers to have it in the explanation. It still does not justify why it has to be a part of a step to omit bin-warppers and skip chain-lint that has nothing to do it, though.
Oh, that ;-)
To be honest, I reflexively do that on Windows because it saves _so much time_.
I will amend the commit message to clarify that, too.
Ciao,
Dscho
Re: [PATCH 1/5] cmake: align CTest definition with Git's CI runs
In reply to Johannes Schindelin via GitGitGadgetQuoted 22 lines · Johannes Schindelin via GitGitGadget
From: Johannes Schindelin <[email protected]>
In Git's CI runs, the Windows tests are run with
--no-bin-wrappersand--no-chain-lint, mainly to win back some time caused by the serious performance penalty paid for the tests relying so heavily on POSIX shell scripting, which only works by using a POSIX emulation layer.Let's do the same when running the tests, say, in Visual Studio.
While at it, enable the command trace via
-xand verbose output via-v, otherwise it would be near impossible to diagnose any problems.Signed-off-by Johannes Schindelin <[email protected]>--- contrib/buildsystems/CMakeLists.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)contrib/buildsystems/CMakeLists.txt +0
-0
contrib/buildsystems/CMakeLists.txt file(GLOB test_scipts "${CMAKE_SOURCE_DIR}/t/t[0-9]*.sh") · line 1093
Quoted 7 lines
contrib/buildsystems/CMakeLists.txt +1
-1@@ -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()
Is this authored before a561962479c (cmake: fix CMakeLists.txt on Linux, 2022-05-24) was merged?
The "say, in Visual Studio" seems to elide that we'll now run these tests differently when you run with cmake everywhere.
It seems much better to pass some "test arguments" to cmake itself, which we'd then call from the ci specifically. Then e.g. a Linux user of cmake wouldn't wonder why the "make test" spots e.g. a chain-lint issue that the cmake testing would hide.
PATCH 2/5 cmake: copy the merge tools for testing
In reply to Johannes Schindelin via GitGitGadgetFrom: Johannes Schindelin <[email protected]>
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.
--- contrib/buildsystems/CMakeLists.txt | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
contrib/buildsystems/CMakeLists.txt +2 -1
@@ -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()
Signature · 1 line
gitgitgadget
PATCH 3/5 tests: explicitly skip `chmod` calls on Windows
In reply to Johannes Schindelin via GitGitGadgetFrom: Johannes Schindelin <[email protected]>
On Windows, we use a POSIX emulation layer, the MSYS2 runtime, to allow for running shell scripts (albeit at a hefty performance cost, Git's test suite takes ~700 seconds to complete on Linux, according to Git's CI runs, while it takes more than 6,000 seconds on Windows).
This emulation layer has a funny quirk when it comes to chmod
invocations: it pretends that it succeeded, when in reality it did not do a thing (because the Access Control Lists used in Windows' permission model are so different to Unix' default permission model that Git's test suite assumes to be in effect).
Git's test suite relies on this quirk by assuming that the chmod calls in test_chmod and test_write_script simply succeed on Windows (without actually doing anything).
However, this quirk is only in effect as long as chmod is run inside the pseudo Unix root directory structure or within the home directory. When run outside, such invocations fail like this:
chmod: changing permissions of '<file>': Invalid argument
Now, when running Git's tests in, say, Visual Studio, we frequently are in a worktree where the chmod invocations would fail.
Let's accommodate for that by explicitly skipping those chmod invocations on Windows.
--- t/test-lib-functions.sh | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)
t/test-lib-functions.sh +8 -2
@@ -492,7 +492,10 @@ test_commit_bulk () { # of a file in the working directory and add it to the index. test_chmod () { - chmod "$@" && + if test_have_prereq !MINGW + then + chmod "$@" + fi && git update-index --add "--chmod=$@" }
@@ -548,7 +551,10 @@ write_script () { echo "#!${2-"$SHELL_PATH"}" && cat } >"$1" && - chmod +x "$1" + if test_have_prereq !MINGW + then + chmod +x "$1" + fi } # Usage: test_hook [options] <hook-name> <<-\EOF
Signature · 1 line
gitgitgadget
Re: [PATCH 3/5] tests: explicitly skip `chmod` calls on Windows
Quoted 7 lines · Johannes Schindelin via GitGitGadget
From: Johannes Schindelin <[email protected]> [...]
However, this quirk is only in effect as long aschmodis run inside the pseudo Unix root directory structure or within the home directory. When run outside, such invocations fail like this:chmod: changing permissions of '<file>': Invalid argument
..ok, but...
Quoted 17 lines
t/test-lib-functions.sh +4
-1@@ -492,7 +492,10 @@ test_commit_bulk () { # of a file in the working directory and add it to the index. test_chmod () { - chmod "$@" && + if test_have_prereq !MINGW + then + chmod "$@" + fi && git update-index --add "--chmod=$@" }
t/test-lib-functions.sh write_script () { · line 557
Quoted 8 lines
t/test-lib-functions.sh +4
-1@@ -548,7 +551,10 @@ write_script () { echo "#!${2-"$SHELL_PATH"}" && cat } >"$1" && - chmod +x "$1" + if test_have_prereq !MINGW + then + chmod +x "$1" + fi
... you get +x semantics by default, so we didn't need that "chmod +x" in the first place?
The rest of "test_chmod" seems to *happen to* pass +x or -x, but we don't care about that, regardless of the "pseudo Unix root directory"?
What if we get a "test_chmod -o <file>", won't this silently do the wrong thing?
If so isn't something in this direction (untested) a more targeted & obvious fix?:
t/test-lib.sh +10 -0
@@ -1690,6 +1690,16 @@ case $uname_s in find () { /usr/bin/find "$@" } + chmod () { + case "$1" in + +x|-x) + return; + ;; + *) + ;; + esac && + /usr/bin/chmod "$@" + } # git sees Windows-style pwd pwd () { builtin pwd -W
Re: [PATCH 3/5] tests: explicitly skip `chmod` calls on Windows
Hi Ævar,
Quoted 28 lines · Ævar Arnfjörð Bjarmason
Quoted 7 lines · Johannes Schindelin via GitGitGadget
From: Johannes Schindelin <[email protected]> [...]
However, this quirk is only in effect as long aschmodis run inside the pseudo Unix root directory structure or within the home directory. When run outside, such invocations fail like this:chmod: changing permissions of '<file>': Invalid argument
..ok, but...
Quoted 16 lines
t/test-lib-functions.sh +4
-1@@ -492,7 +492,10 @@ test_commit_bulk () { # of a file in the working directory and add it to the index. test_chmod () { - chmod "$@" && + if test_have_prereq !MINGW + then + chmod "$@" + fi && git update-index --add "--chmod=$@" }
Quoted 12 lines
t/test-lib-functions.sh write_script () { · line 557
Quoted 9 lines
t/test-lib-functions.sh +4
-1@@ -548,7 +551,10 @@ write_script () { echo "#!${2-"$SHELL_PATH"}" && cat } >"$1" && - chmod +x "$1" + if test_have_prereq !MINGW + then + chmod +x "$1" + fi... you get +x semantics by default, so we didn't need that "chmod +x" in the first place?
No. We do not get that chmod +x semantics by default. Those chmod +x statements are treated as (expensive) no-ops by default. This is what I meant when I said this in the commit message (that is missing from the quoted text above):
[...] it pretends that it succeeded, when in reality it did not do a thing [...]
I do not know how to say this more clearly.
Quoted 2 lines
The rest of "
test_chmod" seems to *happen to* pass +x or -x, but we don't care about that, regardless of the "pseudo Unix root directory"?
The rest of "test_chmod" is even quoted above, so we do not need to leave anybody guessing as to what it does:
git update-index --add "--chmod=$@"
This asks Git to update the index with the executable bit explicitly turned on or off, regardless of the information that is available on disk.
And yes, Git does what we expect it to do here.
Quoted 2 lines
What if we get a "
test_chmod-o <file>", won't this silently do the wrong thing?
But we don't?
The code under discussion is Git's test suite, after all, not something that every Git user is expected to use in more ways than the core Git developers can imagine.
So even a cursory git grep test_chmod upstream/seen could have shown that there is no such user, and if this was supposed to be a "But what if we do that at some stage in the future?" feedback, said feedback could be construed as to intentionally use up valuable contributor time.
Quoted 16 lines
If so isn't something in this direction (untested) a more targeted & obvious fix?:
t/test-lib.sh +3
-0@@ -1690,6 +1690,16 @@ case $uname_s in find () { /usr/bin/find "$@" } + chmod () { + case "$1" in + +x|-x)
t/test-lib.sh case $uname_s in · line 1705
Quoted 10 lines
t/test-lib.sh +7
-0+ return; + ;; + *) + ;; + esac && + /usr/bin/chmod "$@" + } # git sees Windows-style pwd pwd () { builtin pwd -W
In that form, I will reject this suggestion as over-engineered and convoluted. Why is the *) clause empty? Why is there an early return guarding a single statement? Why is the /usr/bin/chmod call not in the *) clause to begin with? Why does this code take pains to handle cases other than -x and +x when we do not have any callers, and even in the experimental patches, there are no such users in sight? I would like to encourage you to address such issues during review before even sending the mail in the future.
Having said that, there is a nugget in this feedback that I find valuable. Instead of wasting the run time (even on non-Windows platforms!) to determine whether the MINGW prereq is set in order to skip the chmod call or not, we can make chmod a no-op explicitly in that case $uname_s block.
I will make it so.
Ciao,
Johannes
Re: [PATCH 3/5] tests: explicitly skip `chmod` calls on Windows
Hi Ævar,
Quoted 6 lines · Johannes Schindelin
Instead of wasting the run time (even on non-Windows platforms!) to determine whether the
MINGWprereq is set in order to skip thechmodcall or not, we can makechmoda no-op explicitly in thatcase $uname_sblock.I will make it so.
tl;dr this patch needs to be dropped, without the suggested replacement.
Gaaah!
After struggling with this for much longer than I care to admit, and even debugging inside the MSYS2 runtime (which is a level boss if there ever was one), I found out that all about this patch was wrong.
The suggested patch (making chmod a no-op) was wrong: the test suite started failing left and right. Why? Because chmod is not a _complete_ no-op. The test suite does not use the -o flag you suggested (which would not make sense in Git's test suite, whether or not you remove file permissions for "others" than the current user or group, Git behaves identically), but it _does_ use the -w/+w flags, and those _are_ respected, even on Windows.
The way the write permission bits are translated to ACLs is admittedly somewhat magical or all kinds of wrong, too, depending on your point of view.
But my original patch was also wrong. Why? Because it claimed that chmod +x does not work outside of MSYS2's pseudo Unix root directory tree. And that's simply not true.
Whether you call chmod +x C:/Users/avar/my-file.txt or chmod +x /tmp/avar.sh, it "succeeds" (by silently ignoring the flag that is inapplicable on Windows, whether or not a file is executable is determined by its file extension, and yes, that means that shell scripts are never executable and we have to live with a nasty hack in Git to pretend that they are, based on their contents starting with a hash-bang line).
So why did I claim that the chmod +x invocation does not work outside of that pseudo Unix root directory tree? Because it actually did not work, at least for me! But it worked on the build agents. Why? Because I have a command in my ~/.profile that mounts several of my Git for Windows SDKs as "short-cuts" like /sdk64, /sdk32 and the likes (think of these as bind-mounts). And those mount commands did not specify the noacl flag.
So what's that noacl flag? It is magic, I can tell you. It basically makes all this pretense work where MSYS2 pretends that we are in a Unix environment with Unix permissions when we're not actually. The details are too gnarly and involved to explain, I won't write them up here in order to avoid putting even more readers to sleep than I must have done already.
So my analysis was based on observation instead of inspection, cutting corners, and I dearly regret that now. By default, MSYS2 "mounts" the system drives as /c, /d, etc with that noacl flag, the same as it mounts the pseudo Unix root as /. And that is why the Git test suite does not throw up every time chmod +x is called. And it was a simple pilot error on my part that caused it to fail on my system, and I did not realize that the problem was confined to my system, and the bug was in my ~/.profile and not in Git's code base.
Working on Git for Windows is never boring.
Ciao,
Johannes
PATCH 4/5 add -p: avoid ambiguous signed/unsigned comparison
In reply to Johannes Schindelin via GitGitGadgetFrom: Johannes Schindelin <[email protected]>
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.
--- add-patch.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
add-patch.c +1 -1
@@ -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);
Signature · 1 line
gitgitgadget
Re: [PATCH 4/5] add -p: avoid ambiguous signed/unsigned comparison
"Johannes Schindelin via GitGitGadget" <[email protected]>
Quoted 23 lines · writes:
From: Johannes Schindelin <[email protected]>
In the interactive
addoperation, 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
ito the unsigned valuemode_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'sgotocommand shows no hunks by default because it casts a negative start offset to a pretty large unsigned value, breaking the "goto hunk" test case int3701-add-interactive.sh.
Quoted 5 lines
Let's avoid that by converting the unsigned bit explicitly to a signed integer.
Signed-off-by Johannes Schindelin <[email protected]>---
This looks more like a fix to a general problem, not limited to windows or cmake, that we had since 9254bdfb (built-in add -p:
implement the 'g' ("goto") command, 2019-12-13).
Please pull this out of the series and let's have it reviewed separately.
Thanks.
Quoted 16 lines
add-patch.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)add-patch.c +1
-1@@ -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);
Re: [PATCH 4/5] add -p: avoid ambiguous signed/unsigned comparison
Hi Junio,
Quoted 32 lines · Junio C Hamano
"Johannes Schindelin via GitGitGadget" <[email protected]>
Quoted 23 lines · writes:
From: Johannes Schindelin <[email protected]>
In the interactive
addoperation, 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
ito the unsigned valuemode_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'sgotocommand shows no hunks by default because it casts a negative start offset to a pretty large unsigned value, breaking the "goto hunk" test case int3701-add-interactive.sh.Quoted 5 lines
Let's avoid that by converting the unsigned bit explicitly to a signed integer.
Signed-off-by Johannes Schindelin <[email protected]>---
Quoted 6 lines
This looks more like a fix to a general problem, not limited to windows or cmake, that we had since
9254bdfb(built-in add -p:
implement the 'g' ("goto") command, 2019-12-13).Please pull this out of the series and let's have it reviewed separately.
The scope of this patch series is to fix running the tests in Visual Studio when building using CMake.
Pulling out this patch would break that patch series because it would leave that breakage in place.
Except if you are asking to put this patch series on the back burner and prioritize the patch that fixes an ambiguous implicit cast between signed and unsigned data types?
However, that would mean that I'd now have to address all of those implicit casts, which is unfortunately a larger amount of work than I can justify to take on.
Therefore I move that in this instance, the perfect is the enemy of the good, and that the patch should remain within this patch series, even if the larger-scoped project to avoid any implicit signed/unsigned casts remains unaddressed.
BTW I would have expected your review to ask the (in hindsight, obvious) question why the test suite still passes even with vs-test exercising the code that is compiled using Visual C?
The answer to that would have been that the vs-test job of our CI runs defines NO_PERL, and t3701 is skipped completely if that is the case.
Ciao,
Dscho
Re: [PATCH 4/5] add -p: avoid ambiguous signed/unsigned comparison
Quoted 2 lines · Johannes Schindelin
The scope of this patch series is to fix running the tests in Visual Studio when building using CMake.
That's only the perspective of who cares about VS+CMake. From my point of view who wants a healthy Git overall, the priority is different. "add -p" fix is wider than the context of VS, and I do not discount the need that we need to fix it before we can call VS+CMake issue fixed (and I do not mean to say it is unnecessary to fix VS+CMake). It's just this one can proceed with help by those who do not care about or have no environment to help with VS+CMake because it is more generic, and I do not think you mind to make the rest of the narrower VS+CMake topic _depend_ on it.
Quoted 2 lines
Pulling out this patch would break that patch series because it would leave that breakage in place.
We deal with topic that depends on another topic all the time, and I do not think there is anything that makes VS+CMake topic so special that it cannot be dependent on another topic. It's just the matter of splitting this out and make it [1/1], and make the remainder to [1-4/4] and mark them rely on add-p fix when you send the latter out, isn't it? Puzzled...
Thanks.
Re: [PATCH 4/5] add -p: avoid ambiguous signed/unsigned comparison
Hi Junio,
Quoted 5 lines · Junio C Hamano
Quoted 2 lines · Johannes Schindelin
The scope of this patch series is to fix running the tests in Visual Studio when building using CMake.
Quoted 9 lines
That's only the perspective of who cares about VS+CMake. From my point of view who wants a healthy Git overall, the priority is different. "add -p" fix is wider than the context of VS, and I do not discount the need that we need to fix it before we can call VS+CMake issue fixed (and I do not mean to say it is unnecessary to fix VS+CMake). It's just this one can proceed with help by those who do not care about or have no environment to help with VS+CMake because it is more generic, and I do not think you mind to make the rest of the narrower VS+CMake topic _depend_ on it.
Sure.
But if I pull out that patch into its code contribution, all of a sudden the scope of _that_ contribution is to address an implicit signed/unsigned cast. What other purpose could it have? And if we're addressing that signed/unsigned cast, we cannot just fix this single one. We need to fix them all, no?
So let's have a look at that project, since you are implicitly volunteering me for it. We do have this in our config.mak.dev:
# These are disabled because we have these all over the place. DEVELOPER_CFLAGS += -Wno-empty-body DEVELOPER_CFLAGS += -Wno-missing-field-initializers DEVELOPER_CFLAGS += -Wno-sign-compare DEVELOPER_CFLAGS += -Wno-unused-parameter endif
Note the -Wno-sign-compare part. If I comment that out, I get these reports:
-- snip --
diff.h:210:16: error: comparison of integer expressions of different signedness: ‘int’ and ‘long unsigned int’ [-Werror=sign-compare]
diff.h:210:16: error: comparison of integer expressions of different signedness: ‘int’ and ‘long unsigned int’ [-Werror=sign-compare]
diff.h:210:16: error: comparison of integer expressions of different signedness: ‘int’ and ‘long unsigned int’ [-Werror=sign-compare]
diff.h:210:16: error: comparison of integer expressions of different signedness: ‘int’ and ‘long unsigned int’ [-Werror=sign-compare] add-interactive.c:207:21: error: comparison of integer expressions of different signedness: ‘int’ and ‘size_t’ {aka ‘long unsigned int’} [-Werror=sign-compare] add-interactive.c:210:17: error: comparison of integer expressions of different signedness: ‘int’ and ‘size_t’ {aka ‘long unsigned int’} [-Werror=sign-compare] add-interactive.c:238:16: error: comparison of integer expressions of different signedness: ‘int’ and ‘size_t’ {aka ‘long unsigned int’} [-Werror=sign-compare] add-patch.c:423:16: error: comparison of integer expressions of different signedness: ‘size_t’ {aka ‘long unsigned int’} and ‘int’ [-Werror=sign-compare] add-interactive.c:379:25: error: comparison of integer expressions of different signedness: ‘ssize_t’ {aka ‘long int’} and ‘size_t’ {aka ‘long unsigned int’} [-Werror=sign-compare] add-interactive.c:389:11: error: comparison of integer expressions of different signedness: ‘ssize_t’ {aka ‘long int’} and ‘size_t’ {aka ‘long unsigned int’} [-Werror=sign-compare] [... 1260 more issues ...]
-- snap --
You see how that increases the amount of work you are asking me to do?
The worst part is that gcc (at least of version 9.4.0 which I have available in my Ubuntu) does not even catch the line that is fixed by the patch we are trying to discuss here. It catches 10 issues in add-patch.c, but not the i < file_diff->mode_change one.
Neither does Visual C 2022, nor the gcc I have in Git for Windows' SDK, which is v12.1.0.
So I would first need to find a tool that identifies all code locations that compare signed with unsigned values.
And that's even before analyzing carefully how to address them (not all instances will be as easy as upcasting from an unsigned bit to int, some of those instances are about size_t vs ssize_t).
Quoted 9 lines
Quoted 2 lines
Pulling out this patch would break that patch series because it would leave that breakage in place.
We deal with topic that depends on another topic all the time, and I do not think there is anything that makes VS+CMake topic so special that it cannot be dependent on another topic. It's just the matter of splitting this out and make it [1/1], and make the remainder to [1-4/4] and mark them rely on add-p fix when you send the latter out, isn't it? Puzzled...
Sure, if you think that the signed/unsigned comparison project is more important than fixing running Git's test suite inside Visual Studio, or
worse: if you are asking me to do a less than thorough job on those signed/unsigned fixes by fixing only a single instance and leaving all others unaddressed in a patch series that has nothing to do with Visual Studio, then I understand how my stance could confuse you.
But my intention with this patch series is to fix running Git's test suite in Visual Studio. And my intention is to provide a complete solution in my patch series, no half measures.
As such, I would be loathe to have my authorship on a single patch that solves neither the Visual Studio/CTest problem nor the vast majority of the signed/unsigned problems. It would be incomplete in any way you look at it. I would consider such a contribution lackluster, sloppy and definitely not up to my standards.
Ciao,
Dscho
Re: [PATCH 4/5] add -p: avoid ambiguous signed/unsigned comparison
In reply to Johannes Schindelin via GitGitGadgetHi Dscho
Quoted 17 lines · Johannes Schindelin via GitGitGadget
From: Johannes Schindelin <[email protected]>
In the interactive
addoperation, 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
ito the unsigned valuemode_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".
Quoted 8 lines
Which means that Visual C's behavior can (and does) differ from GCC's:
When compiling Git using the latter,add -p'sgotocommand shows no hunks by default because it casts a negative start offset to a pretty large unsigned value, breaking the "goto hunk" test case int3701-add-interactive.sh.Let's avoid that by converting the unsigned bit explicitly to a signed integer.
Oh the joys of bit-fields!
add-patch.c soft_increment: · line 1548
Quoted 12 lines
Signed-off-by Johannes Schindelin <[email protected]>--- add-patch.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)add-patch.c +0
-0@@ -1547,7 +1547,7 @@ soft_increment:strbuf_remove (&s->answer,0 ,1 );strbuf_trim (&s->answer);
Unrelated to this change but why don't we just call strbuf_reset() here?
add-patch.c soft_increment: · line 1550
Quoted 3 lines
add-patch.c +1
-1i = hunk_index -DISPLAY_HUNKS_LINES /2 ; -if (i < file_diff->mode_change) +if (i < (int )file_diff->mode_change)
The change itself looks good
Best Wishes
Phillip
Quoted 3 lines
add-patch.c +0
-0i = file_diff->mode_change;while (s->answer.len ==0 ) { i =display_hunks (s, file_diff, i);
Re: [PATCH 4/5] add -p: avoid ambiguous signed/unsigned comparison
Hi Phillip,
Quoted 11 lines · Phillip Wood
add-patch.c soft_increment: · line 1548
Quoted 7 lines · Johannes Schindelin via GitGitGadget
add-patch.c +0
-0@@ -1547,7 +1547,7 @@ soft_increment:strbuf_remove (&s->answer,0 ,1 );strbuf_trim (&s->answer);Unrelated to this change but why don't we just call
strbuf_reset()here?
This part of the code is used when the g command (to "go to hunk") was issued by the user. And that g command allows for a number to be specified, e.g. g1 to go to the first hunk.
The strbuf_remove(&s->answer, 0, 1) removes the g from the command.
The strbuf_trim(&s->answer) allows for whitespace between the g and the number, e.g. g 1 should also go to the first hunk.
If we called strbuf_reset() here, we would remove the number completely.
Ciao,
Dscho
Re: [PATCH 4/5] add -p: avoid ambiguous signed/unsigned comparison
Hi Dscho
Quoted 20 lines · Johannes Schindelin
Hi Phillip,
Quoted 11 lines · Phillip Wood
add-patch.c soft_increment: · line 1548
Quoted 7 lines · Johannes Schindelin via GitGitGadget
add-patch.c +0
-0@@ -1547,7 +1547,7 @@ soft_increment:strbuf_remove (&s->answer,0 ,1 );strbuf_trim (&s->answer);Unrelated to this change but why don't we just call
strbuf_reset()here?This part of the code is used when the
gcommand (to "go to hunk") was issued by the user. And thatgcommand allows for a number to be specified, e.g.g1to go to the first hunk.
Quoted 6 lines
The
strbuf_remove(&s->answer, 0, 1)removes thegfrom the command.The
strbuf_trim(&s->answer)allows for whitespace between thegand the number, e.g.g 1should also go to the first hunk.If we called
strbuf_reset()here, we would remove the number completely.
Oh so if the user is not using interactive.singleKey then they can skip the prompt which displays the hunks and asks which one they want to jump to by guessing the number of the hunk they want and typing "g <n>".
Thanks
Phillip
Quoted 2 lines
Ciao,
Dscho
Re: [PATCH 4/5] add -p: avoid ambiguous signed/unsigned comparison
Hi Phillip,
Quoted 22 lines · Phillip Wood
Quoted 20 lines · Johannes Schindelin
Quoted 11 lines · Phillip Wood
add-patch.c soft_increment: · line 1548
Quoted 7 lines · Johannes Schindelin via GitGitGadget
add-patch.c +0
-0@@ -1547,7 +1547,7 @@ soft_increment:strbuf_remove (&s->answer,0 ,1 );strbuf_trim (&s->answer);Unrelated to this change but why don't we just call
strbuf_reset()here?This part of the code is used when the
gcommand (to "go to hunk") was issued by the user. And thatgcommand allows for a number to be specified, e.g.g1to go to the first hunk.The
strbuf_remove(&s->answer, 0, 1)removes thegfrom the command.
Quoted 8 lines
Quoted 4 lines
The
strbuf_trim(&s->answer)allows for whitespace between thegand the number, e.g.g 1should also go to the first hunk.If we called
strbuf_reset()here, we would remove the number completely.Oh so if the user is not using interactive.singleKey then they can skip the prompt which displays the hunks and asks which one they want to jump to by guessing the number of the hunk they want and typing "g <n>".
Yes, precisely.
I forgot that you are using interactive.singleKey (I planned on opting into that mode, but for some reason I never get around to flip the switch). The default mode is not the single key mode, though.
Ciao,
Dscho
PATCH 5/5 cmake: avoid editing t/test-lib.sh
In reply to Johannes Schindelin via GitGitGadgetFrom: Johannes Schindelin <[email protected]>
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).
--- .gitignore | 1 + Makefile | 1 + contrib/buildsystems/CMakeLists.txt | 7 +------ t/test-lib.sh | 11 ++++++++++- 4 files changed, 13 insertions(+), 7 deletions(-)
.gitignore +1 -0
@@ -2,6 +2,7 @@ /fuzz_corpora /fuzz-pack-headers /fuzz-pack-idx +/GIT-BUILD-DIR /GIT-BUILD-OPTIONS /GIT-CFLAGS /GIT-LDFLAGS
Makefile +1 -0
@@ -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
contrib/buildsystems/CMakeLists.txt +1 -6
@@ -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/)
t/test-lib.sh +10 -1
@@ -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
Signature · 1 line
gitgitgadget
Re: [PATCH 5/5] cmake: avoid editing t/test-lib.sh
Quoted 90 lines · Johannes Schindelin via GitGitGadget
From: Johannes Schindelin <[email protected]>
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.shto overrideGIT_BUILD_DIRto point somewhere else than the parent of thet/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.shto detect the presence of a file calledGIT-BUILD-DIRin 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
makeinvocation after building it using CMake, we ensure that theGIT-BUILD-DIRfile is deleted (for convenience, this is done as part of theMakefilerule that is already run with everymakeinvocation to ensure thatGIT-BUILD-OPTIONSis up to date).Signed-off-by Johannes Schindelin <[email protected]>--- .gitignore | 1 + Makefile | 1 + contrib/buildsystems/CMakeLists.txt | 7 +------ t/test-lib.sh | 11 ++++++++++- 4 files changed, 13 insertions(+), 7 deletions(-).gitignore +1
-0@@ -2,6 +2,7 @@ /fuzz_corpora /fuzz-pack-headers /fuzz-pack-idx +/GIT-BUILD-DIR /GIT-BUILD-OPTIONS /GIT-CFLAGS /GIT-LDFLAGSMakefile +1
-0@@ -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_PYTHONcontrib/buildsystems/CMakeLists.txt +1
-6@@ -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/)t/test-lib.sh +3
-1@@ -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
t/test-lib.sh then · line 57
Quoted 10 lines
t/test-lib.sh +7
-0+ # 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
So we'll now skip the assertion added in 9dbf20e7f62 (test-lib: correct and assert TEST_DIRECTORY overriding, 2022-02-27), shouldn't these tests run inside a "t" directory generated in contrib/buildsystems/ ?
This really seems like a hack-upon-hack, and will break some workflows when you switch back & forth, e.g.:
make
# run cmake
make -C t
Will now pick up this new file, and result in broken tests, as we'll rely on the now-stale file.
IOW you're making the assumption that by piggy-backing on the GIT-BUILD-OPTIONS rule that anything that needs it will also *re-run it*, but that's not the case.
I can think of ways to get around it, but it would be nasty as we'd need to complete the dependency graph between the two, and figure out the various interactions.
But why do we need this at all? Right now I tried:
* (Manually) removing the existing hack
* Copying t/*lib*.sh over to contrib/buildsystems/t
* cd contrib/buildsystems/t
* ../../../t/t0001-init.sh
Which (aside from a small fixable nit about oid-info) Just Works, because the cmake build is already creating a GIT-BUILD-OPTIONS.
Presumably that "copying" step should be simlynking, or we'd be smarter about doing includes from test-lib.sh.
But isn't that a much better approach? Rather than working around the directory discovery just teach it to run from the generated t/ directory directly?
Re: [PATCH 5/5] cmake: avoid editing t/test-lib.sh
Hi Ævar,
Quoted 125 lines · Ævar Arnfjörð Bjarmason
Quoted 90 lines · Johannes Schindelin via GitGitGadget
From: Johannes Schindelin <[email protected]>
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.shto overrideGIT_BUILD_DIRto point somewhere else than the parent of thet/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.shto detect the presence of a file calledGIT-BUILD-DIRin 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
makeinvocation after building it using CMake, we ensure that theGIT-BUILD-DIRfile is deleted (for convenience, this is done as part of theMakefilerule that is already run with everymakeinvocation to ensure thatGIT-BUILD-OPTIONSis up to date).Signed-off-by Johannes Schindelin <[email protected]>--- .gitignore | 1 + Makefile | 1 + contrib/buildsystems/CMakeLists.txt | 7 +------ t/test-lib.sh | 11 ++++++++++- 4 files changed, 13 insertions(+), 7 deletions(-).gitignore +1
-0@@ -2,6 +2,7 @@ /fuzz_corpora /fuzz-pack-headers /fuzz-pack-idx +/GIT-BUILD-DIR /GIT-BUILD-OPTIONS /GIT-CFLAGS /GIT-LDFLAGSMakefile +1
-0@@ -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_PYTHONcontrib/buildsystems/CMakeLists.txt +1
-6@@ -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/)t/test-lib.sh +3
-1@@ -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 1t/test-lib.sh then · line 57
Quoted 10 lines
t/test-lib.sh +7
-0+ # 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 1So we'll now skip the assertion added in
9dbf20e7f62(test-lib: correct and assertTEST_DIRECTORYoverriding, 2022-02-27), shouldn't these tests run inside a "t" directory generated in contrib/buildsystems/ ?This really seems like a hack-upon-hack, and will break some workflows when you switch back & forth, e.g.:
make
# run cmake
make -C tWill now pick up this new file, and result in broken tests, as we'll rely on the now-stale file.
IOW you're making the assumption that by piggy-backing on the GIT-BUILD-OPTIONS rule that anything that needs it will also *re-run it*, but that's not the case.
I can think of ways to get around it, but it would be nasty as we'd need to complete the dependency graph between the two, and figure out the various interactions.
Quoted 6 lines
But why do we need this at all? Right now I tried:
* (Manually) removing the existing hack
* Copyingt/*lib*.shover to contrib/buildsystems/t
* cd contrib/buildsystems/t
*../../../t/t0001-init.sh
I appreciate that you want to make it easier for developers. From my experience with (Windows) developers, I have to point out that what you describe is _not_ easier than cd t && ./t0001-init.sh, though.
It might be easier from some point of view that seems vaguely familiar, but the user experience of this approach looks prohibitively bad to me.
Quoted 9 lines
Which (aside from a small fixable nit about oid-info) Just Works, because the cmake build is already creating a GIT-BUILD-OPTIONS.
Presumably that "copying" step should be simlynking, or we'd be smarter about doing includes from
test-lib.sh.But isn't that a much better approach? Rather than working around the directory discovery just teach it to run from the generated t/ directory directly?
I am sure I could poke more holes into that approach, probably whipping out something something about contrib/, but the user experience alone (so, dear developer, contrary to what you read in the README and in other documentation flying all over the internet, yes, you need to switch to contrib/buildsystems, no wait, there's a typo, there's no dash between build and systems, there you go, why buildsystems? Well, it's just the way it is. Yes. Now, type out, no, no, wait, *wait!*, you cannot run t0006-date.sh directly as documented, no, wait, you need to prefix it with this here, and now carefully count the dots, yep, that's right, six dots in total, three slashes, then a t/, yes, I know you were already in t/ but not _that_ t/, there are now two of 'em, yes, it is unintuitive, you're absolutely correct, but you know...) is enough for me to not want to consider this approach any further.
Ciao,
Johannes
Re: [PATCH 5/5] cmake: avoid editing t/test-lib.sh
In reply to Johannes Schindelin via GitGitGadgetHi Dscho
Quoted 11 lines · Johannes Schindelin via GitGitGadget
From: Johannes Schindelin <[email protected]>
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.shto overrideGIT_BUILD_DIRto point somewhere else than the parent of thet/directory.
Quoted 8 lines
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.shto detect the presence of a file calledGIT-BUILD-DIRin 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.
I think it is really good to get away from editing the test files, but one of the nice things about CMake's out of tree builds is that you can have several build directories with different build configurations and this change does not support that. Could we pass the build directory to the test scripts as a commandline option or environment variable instead? e.g.
foreach(tsh ${test_scipts})
add_test(NAME ${tsh}
- COMMAND ${SH_EXE} ${tsh} --no-bin-wrappers --no-chain-lint -vx
+ COMMAND ${SH_EXE} ${tsh} --no-bin-wrappers --no-chain-lint --build-dir=${CMAKE_BINARY_DIR} -vx
WORKING_DIRECTORY ${CMAKE_SOURCE_DIR}/t)
endforeach()Doing that would avoid changing the main Makefile to remove a file which almost certainly does not exist every time make is invoked as well.
Best Wishes
Phillip
Quoted 80 lines
To support building Git via a regular
makeinvocation after building it using CMake, we ensure that theGIT-BUILD-DIRfile is deleted (for convenience, this is done as part of theMakefilerule that is already run with everymakeinvocation to ensure thatGIT-BUILD-OPTIONSis up to date).Signed-off-by Johannes Schindelin <[email protected]>--- .gitignore | 1 + Makefile | 1 + contrib/buildsystems/CMakeLists.txt | 7 +------ t/test-lib.sh | 11 ++++++++++- 4 files changed, 13 insertions(+), 7 deletions(-).gitignore +1
-0@@ -2,6 +2,7 @@ /fuzz_corpora /fuzz-pack-headers /fuzz-pack-idx +/GIT-BUILD-DIR /GIT-BUILD-OPTIONS /GIT-CFLAGS /GIT-LDFLAGSMakefile +1
-0@@ -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_PYTHONcontrib/buildsystems/CMakeLists.txt +1
-6@@ -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/)t/test-lib.sh +10
-1@@ -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
Re: [PATCH 5/5] cmake: avoid editing t/test-lib.sh
Hi Phillip,
Quoted 36 lines · Phillip Wood
Quoted 11 lines · Johannes Schindelin via GitGitGadget
From: Johannes Schindelin <[email protected]>
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.shto overrideGIT_BUILD_DIRto point somewhere else than the parent of thet/directory.Quoted 8 lines
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.shto detect the presence of a file calledGIT-BUILD-DIRin 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.I think it is really good to get away from editing the test files, but one of the nice things about CMake's out of tree builds is that you can have several build directories with different build configurations and this change does not support that. Could we pass the build directory to the test scripts as a commandline option or environment variable instead? e.g.
foreach(tsh ${test_scipts}) add_test(NAME ${tsh}(quoted diff) +1
-2- COMMAND ${SH_EXE} ${tsh} --no-bin-wrappers --no-chain-lint -vx + COMMAND ${SH_EXE} ${tsh} --no-bin-wrappers --no-chain-lint --build-dir=${CMAKE_BINARY_DIR} -vx WORKING_DIRECTORY ${CMAKE_SOURCE_DIR}/t) endforeach()
Quoted 2 lines
Doing that would avoid changing the main
Makefileto remove a file which almost certainly does not exist every time make is invoked as well.
This is indeed tempting, but one of the things I recommend to Visual Studio users for ages already is to run the tests in a Git Bash, i.e. outside of Visual Studio, to allow for passing --run=... options and the likes. This recommendation predates support for CTest, naturally, but it is still valid.
And that recommended way of running tests would be broken by the suggested change because the tests would no longer run except when using CTest.
Besides, while the semantics look tempting, the implementation details do not. The reason is that we use GIT_BUILD_DIR to source the GIT-BUILD-OPTIONS and to validate that Git was built, long before we parse the command-line in t/test-lib.sh. I want to refrain from the prerequisite extensive refactoring required to support this, at least in the context of this here patch series.
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)?
Ciao,
Dscho
Re: [PATCH 5/5] cmake: avoid editing t/test-lib.sh
Hi Dscho
Quoted 49 lines · Johannes Schindelin
Hi Phillip,
Quoted 36 lines · Phillip Wood
Quoted 11 lines · Johannes Schindelin via GitGitGadget
From: Johannes Schindelin <[email protected]>
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.shto overrideGIT_BUILD_DIRto point somewhere else than the parent of thet/directory.Quoted 8 lines
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.shto detect the presence of a file calledGIT-BUILD-DIRin 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.I think it is really good to get away from editing the test files, but one of the nice things about CMake's out of tree builds is that you can have several build directories with different build configurations and this change does not support that. Could we pass the build directory to the test scripts as a commandline option or environment variable instead? e.g.
foreach(tsh ${test_scipts}) add_test(NAME ${tsh}(quoted diff) +1
-2- COMMAND ${SH_EXE} ${tsh} --no-bin-wrappers --no-chain-lint -vx + COMMAND ${SH_EXE} ${tsh} --no-bin-wrappers --no-chain-lint --build-dir=${CMAKE_BINARY_DIR} -vx WORKING_DIRECTORY ${CMAKE_SOURCE_DIR}/t) endforeach()Quoted 2 lines
Doing that would avoid changing the main
Makefileto remove a file which almost certainly does not exist every time make is invoked as well.This is indeed tempting, but one of the things I recommend to Visual Studio users for ages already is to run the tests in a Git Bash, i.e. outside of Visual Studio, to allow for passing
--run=...options and the likes. This recommendation predates support for CTest, naturally, but it is still valid.
Quoted 2 lines
And that recommended way of running tests would be broken by the suggested change because the tests would no longer run except when using CTest.
Yes the recommendation would have to include adding --build-dir which would be a pain
Quoted 6 lines
Besides, while the semantics look tempting, the implementation details do not. The reason is that we use
GIT_BUILD_DIRto source theGIT-BUILD-OPTIONSand to validate that Git was built, long before we parse the command-line int/test-lib.sh. I want to refrain from the prerequisite extensive refactoring required to support this, at least in the context of this here patch series.
That sounds awkward.
Quoted 3 lines
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)?
It is definitely an improvement for the CMake side, I'm not sure it is an improvement for the Makefile but it sounds like the best option overall.
Best Wishes
Phillip
Quoted 2 lines
Ciao,
Dscho
v2 cover Some fixes and an improvement for using CTest on Windows
In reply to Johannes Schindelin via GitGitGadgetVisual Studio users enjoy support for running the test suite via CTest, thanks to Git's CMake definition.
In https://github.com/git-for-windows/git/issues/3966, it has been reported that this does not work out of the box, though, but causes a couple of test failures instead. These problems are not caught by Git's CI runs because the vs-tests jobs actually use prove to run the test suite, not CTest.
In addition to fixing these problems, this patch series also addresses a long-standing gripe I have with the way Git's CMake definition supports
CTest: It edits t/test-lib.sh, which leaves this file eternally modified (but these modification should never be committed, they refer to a local-only, configuration-dependent directory).
Note: The signed/unsigned comparison bug in git add -p that is fixed in this here patch series is a relatively big one, and it merits further investigation whether there are similar bugs lurking in Git's code base. However, this is a much bigger project than can be addressed as part of this patch series, in particular because the analysis would require tools other than GCC's -Wsign-compare option (which totally misses the instance that is fixed in this here patch series).
Changes since v1:
* Clarified why it is a good idea to pass --no-bin-wrappers and --no-chain-lint when running on Windows.
* Clarified why the add -p bug has not been caught earlier.
* Clarified the scope of this patch series to fix running Git's tests within Visual Studio.
* Increased the time-out for the very slow t7112 test script.
* The test_chmod was determined to be not only faulty, but unneeded, and was dropped.
Johannes Schindelin (5):
cmake: make it easier to diagnose regressions in CTest runs
cmake: copy the merge tools for testing
add -p: avoid ambiguous signed/unsigned comparison
cmake: avoid editing t/test-lib.sh
cmake: increase time-out for a long-running test
.gitignore | 1 + Makefile | 1 + add-patch.c | 2 +- contrib/buildsystems/CMakeLists.txt | 16 ++++++++-------- t/test-lib.sh | 11 ++++++++++- 5 files changed, 21 insertions(+), 10 deletions(-)
base-commit: bbea4dcf42b28eb7ce64a6306cdde875ae5d09ca
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1320%2Fdscho%2Fctest-on-windows-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1320/dscho/ctest-on-windows-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1320
Range-diff vs v1:
1: 9cf14984c0a ! 1: e00cb37b98a cmake: align CTest definition with Git's CI runs
@@ Metadata
Author: Johannes Schindelin <[email protected]>
## Commit message ##
- cmake: align CTest definition with Git's CI runs
+ cmake: make it easier to diagnose regressions in CTest runs
- In Git's CI runs, the Windows tests are run with --no-bin-wrappers and
- --no-chain-lint, mainly to win back some time caused by the serious
- performance penalty paid for the tests relying so heavily on POSIX shell
- scripting, which only works by using a POSIX emulation layer.
+ 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.
- Let's do the same when running the tests, say, in Visual Studio.
+ 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.
- While at it, enable the command trace via -x and verbose output via
- -v, otherwise it would be near impossible to diagnose any problems.
+ 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.
2: 86ab58b6508 = 2: de7b47a9aa7 cmake: copy the merge tools for testing 3: 79abfa82c32 < -: ----------- tests: explicitly skip `chmod` calls on Windows 4: 4d24a4345ba ! 3: f96d5ab484c add -p: avoid ambiguous signed/unsigned comparison
@@ Commit message
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.
+
## add-patch.c ##
5: c7fc5a4ee4c = 4: 22473d6b8f3 cmake: avoid editing t/test-lib.sh -: ----------- > 5: 6aaa675301c cmake: increase time-out for a long-running test
Signature · 1 line
gitgitgadget
PATCH v2 1/5 cmake: make it easier to diagnose regressions in CTest runs
From: Johannes Schindelin <[email protected]>
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.
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.
--- contrib/buildsystems/CMakeLists.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
contrib/buildsystems/CMakeLists.txt +1 -1
@@ -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()
Signature · 1 line
gitgitgadget
Re: [PATCH v2 1/5] cmake: make it easier to diagnose regressions in CTest runs
Quoted 9 lines · Johannes Schindelin via GitGitGadget
From: Johannes Schindelin <[email protected]>
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.
-vand-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.
At first I wondered whether there's a way to make arg specification more intuitive, rather than silently changing defaults. Unfortunately, it looks like even in the latest versions of CTest don't really support passing arguments through to tests [1] (and the workarounds are unpleasant at best).
But then, you mentioned that there *is* a cumbersome/unintuitive approach to passing the options; what approach were you thinking?
[1] https://gitlab.kitware.com/cmake/cmake/-/issues/20470
Quoted 4 lines
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.
Makes sense, I don't see any harm in providing more verbose output by default here.
Quoted 7 lines
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-wrappersand--no-chain-lintoptions. This makes the test runs noticeably faster because thebin-wrappers/scripts as well as thechain-lintcode 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.
I'm a bit more hesitant on including these. I see how the performance benefit (on Windows in particular) would make typical user experience nicer. But, if someone develops locally with '--no-chain-lint' specified, they'll be much more likely to miss a broken && chain (personally, I get caught by chain lint errors *all the time* when I'm adding/updating tests) and cause unnecessary churn in their patch submissions. So, my recommendation would be to drop '--no-chain-lint' here (unless it's less important to the average developer than I think it is).
It's also possible that '--no-bin-wrappers' does something weird with their local installation of Git but I think it's safe enough to make the default if the performance gain is substantial.
Quoted 11 lines
Signed-off-by Johannes Schindelin <[email protected]>--- contrib/buildsystems/CMakeLists.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)contrib/buildsystems/CMakeLists.txt +0
-0
contrib/buildsystems/CMakeLists.txt file(GLOB test_scipts "${CMAKE_SOURCE_DIR}/t/t[0-9]*.sh") · line 1091
Quoted 5 lines
contrib/buildsystems/CMakeLists.txt +1
-1@@ -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
As you have it here, I don't think there's a way for a user to override these defaults (unless there's something about the manual workaround you mentioned earlier that allows it). Since a user could feasibly want to set their own options, could you add a build variable to CMakeLists.txt like 'GIT_TEST_OPTS'? You could use it to set the default options you have here, but a user could still specify their own value at build time to override.
Quoted 3 lines
contrib/buildsystems/CMakeLists.txt +0
-0WORKING_DIRECTORY ${CMAKE_SOURCE_DIR}/t) endforeach()
Re: [PATCH v2 1/5] cmake: make it easier to diagnose regressions in CTest runs
Hi Victoria,
Quoted 11 lines · Victoria Dye
Quoted 9 lines · Johannes Schindelin via GitGitGadget
From: Johannes Schindelin <[email protected]>
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.
-vand-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.
Quoted 7 lines
At first I wondered whether there's a way to make arg specification more intuitive, rather than silently changing defaults. Unfortunately, it looks like even in the latest versions of CTest don't really support passing arguments through to tests [1] (and the workarounds are unpleasant at best).
But then, you mentioned that there *is* a cumbersome/unintuitive approach to passing the options; what approach were you thinking?
Thank you for pointing out that I assumed this to be more obvious than it actually is.
The cumbersome way is to edit this part of the CMake definition
(https://github.com/git/git/blob/v2.38.0/contrib/buildsystems/CMakeLists.txt#L1091-L1096):
#test
foreach(tsh ${test_scipts})
add_test(NAME ${tsh}
COMMAND ${SH_EXE} ${tsh}
WORKING_DIRECTORY
${CMAKE_SOURCE_DIR}/t)
endforeach()To pass an option like -v --run=1,29, the user would have to edit the line COMMAND ${SH_EXE} ${tsh}, appending the options, and then re-configure the CMake cache. And then not forget to _only_ run the test script in question because those options do not make sense for the other tests. And then not forget to undo the changes and re-configure the CMake cache.
Quoted 46 lines
[1] https://gitlab.kitware.com/cmake/cmake/-/issues/20470
Quoted 4 lines
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.
Makes sense, I don't see any harm in providing more verbose output by default here.
Quoted 7 lines
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-wrappersand--no-chain-lintoptions. This makes the test runs noticeably faster because thebin-wrappers/scripts as well as thechain-lintcode 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.I'm a bit more hesitant on including these. I see how the performance benefit (on Windows in particular) would make typical user experience nicer. But, if someone develops locally with '--no-chain-lint' specified, they'll be much more likely to miss a broken && chain (personally, I get caught by chain lint errors *all the time* when I'm adding/updating tests) and cause unnecessary churn in their patch submissions. So, my recommendation would be to drop '--no-chain-lint' here (unless it's less important to the average developer than I think it is).
It's also possible that '--no-bin-wrappers' does something weird with their local installation of Git but I think it's safe enough to make the default if the performance gain is substantial.
Quoted 15 lines
Signed-off-by Johannes Schindelin <[email protected]>--- contrib/buildsystems/CMakeLists.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)contrib/buildsystems/CMakeLists.txt +0
-1@@ -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}
Quoted 8 lines
contrib/buildsystems/CMakeLists.txt file(GLOB test_scipts "${CMAKE_SOURCE_DIR}/t/t[0-9]*.sh") · line 1091
Quoted 1 line
contrib/buildsystems/CMakeLists.txt +1
-0+ COMMAND ${SH_EXE} ${tsh} --no-bin-wrappers --no-chain-lint -vxAs you have it here, I don't think there's a way for a user to override these defaults (unless there's something about the manual workaround you mentioned earlier that allows it). Since a user could feasibly want to set their own options, could you add a build variable to CMakeLists.txt like '
GIT_TEST_OPTS'? You could use it to set the default options you have here, but a user could still specify their own value at build time to override.
Indeed, there is no way to override this, unless the user edits the (Git-tracked) CMakeLists.txt file and then re-configures the cache.
In my experience, the most common scenario where a developer needs to pass options to test script is when they need to restrict execution to individual test cases via, say, --run=1,29.
Such a use case cannot be covered via the CMake approach because it is not fine-grained enough: any options the user could set would apply to the entire test suite.
I added a comment to the commit message advising users to run the test script manually via Git Bash instead, if they wish to pass a different set of command-line options to the test script.
Thanks,
Dscho
Quoted 6 lines
Quoted 4 lines
contrib/buildsystems/CMakeLists.txt +0
-0WORKING_DIRECTORY ${CMAKE_SOURCE_DIR}/t) endforeach()
Re: [PATCH v2 1/5] cmake: make it easier to diagnose regressions in CTest runs
In reply to Johannes Schindelin via GitGitGadgetQuoted 31 lines · Johannes Schindelin via GitGitGadget
From: Johannes Schindelin <[email protected]>
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.
-vand-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.
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-wrappersand--no-chain-lintoptions. This makes the test runs noticeably faster because thebin-wrappers/scripts as well as thechain-lintcode 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.Signed-off-by Johannes Schindelin <[email protected]>--- contrib/buildsystems/CMakeLists.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)contrib/buildsystems/CMakeLists.txt +0
-0
contrib/buildsystems/CMakeLists.txt file(GLOB test_scipts "${CMAKE_SOURCE_DIR}/t/t[0-9]*.sh") · line 1093
Quoted 7 lines
contrib/buildsystems/CMakeLists.txt +1
-1@@ -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()
The -vx part of this looks sensible, and matches the commit message $subject.
I think the "--no-bin-wrappers --no-chain-lint" and the "while at it" here should be stripped out, and put into its own commit.
Which, as I commented on in the v1[1] is making the implicit assumption that this cmake file is only used on Windows, but since 561962479c (cmake: fix CMakeLists.txt on Linux, 2022-05-24) that isn't the case.
So, perhaps we should have a performance hack due to Windows's slowness, but:
A. It should be guarded by some "is windows?" check. Your commit
message justifies why this is a good idea on Windows, but completely elides over the fact that this isn't Windows-specific code anymore.
B. We can still build wind "make" or "cmake", I don't see a reason for
why such a perf hack (if we decide to keep it) should depend on the build system.
C. Since I sent [1] we've had submitted chainlint.pl in-flight
series. It's partly trying to take special considerations to be fast on Windows. I don't think it's the case with that series that this needs to be skipped on Windows anymore (and if it is, Eric would like to know).
Re: [PATCH v2 1/5] cmake: make it easier to diagnose regressions in CTest runs
Quoted 13 lines · Ævar Arnfjörð Bjarmason
contrib/buildsystems/CMakeLists.txt file(GLOB test_scipts "${CMAKE_SOURCE_DIR}/t/t[0-9]*.sh") · line 1091
Quoted 8 lines · Johannes Schindelin via GitGitGadget
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.
-vand-x.[...]Signed-off-by Johannes Schindelin <[email protected]>---
contrib/buildsystems/CMakeLists.txt +1
-0+ COMMAND ${SH_EXE} ${tsh} --no-bin-wrappers --no-chain-lint -vxI think the "--no-bin-wrappers --no-chain-lint" and the "while at it" here should be stripped out, and put into its own commit.
Quoted 8 lines
So, perhaps we should have a performance hack due to Windows's slowness, but:
C. Since I sent [1] we've had submitted chainlint.pl in-flight
series. It's partly trying to take special considerations to be fast on Windows. I don't think it's the case with that series that this needs to be skipped on Windows anymore (and if it is, Eric would like to know).
I had the opportunity to test chainlint.pl on an 8-core machine using both Linux and Windows 10. The machine is more than a few years old, though not nearly as old as my own development machine. Although chainlint.pl checked all test definitions in the entire project in less than one _second_ on Linux, it took two _minutes_ on Windows 10 on the same machine (using all 8 cores). Despite the fact that Perl is run just once to check all test definitions -- unlike chainlint.sed running 'sed' tens of thousands of times, chainlint.pl is still painfully slow on Windows, so I certainly understand the desire to use --no-chain-lint, at least on Windows.
PATCH v2 2/5 cmake: copy the merge tools for testing
In reply to Johannes Schindelin via GitGitGadgetFrom: Johannes Schindelin <[email protected]>
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.
--- contrib/buildsystems/CMakeLists.txt | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
contrib/buildsystems/CMakeLists.txt +2 -1
@@ -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()
Signature · 1 line
gitgitgadget
PATCH v2 3/5 add -p: avoid ambiguous signed/unsigned comparison
In reply to Johannes Schindelin via GitGitGadgetFrom: Johannes Schindelin <[email protected]>
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.
--- add-patch.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
add-patch.c +1 -1
@@ -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);
Signature · 1 line
gitgitgadget
PATCH v2 4/5 cmake: avoid editing t/test-lib.sh
In reply to Johannes Schindelin via GitGitGadgetFrom: Johannes Schindelin <[email protected]>
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).
--- .gitignore | 1 + Makefile | 1 + contrib/buildsystems/CMakeLists.txt | 7 +------ t/test-lib.sh | 11 ++++++++++- 4 files changed, 13 insertions(+), 7 deletions(-)
.gitignore +1 -0
@@ -2,6 +2,7 @@ /fuzz_corpora /fuzz-pack-headers /fuzz-pack-idx +/GIT-BUILD-DIR /GIT-BUILD-OPTIONS /GIT-CFLAGS /GIT-LDFLAGS
Makefile +1 -0
@@ -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
contrib/buildsystems/CMakeLists.txt +1 -6
@@ -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/)
t/test-lib.sh +10 -1
@@ -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
Signature · 1 line
gitgitgadget
Re: [PATCH v2 4/5] cmake: avoid editing t/test-lib.sh
Quoted 90 lines · Johannes Schindelin via GitGitGadget
From: Johannes Schindelin <[email protected]>
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.shto overrideGIT_BUILD_DIRto point somewhere else than the parent of thet/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.shto detect the presence of a file calledGIT-BUILD-DIRin 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
makeinvocation after building it using CMake, we ensure that theGIT-BUILD-DIRfile is deleted (for convenience, this is done as part of theMakefilerule that is already run with everymakeinvocation to ensure thatGIT-BUILD-OPTIONSis up to date).Signed-off-by Johannes Schindelin <[email protected]>--- .gitignore | 1 + Makefile | 1 + contrib/buildsystems/CMakeLists.txt | 7 +------ t/test-lib.sh | 11 ++++++++++- 4 files changed, 13 insertions(+), 7 deletions(-).gitignore +1
-0@@ -2,6 +2,7 @@ /fuzz_corpora /fuzz-pack-headers /fuzz-pack-idx +/GIT-BUILD-DIR /GIT-BUILD-OPTIONS /GIT-CFLAGS /GIT-LDFLAGSMakefile +1
-0@@ -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_PYTHONcontrib/buildsystems/CMakeLists.txt +1
-6@@ -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/)t/test-lib.sh +3
-1@@ -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
t/test-lib.sh then · line 57
Quoted 10 lines
t/test-lib.sh +7
-0+ # 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
As pointed out in the v1 this breaks the cmake<->make interaction in some scenarios, but from some brief testing there seemed to be an easy workaround which didn't suffer from that problem:
https://lore.kernel.org/git/[email protected]/
Re: [PATCH v2 4/5] cmake: avoid editing t/test-lib.sh
Hi Ævar,
Quoted 99 lines · Ævar Arnfjörð Bjarmason
Quoted 97 lines · Johannes Schindelin via GitGitGadget
From: Johannes Schindelin <[email protected]>
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.shto overrideGIT_BUILD_DIRto point somewhere else than the parent of thet/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.shto detect the presence of a file calledGIT-BUILD-DIRin 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
makeinvocation after building it using CMake, we ensure that theGIT-BUILD-DIRfile is deleted (for convenience, this is done as part of theMakefilerule that is already run with everymakeinvocation to ensure thatGIT-BUILD-OPTIONSis up to date).Signed-off-by Johannes Schindelin <[email protected]>--- .gitignore | 1 + Makefile | 1 + contrib/buildsystems/CMakeLists.txt | 7 +------ t/test-lib.sh | 11 ++++++++++- 4 files changed, 13 insertions(+), 7 deletions(-).gitignore +1
-0@@ -2,6 +2,7 @@ /fuzz_corpora /fuzz-pack-headers /fuzz-pack-idx +/GIT-BUILD-DIR /GIT-BUILD-OPTIONS /GIT-CFLAGS /GIT-LDFLAGSMakefile +1
-0@@ -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_PYTHONcontrib/buildsystems/CMakeLists.txt +1
-6@@ -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/)t/test-lib.sh +10
-1@@ -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"
Quoted 8 lines
t/test-lib.sh then · line 57
Quoted 3 lines
t/test-lib.sh +0
-0then echo "PANIC: Running in a $TEST_DIRECTORY that doesn't end in '/t'?" >&2 exit 1As pointed out in the v1 this breaks the cmake<->make interaction in some scenarios, but from some brief testing there seemed to be an easy workaround which didn't suffer from that problem:
https://lore.kernel.org/git/[email protected]/
I do not think that the CMake <-> make interaction will come up in any other scenario than your and my tests in the context of this mailing list thread.
Therefore, I am certain that we need not cater to that scenario at all.
Ciao,
Johannes
Re: [PATCH v2 4/5] cmake: avoid editing t/test-lib.sh
Quoted 112 lines · Johannes Schindelin
Hi Ævar,
Quoted 99 lines · Ævar Arnfjörð Bjarmason
Quoted 97 lines · Johannes Schindelin via GitGitGadget
From: Johannes Schindelin <[email protected]>
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.shto overrideGIT_BUILD_DIRto point somewhere else than the parent of thet/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.shto detect the presence of a file calledGIT-BUILD-DIRin 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
makeinvocation after building it using CMake, we ensure that theGIT-BUILD-DIRfile is deleted (for convenience, this is done as part of theMakefilerule that is already run with everymakeinvocation to ensure thatGIT-BUILD-OPTIONSis up to date).Signed-off-by Johannes Schindelin <[email protected]>--- .gitignore | 1 + Makefile | 1 + contrib/buildsystems/CMakeLists.txt | 7 +------ t/test-lib.sh | 11 ++++++++++- 4 files changed, 13 insertions(+), 7 deletions(-).gitignore +1
-0@@ -2,6 +2,7 @@ /fuzz_corpora /fuzz-pack-headers /fuzz-pack-idx +/GIT-BUILD-DIR /GIT-BUILD-OPTIONS /GIT-CFLAGS /GIT-LDFLAGSMakefile +1
-0@@ -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_PYTHONcontrib/buildsystems/CMakeLists.txt +1
-6@@ -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/)t/test-lib.sh +10
-1@@ -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"Quoted 8 lines
t/test-lib.sh then · line 57
Quoted 3 lines
t/test-lib.sh +0
-0then echo "PANIC: Running in a $TEST_DIRECTORY that doesn't end in '/t'?" >&2 exit 1As pointed out in the v1 this breaks the cmake<->make interaction in some scenarios, but from some brief testing there seemed to be an easy workaround which didn't suffer from that problem:
https://lore.kernel.org/git/[email protected]/
Quoted 5 lines
I do not think that the CMake <-> make interaction will come up in any other scenario than your and my tests in the context of this mailing list thread.
Therefore, I am certain that we need not cater to that scenario at all.
I run it like that now when I build locally, and it slots nicely in with:
1. First run a bunch of 'build' targets
2. Then run a bunch of 'test' targets
I'm not opposed to having this e.g. as a special-case on Windows, or that we pick up when you *only* have cmake/ctest unconditionally, and auto-do the right thing.
But I do think that:
A. Getting a "run test-lib against this build dir over there" is
*different* from the bundled behavior of the auto-flip-flopping.
B. Your case for doing it this way seems to be the
Windows-specific case noted in 1/5, can't we just make that part Windows-specific then? I don't get why it needs to be bundled up with cmake everywhere....
Re: [PATCH v2 4/5] cmake: avoid editing t/test-lib.sh
In reply to Johannes Schindelin via GitGitGadgetQuoted 20 lines · Johannes Schindelin via GitGitGadget
From: Johannes Schindelin <[email protected]>
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.shto overrideGIT_BUILD_DIRto point somewhere else than the parent of thet/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.shto detect the presence of a file calledGIT-BUILD-DIRin 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.
Quoted 5 lines
To support building Git via a regular
makeinvocation after building it using CMake, we ensure that theGIT-BUILD-DIRfile is deleted (for convenience, this is done as part of theMakefilerule that is already run with everymakeinvocation to ensure thatGIT-BUILD-OPTIONSis 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]:
Quoted 3 lines
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.
Quoted 65 lines
Signed-off-by Johannes Schindelin <[email protected]>--- .gitignore | 1 + Makefile | 1 + contrib/buildsystems/CMakeLists.txt | 7 +------ t/test-lib.sh | 11 ++++++++++- 4 files changed, 13 insertions(+), 7 deletions(-).gitignore +1
-0@@ -2,6 +2,7 @@ /fuzz_corpora /fuzz-pack-headers /fuzz-pack-idx +/GIT-BUILD-DIR /GIT-BUILD-OPTIONS /GIT-CFLAGS /GIT-LDFLAGSMakefile +1
-0@@ -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_PYTHONcontrib/buildsystems/CMakeLists.txt +1
-6@@ -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/)t/test-lib.sh +3
-1@@ -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
t/test-lib.sh then · line 57
Quoted 10 lines
t/test-lib.sh +7
-0+ # 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-----------------
t/test-lib.sh +5 -4
@@ -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.
Re: [PATCH v2 4/5] cmake: avoid editing t/test-lib.sh
Quoted 27 lines · Victoria Dye
Quoted 20 lines · Johannes Schindelin via GitGitGadget
From: Johannes Schindelin <[email protected]>
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.shto overrideGIT_BUILD_DIRto point somewhere else than the parent of thet/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.shto detect the presence of a file calledGIT-BUILD-DIRin 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.Quoted 5 lines
To support building Git via a regular
makeinvocation after building it using CMake, we ensure that theGIT-BUILD-DIRfile is deleted (for convenience, this is done as part of theMakefilerule that is already run with everymakeinvocation to ensure thatGIT-BUILD-OPTIONSis up to date).
Quoted 5 lines
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]:
Forgot this link: https://lore.kernel.org/git/[email protected]/
Quoted 17 lines
Quoted 4 lines
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.
...
Quoted 15 lines
Quoted 15 lines
t/test-lib.sh +6
-1@@ -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*)
Quoted 8 lines
t/test-lib.sh then · line 57
Quoted 7 lines
t/test-lib.sh +4
-0+ 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 1Referring to Ævar's review in [1] - while I'm not overly concerned about the
AND this one (which should be [2]): https://lore.kernel.org/git/[email protected]/
Sorry about that!
Quoted 38 lines
"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-----------------
t/test-lib.sh +5
-5@@ -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.
Re: [PATCH v2 4/5] cmake: avoid editing t/test-lib.sh
In reply to Victoria DyeQuoted 9 lines · Victoria Dye
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.
...
... "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:
Thanks for a quality review.
Re: [PATCH v2 4/5] cmake: avoid editing t/test-lib.sh
In reply to Victoria DyeHi Victoria,
Quoted 61 lines · Victoria Dye
Quoted 13 lines · Johannes Schindelin via GitGitGadget
[...]
t/test-lib.sh +3
-1@@ -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 1t/test-lib.sh then · line 57
Quoted 10 lines
t/test-lib.sh +7
-0+ # 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 1Referring 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-----------------
t/test-lib.sh +5
-5@@ -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<-----------------
Quoted 3 lines
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.
Good point!
Thank you,
Dscho
PATCH v2 5/5 cmake: increase time-out for a long-running test
In reply to Johannes Schindelin via GitGitGadgetFrom: Johannes Schindelin <[email protected]>
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.
--- contrib/buildsystems/CMakeLists.txt | 4 ++++ 1 file changed, 4 insertions(+)
contrib/buildsystems/CMakeLists.txt +4 -0
@@ -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
Signature · 1 line
gitgitgadget
Re: [PATCH v2 5/5] cmake: increase time-out for a long-running test
Quoted 29 lines · Johannes Schindelin via GitGitGadget
From: Johannes Schindelin <[email protected]>
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 <[email protected]>--- contrib/buildsystems/CMakeLists.txt | 4 ++++ 1 file changed, 4 insertions(+)contrib/buildsystems/CMakeLists.txt +0
-0
contrib/buildsystems/CMakeLists.txt foreach(tsh ${test_scipts}) · line 1095
Quoted 8 lines
contrib/buildsystems/CMakeLists.txt +4
-0@@ -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
I don't see per [1] that it would have any negative effect to just bump the timeout a lot more, and for all the tests.
If we're running into 3600 seconds, then setting this to 4000 seconds seems to be a smal stopgap at best. That's just over a 10% increase, so if one person ran into it it 3600 someone with a slightly slower system should be running into the same still, or if we just add a few more tests to 7112 (or some other slow test).
So why not set this ta 3600*10 or whatever for *all* of the test scripts instead of playing whack-a-mole?
1. https://cmake.org/cmake/help/latest/prop_test/TIMEOUT.html
Re: [PATCH v2 5/5] cmake: increase time-out for a long-running test
Quoted 35 lines · Ævar Arnfjörð Bjarmason
Quoted 32 lines · Johannes Schindelin via GitGitGadget
From: Johannes Schindelin <[email protected]>
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 <[email protected]>--- contrib/buildsystems/CMakeLists.txt | 4 ++++ 1 file changed, 4 insertions(+)contrib/buildsystems/CMakeLists.txt +0
-0@@ -1088,4 +1088,8 @@ foreach(tsh ${test_scipts}) WORKING_DIRECTORY ${CMAKE_SOURCE_DIR}/t) endforeach()
Quoted 8 lines
contrib/buildsystems/CMakeLists.txt foreach(tsh ${test_scipts}) · line 1095
Quoted 5 lines
contrib/buildsystems/CMakeLists.txt +4
-0+# 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_TESTINGI don't see per [1] that it would have any negative effect to just bump the timeout a lot more, and for all the tests.
I'm a bit confused as to how that link (which explains that the 'TIMEOUT' property is used to determine when to kill a test process - more or less standard behavior for timeouts in applications) shows that excessively long timeouts on every test "[wouldn't] have any negative effect."
In terms of the timeout length, the stated reason for adding the timeout in the first place is:
Quoted 2 lines
Quoted 2 lines
Let's increase the time-out as a stop gap to unblock developers wishing to run Git's test suite in Visual Studio.
I would consider ~1 hour a manageable (albeit frustrating) amount of time to wait for a test, but not 10 hours (as you suggest later).
As for setting the timeout on all tests: that might be helpful, only because (AFAICT from CMake's source code & documentation) the default timeout is "no timeout." If setting a global default, though, I'd prefer a shorter timespan (e.g. 1800s) so that a timeout properly indicates a failure condition ("this test runs much longer than it should, so we may have introduced a performance bug").
Practically, though, I don't feel too strongly about whether or not to set a global default. If we wanted to add timeouts to more tests in the future, I'd want a more extensible solution with carefully-selected timeouts. But if it's just this one test running too long, special case-ing a timeout for it with a comment explaining the reasoning seems appropriate to me.
Quoted 6 lines
If we're running into 3600 seconds, then setting this to 4000 seconds seems to be a smal stopgap at best. That's just over a 10% increase, so if one person ran into it it 3600 someone with a slightly slower system should be running into the same still, or if we just add a few more tests to 7112 (or some other slow test).
I don't think this patch is suggesting that timeout failures are happening at 3600s, rather that the peak observed execution time of the test is 3600s. If that's the case, <peak execution time> + an 11% buffer is a reasonable choice for the value.
Quoted 5 lines
So why not set this ta 3600*10 or whatever for *all* of the test scripts instead of playing whack-a-mole?
1. https://cmake.org/cmake/help/latest/prop_test/TIMEOUT.html
Quoted 33 lines · Johannes Schindelin via GitGitGadget
Visual Studio users enjoy support for running the test suite via CTest, thanks to Git's CMake definition.
In https://github.com/git-for-windows/git/issues/3966, it has been reported that this does not work out of the box, though, but causes a couple of test failures instead. These problems are not caught by Git's CI runs because the vs-tests jobs actually use prove to run the test suite, not CTest.
In addition to fixing these problems, this patch series also addresses a long-standing gripe I have with the way Git's CMake definition supports
CTest: It editst/test-lib.sh, which leaves this file eternally modified (but these modification should never be committed, they refer to a local-only, configuration-dependent directory).Note: The signed/unsigned comparison bug in git add -p that is fixed in this here patch series is a relatively big one, and it merits further investigation whether there are similar bugs lurking in Git's code base. However, this is a much bigger project than can be addressed as part of this patch series, in particular because the analysis would require tools other than GCC's -Wsign-compare option (which totally misses the instance that is fixed in this here patch series).
Changes since v1:
* Clarified why it is a good idea to pass --no-bin-wrappers and --no-chain-lint when running on Windows.
* Clarified why the add -p bug has not been caught earlier.
* Clarified the scope of this patch series to fix running Git's tests within Visual Studio.
* Increased the time-out for the very slow t7112 test script.
* Thetest_chmodwas determined to be not only faulty, but unneeded, and was dropped.
Quoted 6 lines
Johannes Schindelin (5):
cmake: make it easier to diagnose regressions in CTest runs
cmake: copy the merge tools for testing
add -p: avoid ambiguous signed/unsigned comparison
cmake: avoid editingt/test-lib.sh
cmake: increase time-out for a long-running test
I've reviewed patches 1-3 & 5 and started looking over 4, but I'd like to spend a bit more time on it (mostly to understand pros/cons of the 'GIT-BUILD-DIR' vs. any alternatives). I'm planning to finish that up tomorrow.
Thanks for your patience!
Quoted 69 lines
.gitignore | 1 + Makefile | 1 + add-patch.c | 2 +- contrib/buildsystems/CMakeLists.txt | 16 ++++++++-------- t/test-lib.sh | 11 ++++++++++- 5 files changed, 21 insertions(+), 10 deletions(-)base-commit:
bbea4dcf42b28eb7ce64a6306cdde875ae5d09ca
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1320%2Fdscho%2Fctest-on-windows-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1320/dscho/ctest-on-windows-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1320Range-diff vs v1:
1:
9cf14984c0a! 1:e00cb37b98acmake: align CTest definition with Git's CI runs
@@ Metadata
Author: Johannes Schindelin <[email protected]>## Commit message ##
- cmake: align CTest definition with Git's CI runs
+ cmake: make it easier to diagnose regressions in CTest runs- In Git's CI runs, the Windows tests are run with
--no-bin-wrappersand
---no-chain-lint, mainly to win back some time caused by the serious
- performance penalty paid for the tests relying so heavily on POSIX shell
- scripting, which only works by using a POSIX emulation layer.
+ 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.-vand-x.- Let's do the same when running the tests, say, in Visual Studio.
+ 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.- While at it, enable the command trace via
-xand verbose output via
--v, otherwise it would be near impossible to diagnose any problems.
+ 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-wrappersand
+--no-chain-lintoptions. This makes the test runs noticeably faster
+ because thebin-wrappers/scripts as well as thechain-lintcode
+ 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.Signed-off-by Johannes Schindelin <[email protected]>2: 86ab58b6508 = 2: de7b47a9aa7 cmake: copy the merge tools for testing 3: 79abfa82c32 < -: ----------- tests: explicitly skip `chmod` calls on Windows 4: 4d24a4345ba ! 3: f96d5ab484c add -p: avoid ambiguous signed/unsigned comparison@@ Commit message
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 whenNO_PERLis set,
+ which is the case in thevs-testjobs of Git's CI runs.
+Signed-off-by Johannes Schindelin <[email protected]>##
add-patch.c##5: c7fc5a4ee4c = 4: 22473d6b8f3 cmake: avoid editing t/test-lib.sh -: ----------- > 5: 6aaa675301c cmake: increase time-out for a long-running test
v3 cover Some fixes and an improvement for using CTest on Windows
In reply to Johannes Schindelin via GitGitGadgetVisual Studio users enjoy support for running the test suite via CTest, thanks to Git's CMake definition.
In https://github.com/git-for-windows/git/issues/3966, it has been reported that this does not work out of the box, though, but causes a couple of test failures instead. These problems are not caught by Git's CI runs because the vs-tests jobs actually use prove to run the test suite, not CTest.
In addition to fixing these problems, this patch series also addresses a long-standing gripe I have with the way Git's CMake definition supports
CTest: It edits t/test-lib.sh, which leaves this file eternally modified (but these modification should never be committed, they refer to a local-only, configuration-dependent directory).
Note: The signed/unsigned comparison bug in git add -p that is fixed in this here patch series is a relatively big one, and it merits further investigation whether there are similar bugs lurking in Git's code base. However, this is a much bigger project than can be addressed as part of this patch series, in particular because the analysis would require tools other than GCC's -Wsign-compare option (which totally misses the instance that is fixed in this here patch series).
Changes since v2:
* Enhanced a commit message to clarify how the "cumbersome approach" looks like when a developer wishes to pass options to the test scripts in Visual Studio.
* Changed the base branch to ac/fuzzers, to avoid merge conflicts in .gitignore.
* Moved the PANIC check in test-lib.sh so that it is not skipped when the file GIT-BUILD-DIR exists.
Changes since v1:
* Clarified why it is a good idea to pass --no-bin-wrappers and --no-chain-lint when running on Windows.
* Clarified why the add -p bug has not been caught earlier.
* Clarified the scope of this patch series to fix running Git's tests within Visual Studio.
* Increased the time-out for the very slow t7112 test script.
* The test_chmod was determined to be not only faulty, but unneeded, and was dropped.
Johannes Schindelin (5):
cmake: make it easier to diagnose regressions in CTest runs
cmake: copy the merge tools for testing
add -p: avoid ambiguous signed/unsigned comparison
cmake: avoid editing t/test-lib.sh
cmake: increase time-out for a long-running test
.gitignore | 1 + Makefile | 1 + add-patch.c | 2 +- contrib/buildsystems/CMakeLists.txt | 16 ++++++++-------- t/test-lib.sh | 10 ++++++++++ 5 files changed, 21 insertions(+), 9 deletions(-)
base-commit: 6713bfc70c4dc6da1fa4084f000b72f5d74fecfb
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1320%2Fdscho%2Fctest-on-windows-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1320/dscho/ctest-on-windows-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/1320
Range-diff vs v2:
1: e00cb37b98a ! 1: 356b2e9a100 cmake: make it easier to diagnose regressions in CTest runs
@@ Commit message
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.
+ 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
@@ Commit message
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.
+ --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.
2: de7b47a9aa7 = 2: 9faca9d5cbe cmake: copy the merge tools for testing 3: f96d5ab484c = 3: 41a8021a4bd add -p: avoid ambiguous signed/unsigned comparison 4: 22473d6b8f3 ! 4: 5b0c2a150e9 cmake: avoid editing t/test-lib.sh
@@ Commit message
## .gitignore ##
@@
/fuzz_corpora
- /fuzz-pack-headers
- /fuzz-pack-idx
+/GIT-BUILD-DIR
/GIT-BUILD-OPTIONS
/GIT-CFLAGS
@@ contrib/buildsystems/CMakeLists.txt: endif()
## t/test-lib.sh ##
@@ t/test-lib.sh: then
- TEST_OUTPUT_DIRECTORY=$TEST_DIRECTORY
+ echo "PANIC: Running in a $TEST_DIRECTORY that doesn't end in '/t'?" >&2
+ exit 1
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
@@ t/test-lib.sh: 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
+ # adding the delimiter if VAR or VALUE is empty. I.e. a generalized:
5: 6aaa675301c = 5: 40cf872f483 cmake: increase time-out for a long-running test
Signature · 1 line
gitgitgadget
PATCH v3 1/5 cmake: make it easier to diagnose regressions in CTest runs
From: Johannes Schindelin <[email protected]>
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.
--- contrib/buildsystems/CMakeLists.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
contrib/buildsystems/CMakeLists.txt +1 -1
@@ -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()
Signature · 1 line
gitgitgadget
Re: [PATCH v3 1/5] cmake: make it easier to diagnose regressions in CTest runs
Quoted 45 lines · Johannes Schindelin via GitGitGadget
From: Johannes Schindelin <[email protected]>
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.
-vand-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-wrappersand--no-chain-lintoptions.This makes the test runs noticeably faster because the
bin-wrappers/scripts as well as thechain-lintcode 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 <[email protected]>--- contrib/buildsystems/CMakeLists.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)contrib/buildsystems/CMakeLists.txt +0
-0
contrib/buildsystems/CMakeLists.txt file(GLOB test_scipts "${CMAKE_SOURCE_DIR}/t/t[0-9]*.sh") · line 1093
Quoted 7 lines
contrib/buildsystems/CMakeLists.txt +1
-1@@ -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()
This all seems sensible, but where you lose me is why this needs to be cmake-specific.
Maybe I'm just missing something, but everything you're describing seems to me to be Windows-specific, e.g. "chain lint is slow on windows" and the like.
Why aren't we just declaring that it's not worth it on Windows, but doing so in test-lib.sh, or at least doing it in both cmake's recipe & the Makefile?
I can see why we'd e.g. want it not to run with "make test", but want it when you run individual tests, but then both could just set some "this is running as a batch" env flag or whatever.
Also, even then everything you're describing is eliding that:
- This ill makes sense for Windows, sure, but the cmake recipe is no longer Windows-specific, was this authored before that happened & rebased? Why do we want cmake/ctest on Linux to have --no-chain-lint?
- Having cmake be some "mostly the Makefile, but also other defaults" is really changing the scope of the whole cmake build alternative, which until now (but I may have missed some other special-cases) has been "let's build it like the Makefile".
- That "ctest" doesn't work on Linux *at all* currently, this fixes it, but is really sweeping the underlying issue under the rug. AFAICT it's been limping along because there's no "chmod +x" or whatever on Windows (IIRC we magically look at she shebang).
For that last one & more I hacked up
https://github.com/avar/git/tree/avar/cmake-test-path a few days ago as an alternative to some parts of this series, but given past exchanges wanted to ask you first what you thought about it off-list, but didn't get around to it..
But anyway, I do think starting with something like
https://github.com/avar/git/commit/5bd0ee2bc826d2bb358af9d93c88ba28584bbda1
makes more sense, i.e. get "ctest" to work at all with the bin-wrappers/* in a portable way, and then *maybe* tweak it/Makefile to pass some custom flags on some platform(s).
PATCH v3 2/5 cmake: copy the merge tools for testing
In reply to Johannes Schindelin via GitGitGadgetFrom: Johannes Schindelin <[email protected]>
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.
--- contrib/buildsystems/CMakeLists.txt | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
contrib/buildsystems/CMakeLists.txt +2 -1
@@ -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()
Signature · 1 line
gitgitgadget
Re: [PATCH v3 2/5] cmake: copy the merge tools for testing
Quoted 16 lines · Johannes Schindelin via GitGitGadget
From: Johannes Schindelin <[email protected]>
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 <[email protected]>--- contrib/buildsystems/CMakeLists.txt | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)contrib/buildsystems/CMakeLists.txt +0
-0
contrib/buildsystems/CMakeLists.txt if(NOT ${CMAKE_BINARY_DIR}/CMakeCache.txt STREQUAL ${CACHE_PATH}) · line 1085
Quoted 9 lines
contrib/buildsystems/CMakeLists.txt +2
-1@@ -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()
I mentioned that I hacked up a "root cause fix" for some issues in cmake in 1/5. Continuing that this works, but is really just a hack-for-the-hack fix, where we should fix the root cause:
Which actually, is really simple:
https://github.com/avar/git/commit/30f2265fd07aee97ea66f6e84a824d85d241e245
As you know we fake up the $GIT_BUILD_DIR for cmake, but then later in the recipe (i.e. here) we have to scurry around and adjust that because for some stuff we mean contrib/buildsystems/out/<path>, whereas in others we mean the original <path>.
So here (and this isn't on you, we had this before) if you run "cmake" and e.g. tweak the mergetools sources we'll need to re-copy them, blah blah blah.
But we can just ... not copy it at all, and have the test-lib.sh learn that some stuff it needs from whatever the "build dir" is, but other stuff is *always* relative to the root of the project.
So, for most of that & the context it simply goes away, we don't need to copy po/is.po, the tests can just know where to find it, ditto mergetools/tkdiff (which is not generated), and the bash completion.
PATCH v3 3/5 add -p: avoid ambiguous signed/unsigned comparison
In reply to Johannes Schindelin via GitGitGadgetFrom: Johannes Schindelin <[email protected]>
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.
--- add-patch.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
add-patch.c +1 -1
@@ -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);
Signature · 1 line
gitgitgadget
Re: [PATCH v3 3/5] add -p: avoid ambiguous signed/unsigned comparison
Quoted 40 lines · Johannes Schindelin via GitGitGadget
From: Johannes Schindelin <[email protected]>
In the interactive
addoperation, 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
ito the unsigned valuemode_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'sgotocommand shows no hunks by default because it casts a negative start offset to a pretty large unsigned value, breaking the "goto hunk" test case int3701-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_PERLis set, which is the case in thevs-testjobs of Git's CI runs.Signed-off-by Johannes Schindelin <[email protected]>--- add-patch.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)add-patch.c +0
-0
add-patch.c soft_increment: · line 1553
Quoted 8 lines
add-patch.c +1
-1@@ -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);
Junio pointed out in the last round that this really could use a separate submission, and I tend to agree. It's really nothing to do with cmake...
PATCH v3 4/5 cmake: avoid editing t/test-lib.sh
In reply to Johannes Schindelin via GitGitGadgetFrom: Johannes Schindelin <[email protected]>
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).
--- .gitignore | 1 + Makefile | 1 + contrib/buildsystems/CMakeLists.txt | 7 +------ t/test-lib.sh | 10 ++++++++++ 4 files changed, 13 insertions(+), 6 deletions(-)
.gitignore +1 -0
@@ -1,4 +1,5 @@ /fuzz_corpora +/GIT-BUILD-DIR /GIT-BUILD-OPTIONS /GIT-CFLAGS /GIT-LDFLAGS
Makefile +1 -0
@@ -3029,6 +3029,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
contrib/buildsystems/CMakeLists.txt +1 -6
@@ -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/)
t/test-lib.sh +10 -0
@@ -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:
Signature · 1 line
gitgitgadget
Re: [PATCH v3 4/5] cmake: avoid editing t/test-lib.sh
Quoted 20 lines · Johannes Schindelin via GitGitGadget
From: Johannes Schindelin <[email protected]>
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.shto overrideGIT_BUILD_DIRto point somewhere else than the parent of thet/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.shto detect the presence of a file calledGIT-BUILD-DIRin 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.
Quoted 7 lines
To support building Git via a regular
makeinvocation after building it using CMake, we ensure that theGIT-BUILD-DIRfile is deleted (for convenience, this is done as part of theMakefilerule that is already run with everymakeinvocation to ensure thatGIT-BUILD-OPTIONSis up to date).Signed-off-by Johannes Schindelin <[email protected]>
Re my earlier feedback, I came up with this as an alternative, which nicely allows us to have "cmake" and "make" play together, you can even run them concurrently!:
https://github.com/avar/git/commit/30f2265fd07aee97ea66f6e84a824d85d241e245
In case that OID changes it's on my
https://github.com/avar/git/commits/avar/cmake-test-path branch, currently 30f2265fd07 (cmake & test-lib.sh: add a $GIT_SOURCE_DIR variable, 2022-10-14).
And it...
Makefile else · line 3032
Quoted 9 lines
Makefile +1
-0@@ -3029,6 +3029,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
...allows us to get rid of this, which you understandably need with your approach, but which I'd *really* prefer we not have. Let's not sneak things into make's dependency DAG that it doesn't know about in FORCE'd shell command (but more on that later).
Quoted 12 lines
Makefile +0
-0### Detect Python interpreter path changes ifndef NO_PYTHONcontrib/buildsystems/CMakeLists.txt +0
-2@@ -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})
contrib/buildsystems/CMakeLists.txt endif() · line 1075
Quoted 10 lines
contrib/buildsystems/CMakeLists.txt +1
-4#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/)
...and this whole section just goes away, we don't need any cmake-specifi hacking here, and actually it's not cmake-specific at all. It's just a "GIT_TEST_INSTALLED for things that are built, not installed". E.g.:
(cd g/git.scratch && make)
(cd g/git && make clean && GIT_TEST_BUILD_DIR="$PWD/../git.scratch" make -C t)Supporting cmake then just becomes a special-case of test-lib.sh knowing "hey, my built stuff is at <dir> instead of "../".
Quoted 8 lines
t/test-lib.sh +0
-0@@ -47,6 +47,16 @@ then echo "PANIC: Running in a $TEST_DIRECTORY that doesn't end in '/t'?" >&2 exit 1 fi
t/test-lib.sh then · line 59
Quoted 10 lines
t/test-lib.sh +10
-0+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
...but one thing that I migh thave missed (and would really appreciate your testing for) is that I didn't invoke cygpath in my version. CI passes, but since Windows CI doesn't use "ctest" that doesn't tell us much, and in any case that's Cygwin, no, which we don't run anyway there?
Anyway, we could run that "cypath" easily in the cmake recipe itself, or just pass a "hey, please make this canonical" flag to test-lib.sh.
But anyway, one thing that approach explicitly leaves out is that you want to be able to:
1. Build with cmake
2. cd t
3. Run a test
And have the test itself know to locate and use the cmake binaries instead of the "main" binaries.
Now, I suspect that we don't actually have cases anyone cares about where we have *both*, but that's how this code behaves. I.e. a top-level:
make test
Will wpe that GIT-BUILD-DIR and use the "make" built "git", but e.g.:
make
<build with cmake>
cd t
# At this point I forgot I used cmake earlier ./t0001-init.sh # silently uses cmake...
I can see thy case for auto-discovery, per the IDE case you mentioned, but isn't it much better to just make this part of the slightly later part (but we need to set it up here now) part where we discover the built "git" and:
A. Do we not have it in ../git?
B. Do we have it it contrib/buildsystems/out/git
Then (pseudocode):
if (!A && B)
use_cmake();
else if (A && B)
die("you have both, pick one!");Or just say that "make" entry points always run with stuff it built, and "ctest" runs with contrib/buildsystems/out/git, that's explicitly what you don't want though...
Anyway, to wrap this up, I really wish the interaction between the two wouldn't have these pitfalls. I get that you want to support it on the specific Windows IDE case, but can't we more narrowlry do that without:
(cd t && ./t0001-init.sh)
Having this new "pick either one" behavior? Cheers.
Re: [PATCH v3 4/5] cmake: avoid editing t/test-lib.sh
Hi Ævar,
you did not even give me a chance to send my reply to your original mail ;-)
Quoted 29 lines · Ævar Arnfjörð Bjarmason
Quoted 26 lines · Johannes Schindelin via GitGitGadget
From: Johannes Schindelin <[email protected]>
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.shto overrideGIT_BUILD_DIRto point somewhere else than the parent of thet/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.shto detect the presence of a file calledGIT-BUILD-DIRin 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
makeinvocation after building it using CMake, we ensure that theGIT-BUILD-DIRfile is deleted (for convenience, this is done as part of theMakefilerule that is already run with everymakeinvocation to ensure thatGIT-BUILD-OPTIONSis up to date).
Quoted 7 lines
Quoted 1 line
Signed-off-by Johannes Schindelin <[email protected]>Re my earlier feedback, I came up with this as an alternative, which nicely allows us to have "cmake" and "make" play together, you can even run them concurrently!:
https://github.com/avar/git/commit/30f2265fd07aee97ea66f6e84a824d85d241e245
This approach _still_ modifies the test-lib.sh, which is the entire reason for the patch under review.
I hope you find an elegant, user-friendly alternative that leaves test-lib.sh unmodified even when building via CMake. I would gladly take that and drop my GIT-BUILD-DIR patch.
Ciao,
Johannes
PATCH v3 5/5 cmake: increase time-out for a long-running test
In reply to Johannes Schindelin via GitGitGadgetFrom: Johannes Schindelin <[email protected]>
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.
--- contrib/buildsystems/CMakeLists.txt | 4 ++++ 1 file changed, 4 insertions(+)
contrib/buildsystems/CMakeLists.txt +4 -0
@@ -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
Signature · 1 line
gitgitgadget