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

Node executable path always prepended to PATH when running a package.json script #5935

Closed
jasongrout opened this issue Jun 5, 2018 · 14 comments
Closed
Assignees
Labels

Comments

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Jun 5, 2018

Do you want to request a feature or report a bug?
Feature

What is the current behavior?

There is no way to eliminate the explicit prepending of the node executable directory to the PATH environment variable when running package.json scripts.

Npm has a --scripts-prepend-node-path cli switch and config option, which defaults to auto, which only prepends the node executable directory to the path if it is not in the path. Also, setting the option (i.e., to true) prepends the node executable path just before the existing PATH, but after all of the local .bin directories. See https://docs.npmjs.com/cli/run-script.

Yarn, on the other hand, always prepends the node executable path (

// Include the directory that contains node so that we can guarantee that the scripts
// will always run with the exact same Node release than the one use to run Yarn
pathParts.unshift(path.dirname(process.execPath));
). Furthermore, it redundantly prepends the node executable path to the very front of the new path (overriding any local .bin directories) if the --scripts-prepend-node-path switch is given (
if (config.scriptsPrependNodePath) {
pathParts.unshift(path.join(path.dirname(process.execPath)));
}
). This last override was recently added in #5787 to address #5776.

Yarn's behavior is problematic. For example, if node is installed at the system level (e.g., /usr/bin), this effectively prepends /usr/bin to the path with no recourse, which completely inverts and breaks a normal PATH ordering of having local directories override the global /usr/bin directory. We saw this break a package.json script that called out to python, for example (it can break python virtual environments). You can also imagine many other situations when inverting the PATH priority like this can break things.

Furthermore, the behavior introduced in #5787 seems problematic, in that the global node path now overrides any local .bin directories prepended to the path, directly contradicting the npm behavior (but having the same option name).

What is the expected behavior?
I think npm's behavior here makes sense:

  1. By default, only prepend the node executable path to PATH if it is not already in PATH. And in that case, only prepend it just before the existing PATH, but after any local node .bin directories, etc (i.e., do the intelligent prepending at
    // Include the directory that contains node so that we can guarantee that the scripts
    // will always run with the exact same Node release than the one use to run Yarn
    pathParts.unshift(path.dirname(process.execPath));
    , and delete the prepending at
    if (config.scriptsPrependNodePath) {
    pathParts.unshift(path.join(path.dirname(process.execPath)));
    }
    ).
  2. Make it possible to explicitly enable or disable this prepending using --scripts-prepend-node-path

CC @ivanov

Please mention your node.js, yarn and operating system version.
yarn 1.7.0
node 8.11.2
npm 5.6.0
macOS 10.13

@ghost ghost assigned torifat Jun 5, 2018
@ghost ghost added the triaged label Jun 5, 2018
@jasongrout jasongrout changed the title node path *always* prepended, despite --scripts-prepend-node-path false Node executable path *always* prepended when running a package.json script Jun 5, 2018
@jasongrout jasongrout changed the title Node executable path *always* prepended when running a package.json script Node executable path always prepended when running a package.json script Jun 5, 2018
@jasongrout jasongrout changed the title Node executable path always prepended when running a package.json script Node executable path always prepended to PATH when running a package.json script Jun 5, 2018
@indirectlylit
Copy link

@indirectlylit indirectlylit commented Jun 9, 2018

see also #5874

Loading

@jasongrout
Copy link
Contributor Author

@jasongrout jasongrout commented Jun 9, 2018

Thanks for the reference. That definitely seems like the same issue.

Loading

@jasongrout
Copy link
Contributor Author

@jasongrout jasongrout commented Jun 9, 2018

Loading

@arcanis
Copy link
Member

@arcanis arcanis commented Jun 11, 2018

I'll fix this for the 1.9.0.

