Add a --preserve-symlinks-main option #19911

dgoldstein0 · 2018-04-10T08:48:05Z

which behaves like --preserve-symlinks but for require.main. Fixes #19383

Checklist

I realize there is more to do here, but I wanted to send this out early for feedback. If there aren't any qualms with the approach, I'll figure out testing / documentation / etc.

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

jasnell

Needs some docs in doc/api/cli.md and doc/node.1

jasnell · 2018-04-10T15:30:07Z

src/node.cc

@@ -3526,6 +3531,8 @@ static void PrintHelp() {
         "                             note: linked-in ICU data is present\n"
 #endif
         "  --preserve-symlinks        preserve symbolic links when resolving\n"
+         "  --preserve-symlinks-main   preserve symbolic links when resolving "


this should include a \n after resolving and the next line should be indented to align.

vsemozhetbyt

Thank you. Sorry for format nits burden)

vsemozhetbyt · 2018-04-11T10:35:24Z

doc/api/cli.md

@@ -296,6 +296,30 @@ are linked from more than one location in the dependency tree (Node.js would
 see those as two separate modules and would attempt to load the module multiple
 times, causing an exception to be thrown).

+Note also that the `--preserve-symlinks` flag does not apply to the main
+module - this special case helps `node --preserve-symlinks node_module/.bin/<foo>`


We wrap lines after 80 characters, otherwise, there will be a doc linting issue.

vsemozhetbyt · 2018-04-11T10:36:37Z

doc/api/cli.md

+
+### `--preserve-symlinks-main`
+<!-- YAML
+added: TODO


added: REPLACEME is more common)

vsemozhetbyt · 2018-04-11T10:39:06Z

doc/api/cli.md

+
+This flag exists so that the main module can be opted-in to the same behavior
+that `--preserve-symlinks` gives to all other imports; they are separate flags,
+however, for backwards compatibility with older node.js versions.


Nits: backwards -> backward and node.js -> Node.js

vsemozhetbyt · 2018-04-11T10:40:42Z

doc/api/cli.md

+
+Note that `--preserve-symlinks-main` does not imply `--preserve-symlinks`; it
+is expected that `--preserve-symlinks-main` will be used in addition to
+`--preserve-symlinks` when it is not desirable to follow symlinks before resolving


This line also exceeds 80 chars limit.

vsemozhetbyt · 2018-04-11T10:44:04Z

lib/internal/modules/cjs/loader.js

