From: Victoria Dye <vdye@github.com>
To: Johannes Schindelin via GitGitGadget <gitgitgadget@gmail.com>,
git@vger.kernel.org
Cc: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>,
"Phillip Wood" <phillip.wood123@gmail.com>,
"Johannes Schindelin" <johannes.schindelin@gmx.de>
Subject: Re: [PATCH v2 0/5] Some fixes and an improvement for using CTest on Windows
Date: Wed, 7 Sep 2022 20:51:55 -0700 [thread overview]
Message-ID: <2313a3ee-8baf-3589-a90b-cd7ee3bf5dfa@github.com> (raw)
In-Reply-To: <pull.1320.v2.git.1661243463.gitgitgadget@gmail.com>
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
>
next prev parent reply other threads:[~2022-09-08 3:52 UTC|newest]
Thread overview: 58+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-08-10 15:02 [PATCH 0/5] Some fixes and an improvement for using CTest on Windows Johannes Schindelin via GitGitGadget
2022-08-10 15:02 ` [PATCH 1/5] cmake: align CTest definition with Git's CI runs Johannes Schindelin via GitGitGadget
2022-08-10 17:48 ` Junio C Hamano
2022-08-16 10:11 ` Johannes Schindelin
2022-08-16 15:15 ` Junio C Hamano
2022-08-19 13:57 ` Johannes Schindelin
2022-08-11 11:18 ` Ævar Arnfjörð Bjarmason
2022-08-10 15:02 ` [PATCH 2/5] cmake: copy the merge tools for testing Johannes Schindelin via GitGitGadget
2022-08-10 15:02 ` [PATCH 3/5] tests: explicitly skip `chmod` calls on Windows Johannes Schindelin via GitGitGadget
2022-08-11 11:22 ` Ævar Arnfjörð Bjarmason
2022-08-22 10:19 ` Johannes Schindelin
2022-08-23 7:34 ` Johannes Schindelin
2022-08-10 15:02 ` [PATCH 4/5] add -p: avoid ambiguous signed/unsigned comparison Johannes Schindelin via GitGitGadget
2022-08-10 17:54 ` Junio C Hamano
2022-08-16 9:56 ` Johannes Schindelin
2022-08-16 15:10 ` Junio C Hamano
2022-08-19 14:52 ` Johannes Schindelin
2022-08-11 12:49 ` Phillip Wood
2022-08-16 10:00 ` Johannes Schindelin
2022-08-16 14:23 ` Phillip Wood
2022-08-19 14:07 ` Johannes Schindelin
2022-08-10 15:02 ` [PATCH 5/5] cmake: avoid editing t/test-lib.sh Johannes Schindelin via GitGitGadget
2022-08-11 11:35 ` Ævar Arnfjörð Bjarmason
2022-10-18 14:02 ` Johannes Schindelin
2022-08-11 12:58 ` Phillip Wood
2022-08-16 10:09 ` Johannes Schindelin
2022-08-16 14:27 ` Phillip Wood
2022-08-23 8:30 ` [PATCH v2 0/5] Some fixes and an improvement for using CTest on Windows Johannes Schindelin via GitGitGadget
2022-08-23 8:30 ` [PATCH v2 1/5] cmake: make it easier to diagnose regressions in CTest runs Johannes Schindelin via GitGitGadget
2022-09-07 22:10 ` Victoria Dye
2022-10-18 14:02 ` Johannes Schindelin
2022-09-08 7:22 ` Ævar Arnfjörð Bjarmason
2022-09-28 6:55 ` Eric Sunshine
2022-08-23 8:31 ` [PATCH v2 2/5] cmake: copy the merge tools for testing Johannes Schindelin via GitGitGadget
2022-08-23 8:31 ` [PATCH v2 3/5] add -p: avoid ambiguous signed/unsigned comparison Johannes Schindelin via GitGitGadget
2022-08-23 8:31 ` [PATCH v2 4/5] cmake: avoid editing t/test-lib.sh Johannes Schindelin via GitGitGadget
2022-09-08 7:39 ` Ævar Arnfjörð Bjarmason
2022-10-18 14:03 ` Johannes Schindelin
2022-10-18 15:09 ` Ævar Arnfjörð Bjarmason
2022-09-08 23:37 ` Victoria Dye
2022-09-08 23:42 ` Victoria Dye
2022-09-08 23:58 ` Junio C Hamano
2022-10-18 14:03 ` Johannes Schindelin
2022-08-23 8:31 ` [PATCH v2 5/5] cmake: increase time-out for a long-running test Johannes Schindelin via GitGitGadget
2022-09-08 7:34 ` Ævar Arnfjörð Bjarmason
2022-09-08 17:29 ` Victoria Dye
2022-09-08 3:51 ` Victoria Dye [this message]
2022-10-18 10:59 ` [PATCH v3 0/5] Some fixes and an improvement for using CTest on Windows Johannes Schindelin via GitGitGadget
2022-10-18 10:59 ` [PATCH v3 1/5] cmake: make it easier to diagnose regressions in CTest runs Johannes Schindelin via GitGitGadget
2022-10-18 13:41 ` Ævar Arnfjörð Bjarmason
2022-10-18 10:59 ` [PATCH v3 2/5] cmake: copy the merge tools for testing Johannes Schindelin via GitGitGadget
2022-10-18 13:49 ` Ævar Arnfjörð Bjarmason
2022-10-18 10:59 ` [PATCH v3 3/5] add -p: avoid ambiguous signed/unsigned comparison Johannes Schindelin via GitGitGadget
2022-10-18 13:53 ` Ævar Arnfjörð Bjarmason
2022-10-18 10:59 ` [PATCH v3 4/5] cmake: avoid editing t/test-lib.sh Johannes Schindelin via GitGitGadget
2022-10-18 13:54 ` Ævar Arnfjörð Bjarmason
2022-10-18 14:21 ` Johannes Schindelin
2022-10-18 10:59 ` [PATCH v3 5/5] cmake: increase time-out for a long-running test Johannes Schindelin via GitGitGadget
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=2313a3ee-8baf-3589-a90b-cd7ee3bf5dfa@github.com \
--to=vdye@github.com \
--cc=avarab@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitgitgadget@gmail.com \
--cc=johannes.schindelin@gmx.de \
--cc=phillip.wood123@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.