Skip to content
New issue

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

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

Already on GitHub? # to your account

Use java nio to fix broken symlink #165

Merged
merged 1 commit into from
Jan 13, 2017
Merged

Conversation

s0x
Copy link
Contributor

@s0x s0x commented Dec 1, 2016

This fixes #164

@srs
Copy link
Owner

srs commented Dec 2, 2016

Thanks for the pull. Checking it now ;-)

@cbornet
Copy link

cbornet commented Dec 14, 2016

Does it work for a npm specified with npmVersion (not bundled with node) ?

@s0x
Copy link
Contributor Author

s0x commented Dec 14, 2016

That is actually a different issue, which has been resolved by one of the latest versions. This fix does not change any behavior but doesn't use gradles tar plugin which does not support symlinks.

@cbornet
Copy link

cbornet commented Dec 14, 2016

It's probably another issue but it has not been fixed (see #134) and it has the same initial cause (00cc0c8) so I was hoping it would solve it...

@s0x
Copy link
Contributor Author

s0x commented Dec 14, 2016

Oh I see ... actually #134 would be fixed by this change

@cbornet
Copy link

cbornet commented Dec 14, 2016

For the npm bundled with node if I understand well but not for the separately donwloaded npm. Is that correct ?

@s0x
Copy link
Contributor Author

s0x commented Dec 14, 2016

That's correct. I'll have a look on that tomorrow and try to find a solution for that issue as well

@cbornet
Copy link

cbornet commented Dec 14, 2016

Cool ! Thanks !

@cbornet
Copy link

cbornet commented Dec 15, 2016

@s0x Does it also work on Windows ?

@s0x
Copy link
Contributor Author

s0x commented Dec 15, 2016

This does not change anything on the windows installation, since this is handled separately. Is this broken on windows as well?

@cbornet
Copy link

cbornet commented Dec 15, 2016

I don't know. I use Linux. But I need it to work on Windows.

@s0x
Copy link
Contributor Author

s0x commented Dec 15, 2016

Same here.
If there is an issue on windows, please create a issue for that. This discussion does not relate to the MR

@s0x
Copy link
Contributor Author

s0x commented Dec 15, 2016

@srs: Is there anything which prevents you from merging this?

@srs
Copy link
Owner

srs commented Dec 17, 2016

Not sure if this will work on every installation since it's executing the tar command (instead of using the pure-java version). But I assume that tar is always available?

@srs srs added this to the 1.1.0 milestone Dec 17, 2016
@s0x
Copy link
Contributor Author

s0x commented Dec 19, 2016

It seems that there is no pure-java version of it. The other option would be to not use the symlink at all. I'm working on it atm.

@s0x s0x force-pushed the fix-broken-npm-symlink branch from 023e086 to e9b8acf Compare December 19, 2016 10:41
@s0x s0x changed the title Use tar command to preserve symlink Use java nio to fix broken symlink Dec 19, 2016
@s0x
Copy link
Contributor Author

s0x commented Dec 19, 2016

@srs I fixed the symlink using java nio. I guess that is the best option we have

@srs
Copy link
Owner

srs commented Jan 2, 2017

Nice one! Thanks. Now I haven no more concerns. Will pull this tomorrow and a lot of other PR's.

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

Successfully merging this pull request may close these issues.

Broken npm symlink: Unable to call npm in scripts property
3 participants