| Message ID | pull.1320.v2.git.1661243463.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Some fixes and an improvement for using CTest on Windows | expand |
Johannes Schindelin via GitGitGadget wrote: > 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 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 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! > > .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 <Johannes.Schindelin@gmx.de> > > ## 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. > > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> > > 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. > + > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> > > ## 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 >
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 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 <Johannes.Schindelin@gmx.de> ## 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. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> 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. + Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> ## 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