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

Use stable Git for Windows in test-fixtures-windows #1892

Merged

Conversation

EliahKagan
Copy link
Member

Now that Git for Windows 2.49.0 has a stable release, this changes the upgrade step that was added to test-fixtures-windows in 4237e5a (#1870), so that it downloads an installer from the release marked as "latest", rather than the release that has the newest tag. The release marked "latest" is usually a stable release in projects that have any stable releases, and in particular it is a stable release in Git for Windows.

This is not needed to switch from the release candidate to the stable release for 2.49.0. The download logic already in place currently gets the stable release automatically, because it is the newest tag.

Nonetheless, there are three reasons to prefer the "latest" tag to get the stable release, now that the stable release is available. In descending order of significance, they are:

  • We upgrade to work around #1849, for which 2.49.0 is preferable to 2.48.1 (which the Windows runner images currently have). Continuing to take the newest tag will eventually take a pre-release for the next version. That would probably work, but it is not currently a goal.

    There is sometimes a delay between when a stable release of Git for Windows comes out and when the stable runner images are released with it. (Pre-release runner images exist, but they are not run on GitHub-hosted runners.) So even assuming this upgrade step is to be removed once it is no longer needed, it could easily end up remaining long enough for a new Git for Windows pre-release to come out.

  • An update may potentially be released for an earlier minor version (y in x.y.z), in which case the tag for it would be newer and we would downgrade instead. Now that the release marked "latest" is usable here, we can use it and avoid that.

  • If we decide to eventually deliberately test pre-releases, the step added in #1849 would probably not be usable in that form, because it could take either the next pre-release or a patch to an ealier release per the above points, and also for the separate reason that this CI job is not necessarily where we would want to test that.

    (As one example, there is currently no CI testing of the Git for Windows SDK, even though supporting it, in general and for running the test suite, is an explicit goal discussed in #1758, #1761, #1862, and #1864. If that is added, it may be a more opportune way to test prereleases.)

This change also makes the logic slightly simpler, since gh has automatically chooses the release marked "latest" when no tag is specified (whereas before we had to find the tag in on gh release ... invocation to then use it in another).

Now that Git for Windows 2.49.0 has a stable release, this changes
the upgrade step that was added to `test-fixtures-windows` in
4237e5a (GitoxideLabs#1870), so that it downloads an installer from the release
marked as "latest", rather than the release that has the newest
tag. The release marked "latest" is usually a stable release in
projects that have any stable releases, and in particular it is a
stable release in Git for Windows.

This is *not* needed to switch from the release candidate to the
stable release for 2.49.0. The download logic already in place
currently gets the stable release automatically, because it is the
newest tag.

Nonetheless, there are three reasons to prefer the "latest" tag to
get the stable release, now that the stable release is available.
In descending order of significance, they are:

- We upgrade to work around GitoxideLabs#1849, for which 2.49.0 is preferable
  to 2.48.1 (which the Windows runner images currently have).
  Continuing to take the newest tag will eventually take a
  pre-release for the next version. That would probably work, but
  it is not currently a goal.

  There is sometimes a delay between when a stable release of Git
  for Windows comes out and when the stable runner images are
  released with it. (Pre-release runner images exist, but they are
  not run on GitHub-hosted runners.) So even assuming this upgrade
  step is to be removed once it is no longer needed, it could
  easily end up remaining long enough for a new Git for Windows
  pre-release to come out.

- An update may potentially be released for an earlier minor
  version (y in x.y.z), in which case the tag for it would be
  newer and we would downgrade instead. Now that the release
  marked "latest" is usable here, we can use it and avoid that.

- If we decide to eventually deliberately test pre-releases, the
  step added in GitoxideLabs#1849 would probably not be usable in that form,
  because it could take either the next pre-release or a patch to
  an ealier release per the above points, and also for the separate
  reason that this CI job is not necessarily where we would want to
  test that.

  (As one example, there is currently no CI testing of the Git for
  Windows SDK, even though supporting it, in general and for
  running the test suite, is an explicit goal discussed in GitoxideLabs#1758,
  GitoxideLabs#1761, GitoxideLabs#1862, and GitoxideLabs#1864. If that is added, it may be a more
  opportune way to test prereleases.)
The `ci.yml` workflow uses 2-space indents, including in scripts
written in script steps, but there was one place a 4-space ident
was used accidentally.
@EliahKagan
Copy link
Member Author

EliahKagan commented Mar 18, 2025

The actual affected code worked as expected (and as I observed in a previous run in my fork), and both here and in my fork the affected job, test-fixtures-windows, passed in all runs.
 
But in my fork, I got an unexpected failure of gix-fs::fs snapshot::journey in test-32bit in the arm32v7 job:

        FAIL [   0.005s] gix-fs::fs snapshot::journey
──── STDOUT:             gix-fs::fs snapshot::journey

running 1 test
test snapshot::journey ... FAILED

failures:

failures:
    snapshot::journey

test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 26 filtered out; finished in 0.00s

──── STDERR:             gix-fs::fs snapshot::journey

thread 'snapshot::journey' panicked at gix-fs/tests/fs/snapshot.rs:31:5:
assertion `left == right` failed: it picks up the change
  left: "content"
 right: "change"
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

My guess is that it is a rare failure that could sometimes happen and is not related to the change here, because:

  • It is in a completely different job from the only job that anything in this PR modified.
  • It didn't happen here, just in my fork.
  • It didn't happen on the previous commit even in my fork. The commit on which it happens only adjusts indentation for stylistic consistency.

I do find it odd, though. I have not seen such a failure before. The failing assertion is:

std::fs::write(&file_path, "change")?;
let snap = smut.recent_snapshot(check, open)?.expect("content read");
assert_eq!(&**snap, "change", "it picks up the change");

I'll rerun the whole workflow, both here and in my fork, and I'll assume this is okay to merge if is passes and nothing else goes wrong. But I wanted to mention this just in case it could somehow be related (?), and more so in case it is an unrelated rare intermittent failure that we manage to explain or to observe again.

Edit: When rerunning the workflow, the same test case has failed, now in the test-fast job on ubuntu-latest, in what seems to be the same way. Has something changed on main that could cause that test sometimes to fail?

Edit 2: It does not usually fail, but I am repeatedly rerunning the workflow, and I got one more failure, this time in the test-32bit job for i386. I have also been rerunning the tests at the tip of the main branch in my fork, which like the main branch here is at 0bf1d5b, and so far I have not managed to produce it there.

Edit 3: It can happen on main. I got a failure in test-fixtures-windows of that same test. It was the only unexpected failure in the run. I also see that this test was introduced recently in #1884. I hypothesize that this test, as written, occasionally fails, irrespective of any other changes since then. But I don't know what the problem is, nor whether it is in the test or the new code under test. In any case, I think this PR is in the clear.

Edit 4: Here's another occurrence on main in my fork when rerunning it, this time again on test-32bit for arm32v7, and another one after that on test-fast for windows-latest. In view of these. I am now convinced it is only by chance that it was first observed in this PR and that it is no more likely to occur here. I have opened #1896 to report this.

@EliahKagan EliahKagan merged commit 818e115 into GitoxideLabs:main Mar 18, 2025
121 of 129 checks passed
@EliahKagan EliahKagan deleted the run-ci/test-fixtures-windows-stable branch March 18, 2025 15:19
EliahKagan added a commit to EliahKagan/gitoxide that referenced this pull request Apr 4, 2025
Because GitoxideLabs#1849 (git-for-windows/git#5436)
caused some incorrect test failures in `test-fixtures-windows`, we
have been upgrading Git for Windows on the runner for that job:

- We starting doing that in of 4237e5a (GitoxideLabs#1870). At that time, we
  upgraded it to the most recent tag, to select the latest release
  even though it was a release candiate, because Git for Windows
  2.49.0 had not yet had a stable release.

- Then Git for Windows 2.49.0 got a stable release, and starting in
  ddef6d3 (GitoxideLabs#1892) we have upgraded to the release marked "latest"
  (which in Git for Windows is always the latest stable release).

More recently, the Windows Server 2022 runner image (which the
`windows-latest` label we use currently selects) has upgraded its
Git for Windows installation from 2.48.1 to 2.49.0:
https://github.com/actions/runner-images/releases/tag/win22%2F20250330.1

It is therefore no longer necessary for the `test-fixtures-windows`
job to upgrade Git for Windows on the runner. This removes the step
that did that. (Either version of the step could be brought back
from the history if ever needed again.)

This closes GitoxideLabs#1849, per the plan in:
GitoxideLabs#1849 (comment)
EliahKagan added a commit to EliahKagan/gitoxide that referenced this pull request Apr 4, 2025
Because GitoxideLabs#1849 (git-for-windows/git#5436)
caused some incorrect test failures in `test-fixtures-windows`, we
have been upgrading Git for Windows on the runner for that job:

- We starting doing that in of 4237e5a (GitoxideLabs#1870). At that time, we
  upgraded it to the most recent tag, to select the latest release
  even though it was a release candiate, because Git for Windows
  2.49.0 had not yet had a stable release.

- Then Git for Windows 2.49.0 got a stable release, and starting in
  ddef6d3 (GitoxideLabs#1892) we have upgraded to the release marked "latest"
  (which in Git for Windows is always the latest stable release).

More recently, the Windows Server 2022 runner image (which the
`windows-latest` label we use currently selects) has upgraded its
Git for Windows installation from 2.48.1 to 2.49.0:
https://github.com/actions/runner-images/releases/tag/win22%2F20250330.1

It is therefore no longer necessary for the `test-fixtures-windows`
job to upgrade Git for Windows on the runner. This removes the step
that did that. (Either version of the step could be brought back
from the history if ever needed again.)

This closes GitoxideLabs#1849, per the plan in:
GitoxideLabs#1849 (comment)
EliahKagan added a commit to EliahKagan/gitoxide that referenced this pull request Apr 4, 2025
Because GitoxideLabs#1849 (git-for-windows/git#5436)
caused some incorrect test failures in `test-fixtures-windows`, we
have been upgrading Git for Windows on the runner for that job:

- We starting doing that in of 4237e5a (GitoxideLabs#1870). At that time, we
  upgraded it to the most recent tag, to select the latest release
  even though it was a release candiate, because Git for Windows
  2.49.0 had not yet had a stable release.

- Then Git for Windows 2.49.0 got a stable release, and starting in
  ddef6d3 (GitoxideLabs#1892) we have upgraded to the release marked "latest"
  (which in Git for Windows is always the latest stable release).

More recently, the Windows Server 2022 runner image (which the
`windows-latest` label we use currently selects) has upgraded its
Git for Windows installation from 2.48.1 to 2.49.0:
https://github.com/actions/runner-images/releases/tag/win22%2F20250330.1

It is therefore no longer necessary for the `test-fixtures-windows`
job to upgrade Git for Windows on the runner. This removes the step
that did that. (Either version of the step could be brought back
from the history if ever needed again.)

This closes GitoxideLabs#1849, per the plan in:
GitoxideLabs#1849 (comment)
EliahKagan added a commit to EliahKagan/gitoxide that referenced this pull request Apr 5, 2025
Because GitoxideLabs#1849 (git-for-windows/git#5436)
caused some incorrect test failures in `test-fixtures-windows`, we
have been upgrading Git for Windows on the runner for that job:

- We starting doing that in of 4237e5a (GitoxideLabs#1870). At that time, we
  upgraded it to the most recent tag, to select the latest release
  even though it was a release candiate, because Git for Windows
  2.49.0 had not yet had a stable release.

- Then Git for Windows 2.49.0 got a stable release, and starting in
  ddef6d3 (GitoxideLabs#1892) we have upgraded to the release marked "latest"
  (which in Git for Windows is always the latest stable release).

More recently, the Windows Server 2022 runner image (which the
`windows-latest` label we use currently selects) has upgraded its
Git for Windows installation from 2.48.1 to 2.49.0:
https://github.com/actions/runner-images/releases/tag/win22%2F20250330.1

It is therefore no longer necessary for the `test-fixtures-windows`
job to upgrade Git for Windows on the runner. This removes the step
that did that. (Either version of the step could be brought back
from the history if ever needed again.)

This closes GitoxideLabs#1849, per the plan in:
GitoxideLabs#1849 (comment)
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.

1 participant