fix CI #1635
fix CI #1635
Conversation
There was a problem hiding this comment.
Yes, the problem this works around is #1622. But I think there are actually two assertions that fail. I was about to open a PR with a18cc22, but this PR came first. The commit message and diff there may shed further light on exactly what is affected.
I think the approach here of limiting the skipping to CI is better than what I have in a18cc22. But I think the approach in a18cc22 of changing parse_spec to parse_spec_no_baseline is better than the approach of skipping the full assertions. As detailed in #1622, it is only parse_spec that fails, and in it, what fails is only the baseline check. In #1622 I had suggested an overcomplicated approach to doing it without the baseline check; actually, replacing parse_spec with parse_spec_no_baseline achieves this.
Byron
force-pushed
the
fix-ci
branch
3 times, most recently
from
0ba8a31 to
447878f
Compare
|
Thanks a lot for chiming in! |
There was a problem hiding this comment.
This looks great, and definitely includes the good parts of a18cc22.
As for the complexity, in terms of reading ease it should be possible to avoid the duplication by conditionally assigning either parse_spec or parse_spec_no_baseline to a variable. But I think I slightly prefer the way you have done it, since I expect that assertion messages will be clearer this way where it's more explicit, in the assertions, what is being done.
I have one thought about a way to make the change even narrower while still working, commented below.
| // On the CI linux workflow, archived baseline aren't used - instead fixtures will be re-evaluated. | ||
| // As of Git 2.47, its behaviour changed which makes the following assertion fail. | ||
| // We decided to just ignore it until it's clear that this isn't a bug - obviously the traversal order changed. | ||
| let is_in_test_ci_workflow = is_ci::cached() && cfg!(target_os = "linux"); |
There was a problem hiding this comment.
If this were to also require that GIX_TEST_IGNORE_ARCHIVES be set (or, if that is judged too complex, then to replace the linux check with such a check), then parse_spec would continue to be used in the CI test_fast jobs that don't need it because their baselines were computed with an earlier versions of Git and agree with the current gitoxide behavior, while parse_spec_no_baseline would be used in the CI test job that needs it.
There was a problem hiding this comment.
Thank you, that's a much better selector actually.
This PR is a good example for quick & sloppy, but thanks to you it's not going to be merged that way :).
On the CI linux workflow, archived baseline aren't used - instead fixtures will be re-evaluated. As of Git 2.47, its behaviour changed which makes the following assertion fail. We decided to just ignore it until it's clear that this isn't a bug - obviously the traversal order changed. ---- This temporarily suppresses baseline comparisons for two patterns where the baseline checks fail if the `complex_graph` fixture script was run in Git 2.47.0. For now, they are unconditionally suppressed, regardless of the Git version or whether generated archives are used. This change is specific to the `find_youngest_matching_commit::regex_matches` test. This works around, but does not fix, issue #1622. The affected test is still run, but the two directly affected baseline checks are skipped. Co-authored-by: Eliah Kagan <degeneracypressure@gmail.com>
Co-authored-by: Eliah Kagan <degeneracypressure@gmail.com>
In the preceding two commits, the make_rev_spec_parse_repos fixture was modified to avoid giving extra executable permissions to a loose object file where they are not needed, and the affected fixture archive was regenerated. Though the permissions change is itself good and causes no problems, the overall change caused two problems, which are corrected here: 1. I had taken the opportunity to follow better practices when running commands in a shell script whose arguments are formed by parameter expansion: adding quoting where splitting and globbing is not intended but could in principle also be indicated; and preceding the argument formed this way with a `--` to designate it clearly as a non-option argument, since `chmod` follows the XBD Utility Syntax Guidelines, which include `--` recognition. While adding quoting was a good change (in this case, just for clarity that no expansions are intended), the way I added `--` created a new problem where none had existed. This is because I wrongly thought of it as separating non-filename arguments from filename arguments, which is incorrect: in `chmod`, a mode argument is neither an option or an operand to an option. Accordingly, only some implementations of `chmod` allow it to be placed after the mode. This commit corrects that by placing it before the mode argument instead, which is portable while still achieving the goal of establishing the argument after it as as never being meant to be interpreted as an option (regardless of whether the system's `chmod` recognizes options after non-option arguments). 2. Due to GitoxideLabs#1622, regenerating `make_rev_spec_parse_repos.tar` with Git 2.47.0 causes revision::spec::from_bytes::regex::find_youngest_matching_commit::regex_matches to to fail on all systems with all versions of Git, whenever `GIX_TEST_IGNORE_ARCHIVES` is *not* set. This differs from the usual situation where it fails only when that *is* set and only when the available Git is >= 2.47.0. This causes the test to fail in the `test-fast` CI job, since the mitigation in GitoxideLabs#1635 for when the tests are detected to be running on CI deliberately covers only the `GIX_TEST_IGNORE_ARCHIVES` case. In the previous commit, I had regenerated that archive on an Ubuntu 24.04 LTS system with Git 2.47.0 installed from the git-core PPA, causing this problem. This commit regenerates the archive again on a macOS 15.0.1 system with Git 2.39.5 (Apple Git-154), using the command: TZ=UTC cargo nextest run --all --no-fail-fast All tests passed and the archive was successfully remade. I used `TZ=UTC` since I usually regenerate archives on a system whose time zone is configured to be UTC rather than local time, and more specifically because there is an unrelated bug (to be separately reported) causing an unrelated test to fail in some time zones in the two weeks that follow daylight saving time adjustments.
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.
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.
The bug in Git that causes GitoxideLabs#1622 has been fixed since 2.48.0 and does not affect any versions prior to 2.47, but the fix is not backported to subsequent 2.47.* point releases. In particular, 2.47.2 has been released, with backported security fixes, but it does not have backported fixes for this non-security bug. This builds on GitoxideLabs#1635 and GitoxideLabs#1719 by updating the range of Git versions where we skip the affected baseline checks on CI (for platforms this affects on our CI) from `(2, 47, 0)..(2, 47, 2)` to `(2, 47, 0)..(2, 48, 0)`, i.e., with the exclusive upper bound changed from 2.47.2 to the correct value of 2.48.0. This also revises the comments accordingly. For further details, see: GitoxideLabs#1622 (comment) (This change is item (1) there.)
CI images now us Git 2.47, which causes one assertion to fail.
CC @EliahKagan - it's happening :).