This is an archive of the discontinued Mercurial Phabricator instance.

Differential D7233

hghave: disallow symlinks on Windows
ClosedPublic

Authored by mharbison72 on Nov 5 2019, 6:35 PM.

Details

Reviewers
indygreg
Group Reviewers
hg-reviewers
Commits
rHG6792da448437: hghave: disallow symlinks on Windows
rHGe494dc63680c: hghave: disallow symlinks on Windows
Summary

Symlinks on Windows require either a special priviledge, or enabling Developer
Mode. It's probably the latter that is enabled on the new CI machine. But
since Mercurial itself is saying no to symlinks on Windows, the tests for
symlinks shouldn't be attempted. This should fix a lot of the noise in the py3
tests.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

mharbison72 created this revision.Nov 5 2019, 6:35 PM
indygreg added a subscriber: indygreg.Nov 5 2019, 10:25 PM

Shouldn't we be making this change in the core product as well? Otherwise Mercurial may attempt to use symlinks on Windows and we could blow up due to not having any test coverage?

mharbison72 added a comment.Nov 5 2019, 10:42 PM
In D7233#106184, @indygreg wrote:

Shouldn't we be making this change in the core product as well? Otherwise Mercurial may attempt to use symlinks on Windows and we could blow up due to not having any test coverage?

This should be what's disabling it in core, via util.checklink():

https://www.mercurial-scm.org/repo/hg/file/5.2/mercurial/windows.py#l276

Just for fun, I enabled that and ran the tests in an admin window. What I learned is that ln -s doesn't make symlinks, so I had to substitute a os.symlink() snippet, but there are more than a couple of these. I figure when we want to enable it, we can patch in a shell alias in MSYS like is done for pwd[1], and point it to a *.py implementation. And then I hit a test where a symlink without a target is tard and then extracted... and MSYS tar didn't like that. I set it aside at that point, but there may be other issues.

[1] https://www.mercurial-scm.org/repo/hg/file/5.2/tests/run-tests.py#l1726

indygreg accepted this revision.Nov 5 2019, 10:46 PM
In D7233#106273, @mharbison72 wrote:
In D7233#106184, @indygreg wrote:

Shouldn't we be making this change in the core product as well? Otherwise Mercurial may attempt to use symlinks on Windows and we could blow up due to not having any test coverage?

This should be what's disabling it in core, via util.checklink():
https://www.mercurial-scm.org/repo/hg/file/5.2/mercurial/windows.py#l276

Ahh, cool.

Just for fun, I enabled that and ran the tests in an admin window. What I learned is that ln -s doesn't make symlinks, so I had to substitute a os.symlink() snippet, but there are more than a couple of these. I figure when we want to enable it, we can patch in a shell alias in MSYS like is done for pwd[1], and point it to a *.py implementation. And then I hit a test where a symlink without a target is tard and then extracted... and MSYS tar didn't like that. I set it aside at that point, but there may be other issues.
[1] https://www.mercurial-scm.org/repo/hg/file/5.2/tests/run-tests.py#l1726

I think symlinks on Windows are too fragile to even consider enabling. So I'm fine leaving them disabled on Windows indefinitely.

This revision is now accepted and ready to land.Nov 5 2019, 10:46 PM
mharbison72 added a commit: rHGe494dc63680c: hghave: disallow symlinks on Windows.Nov 5 2019, 10:48 PM
Closed by commit rHGe494dc63680c: hghave: disallow symlinks on Windows (authored by mharbison72). · Explain Why
This revision was automatically updated to reflect the committed changes.
mharbison72 added a comment.Nov 5 2019, 11:05 PM
In D7233#106279, @indygreg wrote:

I think symlinks on Windows are too fragile to even consider enabling. So I'm fine leaving them disabled on Windows indefinitely.

Out of curiosity, what's fragile about them? I've not used them, and am fine with leaving them off too, especially while it requires admin or developer mode. That seems too weird that they could be created in one terminal and not another potentially. And I wonder if that would cause issues with the cached marker indicating that the filesystem supports symlinks.

Revision Contents
Changeset List

PathPackages
Mtests/hghave.py (3 lines)

Diff 17562

tests/hghave.py

def has_p4(): def has_p4():
return matchoutput('p4 -V', br'Rev\. P4/') and matchoutput( return matchoutput('p4 -V', br'Rev\. P4/') and matchoutput(
'p4d -V', br'Rev\. P4D/' 'p4d -V', br'Rev\. P4D/'
) )
@check("symlink", "symbolic links") @check("symlink", "symbolic links")
def has_symlink(): def has_symlink():
if getattr(os, "symlink", None) is None: # mercurial.windows.checklink() is a hard 'no' at the moment
if os.name == 'nt' or getattr(os, "symlink", None) is None:
return False return False
name = tempfile.mktemp(dir='.', prefix=tempprefix) name = tempfile.mktemp(dir='.', prefix=tempprefix)
try: try:
os.symlink(".", name) os.symlink(".", name)
os.unlink(name) os.unlink(name)
return True return True
except (OSError, AttributeError): except (OSError, AttributeError):
return False return False