Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RFC: Added support, tests for unix+windows symlinks via libuv #4396

Merged
merged 1 commit into from
Jun 12, 2014

Conversation

kmsquire
Copy link
Member

  • For Windows, support is only available on Vista and later
  • Make ln(target, link) -> symlink(target, link)
  • Added symlink tests

@kmsquire
Copy link
Member Author

Useful for #3344, although it's unclear to me whether symlinks are a good idea on Windows.

Because Vista was the first Windows OS to support symlinks, do we have a way to test Windows version? The file tests will fail on Windows XP. (Of course, there are other things in test/file.jl that shouldn't work on Windows at all...)

This works on Linux, but it would be great if someone could test on Windows and OS X.

cc: @loladiro @vtjnash

@JeffBezanson
Copy link
Sponsor Member

I'm guessing this is out of scope for 0.2? @vtjnash @loladiro

@@ -108,6 +108,22 @@ normpath(a::String, b::String...) = normpath(joinpath(a,b...))
abspath(a::String) = normpath(isabspath(a) ? a : joinpath(pwd(),a))
abspath(a::String, b::String...) = abspath(joinpath(a,b...))

function relpath(p1::String, p2::String = pwd())
Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should have unit tests

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I had meant to submit this part separately, but pulled it in accidentally.

@kmsquire
Copy link
Member Author

kmsquire commented Oct 8, 2013

I'll squash if this is okay.

My only concern is that this will fail on Windows XP, which, although soon to lose support from Microsoft, is still probably used somewhat (we've seen issues/mailing list posts to this effect).

@vtjnash
Copy link
Sponsor Member

vtjnash commented Oct 8, 2013

I'm pretty sure ln is the expected abbreviation natural log and shouldn't be used up as a command name. plus, it's a decent two character variable for user code

needs documentation or you'll get the hat of shame

Pkg manager definitely is not allowed to be broken on Windows XP (ref. comment on #3344)

I don't think this is necessary for 0.2, and would like to think we are approaching a feature-stable release.

@kmsquire
Copy link
Member Author

kmsquire commented Oct 8, 2013

I'm pretty sure ln is the expected abbreviation natural log and shouldn't be used up as a command name. plus, it's a decent two character variable for user code

Um, yes, that would make sense. Sorry, little tired/distracted.

I'll add docs, and I agree that it shouldn't hold up 0.2. I also think that it should be used sparingly and carefully (or not at all) in mainline Julia code, especially if that code is meant to run on XP.

@kmsquire
Copy link
Member Author

Did that already in a later commit. I'll squash.

@kmsquire
Copy link
Member Author

kmsquire commented Feb 4, 2014

I added a test for the version of Windows. Tests still need updating.

@StefanKarpinski
Copy link
Sponsor Member

@kmsquire, I know this has been dormant for a long time, but do you think you could rebase it?

* For Windows, support is only available on Vista and later, so...
* Also added macros for testing version of Windows
@kmsquire kmsquire changed the title WIP: Added support, tests for unix+windows symlinks via libuv RFC: Added support, tests for unix+windows symlinks via libuv Jun 11, 2014
@kmsquire
Copy link
Member Author

@StefanKarpinski, it's rebased.

StefanKarpinski added a commit that referenced this pull request Jun 12, 2014
RFC: Added support, tests for unix+windows symlinks via libuv
@StefanKarpinski StefanKarpinski merged commit 07c0e75 into master Jun 12, 2014
@StefanKarpinski StefanKarpinski deleted the kms/libuv_symlink branch June 12, 2014 00:42
@tkelman
Copy link
Contributor

tkelman commented Jun 12, 2014

And fails tests on win7. You need admin rights on Vista and newer to make a symlink.

@kmsquire
Copy link
Member Author

That's a shame. I think it's easiest not to have symlinks at all on Windows
then. They're really foreign beasts there!

On Thursday, June 12, 2014, Tony Kelman notifications@github.com wrote:

And fails tests on win7. You need admin rights on Vista and newer to make
a symlink.


Reply to this email directly or view it on GitHub
#4396 (comment).

@tkelman
Copy link
Contributor

tkelman commented Jun 13, 2014

Agreed. (Before anyone asks, my understanding about why Windows symlinks require admin rights is that they can potentially lead to security vulnerabilities?) There are all sorts of odd solutions out there. Cygwin has their own symlink format that only binaries linked to the cygwin1.dll can understand, MSYS1 makes a copy when you ask for a symlink, and I think MSYS2 makes a junction - I've heard that junctions can have unintended consequences as well.

@Keno
Copy link
Member

Keno commented Jun 13, 2014

Can we put a finger on what these unintended consequences are? We are using
Junctions in the build process and I don't think there have been problems
so far.

On Thu, Jun 12, 2014 at 9:09 PM, Tony Kelman notifications@github.com
wrote:

Agreed. (Before anyone asks, my understanding about why Windows symlinks
require admin rights is that they can potentially lead to security
vulnerabilities?) There are all sorts of odd solutions out there. Cygwin
has their own symlink format that only binaries linked to the cygwin1.dll
can understand, MSYS1 makes a copy when you ask for a symlink, and I think
MSYS2 makes a junction - I've heard that junctions can have unintended
consequences as well.


Reply to this email directly or view it on GitHub
#4396 (comment).

@tkelman
Copy link
Contributor

tkelman commented Jun 13, 2014

I'll try to go digging, see if I can pull up some mailing list discussion I've read about the difference (may have been in a discussion of msys2 vs cygwin differences and reason for forking, etc). According to Wikipedia NTFS junctions only support directories and absolute paths, NTFS symlinks also support files and relative paths.

@StefanKarpinski
Copy link
Sponsor Member

Radical (crappy) option: error out with an informative message on these systems if someone tries to symlink and force the programmer to explicitly handle what to do? Seems awful, but if a program wants to make a symlink and is running on a system that can't make a symlink, what else is there to do?

@kmsquire
Copy link
Member Author

This patch already does that for XP. I think that's reasonable.

On Friday, June 13, 2014, Stefan Karpinski notifications@github.com wrote:

Radical (crappy) option: error out with an informative message on these
systems if someone tries to symlink and force the programmer to explicitly
handle what to do? Seems awful, but if a program wants to make a symlink
and is running on a system that can't make a symlink, what else is there to
do?


Reply to this email directly or view it on GitHub
#4396 (comment).

@tkelman
Copy link
Contributor

tkelman commented Jun 14, 2014

Not a fan of putting in functionality that will error out for non-admin users on a common platform that developers don't test on. Are there locations in base or existing packages where symlinks to files are needed?

@StefanKarpinski
Copy link
Sponsor Member

I'm not either but it's not like we can just not have a symlink function. There's not much we can do about fundamentally broken OSes. Only supporting the least common denominator is not a viable strategy. That way leads to the JVM.

@tkelman
Copy link
Contributor

tkelman commented Jun 14, 2014

If the symlink function's not portable, what have you gained over the previous method of run(ln -s a b)? At least in the latter case, putting a Windows version of coreutils on the path can fake it and not lead to errors. I'm not asking for portability to be the absolute first priority, as that's unrealistic or leads to too many compromises, but at least make the effort (I'll help here, but hostility to usage on Windows isn't encouraging). Documenting when something's not portable, for example, so package authors will be fully informed that it will lead to usability problems (that way leads to Python's uselessness on Windows).

@StefanKarpinski
Copy link
Sponsor Member

It's better than running the command since at least it can be made to work on all OSes that support making symlinks – including more recent Windows versions. If someone can come up with a decent alternative on OSes that can't make symlinks, then we can do that – junctures, whatever – but if the choices are not having a symlink function or having a symlink function that doesn't work everywhere, then I think we clearly have to choose the latter.

@tkelman
Copy link
Contributor

tkelman commented Jun 14, 2014

No, I think you misunderstood me, apologies if I was unclear. Saying "sorry not supported" on Windows XP is fine by me, upgrading to Win7 or Win8 is much easier for Windows users than switching entirely to OSX or Linux.

The issue here is that making symlinks, even on Windows 7 (or presumably 8 or Vista), fails with an error unless the process is run with admin rights. I don't think that viably counts as "can be made to work."

@StefanKarpinski
Copy link
Sponsor Member

So there are no versions of windows where a user can make a symlink?

@tkelman
Copy link
Contributor

tkelman commented Jun 14, 2014

Not without admin rights, as far as I'm aware. According to http://en.wikipedia.org/wiki/NTFS_symbolic_link

The default security settings in Windows Vista/Windows 7 disallow non-elevated administrators and all non-administrators from creating symbolic links. This behavior can be changed running "secpol.msc" the Local Security Policy management console (under: Security Settings\Local Policies\User Rights Assignment\Create symbolic links). It can be worked around by starting cmd.exe with Run as administrator option or the runas command.

I don't have Windows 8 handy to check there - @quinnj you do right, can you test this?

@StefanKarpinski
Copy link
Sponsor Member

Sigh.

@tkelman
Copy link
Contributor

tkelman commented Jun 14, 2014

So, ways forward? Check specifically for a permissions error, catch and retry (with a warning?) using a junction if the target is a directory, or fall back to trying command-line hoping some version of coreutils might be present? Try to tweak the security settings when running Julia's installer (or just write up instructions in docs) so non-admins are allowed to create symlinks? Doesn't look like node/libuv have come up with any better solutions for this either.

@StefanKarpinski
Copy link
Sponsor Member

Try to tweak the security settings when running Julia's installer (or just write up instructions in docs) so non-admins are allowed to create symlinks?

We definitely cannot go changing people's security settings. I think that would qualify us as malware. Give instructions on how to do it is a different matter. That amazingly complex plan does seem like a way forward. Let's see what other Windows specialists think.

cc: @vtjnash, @ihnorton

@quinnj
Copy link
Member

quinnj commented Jun 16, 2014

Sorry for not seeing this thread earlier. Is there something you'd like me to test @tkelman?

@quinnj
Copy link
Member

quinnj commented Jun 16, 2014

Actually I think you just wanted me to run tests. I pulled and rebuilt and I'm now seeing a symlink error,

        From worker 5:       * file
exception on 5: ERROR: symlink: operation not permitted (EPERM)
 in symlink at fs.jl:159
 in runtests at C:\Users\karbarcca\julia\test\testdefs.jl:5
while loading file.jl, in expression starting on line 10
        From worker 4:         [stdio passthrough ok]
MemoryError
C:\Users\karbarcca\julia\test>

@tkelman
Copy link
Contributor

tkelman commented Jun 16, 2014

Just wanted a verification that the same permissions problem is true for symlinks on Windows 8. Could still use some opinions from others whether my semblance of a plan for how to approach this above sounds workable.

@kmsquire
Copy link
Member Author

Well, the title is a little presumptuous, but I just submitted #7274, which should address part of the problems here. It needs testing, and maybe a few changes. Please let me know.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants