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 NODE_PRESERVE_SYMLINKS environment variable #8749

Closed
wants to merge 1 commit into from

Conversation

mlucool
Copy link

@mlucool mlucool commented Sep 23, 2016

Checklist
  • make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

cli

Description of change

Add a way through environment variables to set the --preserve-symlinks
flag. A value of '1' for NODE_PRESERVE_SYMLINKS will enable symlinks.

Fixes: #8509

@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Sep 23, 2016
@@ -277,6 +277,12 @@ added: v0.11.15
Data path for ICU (Intl object) data. Will extend linked-in data when compiled
with small-icu support.

### `NODE_PRESERVE_SYMLINKS=true`
<!-- YAML
added: v6.7.0.
Copy link
Member

Choose a reason for hiding this comment

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

There should be no . here, and generally it’s easiest to simply use added: REPLACEME (this likely won’t end up in v6.7.0, which is going to be a security release on Tuesday).

<!-- YAML
added: v6.7.0.
-->
Instructs the module loader to preserve symbolic links when resolving and
Copy link
Member

@addaleax addaleax Sep 23, 2016

Choose a reason for hiding this comment

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

nit: Could you leave an empty line before this?

@addaleax addaleax added 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. labels Sep 23, 2016
@mlucool
Copy link
Author

mlucool commented Sep 23, 2016

Fixed

Copy link
Member

@imyller imyller left a comment

Choose a reason for hiding this comment

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

LGTM

// Allow for environment set preserving symlinks
if (secure_getenv("NODE_PRESERVE_SYMLINKS")) {
config_preserve_symlinks = true;
}
Copy link
Member

Choose a reason for hiding this comment

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

Please punctuate the comment and can you add a blank line after the block?

The way it's currently written means env NODE_PRESERVE_SYMLINKS= node will turn on the flag. Most seasoned UNIX users probably won't expect that.

Copy link
Author

Choose a reason for hiding this comment

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

Syntax fixed.

What you suggest is the best way to parse the flag, any value except for empty string? Should it handle env NODE_PRESERVE_SYMLINKS=false ?

Copy link
Member

Choose a reason for hiding this comment

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

I'd write it as e.g.:

if (auto var = secure_getenv("NODE_PRESERVE_SYMLINKS"))
  config_preserve_symlinks = (*var != '\0');

An empty string is false, everything else is true. That includes blank strings but I think that is an acceptable functionality vs. complexity trade-off.

Copy link
Author

Choose a reason for hiding this comment

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

Done

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.

LGTM

@Fishrock123
Copy link
Contributor

Uncertain we should make more new env variables for people to rely on here, not sure we will be supporting this with ES Modules... Don't need more to break than already will. 😐

@mlucool
Copy link
Author

mlucool commented Sep 26, 2016

Ideally we should support a .noderc file to allow for setting like this, similar to .npmrc. This seems like the most reasonable way to support it given current constraints.

@bnoordhuis
Copy link
Member

Config file support gets brought up from time to time but node has always been a self-contained binary and IMO it should stay that way.

@mlucool
Copy link
Author

mlucool commented Sep 26, 2016

I understand and am not trying to push one way or another, just buying into what is already there. Any more changes before this can be merged? Can this be pulled into the 6.x series also?

