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

jasnell · 2016-05-02T23:36:44Z

Checklist

tests and code linting passes
a test and/or benchmark is included
documentation is changed or added
the commit message follows commit guidelines

Affected core subsystem(s)

module

Description of change

#5950 sought to address a bug in the module loader with regards to symlinked peer dependencies. The PR passed CITGM and CI on multiple runs and was landed as a semver-major in v6. Unfortunately, it came to light after that the change breaks a number of other cases.

~~This reverts the change and adds two additional test cases (one for the original behavior, and a known-issue test for the symlinked peer dependency case.~~
This puts the new functionality behind the --preserve-symlinks command line flag. The pre-v6 behavior is the default. Using the --preserve-symlinks command line option will make Node.js use the new behavior.

This should be considered to be a temporary solution until a better fix for the symlinked peer dependencies problem is figured out.

Refs: #5950

MylesBorins · 2016-05-02T23:41:13Z

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

jasnell · 2016-05-02T23:45:46Z

@thealphanerd ... may fail on windows... I added another commit that adds gates for windows symlink permissions

Trott · 2016-05-02T23:56:47Z

Is reverting a semver-major commit itself a semver-major change?

jasnell · 2016-05-03T00:02:31Z

No, this would be a bug fix.

dlongley · 2016-05-03T00:45:57Z

Can we, instead, just do realpath on binary modules?

jasnell · 2016-05-03T01:26:20Z

@dlongley ... At this point I definitely think the safest thing to do would be to revert for now then keep working on the problem. The one thing that is painfully clear is that our existing test cases do not have sufficient coverage of functionality that users have come to depend on. Reverting this for now does not mean we're giving up on it, it just means we take a step back, get the test coverage built up that we need to, and take another run at the problem.

bnoordhuis · 2016-05-03T05:07:57Z

test/addons/symlinked-module/submodule.js

+// jshint unused:true
+  var mod = require(path.join(bindingDir, 'binding.node'));
+  assert(mod != null);
+  assert(mod.hello() == 'world');


assert.strictEqual?

Yeah, there are several issues I was noticing with this commit. I modified it just enough to get it running but hadn't gone through a detailed review yet.

@saper .. If you don't mind I'm going to go through and clean these up. I'll keep your name on the commit tho.

Fishrock123 · 2016-05-03T14:22:35Z

This sounds major to me.

No, this would be a bug fix.

Can you explain in detail more?

jasnell · 2016-05-03T16:08:33Z

It's been a long conversation across multiple threads. This commit, while fixing a legitimate bug, broke several other aspects of module loading. Given that, it makes more sense to revert as a bug fix to resolve those issues and take another run at solving this problem in a different way.

saper · 2016-05-03T19:32:44Z

@jasnell I have amended my commit (your branch is at saper@2500a92). No jshint complaints and two node engines tested - one with a bug and one without.

saper · 2016-05-03T19:55:38Z

Maybe it's just a tone of the commit messages but I am not sure why we praise #3402 so much here. This should just revert the change and explain why. The tests for #3402 should be introduced somewhere else (if at all).

jasnell · 2016-05-03T19:59:52Z

The commit message is written to acknowledge the fact that the intent of the reverted commit was to fix a bug that still needs to be fixed after the revert.I can add the new tests from the reverted PR to the known issues tests as part of this revert PR

jasnell · 2016-05-04T20:28:12Z

CTC discussed this today and agreed to move forward with the revert in v6.x.

I will be working on putting the new behavior introduced by #5950 behind a command line flag.

wldcordeiro · 2016-05-04T21:21:39Z

@jasnell If I understood you right having the behavior from pre-6.0 will be behind the flag?

jasnell · 2016-05-04T21:29:26Z

No, the pre-6.0 behavior will be the default, the symlinked peer dependency fix will be what is behind a flag.

jasnell · 2016-05-12T23:55:36Z

@thealphanerd ...updated to use common.skip()

jasnell · 2016-05-13T00:02:23Z

CI is green... will land tomorrow if there are no objections from @nodejs/ctc or @nodejs/collaborators.

jasnell · 2016-05-13T16:08:30Z

One final CI: https://ci.nodejs.org/job/node-test-pull-request/2635/

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.

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 <[email protected]>

jasnell · 2016-05-13T18:45:22Z

Landed in 5d38d54

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 <[email protected]>

- **buffer**: fix lastIndexOf and indexOf in various edge cases (Anna Henningsen) [#6511](#6511) - **child_process**: use /system/bin/sh on android (Ben Noordhuis) [#6745](#6745) - **deps**: - upgrade npm to 3.8.9 (Rebecca Turner) [#6664](#6664) - upgrade to V8 5.0.71.47 (Ali Ijaz Sheikh) [#6572](#6572) - upgrade libuv to 1.9.1 (Saúl Ibarra Corretgé) [#6796](#6796) - Intl: ICU 57 bump (Steven R. Loomis) [#6088](#6088) - **repl**: - copying tabs shouldn't trigger completion (Eugene Obrezkov) [#5958](#5958) - exports `Recoverable` (Blake Embrey) [#3488](#3488) - **src**: add O_NOATIME constant (Rich Trott) [#6492](#6492) - **src,module**: add --preserve-symlinks command line flag (James M Snell) [#6537](#6537) - **util**: adhere to `noDeprecation` set at runtime (Anna Henningsen) [#6683](#6683)

- **buffer**: fix lastIndexOf and indexOf in various edge cases (Anna Henningsen) [#6511](#6511) - **child_process**: use /system/bin/sh on android (Ben Noordhuis) [#6745](#6745) - **deps**: - upgrade npm to 3.8.9 (Rebecca Turner) [#6664](#6664) - upgrade to V8 5.0.71.47 (Ali Ijaz Sheikh) [#6572](#6572) - upgrade libuv to 1.9.1 (Saúl Ibarra Corretgé) [#6796](#6796) - Intl: ICU 57 bump (Steven R. Loomis) [#6088](#6088) - **repl**: - copying tabs shouldn't trigger completion (Eugene Obrezkov) [#5958](#5958) - exports `Recoverable` (Blake Embrey) [#3488](#3488) - **src**: add O_NOATIME constant (Rich Trott) [#6492](#6492) - **src,module**: add --preserve-symlinks command line flag (James M Snell) [#6537](#6537) - **util**: adhere to `noDeprecation` set at runtime (Anna Henningsen) [#6683](#6683) As of this release the 6.X line now includes 64-bit binaries for Linux on Power Systems running in big endian mode in addition to the existing 64-bit binaries for running in little endian mode. PR-URL: #6810

jasnell added the module Issues and PRs related to the module subsystem. label May 2, 2016

This was referenced May 2, 2016

Revert "module: preserve symlinks when requiring" #6536

Closed

Possible Windows Node 6 regression with spawn and a substed drive #6500

Closed

bnoordhuis reviewed May 3, 2016
View reviewed changes

hax mentioned this pull request May 3, 2016

node 6 下 npminstall 的 cache 不行了 cnpm/npminstall#73

Closed

alexanderGugel mentioned this pull request May 3, 2016

module: resolve _-prefixed dependencies to real path #6460

Closed

4 tasks

jasnell force-pushed the revert-5950 branch from dae062c to 468096d Compare May 3, 2016 16:05

Fishrock123 force-pushed the master branch from 05f4313 to eb12f93 Compare May 3, 2016 16:46

jasnell added the ctc-agenda label May 4, 2016

rvagg mentioned this pull request May 4, 2016

Node.js Foundation Core Technical Committee (CTC) Meeting 2016-05-04 #6567

Closed

jasnell removed the ctc-agenda label May 4, 2016

jasnell force-pushed the revert-5950 branch from 56679bb to f224001 Compare May 12, 2016 23:55

jasnell force-pushed the revert-5950 branch from f224001 to a954c36 Compare May 13, 2016 18:42

jasnell closed this May 13, 2016

evanlucas mentioned this pull request May 17, 2016

2016-05-17, Version 6.2.0 (Stable) #6810

Merged

dlongley mentioned this pull request May 23, 2016

npm link doesn't play nice with peer dependencies npm/npm#5875

Closed

rosswarren mentioned this pull request Jul 8, 2016

Add --preserve-symlinks flag for node mochajs/mocha#2364

Merged

This was referenced Sep 7, 2016

Not working on Node V6 (T7318) babel/babel#4261

Closed

Node 6.0 breaks babel-node ("Unexpected token import") (T7336) babel/babel#4275

Closed

MylesBorins mentioned this pull request Jan 6, 2017

Potential Semver Minor Backports nodejs/Release#177

Closed

9 tasks

MylesBorins added dont-land-on-v4.x labels Jan 16, 2017

guybedford mentioned this pull request Mar 16, 2018

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

Closed

mrmeku mentioned this pull request Dec 20, 2019

Preserve symlinks by default when running node under Bazel bazel-contrib/rules_nodejs#1487

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

jasnell commented May 2, 2016 •

edited

Loading

MylesBorins commented May 2, 2016

jasnell commented May 2, 2016

Trott commented May 2, 2016

jasnell commented May 3, 2016

dlongley commented May 3, 2016

jasnell commented May 3, 2016

bnoordhuis May 3, 2016

jasnell May 3, 2016

Fishrock123 commented May 3, 2016 •

edited

Loading

jasnell commented May 3, 2016

saper commented May 3, 2016

saper commented May 3, 2016

jasnell commented May 3, 2016

jasnell commented May 4, 2016

wldcordeiro commented May 4, 2016

jasnell commented May 4, 2016

jasnell commented May 12, 2016

jasnell commented May 13, 2016

jasnell commented May 13, 2016

jasnell commented May 13, 2016

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

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

Conversation

jasnell commented May 2, 2016 • edited Loading

Checklist

Affected core subsystem(s)

Description of change

MylesBorins commented May 2, 2016

jasnell commented May 2, 2016

Trott commented May 2, 2016

jasnell commented May 3, 2016

dlongley commented May 3, 2016

jasnell commented May 3, 2016

bnoordhuis May 3, 2016

Choose a reason for hiding this comment

jasnell May 3, 2016

Choose a reason for hiding this comment

Fishrock123 commented May 3, 2016 • edited Loading

jasnell commented May 3, 2016

saper commented May 3, 2016

saper commented May 3, 2016

jasnell commented May 3, 2016

jasnell commented May 4, 2016

wldcordeiro commented May 4, 2016

jasnell commented May 4, 2016

jasnell commented May 12, 2016

jasnell commented May 13, 2016

jasnell commented May 13, 2016

jasnell commented May 13, 2016

jasnell commented May 2, 2016 •

edited

Loading

Fishrock123 commented May 3, 2016 •

edited

Loading