+          }
+        } else if (preserveSymlinksMain) {
+          // for the main module, we use the preserveSymlinksMain flag instead
+          // mainly for backwards compatibility, as the preserveSymlinks flag


Nit: backwards -> backward?

other places in this file use "backwards" also, for what it's worth. But I'm happy to change.

vsemozhetbyt · 2018-04-11T10:45:13Z

lib/internal/modules/cjs/loader.js

+            filename = toRealPath(basePath);
+          }
+        } else if (preserveSymlinksMain) {
+          // for the main module, we use the preserveSymlinksMain flag instead


// for -> // For?

vsemozhetbyt · 2018-04-11T10:52:02Z

@dgoldstein0 It seems your local email config and GitHub email are different. Please, check these notes to sync them.

dgoldstein0 · 2018-04-12T06:54:19Z

@vsemozhetbyt curious, why does it matter? One is my work email, the other is my personal email; both will reach me. What's happening is that I'm doing this development for work, on my work machine, hence I'm committing with my work email, but I don't have separate github accounts for work and personal stuff.

vsemozhetbyt · 2018-04-12T08:30:45Z

If your local email is not included in your GitHub email list your commits with the local email will not be associated with your GitHub account (you can see it by the common avatar beside your commits in this PR). And when this PR is landed, GitHub will fail to promote you to Node.js Contributors team.

You can just add your second email to GitHub email list: https://github.com/settings/emails

vsemozhetbyt · 2018-04-12T08:34:44Z

cc @jasnell

jasnell · 2018-04-12T16:07:20Z

doc/api/cli.md

+Note also that the `--preserve-symlinks` flag does not apply to the main
+module - this special case helps
+`node --preserve-symlinks node_module/.bin/<foo>` work.  If you want the same
+behavior on the main module, also use `--preserve-symlinks-main`.


Please avoid using you in the docs... this can be reworded to:

The `--preserve-symlinks` flag does not apply to the main module, allowing `node --preserve-symlinks node_module/.bin/<foo>` to work. To apply the same behavior for the main module, also use `--preserve-symlinks-main`.

thanks for the advice. I knew second person wasn't ideal, just wasn't sure how to better phrase it.

dgoldstein0 · 2018-04-13T22:58:28Z

any recommendations on how to test this? I was thinking to take the test case from my issue, and translate that to an automated test - basically the test would shell out to node and invoke it with different options and assert about what stdout it ended up with. Does that seem reasonable? any advice of how to add a test that does this?

BridgeAR · 2018-04-23T08:32:17Z

@dgoldstein0 a test should work similar to the preserve-symlinks flag tests. You can find a couple of these in the test folder e.g., test/es-module/test-esm-preserve-symlinks.js.

dgoldstein0 · 2018-04-23T10:15:27Z

Thanks, I'll check that out

…

On Mon, Apr 23, 2018, 1:33 AM Ruben Bridgewater ***@***.***> wrote: @dgoldstein0 <https://github.com/dgoldstein0> a test should work similar to the preserve-symlinks flag tests. You can find a couple of these in the test folder e.g., test/es-module/test-esm-preserve-symlinks.js. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#19911 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABBndxfM3n6mXNzGzVhcKw9gR7fib8-8ks5trZHJgaJpZM4TN1-y> .

dgoldstein0 · 2018-05-03T01:00:44Z

ok I added a test case. anything else I need to do before we can merge this?

vsemozhetbyt · 2018-05-03T22:47:25Z

сс @jasnell @nodejs/modules

bnoordhuis · 2018-05-04T09:02:31Z

test/es-module/test-esm-preserve-symlinks-main.js

+  // the symlink, and not relative to the symlink target; the file structure set
+  // up above requires this to not crash when loading ./submodule_link.js
+  spawn(process.execPath,
+        flags.concat(['--preserve-symlinks', '--preserve-symlinks-main', entry_link_absolute_path]),


Long line. I expect this won't pass make lint.

vsemozhetbyt · 2018-05-07T22:16:46Z

CI: https://ci.nodejs.org/job/node-test-pull-request/14706/

vsemozhetbyt · 2018-05-07T22:44:18Z

@dgoldstein0 Sorry, it seems CI fails due to merge conflicts. Can you rebase and resolve conflicts?

dgoldstein0 · 2018-05-07T22:46:15Z

hm, that's weird, github thinks there are no conflicts. Well I'll dig.

…ymlinks but for require.main

vsemozhetbyt · 2018-05-07T22:53:10Z

GitHub and CI resolvings seem to differ sometimes(

dgoldstein0 · 2018-05-07T22:58:27Z

ok rebased now. I think this should be better.

vsemozhetbyt · 2018-05-07T23:03:42Z

CI: https://ci.nodejs.org/job/node-test-pull-request/14707/

Refs: nodejs#19911 PR-URL: nodejs#20741 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Refs: #19911 PR-URL: #20741 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

* addons: - Fixed a memory leak for users of `AsyncResource` and N-API. (Michael Dawson) #20668 * assert: - The `error` parameter of `assert.throws()` can be an object containing regular expressions now. (Ruben Bridgewater) #20485 * crypto: - The `authTagLength` option has been made more flexible (Tobias Nießen) #20235) #20039 * esm: - Builtin modules (e.g. `fs`) now provide named exports in ES6 modules. (Gus Caplan) #20403 * http: - Handling of `close` and `aborted` events has been made more consistent. (Robert Nagy) #20075 #20611 * module: - add --preserve-symlinks-main (David Goldstein) #19911 * timers: - `timeout.refresh()` has been added to the public API. (Jeremiah Senkpiel) #20298 * Embedder support: - Functions for creating V8 `Isolate` and `Context` objects with Node.js-specific behaviour have been added to the API. (Allen Yonghuang Wang) #20639 - Node.js `Environment`s clean up resources before exiting now. (Anna Henningsen) #19377 - Support for multi-threaded embedding has been improved. (Anna Henningsen) #20542 #20539 #20541 PR-URL: #20724

jdalton · 2018-06-17T19:27:53Z

lib/internal/modules/cjs/loader.js

+          // symlinks is usually required for the imports in the corresponding
+          // files to resolve; that said, in some use cases following symlinks
+          // causes bigger problems which is why the preserveSymlinksMain option
+          // is needed.


I noticed a bit lower in lib/internal/modules/cjs/loader.js that tryFile has a similar preserveSymlinks && !isMain check. Should it also add the preserveSymlinksMain juggle there?

node/lib/internal/modules/cjs/loader.js

Lines 193 to 199 in 452d73d

function tryFile(requestPath, isMain) {

const rc = stat(requestPath);

if (preserveSymlinks && !isMain) {

return rc === 0 && path.resolve(requestPath);

}

return rc === 0 && toRealPath(requestPath);

}

hm, maybe? I'm really not familiar with this code. --preserve-symlinks-main did pass the test cases I wrote for it, though maybe those aren't exhaustive enough? I wonder if there's something unique about the way the main file is loaded that it doesn't hit the tryFile code path? Just a guess.

If we can find a case where the code misbehaves as written then it's definitely something that we should fix.

reading a little closer... I think that node is supposed to be able to figure out that if asdf.js exists, then node asdf supposed to mean node asdf.js. It looks like what you've noticed is that the code for that doesn't handle --preserve-symlinks-main like it would handle --preserve-symlinks, which does seem like a bug, though at least an easy to work around one (just remember to specify the extension on the command line)

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Apr 10, 2018

jasnell requested changes Apr 10, 2018

View reviewed changes

vsemozhetbyt reviewed Apr 11, 2018

View reviewed changes

jasnell reviewed Apr 12, 2018

View reviewed changes

dgoldstein0 force-pushed the master branch from 768bb2b to 64a5766 Compare April 13, 2018 22:55

jasnell approved these changes May 3, 2018

View reviewed changes

bnoordhuis reviewed May 4, 2018

View reviewed changes

David Goldstein added 2 commits May 7, 2018 22:49

Add a --preserve-symlinks-main option which behaves like --preserve-s…

8a799d9

…ymlinks but for require.main

Update docs for --preserve-symlinks-main

ccd3ce8

David Goldstein added 4 commits May 7, 2018 22:54

fix documentation nits

58c973f

rephrase doc

e434980

Add a test

4d8cfdd

fix lint

0ec6486

dgoldstein0 force-pushed the master branch from 34c5b15 to 0ec6486 Compare May 7, 2018 22:56

Alhadis mentioned this pull request May 15, 2018

doc: fix linter warnings and typos in manpage #20741

Closed

2 tasks

jdalton reviewed Jun 17, 2018

View reviewed changes

ljharb mentioned this pull request Jul 31, 2019

[Fix] preserveSymlinks: false ensure that files are realpathed browserify/resolve#197

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a --preserve-symlinks-main option #19911

Add a --preserve-symlinks-main option #19911

dgoldstein0 commented Apr 10, 2018 •

edited

Loading

jasnell left a comment

jasnell Apr 10, 2018

vsemozhetbyt left a comment

vsemozhetbyt Apr 11, 2018

vsemozhetbyt Apr 11, 2018

vsemozhetbyt Apr 11, 2018

vsemozhetbyt Apr 11, 2018

vsemozhetbyt Apr 11, 2018

dgoldstein0 Apr 12, 2018

vsemozhetbyt Apr 11, 2018

vsemozhetbyt commented Apr 11, 2018 •

edited

Loading

dgoldstein0 commented Apr 12, 2018

vsemozhetbyt commented Apr 12, 2018 •

edited

Loading

vsemozhetbyt commented Apr 12, 2018

jasnell Apr 12, 2018

dgoldstein0 Apr 13, 2018 •

edited

Loading

dgoldstein0 commented Apr 13, 2018

BridgeAR commented Apr 23, 2018

dgoldstein0 commented Apr 23, 2018 via email

dgoldstein0 commented May 3, 2018

vsemozhetbyt commented May 3, 2018

bnoordhuis May 4, 2018

vsemozhetbyt commented May 7, 2018

vsemozhetbyt commented May 7, 2018

dgoldstein0 commented May 7, 2018

vsemozhetbyt commented May 7, 2018

dgoldstein0 commented May 7, 2018

vsemozhetbyt commented May 7, 2018

jdalton Jun 17, 2018

dgoldstein0 Jun 18, 2018

dgoldstein0 Jun 18, 2018

	function tryFile(requestPath, isMain) {
	const rc = stat(requestPath);
	if (preserveSymlinks && !isMain) {
	return rc === 0 && path.resolve(requestPath);
	}
	return rc === 0 && toRealPath(requestPath);
	}

Add a --preserve-symlinks-main option #19911

Add a --preserve-symlinks-main option #19911

Conversation

dgoldstein0 commented Apr 10, 2018 • edited Loading

Checklist

jasnell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vsemozhetbyt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vsemozhetbyt commented Apr 11, 2018 • edited Loading

dgoldstein0 commented Apr 12, 2018

vsemozhetbyt commented Apr 12, 2018 • edited Loading

vsemozhetbyt commented Apr 12, 2018

Choose a reason for hiding this comment

dgoldstein0 Apr 13, 2018 • edited Loading

Choose a reason for hiding this comment

dgoldstein0 commented Apr 13, 2018

BridgeAR commented Apr 23, 2018

dgoldstein0 commented Apr 23, 2018 via email

dgoldstein0 commented May 3, 2018

vsemozhetbyt commented May 3, 2018

Choose a reason for hiding this comment

vsemozhetbyt commented May 7, 2018

vsemozhetbyt commented May 7, 2018

dgoldstein0 commented May 7, 2018

vsemozhetbyt commented May 7, 2018

dgoldstein0 commented May 7, 2018

vsemozhetbyt commented May 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dgoldstein0 commented Apr 10, 2018 •

edited

Loading

vsemozhetbyt commented Apr 11, 2018 •

edited

Loading

vsemozhetbyt commented Apr 12, 2018 •

edited

Loading

dgoldstein0 Apr 13, 2018 •

edited

Loading