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

Fix #210 Windows symbolic links requiring admin #231

Merged
merged 1 commit into from
May 1, 2017
Merged

Fix #210 Windows symbolic links requiring admin #231

merged 1 commit into from
May 1, 2017

Conversation

phated
Copy link
Member

@phated phated commented Apr 27, 2017

This is just a reference PR for #211 because I'm not sure if the original author has time to finish this up.

TODO:

  • Remove new link method
  • Remove the added src tests because they don't do anything
  • Change author of the commit to give original author credit
  • Documentation
  • Don't allow the relative option with junctions
  • Need test for forceJunctionDir (and rename useJunctions) - Support function

Sorry, something went wrong.

@andrewfinnell
Copy link

andrewfinnell commented Apr 27, 2017

Good call. I have been lacking the time. Is there an IRC, HipChat, Slack or other IM group the team is "normally" on?

Also, with the latest Anniversary Update of Windows 10, MS fixed this whole problem for us when I submitted a ticket to their system as well.

This fix is still needed to support pre-update Windows 10 and Windows 7.

To enable proper symlinks on Windows 10+ one needs to Enable Developer Mode in Settings. Now you can open a regular Command Prompt if the user is in the Administrators group and use mklink properly.

Thank goodness for small favors. Also, their Lxss is incredibly featured now that if one has the option of using Win10 over Win7 I say take it. You'll get real symlinks AND a real Ubuntu Kernel which is not virtualized. Meaning aptget works, gcc works, etc.

I will try to look at the issue talked about in this ticket but have no problem with someone else fixing it, even if it takes a "rewrite." No code ownership needed here. I do appreciate the attribution though. Thank you.

@phated
Copy link
Member Author

phated commented Apr 27, 2017

@afinnell we have a private IRC channel but most of us idle in #gulpjs on freenode. We'll likely be creating a team slack as the team grows.

Thanks for all the great information. I might try to incorporate that into the docs.

@phated
Copy link
Member Author

phated commented Apr 27, 2017

Just noticed that hardlink doesn't need a relative option because it is creating a new file.

@phated
Copy link
Member Author

phated commented Apr 27, 2017

@afinnell if you have some time, can you review the new tests I added. I think I used .hardlink properly but I'd like your review to verify.

@phated
Copy link
Member Author

phated commented Apr 27, 2017

Travis seems to be failing pretty hard with these tests. Something with permissions that I need to look into (they were passing on my machine).

@phated
Copy link
Member Author

phated commented Apr 27, 2017

@afinnell going further down this rabbit hole - it seems that neither linux or windows allows hardlinks with directories as the first argument (but osx does for some reasons). I'm not sure the best solution for this problem, any ideas?

@phated
Copy link
Member Author

phated commented Apr 27, 2017

Going to cc @erikkemperman too for the above ^

@phated
Copy link
Member Author

phated commented Apr 27, 2017

It seems like the best solution is to error if file.isDirectory(). It's unfortunate that node isn't quite as cross-platform as it advertises.

@phated
Copy link
Member Author

phated commented Apr 27, 2017

Now that I'm reading the description that @afinnell wrote in the hardlink file, it seems like the goal of this method (linking directories on Windows) is unachievable. I think we might just ship the forceJunctionDir option and call it done.

@phated
Copy link
Member Author

phated commented Apr 28, 2017

@contra @afinnell @erikkemperman if any of you have a chance to review this, I'd appreciate it because it's changed quite a lot over the course of today.

@erikkemperman
Copy link
Member

Unfortunately I won't be much help on Windows issues, having ditched it long ago.

The caveats here strike me as pretty serious, though. Should they perhaps be in README as well?

@phated
Copy link
Member Author

phated commented Apr 30, 2017

I think the caveats are overly cautious. To put it into perspective: npm always uses them on Windows with no way to disable.

@erikkemperman
Copy link
Member

Alrighty then :-)

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.

None yet

3 participants