Hi, On Fri, 10 Feb 2023, Junio C Hamano wrote: > Junio C Hamano <gitster@xxxxxxxxx> writes: > > > Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> writes: > > > >>> I wonder if something like this is in order? > >> > >> I don't have much to contribute on that front, but this is really > >> missing some "why", this worked before, why is it failing now? Do we > >> have any idea. > > > > Your guess is as good as mine. I do not do Windows. > > This morning, I notice that those CI jobs I ran last night that > failed with "whoa, windows tests are somehow reporting that symlinks > are available but not really" issue the patch in this thread were > attempting to address has passed even for branches like 'master' and > 'next' that do not yet have it, and it seems to be because you > re-run these failed jobs. > > Whatever magic you used to fix these failing tests, thanks. The magic was to pester the maintainers of the dependency of the `setup-git-for-windows-sdk` GitHub Action that broke said action. Background (you can skip this unless you're interested in details): Since Git's test suite relies so heavily on Unix shell scripting, which is really foreign on Windows, we have to use the MSYS2 Bash to run it. And when I say "MSYS2 Bash", it is short-hand for "a Bash that uses the POSIX emulation layer provided by the MSYS2 runtime". The MSYS2 runtime _does_ have support for actual symbolic links, it just depends on certain Windows features (and then it still is not quite the same as Unix symbolic links because on Windows, the symbolic links are either pointing to files or to directories and you have to specify that when creating them). This support is toggled via the environment variable `MSYS`. To enable support for symbolic links, set it to `winsymlinks:nativestrict`. Now, the dependency of `setup-git-for-windows-sdk` that is used for caching the minimal subset of Git for Windows' SDK needed for git/git's CI runs is called `@actions/cache`, and it recently saw a contribution to allow for caching symbolic links even on Windows. This change set the `MSYS` environment variable in the way indicated above. Unfortunately, it set it not only for the `tar` invocation that needed it, but it was generous enough to extend that setting to the entirety of the containing workflow run. Affecting, among other things, Git's test suite. The `@actions/cache` PR with the bug fix was actually already opened when I noticed the problem, which was only because I released a new version of the `setup-git-for-windows-sdk` Action that supports Windows/ARM64 better (and which unfortunately dragged in the borked dependency). But that PR had not been merged yet. Once it had been merged, and a new version of that dependency had been fast-tracked (I want to believe that my gentle nudging played a part in expediting this), I released a new version of the `setup-git-for-windows-sdk` Action and re-ran the failed workflow runs. And subsequently I ran out of time and could not update anyone about what I had done to fix this. > Do you have an insight on why and how these were failing? The patch > in this thread was a band-aid without knowing why all of a sudden > "ln -s x y && test -h y" started passing (while compat/mingw.c still > says readlink() is not supported). If we know that such a breakage > is not expected, we can drop this workaroun, which would be great. The patch that started this thread looks a lot like papering over the issue to me. And a cleaner way to accomplish that would be to force-set `MSYS` to an explicit value. It would be nice to eventually get to a point to have Git's test suite pass with symbolic links enabled on Windows, but introducing a new test helper won't do that. The actual root cause of the failures we saw when `MSYS=winsymlinks:nativestrict` was graced onto us was that Git's test suite creates a symbolic link to `/dev/null` and then expects Git's `opendir()` call on the symbolic link to do something which it does not do on Windows. I _guess_ that the symptom is caused by `opendir()` resolving the symbolic link and then trying to open the `NUL` device as a directory, which makes no sense whatsoever. But I would contend that the actual culprit is (from the cursory look I took) that Git's test suite is abusing `/dev/null` as being some kind of empty directory instead of explicitly creating an empty directory and using _that_. This is _not_ at all the full and thorough analysis I expect of code reviews, especially of my own reviews, please do not mistake my reply for that. I have more pressing things to attend to than to fix what seems like a "wouldn't it be nice" rather than an "OMG must fix rn!" problem, and therefore I cannot justify spending more time on it for the time being. Ciao, Johannes
