--preserve-symlinks should (optionally?) apply to the require.main module

[dgoldstein0]

• Version: 8.9.3
• Platform: Linux
• Subsystem: module loading

From #9673:

--preserve-symlinks does preserve symlinks for all require() calls, but strangely not that of the entry.js file passed on node's command line.

This still seems to be the case today: it is impossible to get node to preserve symlinks for the entry point file. A quick demo of today's behavior:

// a.js
console.log("problem");

// entry.js
require("./a");

// app/a.js
console.log("no problem");

symlink app/entry.js -> entry.js

Then run node --preserve-symlinks app/entry.js.

What I expected: "no problem" printed
What actually happens: "problem" printed

--

This is a problem for me because I am supporting node integration with bazel (Google's open source build system), and bazel generates binaries by building all the files, and creating a symlink tree for each binary that gives it access to all the files it needs; our bazelled node binaries need --preserveSymlinks so that they resolve modules relative to the symlink, and not to the target of the symlink. While we've worked around the problem, our workaround breaks require.main and module.parent among others, so code that works under vanilla node won't work in our bazelled binaries. The exact details are documented here: https://github.com/dropbox/rules_node#node_binary - the BAZEL_NODE_MAIN_ID hack is our workaround, which imo is rather ugly.

If either:
a) --preserve-symlinks applied the same to the entry point as all other modules
b) there was an extra flag to enable the same symlink preserving behavior for the entry point

Then we could remove this hack and have better compatibility with existing npm modules.

[dgoldstein0]

fwiw I suspect that the main module is treated specially to support symlinks like node_modules/.bin/tsc. Possibly we could special case those to follow exactly one level of symlinks, but all other cases we do the standard preserve-symlinks behavior (when the flag is passed, that is). Or just keep it simple and allow e.g. a --preserve-symlinks-main that will apply the --preserve-symlinks behavior to the main file, and leave .bin/ special cases up to the caller to figure out.

[guybedford]

@dgoldstein0 looking at the code this does seem to be quite intentional, although I'm not sure what the intended use case was for this.

node_modules/.bin sounds like a good reasoning though to me.

Interestingly we don't do this in the ES module resolver, so this makes me think we should probably follow the same actually. So while unrelated, thanks for bringing awareness to this :)

[guybedford]

Looking through the discussion at #6537 I can't actually seem to find any good reason for skipping preserveSymlinks for the main entry point actually.

I'd be interested to hear what others think here as it would be nice to ensure consistency with the ES loader - should we try make preserveSymlinks work for the main, or bring this behaviour into the ES resolver?

//cc @jasnell

[dgoldstein0]

bump. any updates on this? did it get lost in other people's inboxes?

[guybedford]

@dgoldstein0 in putting together #19388 I did review #6537 quite closely on this question, and as far as I can tell at https://github.com/nodejs/node/pull/6537/files#diff-d1234a869b3d648ebfcdce5a76747d71L116 this is a very deliberate feature likely for supporting Node bins.

Changing such a thing would these use cases as well as break backwards compatibility, so I don't see that it would be that possible at this point.

But there may be deeper arguments to be made here.

[dgoldstein0]

Well if the point is to support .bin symlinks, perhaps those can be special cased differently? Or we can just have yet another flag for it. All I really want is to have a way to get the preserve symlinks behavior even on main. Having it behind yet another flag, while ugly, works better for my use case than the workarounds we've resorted to, and wouldn't pose any backwards compatibility risk.

…

On Wed, Mar 28, 2018, 1:32 AM Guy Bedford ***@***.***> wrote: @dgoldstein0 <https://github.com/dgoldstein0> in putting together #19388 <#19388> I did review #6537 <#6537> quite closely on this question, and as far as I can tell at https://github.com/nodejs/node/pull/6537/files#diff-d1234a869b3d648ebfcdce5a76747d71L116 this is a very deliberate feature likely for supporting Node bins. Changing such a thing would these use cases as well as break backwards compatibility, so I don't see that it would be that possible at this point. But there may be deeper arguments to be made here. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#19383 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABBnd7qDUGhLTbxKJ6Np9-Z0u9fLviSiks5ti0qbgaJpZM4StNVQ> .

[guybedford]

@dgoldstein0 as I say I'm not sure there's a way to do that without breaking existing .bin cases with --preserveSymlinks at this point. Perhaps you could describe your use case here a little further?

[dgoldstein0]

it's already explained in the ticket? Anyhow I am suggesting an extra flag in addition to --preserveSymlinks. e.g. --preserveSymlinksMain which would apply the same behavior as --preserveSymlinks to the main module. but to be even more clear: the way bazel works is to create "binaries" which are an executable script and a "runfiles" folder, which is all the data needed for the executable to run. For dynamic languages, generally the executable is just a shell script that sets up the environment and then invokes the right files from the runfiles - for node binaries, this is something like `$RUNFILES/node --preserveSymlinks $RUNFILES/<main>` (the runfiles folder is set as an environment variable). Python works much the same way, For correctness, module resolution MUST be done relative to the locations within the runfiles folder. If node ever follows a symlink out of this folder, it can end up loading code that is not part of the bazel-built binary; which can cause horrific effects for bazel-based selective testing, as we won't rerun tests when this phantom dependencies change, and can end up falsely thinking that our tests pass when they should be failing. This is completely unacceptable behavior, so we've worked around it as follows: we make bazel product a dummy main module which `require()`s the real one, which is making `require.main` point to the dummy main. Our dummy main fix breaks `require.main` and breaks `module.parent` in the intended main file: I should be able to take any third party's npm-published package, look at one of their `.bin/` symlinks, figure out what it points at, and turn that into a `node_binary` with bazel; but as long as we inject our own entry point, `require.main` and `module.parent` misbehave in such binaries. The only way for us to safely be able to use external code without potentially forking it for these cases is to have a way to enable --preserveSymlinks behavior for the main module. It is completely acceptable that .bin/ links won't work for us, as we can just read what they point to and point bazel to use the symlink targets as the main module.

…

On Wed, Mar 28, 2018 at 2:04 PM Guy Bedford ***@***.***> wrote: @dgoldstein0 <https://github.com/dgoldstein0> as I say I'm not sure there's a way to do that without breaking existing .bin cases with --preserveSymlinks at this point. Perhaps you could describe your use case here a little further? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#19383 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABBnd9PQM9qWjA4x9zuJR2O205LboQAvks5ti_rPgaJpZM4StNVQ> .

[guybedford]

Right, I get what you mean. Are you proposing a --preserve-symlinks-main flag or similar then? Perhaps the best route forward is to create a PR and then see how that fares in review. I'm not sure how I feel about it personally.

[dgoldstein0]

I suppose so. I think it's ugly, but it would fix my use case and satisfies backwards compatibilty so seems like the least contentious possibility.

…

On Sat, Mar 31, 2018, 8:58 AM Guy Bedford ***@***.***> wrote: Right, I get what you mean. Are you proposing a --preserve-symlinks-main flag or similar then? Perhaps the best route forward is to create a PR and then see how that fares in review. I'm not sure how I feel about it personally. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#19383 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABBnd0lTWqeWS2-RdG434t6xS7t8jOzWks5tj31TgaJpZM4StNVQ> .

[vsemozhetbyt]

Closed in 2365965