-
-
Notifications
You must be signed in to change notification settings - Fork 330
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
Use Git 2.49.0 release candidate in test-fixtures-windows
#1870
Conversation
9db941a
to
71a983c
Compare
This reverts commit cdee7ff.
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.
71a983c
to
4237e5a
Compare
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 |
There was a problem hiding this 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.
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.
No problem!
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 |
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 thetest-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 rungit 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 seconds90 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 theupdate-git-for-windows
subcommand to get a release candidate. Also, I am trying to avoid usinggit
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 usinggit
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 upgradegit
before theactions/checkout
step, because thenactions/checkout
uses the upgradedgit
installation. (If somehow the outcome of the installation is thatgit
is entirely absent, thenactions/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 ofgit
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.