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

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

Closed
wants to merge 1 commit into from

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented May 2, 2016

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

@jasnell jasnell added the module Issues and PRs related to the module subsystem. label May 2, 2016
@MylesBorins
Copy link
Contributor

@jasnell
Copy link
Member Author

jasnell commented May 2, 2016

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

@Trott
Copy link
Member

Trott commented May 2, 2016

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

@jasnell
Copy link
Member Author

jasnell commented May 3, 2016

No, this would be a bug fix.

@dlongley
Copy link

dlongley commented May 3, 2016

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

@jasnell
Copy link
Member Author

jasnell commented May 3, 2016

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

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

Choose a reason for hiding this comment

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

assert.strictEqual?

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Fishrock123 commented May 3, 2016

This sounds major to me.

No, this would be a bug fix.

Can you explain in detail more?

@jasnell
Copy link
Member Author

jasnell commented May 3, 2016

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

saper commented May 3, 2016

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

saper commented May 3, 2016

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

jasnell commented May 3, 2016

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

jasnell commented May 4, 2016

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.

@jasnell jasnell removed the ctc-agenda label May 4, 2016
@wldcordeiro
Copy link

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

@jasnell
Copy link
Member Author

jasnell commented May 4, 2016

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

@jasnell
Copy link
Member Author

jasnell commented May 12, 2016

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

@jasnell
Copy link
Member Author

jasnell commented May 13, 2016

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

@jasnell
Copy link
Member Author

jasnell commented May 13, 2016

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

jasnell commented May 13, 2016

Landed in 5d38d54

@jasnell jasnell closed this May 13, 2016
evanlucas pushed a commit that referenced this pull request May 17, 2016
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
- **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)
evanlucas added a commit that referenced this pull request May 17, 2016
- **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
evanlucas added a commit that referenced this pull request May 17, 2016
- **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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module Issues and PRs related to the module subsystem. 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.