Submitter | phabricator |
---|---|
Date | Nov. 5, 2019, 11:35 p.m. |
Message ID | <differential-rev-PHID-DREV-ai2zage57x4kyxgj5mob-req@mercurial-scm.org> |
Download | mbox | patch |
Permalink | /patch/42758/ |
State | Superseded |
Headers | show |
Comments
indygreg added a comment. 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? REPOSITORY rHG Mercurial CHANGES SINCE LAST ACTION https://phab.mercurial-scm.org/D7233/new/ REVISION DETAIL https://phab.mercurial-scm.org/D7233 To: mharbison72, #hg-reviewers Cc: indygreg, mercurial-devel
mharbison72 added a comment.
In D7233#106184 <https://phab.mercurial-scm.org/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 `tar`d 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
REPOSITORY
rHG Mercurial
CHANGES SINCE LAST ACTION
https://phab.mercurial-scm.org/D7233/new/
REVISION DETAIL
https://phab.mercurial-scm.org/D7233
To: mharbison72, #hg-reviewers
Cc: indygreg, mercurial-devel
This revision is now accepted and ready to land. indygreg added a comment. indygreg accepted this revision. In D7233#106273 <https://phab.mercurial-scm.org/D7233#106273>, @mharbison72 wrote: > In D7233#106184 <https://phab.mercurial-scm.org/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 `tar`d 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. REPOSITORY rHG Mercurial CHANGES SINCE LAST ACTION https://phab.mercurial-scm.org/D7233/new/ REVISION DETAIL https://phab.mercurial-scm.org/D7233 To: mharbison72, #hg-reviewers, indygreg Cc: indygreg, mercurial-devel
mharbison72 added a comment.
In D7233#106279 <https://phab.mercurial-scm.org/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.
REPOSITORY
rHG Mercurial
CHANGES SINCE LAST ACTION
https://phab.mercurial-scm.org/D7233/new/
REVISION DETAIL
https://phab.mercurial-scm.org/D7233
To: mharbison72, #hg-reviewers, indygreg
Cc: indygreg, mercurial-devel
Patch
diff --git a/tests/hghave.py b/tests/hghave.py --- a/tests/hghave.py +++ b/tests/hghave.py @@ -431,7 +431,8 @@ @check("symlink", "symbolic links") 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 name = tempfile.mktemp(dir='.', prefix=tempprefix) try: