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

module: preserve symlinks when requiring #5950

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
@lamara
Contributor

lamara commented Mar 30, 2016

In reference to #3402 , which is a good overview of the issues this patch deals with.

Currently, required modules use the real location of the package/file as their __filename and __dirname, instead of the symlinked path if it exists. This behaviour is undocumented (it even goes against documentation in certain scenarios), creating hard-to-debug problems for developers who wish to leverage filesystem abstractions to lay out their application.

This patch resolves all required modules to their canonical path while still preserving any symlinks within the path, instead of resolving to their canonical realpath. The one special case observed is when the main module is loaded -- in this case, the realpath does need to be used in order for the main module to load properly.

This fix, as far as I can tell, will not have much, if any, impact on the current node ecosystem -- all tests pass through canary in the goldmine (https://github.com/nodejs/citgm) (except for module tests that explicitly forbid being run with pre-release versions), which makes sense because symlinks generally aren't going to be created (if ever) through the npm intstall -> npm test/start cycle. This fix will mainly help out developers who are working on multiple modules at once and want to lay out their modules in a sane way using symlinks.

@MylesBorins

This comment has been minimized.

Member

MylesBorins commented Mar 30, 2016

I'll get a ci run of citgm going when I get home

@MylesBorins

This comment has been minimized.

Member

MylesBorins commented Mar 30, 2016

It might be good to run citgm against some of the file watching libraries, likely to unearth edges if there are any

@kzc

This comment has been minimized.

kzc commented Mar 30, 2016

@lamara Thanks for the pull request. I'm very much on board with the goal of this patch - don't resolve symlinks except for main. But I'm just wondering if the change to lib/modules.js can't be simpler along the lines of #3402 (comment). Is it necessary to add an isMain parameter to all those functions?

The name of this PR also threw me for a loop - "preserve symlinks" versus #3402 "don't resolve symlinks". I guess either way is fine as long as the same goal is achieved.

@dlongley

This comment has been minimized.

dlongley commented Mar 30, 2016

@kzc,

But I'm just wondering if the change to lib/modules.js can't be simpler along the lines of #3402 (comment).

This PR actually started out using that patch -- but simple change alone caused all kinds of modules to fail. @lamara may have the details as to why.

@dlongley

This comment has been minimized.

dlongley commented Mar 30, 2016

@kzc,

I feel that even "don't resolve symlinks" is a little ambiguous, it might be better phrased "don't dereference symlinks" or "don't expand symlinks" since resolving symlinks may just involve resolving a symlink relative path to an absolute one. The terminology here is a bit murky in how it is commonly used.

@kzc

This comment has been minimized.

kzc commented Mar 30, 2016

@dlongley @lamara I had tested a couple of scenarios, but if my patch simply doesn't work I withdraw my question. I support any fix for the require() symlink problem.

@kzc

This comment has been minimized.

kzc commented Mar 30, 2016

This pull request does not appear to handle the scenario described in #5481 . (Neither did my patch for that matter.)

It comes down to resolution of modules when the main script happens to be a symlink and modules need to be require()'d from the original directory (not the symlinked one).

@dlongley

This comment has been minimized.

dlongley commented Mar 30, 2016

@kzc,

This pull request does not appear to handle the scenario described in #5481.

I think that issue may require some new configuration feature. I don't think you can easily support both types of behavior at the same time without making it difficult to reason about and debug (or without breaking existing behavior).

@kzc

This comment has been minimized.

kzc commented Mar 30, 2016

@dlongley Indeed. You'd definitely need a command line flag along the lines of --do-not-resolve-symlinks-for-require (or hopefully shorter!).

A workaround for #5481 would be simply to use this PR as is and create a regular non-symlinked JS file that require()s the symlinked main script.

@kzc

This comment has been minimized.

kzc commented Mar 30, 2016

A workaround for #5481 without the need for a new command line flag:

node-with-PR5950 -e "require('./symlinked-main-script.js')"

Edit: this also appears to work:

node-with-PR5950 -r ./symlinked-main-script.js -e ""
@lamara

This comment has been minimized.

Contributor

lamara commented Mar 30, 2016

@kzc Definitely wanted a 2-line implementation like #3402 (comment) , but as @dlongley said, calling fs.realpathSync on the main file and then trying to resolve it was causing hard-to-track issues with a lot of test suites, and the majority of citgm was failing with it. This way, at least, we can be sure that path.resolve is going to be used in all scenarios except when we're loading the main module, in which case we will always fall back to the default behavior.

@jasnell

This comment has been minimized.

Member

jasnell commented Apr 1, 2016

@nodejs/ctc ... any thoughts on this?

@bnoordhuis

This comment has been minimized.

Member

bnoordhuis commented Apr 1, 2016

@thealphanerd Did citgm find anything?

@MylesBorins

This comment has been minimized.

Member

MylesBorins commented Apr 1, 2016

@MylesBorins

This comment has been minimized.

Member

MylesBorins commented Apr 1, 2016

citgm is all green!

@mwd410

This comment has been minimized.

mwd410 commented Apr 7, 2016

Forgive me if this has been discussed, but why would we have an opt-out flag as discussed here and in #3402? Seems to me in order to justify not bumping the major version, everything should work exactly as it does today without further intervention. Why not have an opt-in flag until the next major version, where it becomes the standard?

@kzc

This comment has been minimized.

kzc commented Apr 7, 2016

I believe this PR should be a semver major. But opt-in or opt-out flag is not required any longer due the workaround outlined in #5950 (comment). Only the main script needs special symlink resolution. No one has demonstrated any real life module use that depends on the behavior of resolving symlinks that this patch can't handle. This pull request restores the expected behavior of symlinks on file systems while preserving node backwards compatibility.

@dlongley

This comment has been minimized.

dlongley commented Apr 7, 2016

Is anything else required to get this PR merged? Will it make it into node 6.x?

cc: @nodejs/ctc

@MylesBorins MylesBorins self-assigned this Apr 7, 2016

@MylesBorins

This comment has been minimized.

Member

MylesBorins commented Apr 7, 2016

I'm assigning this issue to myself and will try and help to get a definitive answer about if this will be in v6 or not.

@mattcollier

This comment has been minimized.

mattcollier commented Apr 15, 2016

@thealphanerd Can you provide any status updates on this? Thank you!

@msporny

This comment has been minimized.

msporny commented Apr 15, 2016

@thealphanerd any updates on this getting pulled into v6? +1 for it getting it pulled into that version. This bug is causing us some serious grief and this patch would really help us out.

@kzc

This comment has been minimized.

kzc commented Apr 15, 2016

Looks like this PR needs a rebase + update due to recent changes in lib/module.js

@MylesBorins

This comment has been minimized.

Member

MylesBorins commented Apr 15, 2016

@lamara would you be able to rebase this against master and I'll work on getting some clarity on this next week. I've been traveling internationally the last week and have not been as on top of things as normal (hello from rainy Montevideo)

@kzc

This comment has been minimized.

kzc commented Apr 15, 2016

This "don't resolve symlinks" issue appears to be quite popular among node users:

Issues sorted by most thumbs up:

https://github.com/nodejs/node/issues?q=is%3Aissue+is%3Aopen+sort%3Areactions-%2B1-desc

Pull requests sorted by most thumbs up:

https://github.com/nodejs/node/pulls?q=is%3Apr+is%3Aopen+sort%3Areactions-%2B1-desc

@dlongley dlongley force-pushed the digitalbazaar:symlink-require branch from 1faf874 Apr 15, 2016

@MylesBorins

This comment has been minimized.

Member

MylesBorins commented May 2, 2016

if we revert this and close the relevant open PR's I would love to see all the tests we have written to verify functionality ported to master to help facilitate finding a non breaking implementation

@jasnell

This comment has been minimized.

Member

jasnell commented May 2, 2016

I'm preparing a revert now and am including two test cases that cover the basics. I'd like to see more added so once I push the branch feel free to open PRs against it with additional tests.

jasnell added a commit to jasnell/node that referenced this pull request May 2, 2016

test: test cases for circular symlinked deps and peer deps
Adds a test case in the known-test cases to catch nodejs#5950
Adds a test to verify that circular symlinked deps do not recurse
forever.

@jasnell jasnell referenced this pull request May 2, 2016

Closed

module: add --preserve-symlinks command line flag #6537

0 of 4 tasks complete
@dlongley

This comment has been minimized.

dlongley commented May 3, 2016

@jasnell,

I'm preparing a revert now ...

As an alternative to a revert, another option is to change the isMain behavior so we don't ever use realpath.

Edit: Nevermind -- I see that realpath is used to uniquely identify binary modules (it seems we should have some other mechanism for this). But, in that case, we could change the behavior so we only use it for binary modules.

jasnell added a commit to jasnell/node that referenced this pull request May 3, 2016

test: test cases for circular symlinked deps and peer deps
Adds a test case in the known-test cases to catch nodejs#5950
Adds a test to verify that circular symlinked deps do not recurse
forever.

@Fishrock123 Fishrock123 removed the ctc-agenda label May 4, 2016

@Gared Gared referenced this pull request May 5, 2016

Closed

Not working on Node 6.0.0 #2956

@kzc

This comment has been minimized.

kzc commented May 11, 2016

Last week's CTC meeting minutes:

Revert 5950 #6537

+* James: Bug picked up in v6 for symlinked peer dependencies, our tests/CI didn.t pick it up unfortunately. Proposal is to revert the change and review it after adding some tests for this.
+* Rod: there was some discussion about a revert being semver major
+* Jeremiah: not optimal, reverting is technically a semver-major
+* Rod: does anyone have an objection to a revert at this stage?
+* Jeremiah: I believe some of the new behaviour was useful, is there a path to reintroducing it?
+* James: least invasive route would be to add the new behaviour behind a flag, there are some other approaches being investigated. The folks who proposed this change in the first place are behind the reversion at this stage.

7f1111b

@jasnell I wouldn't say that statement is completely accurate. I think the supporters of #5950 would prefer an approach where the new module resolution scheme could seamlessly co-exist with the old one. The new resolution scheme has a number of useful characteristics that simplifies testing and fixes peer dependencies. I appreciate that others have relied on the old realpath behavior in ways we did not anticipate such as alternate package managers and the binary module singleton issue (when symlinked). But if faced with a revert as the only option, the flag approach is better than nothing.

@saper

This comment has been minimized.

saper commented May 12, 2016

I have re-launched #6537 to revert this and work on another solution in the background.

jasnell added a commit to jasnell/node that referenced this pull request May 13, 2016

src: add --preserve-symlinks command line flag
Add the `--preserve-symlinks` flag. This makes the changes added
in nodejs#5950 conditional. By default the old behavior is used. With
the flag set, symlinks are preserved, switching to the new
behavior. This should be considered to be a temporary solution
until we figure out how to solve the symlinked peer dependency
problem in a more general way that does not break everything
else.

Additional test cases are included.

jasnell added a commit that referenced this pull request May 13, 2016

src,module: add --preserve-symlinks command line flag
Add the `--preserve-symlinks` flag. This makes the changes added
in #5950 conditional. By default the old behavior is used. With
the flag set, symlinks are preserved, switching to the new
behavior. This should be considered to be a temporary solution
until we figure out how to solve the symlinked peer dependency
problem in a more general way that does not break everything
else.

Additional test cases are included.

PR-URL: #6537
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

evanlucas added a commit that referenced this pull request May 17, 2016

src,module: add --preserve-symlinks command line flag
Add the `--preserve-symlinks` flag. This makes the changes added
in #5950 conditional. By default the old behavior is used. With
the flag set, symlinks are preserved, switching to the new
behavior. This should be considered to be a temporary solution
until we figure out how to solve the symlinked peer dependency
problem in a more general way that does not break everything
else.

Additional test cases are included.

PR-URL: #6537
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@bmeck

This comment has been minimized.

Member

bmeck commented May 18, 2016

I am going to come in here, late to the party and say we keep the realpath behavior. Not attempt to do this. The realpath behavior allows singleton modules to be singletons. I am not quite clear on what the use case for this is outside of docs matching behavior (in which case I would rather muck with docs than touch module loader).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment