Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refine complex_graph regex_matches partial suppressions #1719

Merged

Conversation

EliahKagan
Copy link
Member

@EliahKagan EliahKagan commented Dec 10, 2024

This builds on #1635 along the lines of the suggestion in #1622 (comment), by:

  • Updating the comment about #1622, including by adding links to Git mailing list posts by Aarni Koskela, who discovered the bug that turns out to be the cause of this, and Patrick Steinhardt, who analyzed the bug and wrote a fix (currently in testing); and #1622 (comment), which summarizes that and reports on its connection to #1622.

  • Narrowing the partial suppression of the failing test code (which consists of conditionally using parse_spec_no_baseline instead of parse_spec in some assertions) so that it is only done if Git is at one of the versions that is known to be affected.

    If any future Git versions are affected, such as by the currently cooking patch not being merged as soon as I expect, then this could potentially fail on CI again. But that is something we would probably want to find out about.

This builds on GitoxideLabs#1635 by:

- Updating the comment about GitoxideLabs#1622, including by adding links to
  Git mailing list posts by Aarni Koskela, who discovered the bug
  that turns out to be the cause of this, and Patrick Steinhardt,
  who analyzed the bug and wrote a fix (currently in testing); and
  GitoxideLabs#1622 (comment),
  which summarizes that and reports on its connection to GitoxideLabs#1622.

- Narrowing the partial suppression of the failing test code (which
  consists of conditionally using `parse_spec_no_baseline` instead
  of `parse_spec` in some assertions) so that it is only done if
  Git is at one of the versions that is known to be affected.

  If any future Git versions are affected, such as by the currently
  cooking patch not being merged as soon as I expect, then this
  could potentially fail on CI again. But that is something we
  would probably want to find out about.
@EliahKagan EliahKagan force-pushed the run-ci/complex-graph-no-baseline branch from 72eacb8 to f4b4bf0 Compare December 10, 2024 14:52
Copy link
Member

@Byron Byron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the perfect implementation to solve the problem at hand!

I had something much more simplistic in mind, but also something that would have been very broken :D.

@Byron Byron enabled auto-merge December 10, 2024 15:20
@EliahKagan
Copy link
Member Author

EliahKagan commented Dec 10, 2024

The pure-rust-build job is stalled on the apt-get update command in the container. This didn't happen in my fork, and rerunning that job in my fork a couple times doesn't produce it. So I recommend canceling and rerunning that job here. I expect that it will pass if that is done.

This occasionally happens locally but I don't think I've seen it on CI before. Maybe using the us. country mirror would make it less likely (since I think the GitHub-hosted runners are usually deployed in regions in the US). This should be easy to do, but hard to test effectively if the problem occurs only rarely. It may be possible to observe whether that makes things faster, though. Anyway, whether or not such a change is made, I don't recommend it be done as part of this PR.

@Byron
Copy link
Member

Byron commented Dec 10, 2024

Thanks for the hint - I cancelled and restarted and would think that it will go through now.

From my experience it's very rare indeed, and I'd hope it stays that way.

@Byron Byron merged commit f8ba4b9 into GitoxideLabs:main Dec 10, 2024
20 checks passed
@EliahKagan EliahKagan deleted the run-ci/complex-graph-no-baseline branch December 10, 2024 16:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants