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

preferLocal prevents local from working #196

Closed
arcanis opened this issue Mar 28, 2019 · 18 comments · Fixed by #377
Closed

preferLocal prevents local from working #196

arcanis opened this issue Mar 28, 2019 · 18 comments · Fixed by #377

Comments

@arcanis
Copy link

arcanis commented Mar 28, 2019

#191 (comment)

Yarn already sets the PATH when running scripts so that the right Yarn binary (and all other binaries that your application should have access to) is always in the PATH. But because execa modifies it as well (and it does it without actually knowing which paths it should use, it only makes assumptions), it ends up overriding the Yarn-generated folder. In the end, the wrong files are called.

I would strongly suggest defaulting it to false, as it's very intrusive and can lead to pretty hard to debug issues (I myself spent an indecent amount of time trying to figure out what happened before I started to consider that maybe execa was doing something strange).

Example:

package.json

{
  "dependencies": {
    "execa": "^1.0.0"
  },
  "scripts": {
    "foo": "yarn --version",
    "bar": "node ./example.js"
  }
}

example.js

const execa = require('execa');

execa('yarn', ['--version']).then(v => {
  console.log(v.stdout);
});

command line

# npm installs the bins in the same folder as Node, so `node` and `yarn` are siblings
$> npm install -g [email protected]

# This command enforces the Yarn version inside this one project
$> yarn policies set-version 1.15.2

$> yarn foo
1.15.2

$> yarn bar
1.15.1

In this case, you see that Yarn rightfully enforced the use of the 1.15.2 release when calling foo. Unfortunately, since execa overrides the PATH with dirname(node), it also overrode in the process the Yarn binary that we set in the path, which causes the wrong one to be used.

It's even more of a problem with Plug'n'Play, since we inject the path to the binaries within the PATH (rather than always injecting the same node_modules/.bin folder). A similar problem then occurs where if you previously ran npm install -g babel-cli, then your execa scripts will use it instead of the local one.

@ehmicky
Copy link
Collaborator

ehmicky commented Mar 28, 2019

Thanks for reporting this.

Could you please describe in few words what yarn policies set-version is doing under the hood? For example how it manipulates $PATH, or anything else it is doing. Also where is each yarn binary located? I am not familiar with the underlying logic there, so further description would help me.

@arcanis
Copy link
Author

arcanis commented Mar 28, 2019

It just downloads a Yarn version from Github, stores it within a directory local to the project (which one exactly is unspecified), and updates the .yarnrc file to refer to it (via the yarn-path option). When Yarn starts, it checks whether a file is referenced by this mean and defers the execution to it if it finds one. This way, all members of your team are guaranteed to use the same version no matter what their global one is.

When you run yarn run, we create a mock shellscript named yarn in a temporary directory that, when called, defer the execution to process.argv0. We then unshift this temporary directory into the PATH. This guarantees that the nested scripts are always setup in such a way that calling the global yarn will always use the same one as the one originally used.

@ehmicky
Copy link
Collaborator

ehmicky commented Mar 28, 2019

This is a little complex, so I'm going to try to paraphrase it to make sure I understood.

The original problem yarn is solving with yarn policies set-version: yarn is meant to be installed globally. However some users might want to enforce a specific version of yarn locally.

How it is solved: a configuration/setting can be set in the local directory to indicate the preferred yarn version locally. When calling yarn globally, this setting is used (if it's set) to spawn the right yarn version.

How the spawning occurs: what I might understand is that:

  • there is a shell script in a temporary directory.
  • that temporary directory is added to the $PATH with the highest priority ("unshifted").
  • this shell script is called yarn
  • because of the three points above, this shell script is what gets executed when users type yarn in a terminal (?).

Things I am not sure:

  • When users type yarn run, which yarn gets executed? The globally installed binary, or that shell script?
  • is that temporary directory always in the $PATH, or is it added by yarn run during the duration of that process?
  • What is this shell script doing? I don't understand what you mean by "defer the execution to process.argv0".

Could you correct and complete my current understanding? Thanks for helping me out understand this :)

@arcanis
Copy link
Author

arcanis commented Mar 28, 2019

No problem! 🙂

How the spawning occurs: I don't completely understand how this works, could you explain it further?

If the global Yarn detects a local yarn-path settings it calls require or spawn on it, depending on the context (cf the code which might help to understand what I mean).

When users type yarn run, which yarn gets executed? The globally installed binary, or that shell script?

