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

Re: [PATCH v2 0/5] Some fixes and an improvement for using CTest on Windows

From
Victoria Dye <vdye@github.com>
Date
Sep 8, 2022, 03:51 UTC
Message-ID
<2313a3ee-8baf-3589-a90b-cd7ee3bf5dfa@github.com>
In-Reply-To
<pull.1320.v2.git.1661243463.gitgitgadget@gmail.com>
Johannes Schindelin via GitGitGadget wrote:
Show 39 quoted lines
> 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!
Show 69 quoted 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/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
> 
Previous: Johannes Schindelin via GitGitGadgetNext: Johannes Schindelin via GitGitGadget
Message 47 of 58 in “Some fixes and an improvement for using CTest on Windows”
  1. 0/5 Some fixes and an improvement for using CTest on WindowsJohannes Schindelin via GitGitGadget, Aug 10, 2022
  2. 1/5 cmake: align CTest definition with Git's CI runsJohannes Schindelin via GitGitGadget, Aug 10, 2022
  3. Junio C HamanoAug 10, 2022
  4. Johannes SchindelinAug 16, 2022
  5. Junio C HamanoAug 16, 2022
  6. Johannes SchindelinAug 19, 2022
  7. Ævar Arnfjörð BjarmasonAug 11, 2022
  8. 2/5 cmake: copy the merge tools for testingJohannes Schindelin via GitGitGadget, Aug 10, 2022
  9. 3/5 tests: explicitly skip `chmod` calls on WindowsJohannes Schindelin via GitGitGadget, Aug 10, 2022
  10. Ævar Arnfjörð BjarmasonAug 11, 2022
  11. Johannes SchindelinAug 22, 2022
  12. Johannes SchindelinAug 23, 2022
  13. 4/5 add -p: avoid ambiguous signed/unsigned comparisonJohannes Schindelin via GitGitGadget, Aug 10, 2022
  14. Junio C HamanoAug 10, 2022
  15. Johannes SchindelinAug 16, 2022
  16. Junio C HamanoAug 16, 2022
  17. Johannes SchindelinAug 19, 2022
  18. Phillip WoodAug 11, 2022
  19. Johannes SchindelinAug 16, 2022
  20. Phillip WoodAug 16, 2022
  21. Johannes SchindelinAug 19, 2022
  22. 5/5 cmake: avoid editing t/test-lib.shJohannes Schindelin via GitGitGadget, Aug 10, 2022
  23. Ævar Arnfjörð BjarmasonAug 11, 2022
  24. Johannes SchindelinOct 18, 2022
  25. Phillip WoodAug 11, 2022
  26. Johannes SchindelinAug 16, 2022
  27. Phillip WoodAug 16, 2022
  28. 0/5 Some fixes and an improvement for using CTest on WindowsJohannes Schindelin via GitGitGadget, Aug 23, 2022
  29. 4/5 cmake: avoid editing t/test-lib.shJohannes Schindelin via GitGitGadget, Aug 23, 2022
  30. Ævar Arnfjörð BjarmasonSep 8, 2022
  31. Johannes SchindelinOct 18, 2022
  32. Ævar Arnfjörð BjarmasonOct 18, 2022
  33. Victoria DyeSep 8, 2022
  34. Victoria DyeSep 8, 2022
  35. Junio C HamanoSep 8, 2022
  36. Johannes SchindelinOct 18, 2022
  37. 5/5 cmake: increase time-out for a long-running testJohannes Schindelin via GitGitGadget, Aug 23, 2022
  38. Ævar Arnfjörð BjarmasonSep 8, 2022
  39. Victoria DyeSep 8, 2022
  40. 2/5 cmake: copy the merge tools for testingJohannes Schindelin via GitGitGadget, Aug 23, 2022
  41. 1/5 cmake: make it easier to diagnose regressions in CTest runsJohannes Schindelin via GitGitGadget, Aug 23, 2022
  42. Victoria DyeSep 7, 2022
  43. Johannes SchindelinOct 18, 2022
  44. Ævar Arnfjörð BjarmasonSep 8, 2022
  45. Eric SunshineSep 28, 2022
  46. 3/5 add -p: avoid ambiguous signed/unsigned comparisonJohannes Schindelin via GitGitGadget, Aug 23, 2022
  47. Victoria DyeSep 8, 2022
  48. 0/5 Some fixes and an improvement for using CTest on WindowsJohannes Schindelin via GitGitGadget, Oct 18, 2022
  49. 1/5 cmake: make it easier to diagnose regressions in CTest runsJohannes Schindelin via GitGitGadget, Oct 18, 2022
  50. Ævar Arnfjörð BjarmasonOct 18, 2022
  51. 2/5 cmake: copy the merge tools for testingJohannes Schindelin via GitGitGadget, Oct 18, 2022
  52. Ævar Arnfjörð BjarmasonOct 18, 2022
  53. 3/5 add -p: avoid ambiguous signed/unsigned comparisonJohannes Schindelin via GitGitGadget, Oct 18, 2022
  54. Ævar Arnfjörð BjarmasonOct 18, 2022
  55. 4/5 cmake: avoid editing t/test-lib.shJohannes Schindelin via GitGitGadget, Oct 18, 2022
  56. Ævar Arnfjörð BjarmasonOct 18, 2022
  57. Johannes SchindelinOct 18, 2022
  58. 5/5 cmake: increase time-out for a long-running testJohannes Schindelin via GitGitGadget, Oct 18, 2022

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

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