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

npm install -g yarn #6

Closed
arcanis opened this issue Sep 3, 2020 · 7 comments
Closed

npm install -g yarn #6

arcanis opened this issue Sep 3, 2020 · 7 comments

Comments

@arcanis
Copy link
Contributor

arcanis commented Sep 3, 2020

With pmm currently installed, the following currently happens when running npm install -g yarn:

npm ERR! code EEXIST
npm ERR! syscall symlink
npm ERR! path ../lib/node_modules/yarn/bin/yarn.js
npm ERR! dest /tmp/tmp.SfN0q4jxL7/node-v15.0.0-nightlyYYYY-MM-DDXXXX-linux-x64/bin/yarn
npm ERR! errno -17
npm ERR! EEXIST: file already exists, symlink '../lib/node_modules/yarn/bin/yarn.js' -> '/tmp/tmp.SfN0q4jxL7/node-v15.0.0-nightlyYYYY-MM-DDXXXX-linux-x64/bin/yarn'
npm ERR! File exists: /tmp/tmp.SfN0q4jxL7/node-v15.0.0-nightlyYYYY-MM-DDXXXX-linux-x64/bin/yarn
npm ERR! Remove the existing file and try again, or run npm
npm ERR! with --force to overwrite files recklessly.

npm ERR! A complete log of this run can be found in:
npm ERR!     /home/arcanis/.npm/_logs/2020-09-03T14_30_46_868Z-debug.log

There are a few options on the ideal behavior:

  • Ask npm to assume --force when overriding the yarn binary (or any other binary covered by pmm). Pros, it preserves the current behavior, anyone who runs npm install -g yarn can keep doing it without changing their workflows. Cons, it means that they would effectively be replacing the pmm binary, so they wouldn't benefit from the "ensures the right version" anymore.

  • Ask npm to exit with a zero exit code when it detects that the yarn binary points to pmm, and print a message explaining the situation with a link to the pmm project. Perhaps we could also make it change the default Yarn version that pmm would use for new projects. Pros, people can keep using their workflow, they don't accidentally overwrite their binaries and lose features. Cons, it's a bit magical, might be confusing to some users.

  • Decide that this is the expected behavior, and that people upgrading to Node 15.x are expected to be aware that running npm install -g yarn isn't needed anymore. Potentially ask npm to improve the error message to explain the situation. Pros, fail early, this would make this improvement noticeable. Cons, it's maybe too noticeable, and CI providers would need to adapt to this change.

For reference, the ideal message I mention would be akin to:

npm ERR! Starting from Node 15.x, Yarn is partially distributed along with Node. To check that
npm ERR! is the case, run `yarn --version` in your project directory. More information on:
npm ERR!     https://nodejs.org/pmm/notice

With the url pointing to longer explanations.

@ljharb
Copy link
Member

ljharb commented Sep 3, 2020

npm install -g npm doesn't have any such error, why wouldn't npm install -g yarn upgrade the installed version of yarn too?

@arcanis
Copy link
Contributor Author

arcanis commented Sep 3, 2020

Because npm install -g works by symlink the binaries into the same location as the Node binary:

lrwxrwxrwx 1 arcanis arcanis  38 Sep  3 21:45 npm -> ../lib/node_modules/npm/bin/npm-cli.js
lrwxrwxrwx 1 arcanis arcanis  38 Sep  3 21:45 npx -> ../lib/node_modules/npm/bin/npx-cli.js

That's fine, except that in the pmm case the binaries are already there, and already point to something:

lrwxrwxrwx 1 arcanis arcanis  36 Aug 21 13:08 yarn -> ../lib/node_modules/pmm/dist/yarn.js
lrwxrwxrwx 1 arcanis arcanis  36 Aug 21 13:08 yarnpkg -> ../lib/node_modules/pmm/dist/yarn.js

Since npm isn't aware of this, when running npm install -g yarn it wants to symlink the yarn binary into:

yarn -> ../lib/node_modules/yarn/bin/yarn.js

Since the "new" symlink is different from the old one, npm fails with the aforementionned error, refusing to overwrite the symlink (which is the correct thing to do - you wouldn't want npm install -g ping to replace your ping utility).

why wouldn't npm install -g yarn upgrade the installed version of yarn too?

The notion of what is an upgrade in the context of pmm is part of what this issue is about. If npm install -g yarn was changing the target of the yarn binary from pmm/dist/yarn.js to yarn/bin/yarn.js, it wouldn't be an upgrade: it would be a replacement. As a result, features would be lost (in particular, users would silently lose the ability to use different versions of Yarn depending on the project they're working).