Initially the global one, which then defers to the local one via the mechanism described above. The local one then enters into the run implementation which creates this temporary directory (let's say /tmp/yarn-bins), generates /tmp/yarn-bins/yarn (our shell script), adds it into the PATH, then finally spawns the command with the fixed PATH. Cf the code for more info.

is that temporary directory always in the $PATH, or is it added by yarn run during the duration of that process?

Only added at the time of yarn run for the duration of the process.

What is this shell script doing? I don't understand what you mean by "defer the execution to process.argv0".

The script is dynamically generated based on the value of process.execPath and process.argv0 (cf code). A typical file will kinda look like this:

#!/usr/bin/env bash
exec "/path/to/node" "/path/to/my/local/yarn-1.5.2.js" "$@"

@ehmicky
Copy link
Collaborator

ehmicky commented Mar 28, 2019

Alright so if I understood correctly, this is what happens:

  • user types yarn run in a terminal.
  • globally installed yarn gets executed.
  • a local configuration setting indicates another yarn version should be used.
  • the locally installed yarn gets executed.
  • a shell script called yarn is created:
    • this shell script is a thin wrapper that defers its execution to the locally installed yarn.
    • it gets added to the PATH with high priority, i.e. further calls of yarn in the current process and child processes will refer to that shell script.

First, please let me know if I now got it right. I still have some questions:

  1. where is the locally installed yarn located? Is it in the local node_modules directory?
  2. when is the local yarn installed?
  3. why are you using that shell script instead of adding the path to the local yarn to the PATH directly?
  4. the last step is to ensure that child processes use the local yarn, am I correct?

@arcanis
Copy link
Author

arcanis commented Mar 28, 2019

where is the locally installed yarn located? Is it in the local node_modules directory?

It's in an unspecified directory. The yarn-path settings can target any location on the disk, no assumption can be made regarding which one it is.

when is the local yarn installed?

Typically when you use yarn policies set-version, but this command is just an helper and the local Yarn can be manually set by changing the yarn-path settings in your local .yarnrc file.

why are you using that shell script instead of adding the path to the local yarn to the PATH directly?

Two reasons:

  • We get to lock Node's version as well as Yarn's version (otherwise it would use the global node).
  • The local Yarn file isn't necessarily called yarn. It might be yarn-1.5.1.js, for example.

the last step is to ensure that child processes use the local yarn, am I correct?

Yep. Basically Yarn does the same thing as execa is meant to do with preferLocal, but we have more information to let us do the right thing without relying on the node_modules heuristic - so preferLocal doesn't really makes sense for us (and is confusing in the general case imo).

@ehmicky
Copy link
Collaborator

ehmicky commented Mar 28, 2019

Ok please excuse me there, but I am going to rewrite the problem again. It's not just for me but also for @sindresorhus so he can get a quick grasp of the problem as well.

This is what happens:

  • user installs yarn globally.
  • user runs yarn policies set-version which indicates that a specific yarn version should be used instead locally. This is done by installing another yarn version (the "local yarn") to some directory at an unspecified location. A local configuration setting is then set.
  • user types yarn run in a terminal.
  • globally installed yarn gets executed.
  • it finds the local configuration setting indicating that another yarn version should be used.
  • the "local yarn" gets executed.
  • a shell script called yarn is created:
    • this shell script is a thin wrapper that defers its execution to the "local yarn" + the current Node version.
    • it gets added to the PATH with high priority, so that further calls of yarn in child processes will refer to that shell script.

Your problem: execa preferLocal defaults to true, which means the steps above do not work. The reason is because the logic should be executed by the global yarn, not the local one.

Some follow-up questions:

  1. did I get the above two correctly?
  2. is the problem that "local yarn" is sometimes installed in the local node_modules/.bin/ directory, or in the process.execPath directory?
  3. If process.execPath is the problem, would this PR solves your problem: Fix repeated execPath directory npm-run-path#5? This is connected to execa does not respect $PATH variable #153.
  4. Why isn't the local yarn calling the global yarn if it detects it's been spawned instead? The reason I ask is that users might call the local yarn anyway, even without execa.

@arcanis
Copy link
Author

arcanis commented Mar 28, 2019

Thanks for bearing with me, appreciated 👍

Your problem: execa preferLocal defaults to true, which means the steps above do not work. The reason is because the logic should be executed by the global yarn, not the local one.

I'd reword that by adding an extra step:

  • execa is called by the running script - preferLocal defaults to true
    • the folder containing Node (which also happens to contains the global Yarn) is added with high priority to the PATH
    • when yarn is called, the first matching entry is the global Yarn (the second would have been the local one, but it's getting ignored)

is the problem that "local yarn" is sometimes installed in the local node_modules/.bin/ directory, or in the process.execPath directory?

It's in the process.execPath directory when installed through npm (we don't recommend that in our documentation, but some people still do it).

If process.execPath is the problem, would this PR solves your problem: sindresorhus/npm-run-path#5? This is connected to #153.

I think it would in my case, yep. It looks fragile though ... for example if the PATH contains /path/to/node/folder/, then it will be duplicated because the trailing slash will prevent an exact match. Same thing if the PATH contains /path/to/./node/folder instead, etc. Symlinks can also cause weird things to happen.

Why isn't the local yarn calling the global yarn if it detects it's been spawned instead? The reason I ask is that users might call the local yarn anyway, even without execa.

Hm I think you've reversed local and global here, since the end goal is to call the local Yarn, not the global Yarn? Assuming it was intended to be the other way around (why doesn't the global Yarn call the local Yarn anyway within yarn run?): that would only work if the third-party packages are guaranteed to use the same .yarnrc file as the local project.