The goal of the faulty code is to force scripts executed through Yarn to use the same Yarn & npm than the ones currently executing Yarn - hijacking the PATH and affecting other binaries is an unfortunate side effects of the current implementation :(

Loading

@jasongrout
Copy link
Contributor Author

@jasongrout jasongrout commented Jun 11, 2018

Thanks!

Loading

briandk added a commit to briandk/transcriptase that referenced this issue Jun 16, 2018
@jasongrout
Copy link
Contributor Author

@jasongrout jasongrout commented Jun 27, 2018

@arcanis - We are trying to decide between downgrading yarn to avoid this bug, or waiting for the fix in 1.9.0. Do you happen to have any updates or timelines for a fix in 1.9.0?

Loading

jasongrout added a commit to jasongrout/jupyterlab that referenced this issue Jun 28, 2018
Fixes jupyterlab#4793

This avoids the bug in yarn noted at yarnpkg/yarn#5935, where a system path can override local paths. This bug can break executing commands from yarn in pipenv environments, for example.
@x-yuri
Copy link
Contributor

@x-yuri x-yuri commented Jun 29, 2018

I've been burnt by this trying to run ruby tests.

@jasongrout Consider the following workaround:

fix-path.sh:

#!/usr/bin/env bash
set -eu

s_path=$(printf "%s" "$PATH" | tr : \\n)
_IFS=$IFS
IFS=$'\n'
a_path=($s_path)
IFS=$_IFS

usr_bin=$(dirname -- "$(which node)")
n_usr_bin=$(egrep "^$usr_bin$" <(printf "%s" "$s_path") | wc -l)
r=()
for (( i = 0; i < ${#a_path[@]}; i++ )); do
    if [ "${a_path[$i]}" = "$usr_bin" ] && (( n_usr_bin > 1 )); then
        (( n_usr_bin-- ))
    else
        r+=("${a_path[$i]}")
    fi
done

PATH=$(
    for p in ${r[@]+"${r[@]}"}; do
        printf "%s\n" "$p"
    done | paste -sd:
)

"$@"

Then, in package.json:

"cmd1": "./fix-path.sh bin/rails test:system",
"cmd2": "./fix-path.sh bash -c 'echo $PATH' | tr : \\\\n"

Loading

@arcanis
Copy link
Member

@arcanis arcanis commented Jun 29, 2018

We are trying to decide between downgrading yarn to avoid this bug, or waiting for the fix in 1.9.0. Do you happen to have any updates or timelines for a fix in 1.9.0?

@jasongrout Hard to tell, I'm working on a large feature I really want to push in the 1.9.0 (and that includes the fix for this problem - which is a bit intricate so I'd prefer to avoid backporting it). I have hopes that I'll be able to publish it in a week or two.

Loading

@jasongrout
Copy link
Contributor Author

@jasongrout jasongrout commented Jun 29, 2018

Loading

@jasongrout
Copy link
Contributor Author

@jasongrout jasongrout commented Jul 19, 2018

@arcanis - I see yarn 1.9.0 was just released. Just curious, was a fix for this merged in 1.9.0?

Loading

@arcanis
Copy link
Member

@arcanis arcanis commented Jul 19, 2018

Unfortunately no, my feature isn't stable enough to have made the cut, and I don't have much time at the moment to work on a backport of this specific issue.

That said, if you're willing to work on a fix, we can merge it sooner (in short, what I do is: in execute-lifecycle-script, instead of adding the node binary path to the PATH, I instead create a temporary directory and symlink both node and yarn there. I then inject this temporary directory in the PATH).

Loading

@jasongrout
Copy link
Contributor Author

@jasongrout jasongrout commented Jul 19, 2018

Thanks for the update. I can't work on this in the next week, but if it's still open after that, I'll see about giving it a shot.

Loading

@jasongrout
Copy link
Contributor Author

@jasongrout jasongrout commented Jul 25, 2018

(as an update, it's not looking like I have very much available time to work on this, so anyone watching this: please go ahead and don't wait for me)

Loading

@bollwyvl
Copy link
Contributor

@bollwyvl bollwyvl commented Jul 26, 2018

Here's a very rough look at what I think you folks are saying (probably wrong):
bollwyvl@9288398#diff-939cc0d319704d4f0d98baf9167ba850R186

the tests do pass locally (on linux), but I don't think it's taking any effect right now, and haven't quite figured out how to test it.

Loading

@arcanis arcanis closed this in #6178 Aug 3, 2018
jasongrout added a commit to jasongrout/yarn that referenced this issue Aug 3, 2018
Fixes yarnpkg#5935

yarnpkg#6178 is a partial fix for yarnpkg#5935. This adds the following:

1. Don’t prepend the path *after* the local directories.
2. Only prepend the path if the node binary found by searching the environment path is different from our executable path (including if no node is found in the path). This takes care of cases where another node binary precedes our node executable in the path.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
6 participants