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

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

Closed
wants to merge 6 commits into from

Conversation

dgoldstein0
Copy link
Contributor

@dgoldstein0 dgoldstein0 commented Apr 10, 2018

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

@nodejs-github-bot 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
Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

src/node.cc Outdated
@@ -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 "
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@vsemozhetbyt vsemozhetbyt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you. Sorry for format nits burden)

doc/api/cli.md Outdated
@@ -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>`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

doc/api/cli.md Outdated

### `--preserve-symlinks-main`
<!-- YAML
added: TODO
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added: REPLACEME is more common)

doc/api/cli.md Outdated

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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

doc/api/cli.md Outdated

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line also exceeds 80 chars limit.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: backwards -> backward?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

// for -> // For?

@vsemozhetbyt
Copy link
Contributor

vsemozhetbyt commented Apr 11, 2018

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

@dgoldstein0
Copy link
Contributor Author

@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
Copy link
Contributor

vsemozhetbyt commented Apr 12, 2018

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
Copy link
Contributor

cc @jasnell

doc/api/cli.md Outdated
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`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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`.

Copy link
Contributor Author

@dgoldstein0 dgoldstein0 Apr 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@dgoldstein0
Copy link
Contributor Author

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
Copy link
Member

@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
Copy link
Contributor Author

dgoldstein0 commented Apr 23, 2018 via email

@dgoldstein0
Copy link
Contributor Author

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

@vsemozhetbyt
Copy link
Contributor

сс @jasnell @nodejs/modules

// 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]),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@vsemozhetbyt
Copy link
Contributor

@vsemozhetbyt
Copy link
Contributor

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

@dgoldstein0
Copy link
Contributor Author

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

@vsemozhetbyt
Copy link
Contributor

GitHub and CI resolvings seem to differ sometimes(

@dgoldstein0
Copy link
Contributor Author

ok rebased now. I think this should be better.

@vsemozhetbyt
Copy link
Contributor

Trott pushed a commit to Trott/io.js that referenced this pull request May 17, 2018
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>
MylesBorins pushed a commit that referenced this pull request May 22, 2018
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>
MylesBorins pushed a commit that referenced this pull request May 22, 2018
* 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
MylesBorins pushed a commit that referenced this pull request May 23, 2018
* 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
MylesBorins pushed a commit that referenced this pull request May 23, 2018
* 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
MylesBorins pushed a commit that referenced this pull request May 23, 2018
* 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
// 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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. 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. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants