Patchwork D7233: hghave: disallow symlinks on Windows

login
register
mail settings
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

phabricator - Nov. 5, 2019, 11:35 p.m.
mharbison72 created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION 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.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D7233

AFFECTED FILES
  tests/hghave.py

CHANGE DETAILS




To: mharbison72, #hg-reviewers
Cc: mercurial-devel
phabricator - Nov. 6, 2019, 3:25 a.m.
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
phabricator - Nov. 6, 2019, 3:43 a.m.
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
phabricator - Nov. 6, 2019, 3:47 a.m.
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
phabricator - Nov. 6, 2019, 4:05 a.m.
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: