ci(win+Meson): build in Release mode, avoiding t7001-mv hangs #1908
+1
−1
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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). Signed-off-by: Johannes Schindelin <[email protected]>
|
/submit |
|
Submitted as [email protected] To fetch this version into To fetch this version to local tag |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): "Johannes Schindelin via GitGitGadget" <[email protected]>
writes:
> From: Johannes Schindelin <[email protected]>
>
> 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.
With mine too and I would presume everybody else's. I've been
annoyed for a long time by one of those sharded Meson-Win test jobs
that hang around until timeout.
Thank you very much for addressing the issue.
> 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.
Interesting.
So another viable fix (no, I am not suggesting a counter-proposal,
but asking a pure question to see if I understand the issue
correctly) is to rewrite "assert(cond)" to "if (cond) BUG(...)"
or something like that, so that it truly fails?
> 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).
OK.
> Signed-off-by: Johannes Schindelin <[email protected]>
> ---
> 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.
>
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1908%2Fdscho%2Fdont-let-win%2BMeson-hang-in-t7001-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1908/dscho/dont-let-win+Meson-hang-in-t7001-v1
> Pull-Request: https://github.com/gitgitgadget/git/pull/1908
>
> .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 |
|
This patch series was integrated into seen via git@018e242. |
|
This patch series was integrated into seen via git@ebaf1e2. |
|
On the Git mailing list, Patrick Steinhardt wrote (reply to this): On Fri, Apr 25, 2025 at 08:18:02AM -0700, Junio C Hamano wrote:
> "Johannes Schindelin via GitGitGadget" <[email protected]>
> writes:
>
> > From: Johannes Schindelin <[email protected]>
> >
> > 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.
>
> With mine too and I would presume everybody else's. I've been
> annoyed for a long time by one of those sharded Meson-Win test jobs
> that hang around until timeout.
>
> Thank you very much for addressing the issue.
Indeed, thanks for fixing the issue! I haven't noticed these failing
jobs yet on my end, probably because on GitLab we only execute the
Win+Meson changes manually. Which is not great given that it makes me
miss issues with Meson like this.
I'll send a patch to run these tests by default so that I can see the
pain myself and address any issues that come up before others are forced
to.
> > 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.
>
> Interesting.
>
> So another viable fix (no, I am not suggesting a counter-proposal,
> but asking a pure question to see if I understand the issue
> correctly) is to rewrite "assert(cond)" to "if (cond) BUG(...)"
> or something like that, so that it truly fails?
On the surface this sounds like a reasonable thing to do, but I don't
have enough context to be really able to tell.
> > 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
So I'm fine with this trivial change as a band aid for now. The diff
looks obviously good to me.
Patrick |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): Patrick Steinhardt <[email protected]> writes:
>> > 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.
>>
>> Interesting.
>>
>> So another viable fix (no, I am not suggesting a counter-proposal,
>> but asking a pure question to see if I understand the issue
>> correctly) is to rewrite "assert(cond)" to "if (cond) BUG(...)"
>> or something like that, so that it truly fails?
>
> On the surface this sounds like a reasonable thing to do, but I don't
> have enough context to be really able to tell.
Interesting again ;-) I didn't realize that it was a fairly recent
development. 0fcd473f (t7001: add failure test which triggers
assertion, 2024-10-22) is what adds the questionable test.
And I do agree with Dscho's assessment that this is "show a bug
without bothering to fix it", which is not what we usually take
without first exploring how involved the necessary fix would be.
I wonder in what bad status would a production build that simply
disabled the assert() is leaving the resulting repository.
Quoting from the last part of my response [*] to the initial report
that eventually turned into the test after 9 months:
[*] https://lore.kernel.org/git/[email protected]/
---- snip snap ----
Thanks for reporting, Kristoffer.
Any takers?
$ git shortlog --since=3.years -s -n -e --no-merges v2.43.0 builtin/mv.c
15 Shaoxuan Yuan <[email protected]>
10 Elijah Newren <[email protected]>
5 Ævar Arnfjörð Bjarmason <[email protected]>
2 Junio C Hamano <[email protected]>
1 Andrzej Hunt <[email protected]>
1 Calvin Wan <[email protected]>
1 Derrick Stolee <[email protected]>
1 Sebastian Thiel <[email protected]>
1 Torsten Bögershausen <[email protected]> |
|
This branch is now known as |
|
This patch series was integrated into seen via git@28131fe. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I was surprised to find this issue today, and that this had not been addressed yet.
Cc: Patrick Steinhardt [email protected]