This is an archive of the discontinued Mercurial Phabricator instance.

hghave: disallow symlinks on Windows
ClosedPublic

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

Details

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

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?

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

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
This revision was automatically updated to reflect the committed changes.

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.