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).

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