@@ -4188,6 +4188,11 @@ void Init(int* argc,
V8::SetFlagsFromString(NODE_V8_OPTIONS, sizeof(NODE_V8_OPTIONS) - 1);
#endif

// Allow for environment set preserving symlinks.
if (auto preserve_symlinks = secure_getenv("NODE_PRESERVE_SYMLINKS")) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this work on Windows?

Copy link
Author

Choose a reason for hiding this comment

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

Which part of this commit are you concerned will not work on windows?

Copy link
Contributor

Choose a reason for hiding this comment

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

ah, disregard. I misread it. Thanks!

@jasnell
Copy link
Member

jasnell commented Sep 27, 2016

@Fishrock123 ... I understand the concern but this really doesn't add a new thing that would be broken as much as it adds a new switch to enable an existing thing (that would be broken). Given that, it's likely less of a concern. If it helps, I would be +1 on leaving this undocumented.

@mlucool
Copy link
Author

mlucool commented Oct 1, 2016

I would prefer we document all features then have hidden ones to use at your own risk.

@Fishrock123 are you ok with merging this as is?

@jasnell
Copy link
Member

jasnell commented Oct 6, 2016

@nodejs/ctc ... any thoughts on this?

@@ -4188,6 +4188,11 @@ void Init(int* argc,
V8::SetFlagsFromString(NODE_V8_OPTIONS, sizeof(NODE_V8_OPTIONS) - 1);
#endif

// Allow for environment set preserving symlinks.
if (auto preserve_symlinks = secure_getenv("NODE_PRESERVE_SYMLINKS")) {
config_preserve_symlinks = (*preserve_symlinks != '\0');
Copy link
Member

Choose a reason for hiding this comment

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

Indentation seems to be off here and I'm having second thoughts about interpreting NODE_PRESERVE_SYMLINKS=0 as "yes please, enable this feature." Perhaps a more robust atoi or strtol-based parser is in order.

Copy link
Author

Choose a reason for hiding this comment

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

Whatever you prefer I will gladly change to. Just let me know.

Copy link
Contributor

Choose a reason for hiding this comment

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

For consistency with other env variables, it should be enabled when set to 1 and disabled otherwise.

Copy link
Author

Choose a reason for hiding this comment

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

So you are suggesting:
config_preserve_symlinks = (*preserve_symlinks == '1');

Copy link
Contributor

@Fishrock123 Fishrock123 Oct 7, 2016

Choose a reason for hiding this comment

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

NODE_PRESERVE_SYMLINKS=1

This what we do NODE_DISABLE_COLORS=1 and NODE_TTY_UNSAFE_ASYNC=1.

See https://nodejs.org/dist/latest-v6.x/docs/api/cli.html#cli_environment_variables

Copy link
Author

Choose a reason for hiding this comment

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

Tested and updated

@bnoordhuis
Copy link
Member

any thoughts on this?

I somewhat agree with @Fishrock123 that more knobs are not necessarily a good thing. More ways to shoot yourself in the foot, more baggage for us to maintain.

@mlucool
Copy link
Author

mlucool commented Oct 7, 2016

@bnoordhuis My reason for this commit is although the ability to set a flag is great, without a way to always enable this feature it becomes very error prone. The errors associated with a missing symlink flag are often non-obvious to a non-expert.

The reason we want to use symlinks is to provide a set of build tools in an enterprise environment without our large number of projects installing a huge redundant sets of dependencies (costs include extra time, dependency management, and inodes). In general, good symlink support is a huge benefit to our enterprise environments.

That being said, aside from wrapping node in script named node (which seems hacky and likely to cause issues when people want to try new versions of node) how else should we maintain certain flags to always be set?

@mlucool mlucool force-pushed the symlink-env-var branch 2 times, most recently from e69f324 to baaac09 Compare October 7, 2016 20:10
@deregtd
Copy link

deregtd commented Oct 12, 2016

Oh god I can't wait until this goes in. Then it can work with react-native as well! So close to actually being able to develop with symlinks!

@amakhrov
Copy link

One more use case is the IED package manager, which makes heavy use of symlinks

@mlucool
Copy link
Author

mlucool commented Oct 13, 2016

@bnoordhuis does this look good to you now? Any further changes?

@bnoordhuis
Copy link
Member

Technical side LGTM but needs a few regression tests. You can look at test/parallel/test-require-symlink.js for examples.

@mlucool
Copy link
Author

mlucool commented Oct 13, 2016

Thanks for the tip @bnoordhuis . Let me know if you have any further comments. It seemed to me like it should go in the same test file as it is testing another variant of the same thing. Copying and pasting the whole case seemed to not be DRY. I could make it two test cases in this file if that is better.

@Trott
Copy link
Member

Trott commented Oct 20, 2016

Raising visibility: @nodejs/collaborators

@sam-github
Copy link
Contributor

I'm generally in favour of any node CLI option being able to be controlled via environment.

Rather than doing this on an ad-hoc, CLI argument by argument basis, I wish that the PR to add an env variable from which any CLI option had progressed. It apparently didn't complete, and I can't find it in https://github.com/nodejs/node-v0.x-archive or here.

@Fishrock123 @misterdjules Do you remember the PR I'm talking about? It proposed a NODE_OPTIONS flag equivalent to RUBYOPT or PERL5OPT.

EDIT: actually, found it, and it didn't, it did only the minimal CLI option thing: #881

Anyhow, I know its frustrating to not get a feature you need now, because of some unspecified maybe in the future feature would be better, so I guess I'd have to be +1 on this.

@imyller
Copy link
Member

imyller commented Oct 24, 2016

@imyller
Copy link
Member

imyller commented Oct 24, 2016

Landing:

  • Approvals (LGTM): 6
  • No objections
  • PR has been open for the minimum time of 48 or 72 hours
  • All of the requested changes have been made
  • CI tests passed (only known/unrelated failures)

imyller pushed a commit to imyller/node that referenced this pull request Oct 24, 2016
Add a way through environment variables to set the --preserve-symlinks
flag. Any non-null value of NODE_PRESERVE_SYMLINKS will enable symlinks.

PR-URL: nodejs#8749
Fixes: nodejs#8509
Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@imyller
Copy link
Member

imyller commented Oct 24, 2016

Landed in d3b1a2b

Thank you, @mlucool

@imyller imyller closed this Oct 24, 2016
@imyller imyller removed their assignment Oct 24, 2016
@mlucool
Copy link
Author

mlucool commented Oct 24, 2016

@imyller thanks for merging! Can this be pulled into the 6.x branch also?

@MylesBorins
Copy link
Contributor

I've thrown an lts watch and lts-agenda tag on here. We will discuss it at the next meeting.

@mlucool
Copy link
Author

mlucool commented Oct 24, 2016

Thanks!

evanlucas pushed a commit that referenced this pull request Nov 2, 2016
Add a way through environment variables to set the --preserve-symlinks
flag. Any non-null value of NODE_PRESERVE_SYMLINKS will enable symlinks.

PR-URL: #8749
Fixes: #8509
Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@zkochan
Copy link

zkochan commented Nov 4, 2016

@Fishrock123, you said

Uncertain we should make more new env variables for people to rely on here, not sure we will be supporting this with ES Modules... Don't need more to break than already will. 😐

Do you mean the preserve symlinks feature might get deprecated? I am one of the contributors at pnpm and the preserve symlinks option is the only reason pnpm can still compete with yarn. (We use a single storage for packages in the file system)

If there's a discussion somewhere about the possibility of --preserve-symlinks deprecation, could you please point me to it?

@refack
Copy link
Contributor

refack commented Nov 24, 2016

What are the chances of rewriting the commit message (Any non-null value of a value of "1"). Since it's reflected in the changelogs.

@mlucool
Copy link
Author

mlucool commented Feb 23, 2017

@MylesBorins are there notes on why this was not going to land on 4/6?

@MylesBorins
Copy link
Contributor

@mlucool too high a probability of behavior changes

@refack
Copy link
Contributor

refack commented Apr 13, 2017

@mlucool I edited the description to match behaviour and docs
REF: #12366 (comment) my sad "war-story"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. 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.

Configuration option to permanently turn on --preserve-symlinks