It might be an acceptable course of action (although it would require a change on npm's side to let it know that overriding the pmm binaries is fine), but I'm curious what other people think. I personally could see it being a good option for a few years, since it's low-friction with existing workflows, but in this case I'd prefer npm to also print a warning explaining what just happened, and how to reverse it (at the moment it would corrupt the Node install forever, as npm uninstall -g yarn would remove the Yarn symlink but without adding back the pmm one - so uninstall would need to be tweaked for uninstalls to restore the previous symlinks).

@ljharb
Copy link
Member

ljharb commented Sep 3, 2020

Couldn't yarn have a postinstall script to detect this case and repair it, without needing any changes in npm or node?

@arcanis
Copy link
Contributor Author

arcanis commented Sep 3, 2020

That's an interesting idea - it probably could, but I believe it would still require to at least make npm override the existing symlink if it's a pmm one (otherwise it'll hit the EEXIST before calling the postinstall). At this point, the same PR could probably add pmm symlink restoration on uninstalls to npm.

@arcanis
Copy link
Contributor Author

arcanis commented Sep 28, 2020

I've made a small hack in Yarn 1.22.7 which should magically make it compatible with Corepack (new name for pmm). While the postinstall was executed too late to prevent the symlink crash, the preinstall seems to be installed early enough to fix the environment.

@arcanis arcanis closed this as completed Sep 28, 2020
@arcanis
Copy link
Contributor Author

arcanis commented Sep 28, 2020

Of note, it won't be enough if the users have installed Node as root: yarnpkg/yarn#8358

@isaacs
Copy link

isaacs commented Sep 30, 2020

This issue should be reopened, I believe.

npm v7 will stubbornly refuse to install top-level global packages if a bin already exists by the same name as one that they're trying to create. If it knows that the bin linking will fail, then it won't unpack the dep or run any scripts for it. Again, my apologies for not catching this sooner.

Some options I can imagine:

  • Corepack's shim could be a symlink to a node program it places at ../lib/node_modules/yarn/bin/yarn.js. Of course, if corepack is placing this bin when installed via npm i -g corepack, then that's definitely bad behavior. If it's being installed as part of a node installation, then it ought to check to ensure that there isn't already a real yarn install in that location.
  • Users can npm install yarn --force to tell it to clobber the existing file.
  • Corepack can put the shim in some other location and tell users to add this to their PATH.
  • Corepack could use shell functions akin to nvm to provide a yarn command on the shell without placing an executable program in the PATH.

ncaq added a commit to ncaq/aws-cdk that referenced this issue Oct 15, 2020
aws-lambda-nodejs have issue

* [npm install -g yarn · Issue #6 · nodejs/corepack](nodejs/corepack#6)
* [npm install yarn --global fails in docker container · Issue aws#8358 · yarnpkg/yarn](yarnpkg/yarn#8358)

From issue comment,

> For future reference, you can (should) pin your version rather than use whatever the latest is on npm (by using yarn@1.22.6, etc) - it's a good practice anyway regardless of the conditions, as you never know which bug could slip by us. You can also use the yarn-path setting to ensure that upgrades go through the appropriate review processes (including CI).
>
> <yarnpkg/yarn#8358 (comment)>

So we'll follow it.

And from issue comment,

> Fwiw we don't plan to add any more features to Yarn 1, as all of our resources have shifted to Yarn 2. The past few commits have been aimed toward making the transition a bit easier, in particular thanks to the Corepack initiative which we hope will make it easier to use Yarn (both 1 & 2) by removing the need to manually install them.
>
> <yarnpkg/yarn#8358 (comment)>

There's not much need to be concerned with the latest version of 1.
mergify bot pushed a commit to aws/aws-cdk that referenced this issue Oct 21, 2020
fixes #10881

# fix from sh to bash

`lib/bundlers.ts` actually uses not `sh` but `bash`,
`bash` is a good way to test.

# lock yarn version

aws-lambda-nodejs have issue

* [npm install -g yarn · Issue #6 · nodejs/corepack](nodejs/corepack#6)
* [npm install yarn --global fails in docker container · Issue #8358 · yarnpkg/yarn](yarnpkg/yarn#8358)

From issue comment,

> For future reference, you can (should) pin your version rather than use whatever the latest is on npm (by using yarn@1.22.6, etc) - it's a good practice anyway regardless of the conditions, as you never know which bug could slip by us. You can also use the yarn-path setting to ensure that upgrades go through the appropriate review processes (including CI).
>
> <yarnpkg/yarn#8358 (comment)>

So we'll follow it.

And from issue comment,

> Fwiw we don't plan to add any more features to Yarn 1, as all of our resources have shifted to Yarn 2. The past few commits have been aimed toward making the transition a bit easier, in particular thanks to the Corepack initiative which we hope will make it easier to use Yarn (both 1 & 2) by removing the need to manually install them.
>
> <yarnpkg/yarn#8358 (comment)>

There's not much need to be concerned with the latest version of 1.

# allow execute command for non root user

`amazon/aws-sam-cli-build-image-nodejs12.x` don't have user that index is 1000.
So create non root user.

`/` in `amazon/aws-sam-cli-build-image-nodejs12.x` permission is `700`.
change to allow execute command for non root user.
I really don't want to change around the permissions,
but I don't have a choice.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants