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 Git 2.49.0 release candidate in test-fixtures-windows #1870

Merged
merged 2 commits into from
Mar 11, 2025

Conversation

EliahKagan
Copy link
Member

@EliahKagan EliahKagan commented Mar 2, 2025

This is to work around #1849, while still running tests and fixture scripts that rely on Git being able to rename symlinks on Windows. The release candidate has the fix from git-for-windows/git#5437 for git-for-windows/git#5436.

As written, this grabs the release associated with the newest tag, so roughly speaking it is "rolling" and will thus pick up subsequent release candidates and the stable release that shall follow them. It can be undone once GitHub-hosted Windows runner images get Git for Windows 2.49.0 (which will likely occur a short time after it has a stable release). This only modifies the test-fixtures-windows job and not other jobs such as the test-fast windows-latest job.

This is a different approach to working around #1849 than I had suggested in #1849 (comment) that I would do. If this is not suitable, then the approach I mentioned there of conditionally skipping tests or suppressing assertions could be considered. A third approach could be to simply add the newly failing tests to etc/test-fixtures-windows-expected-failures-see-issue-1358.txt, though currently that is a list of long-standing failures that occur with all Git for Windows versions and should gradually shrink (#1358). Only one of these approaches should be used. If the one here is not preferred, then I can update this PR to use another.

I think probably any of these approaches is fine. The reason I did it this way is that current work (in #1862 and #1864), and related work that may follow it, seems like it would benefit from being fully tested on Windows on CI with GIX_TEST_IGNORE_ARCHIVES=1, including the three tests that rely on fixture scripts being able to run git mv on a symlink. This could also have been done by downgrading the Git for Windows installation rather than upgrading it. But as long as we are downloading and running an installer, it seems to me that it is better to catch any incompatibilities with recent versions, even at the risk of observing bugs in Git for Windows.

Observing bugs in Git for Windows that would affect the test suite might also be considered an advantage rather than a disadvantage. For now, I've indicated in comments that the new step (which adds about 30 seconds 90 seconds) should be removed when the Windows runners have Git 2.49.0. If this step is made permanent, then I think it would need to be refined, to make sure the sense of "newest" in which it retrieves the release associated with the newest tag is the sense that we actually want. As a temporary workaround for #1849, it seems to me that this is okay.

Ordinarily, when installing Git for Windows on CI, I would use PortableGit, or a package manager like Scoop or Chocolatey. However, the installation already present on the Windows runners is created--or as if created--by the Inno Setup installer (i.e. a non-portable non-SDK non-MinGit full installation of Git for Windows). This includes files that facilitate uninstallation, and that other versions of the Inno Setup installer can detect and replace.

I don't want multiple Git installations on the runner, because they, or at least their bundled MSYS2 installations, can interact (#1862 (comment)), which is not part of what test-fixture-windows seeks to test, and because it would be easier to accidentally use the wrong one. I also do not want to accidentally end up only partially replacing the old installation. Therefore, I have done it by downloading and running the Inno Setup installer. (If we continue upgrading Git for Windows on this CI job, then I could also look at how it is prepared for the runner images, and adjust how we do it accordingly.)

I considered attempting to use git update-git-for-windows instead of downloading and running the Inno Setup installer, but as far as I know there is no way to use the update-git-for-windows subcommand to get a release candidate. Also, I am trying to avoid using git before the upgrade since on Windows there are difficulties replacing files that are in use. People have had trouble using that subcommand on CI, I believe in part due to such difficulties (git-for-windows/git#5230).

The new step precedes actions/checkout rather than following it, which is intentional. In part this is to avoid using git before upgrading it, just in case. But also, if something is ever very badly wrong with the newly installed Git for Windows, then we are more likely to observe it in a way that makes it obvious if we upgrade git before the actions/checkout step, because then actions/checkout uses the upgraded git installation. (If somehow the outcome of the installation is that git is entirely absent, then actions/checkout would actually succeed, because it would fall back to using the GitHub REST API. This would be observable in the action output, but otherwise might not be obvious. But the complete absence of git would be fairly quickly identifiable in test output.)

Edit: When I opened this, RC0 was out. Now, RC1 is out. This second release candidate is automatically selected when the step runs, as intended. See this rerun, on my fork. Separately--though I noticed it at the same time--the step seems to take longer than I originally believed. During testing before opening this PR, I saw a run that took 30 seconds for the Git for Windows upgrade step. But it looks like the more typical case--both with RC0 and RC1--is that it takes about 1 minute and 30 seconds. I don't think caching would necessarily improve that, because I think most of the time is taken by the installation rather than by the download that precedes it.

This is to work around GitoxideLabs#1849, while still running tests and fixture
scripts that rely on Git being able to rename symlinks on Windows.
@EliahKagan EliahKagan force-pushed the run-ci/test-fixtures-windows-rc branch from 71a983c to 4237e5a Compare March 11, 2025 08:44
@EliahKagan
Copy link
Member Author

EliahKagan commented Mar 11, 2025

I've rebased and updated this to replace the change from cdee7ff (added in #1882) with the proposed technique here. But the approach in cdee7ff is what I would have suggested if you don't like the approach advocated here of changing the git version on the runner (in which case this could just not be merged).

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.

Thanks so much, so sorry for the delay and the added chaos.

@Byron Byron merged commit 02bc8cb into GitoxideLabs:main Mar 11, 2025
19 of 21 checks passed
@EliahKagan EliahKagan deleted the run-ci/test-fixtures-windows-rc branch March 11, 2025 11:04
@EliahKagan
Copy link
Member Author

RC2 came out shortly after this was merged. Just as when RC1 came out this automatically used it instead of RC0, now that RC2 is out this automatically uses it instead of RC1 and the tests still pass.

Thanks so much

No problem!

so sorry for the delay and the added chaos.

I think this particular PR has actually benefited from the intervening events. This is because, although working with RC0, RC1, and RC2 suggests it will continue to work as long as we need it, if the technique it uses does turn out to be unsuitable then reverting this whole PR will give the probably next-best solution of listing the failing cases as being expected to fail.

Once Git for Windows 2.49.0 has a stable release and the Windows GHA runners have it, it may make sense to revert 4237e5a. Reverting the whole PR feels like it would be correct in that case but would not, since it would put the tests that shouldn't fail back in the list of tests expected to fail. But this is unlikely to cause trouble, because test-fixtures-windows checks for an exact match to the tests it expects to fail, so wrongly adding passing tests to the list would be caught (and would block auto-merge).

EliahKagan added a commit to EliahKagan/gitoxide that referenced this pull request Mar 18, 2025
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 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.)
EliahKagan added a commit to EliahKagan/gitoxide that referenced this pull request Mar 18, 2025
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.)
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.

2 participants