Re: [PATCH] test: make SYMLINKS prerequisite more robust

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

[-- Attachment #1: Type: text/plain, Size: 4893 bytes --]

Hi,

On Fri, 10 Feb 2023, Junio C Hamano wrote:

> Junio C Hamano <gitster@pobox.com> writes:
>
> > Ævar Arnfjörð Bjarmason <avarab@gmail.com> 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