This isn't the case when using Plug'n'Play, since the third-party packages are located within the cache (and thus the global Yarn wouldn't detect that it needs to forward to the local one).

@ehmicky
Copy link
Collaborator

ehmicky commented Mar 28, 2019

Oh alright, so the problem is that the shell script is meant to have highest priority, but execa with preferLocal: true will add process.execPath to the beginning of $PATH, which makes the global yarn have priority instead.

About the PR sindresorhus/npm-run-path#5: good point that this does not work when process.execPath is twice in the path but under different names. I probably should add some path.normalize(). However symlinks might be harder to detect considering the npm-run-path package:

  1. is synchronous
  2. deals with paths as string (as the core path module) not as filesystem paths (as the core fs module).

For further discussion on that PR though, this might be better to do it on the PR itself. What's important is whether this PR might solve your issue.

About my last comment ("I think you've reversed local and global here") forget it, I just got it wrong :)

This seems like the PR sindresorhus/npm-run-path#5 might solve your problem. To recap what this PR is doing:

  • execa relies on a module npm-run-path to implement the preferLocal feature.
  • npm-run-path adds the process.execPath's directory to the $PATH if it's not present yet. I'm not actually 100% sure why this is done but I am assuming this is to allow firing globally installed binaries.
  • The issue is that when process.execPath is already present (which I think should be almost always? otherwise node would not be in the $PATH), it still adds it. This effectively means it just increases its priority in $PATH.
  • Another potential issue I am thinking of is that maybe it should be added with lowest priority, not highest, although I am not sure of this.
  • This conflicts with several tools, including version managers, etc. and probably yarn as well in your case.

This PR needs to be reviewed and merged by @sindresorhus.

If this PR might solve your issue, feel free to comment on potential issues with its implementation.

@ehmicky
Copy link
Collaborator

ehmicky commented Mar 29, 2019

I think I've finally managed to wrap my head around this issue. I've updated the PR description sindresorhus/npm-run-path#5 accordingly.

The next step for this to move forward would be the get some input from @sindresorhus.

@arcanis
Copy link
Author

arcanis commented Apr 18, 2019

Ping?

@ehmicky
Copy link
Collaborator

ehmicky commented Apr 18, 2019

@sindresorhus I think the three following issues are connected: #196, #153 and sindresorhus/npm-run-path#5 (which seems unlikely to be merged).

Those issues are among the last things left on the v2 milestone.

What could we do to solve the issues experienced by users on this?

@arcanis
Copy link
Author

arcanis commented Apr 30, 2019

Ping bis? 😊

@ehmicky
Copy link
Collaborator

ehmicky commented May 24, 2019

@sindresorhus I thought of a way to move forward with this: see sindresorhus/npm-run-path#5 (comment)

Please let me know what you think. That's one of the last items for v2 release.

@tunnckoCore
Copy link

tunnckoCore commented Jun 17, 2019

Phew. I got it too.

The whole problem is npm-run-path adds the execPath, but should not add it at all, or add it only if some option is passed.

Btw, @ehmicky, check your /tmp folder after each yarn run, every time it creates a directory (i think that's the "unspecified"), in which you find to custom shell scripts - node and yarn

the node shell

#!/bin/sh

# that's the path to `execPath`
exec "/home/charlike/.nvm/versions/node/v12.4.0/bin/node" "$@"

the yarn shell

#!/bin/sh

exec "/home/charlike/.nvm/versions/node/v12.4.0/bin/node" "/home/charlike/.nvm/versions/node/v12.4.0/bin/yarn" "$@"

So in case you use set-version this second string will be different.

2019-06-17-193222_1280x1024_scrot
2019-06-17-193340_1280x1024_scrot

Sorry for the screenshotting, but I'm lazy now to record https://asciinema.org terminal video. 😆

@ehmicky
Copy link
Collaborator

ehmicky commented Jun 24, 2019

@arcanis If I understood your problem correctly, using the preferLocal: false option should partially solve it, could you confirm this?

We are considering changing its default value from true to false (#314).

This would limit this problem to users who want to use preferLocal, but not to other users.

@ehmicky
Copy link
Collaborator

ehmicky commented Jun 24, 2019

I'm closing this since #314 got merged.

@ehmicky ehmicky closed this as completed Jun 24, 2019
@sindresorhus
Copy link
Owner

@ehmicky We still need to fix this, even if it's not the default behavior.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants