From: Johannes Schindelin <johannes.schindelin@xxxxxx>
When the `win+Meson` job was added to Git's CI, modeled after the
`win+vs` job, it overlooked that the latter built the Git artifacts in
release mode.
The reason for this is that there is code in `compat/mingw.c` that turns
on the modal assertion dialogs in debug mode, which are very useful when
debugging interactively (as they offer to attach Visual Studio's
debugger), but they are scarcely useful in CI builds (where that modal
dialog would sit around, waiting for a human being to see and deal with
it, which obviously won't ever happen).
This problem was not realized immediately because of a separate bug: the
`win+Meson` job erroneously built using the `gcc` that is in the `PATH`
by default on hosted GitHub Actions runners. Since that bug was fixed by
switching to `--vsenv`, though, the t7001-mv test consistently timed out
after six hours in the CI builds on GitHub, quite often, and wasting
build minutes without any benefit in return.
The reason for this timeout was a symptom of aforementioned debug mode
problem, where the test case 'nonsense mv triggers assertion failure and
partially updated index' in t7001-mv triggered an assertion.
I originally proposed this here patch to address the timeouts in CI
builds. The Git project decided to address this timeout differently,
though: by fixing the bug that the t7001-mv test case demonstrated. This
does not address the debug mode problem, though, as an `assert()` call
could be triggered in other ways in CI, and it should still not cause
the CI build to hang but should cause Git to error out instead. To avoid
having to accept this here patch, it was then proposed to replace all
`assert()` calls in Git's code base by `BUG()` calls. This might be
reasonable for independent reasons, but it obviously still does not
address the debug mode problem, as `assert()` calls could be easily
re-introduced by mistake, and besides, Git has a couple of dependencies
that all may have their own `assert()` calls (which are then safely
outside the control of the Git project to remove), therefore this here
patch is still needed.
Signed-off-by: Johannes Schindelin <johannes.schindelin@xxxxxx>
---
ci(win+Meson): build in Release mode, avoiding t7001-mv hangs
I was surprised to find this issue today, and that this had not been
addressed yet.
Changes since v1:
* Rewrote the commit message to reflect that this patch is still
needed, even if the symptom that originally motivated the patch was
addressed in a different manner, because it was merely a symptom of
the underlying root cause that CI builds should not let Visual C
build Git in debug mode.
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1908%2Fdscho%2Fdont-let-win%2BMeson-hang-in-t7001-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1908/dscho/dont-let-win+Meson-hang-in-t7001-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1908
Range-diff vs v1:
1: 18d1d18c48a ! 1: c04b6c97b25 ci(win+Meson): build in Release mode, avoiding t7001-mv hangs
@@ Metadata
Author: Johannes Schindelin <Johannes.Schindelin@xxxxxx>
## Commit message ##
- ci(win+Meson): build in Release mode, avoiding t7001-mv hangs
-
- Since switching to `--vsenv`, the t7001-mv test consistently times out
- after six hours in the CI builds on GitHub. This kind of waste is
- inconsistent with my values.
-
- The reason for this timeout is the test case 'nonsense mv triggers
- assertion failure and partially updated index' in t7001-mv (which is
- not even a regression test, but instead merely demonstrates a bug that
- someone thought someone else should fix at some time). As the name
- suggests, it triggers an assertion. The problem with this is that an
- assertion on Windows, at least when run in Debug mode, will open a modal
- dialog that patiently awaits some buttons to be clicked. Which never
- happens in automated builds.
-
- The solution is straight-forward: Just like the `win+VS` job already did
- in forever, build in Release mode (where that modal assertion dialog is
- never shown).
+ ci(win+Meson): build in Release mode
+
+ When the `win+Meson` job was added to Git's CI, modeled after the
+ `win+vs` job, it overlooked that the latter built the Git artifacts in
+ release mode.
+
+ The reason for this is that there is code in `compat/mingw.c` that turns
+ on the modal assertion dialogs in debug mode, which are very useful when
+ debugging interactively (as they offer to attach Visual Studio's
+ debugger), but they are scarcely useful in CI builds (where that modal
+ dialog would sit around, waiting for a human being to see and deal with
+ it, which obviously won't ever happen).
+
+ This problem was not realized immediately because of a separate bug: the
+ `win+Meson` job erroneously built using the `gcc` that is in the `PATH`
+ by default on hosted GitHub Actions runners. Since that bug was fixed by
+ switching to `--vsenv`, though, the t7001-mv test consistently timed out
+ after six hours in the CI builds on GitHub, quite often, and wasting
+ build minutes without any benefit in return.
+
+ The reason for this timeout was a symptom of aforementioned debug mode
+ problem, where the test case 'nonsense mv triggers assertion failure and
+ partially updated index' in t7001-mv triggered an assertion.
+
+ I originally proposed this here patch to address the timeouts in CI
+ builds. The Git project decided to address this timeout differently,
+ though: by fixing the bug that the t7001-mv test case demonstrated. This
+ does not address the debug mode problem, though, as an `assert()` call
+ could be triggered in other ways in CI, and it should still not cause
+ the CI build to hang but should cause Git to error out instead. To avoid
+ having to accept this here patch, it was then proposed to replace all
+ `assert()` calls in Git's code base by `BUG()` calls. This might be
+ reasonable for independent reasons, but it obviously still does not
+ address the debug mode problem, as `assert()` calls could be easily
+ re-introduced by mistake, and besides, Git has a couple of dependencies
+ that all may have their own `assert()` calls (which are then safely
+ outside the control of the Git project to remove), therefore this here
+ patch is still needed.
Signed-off-by: Johannes Schindelin <johannes.schindelin@xxxxxx>
.github/workflows/main.yml | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml
index 83ca8e4182b..275240be5dc 100644
--- a/.github/workflows/main.yml
+++ b/.github/workflows/main.yml
@@ -265,7 +265,7 @@ jobs:
run: pip install meson ninja
- name: Setup
shell: pwsh
- run: meson setup build --vsenv -Dperl=disabled -Dcredential_helpers=wincred
+ run: meson setup build --vsenv -Dbuildtype=release -Dperl=disabled -Dcredential_helpers=wincred
- name: Compile
shell: pwsh
run: meson compile -C build
base-commit: f65182a99e545d2f2bc22e6c1c2da192133b16a3
--
gitgitgadget
