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

[BUG] npx behavior in CI environments #1935

Closed
BethGriggs opened this issue Oct 9, 2020 · 32 comments
Closed

[BUG] npx behavior in CI environments #1935

BethGriggs opened this issue Oct 9, 2020 · 32 comments
Assignees
Labels
Bug thing that needs fixing Release 7.x work is associated with a specific npm 7 release

Comments

@BethGriggs
Copy link

This isn't really a bug, but raising it to discuss the potential impact of changes to npx in CI environments.

Current Behavior:

npx mocha
Need to install the following packages:
 mocha
Ok to proceed? (y)

npx -y mocha works to get the automatic install behaviour, but that would require users to update their CI scripts.

Also, npx -y mocha errors in npm@6 with:

npx -y mocha
ERROR: You must supply a command.
Execute binaries from npm packages.
  npx [options] <command>[@version] [command-arg]...

I think that would complicate the logic required in CI scripts when running across multiple Node.js/npm versions.

Just using Mocha as an example. In Node.js, we use npx in our GH Actions configuration, which I suspect would be broken when GH Actions updates to use a version of Node.js containing npm@7.

Expected Behavior:

  • Unsure, but curious if there could be a workaround that removes the need for users to edit their scripts.

Steps To Reproduce:

  • npx <module> in a CI enironment.

Environment:

  • All CI environments

/cc @MylesBorins @richardlau

@BethGriggs BethGriggs added Bug thing that needs fixing Needs Triage needs review for next steps Release 7.x work is associated with a specific npm 7 release labels Oct 9, 2020
@wesleytodd
Copy link
Contributor

@ruyadorno mentioned in the slack conversation that this was an intentional breaking change, could someone link to the context for that decision?

@ruyadorno
Copy link
Contributor

ruyadorno commented Oct 9, 2020

hi @BethGriggs thanks for letting us know 😄

It's worth mentioning that for non-interactive terminals we are going to patch (ref #1936) npx to only WARN instead of throwing so any combination of Unix pipelines would also avoid the prompt/confirmation:

  • npx mocha < /dev/null

@darcyclarke darcyclarke removed the Needs Triage needs review for next steps label Oct 9, 2020
@darcyclarke darcyclarke added this to the OSS - Sprint 17 milestone Oct 9, 2020
@ruyadorno
Copy link
Contributor

ruyadorno commented Oct 9, 2020

as also mentioned in the slack channel: npm_config_yes=true npx mocha is the working npm6-7 compat version of it

@nlf
Copy link
Contributor

nlf commented Oct 9, 2020

we published 7.0.0-rc.4 today that skips the prompt, but prints a warning, when run in a non-interactive environment. that way you're still alerted that something is being installed but we do not break in CI environments.

for interactive environments the above workaround should do the trick.

can you let us know if this resolves your issue @BethGriggs?

@gengjiawen
Copy link

we published 7.0.0-rc.4 today that skips the prompt, but prints a warning, when run in a non-interactive environment. that way you're still alerted that something is being installed but we do not break in CI environments.

for interactive environments the above workaround should do the trick.

can you let us know if this resolves your issue @BethGriggs?

root@localhost ~# npm install -g npm@7.0.0-rc.4
npm ERR! code MODULE_NOT_FOUND
npm ERR! Cannot find module 'agentkeepalive'
npm ERR! Require stack:
npm ERR! - /usr/local/lib/node_modules/npm/node_modules/make-fetch-happen/agent.js
npm ERR! - /usr/local/lib/node_modules/npm/node_modules/make-fetch-happen/index.js
npm ERR! - /usr/local/lib/node_modules/npm/node_modules/npm-registry-fetch/index.js
npm ERR! - /usr/local/lib/node_modules/npm/lib/utils/metrics.js
npm ERR! - /usr/local/lib/node_modules/npm/lib/npm.js
npm ERR! - /usr/local/lib/node_modules/npm/lib/cli.js
npm ERR! - /usr/local/lib/node_modules/npm/bin/npm-cli.js

npm ERR! A complete log of this run can be found in:
npm ERR!     /root/.npm/_logs/2020-10-10T03_17_28_890Z-debug.log

@BethGriggs
Copy link
Author

BethGriggs commented Oct 10, 2020

can you let us know if this resolves your issue @BethGriggs?

I think the concern has been addressed to reduce the impact in CI environments, thanks!

@nlf
Copy link
Contributor

nlf commented Oct 12, 2020

closing since the original issue is resolved.

@gengjiawen if you're able to reproduce the problem you had updating, please do open another issue and we can discuss it there

@nlf nlf closed this as completed Oct 12, 2020
@kentcdodds
Copy link
Contributor

kentcdodds commented Oct 20, 2020

FWIW, I bumped into this in TravisCI: https://travis-ci.com/github/kentcdodds/advanced-react-hooks/builds/191260988

image

I think that @BethGriggs' original concern is still an issue 😬

@ruyadorno
Copy link
Contributor

hmmm 🤔 maybe we should also check for the environment using @npmcli/ci-detect on top of checking for process.stdin.isTTY @nlf ?

@MylesBorins MylesBorins reopened this Oct 20, 2020
kentcdodds added a commit to testing-library/user-event that referenced this issue Oct 22, 2020
jesujcastillom pushed a commit to jesujcastillom/user-event that referenced this issue Oct 23, 2020
@FezVrasta
Copy link

FezVrasta commented Oct 26, 2020

Hitting this too, it broken all our CI scripts... Why did you introduce a breaking change in a minor/patch release?

For anyone having this issue on Circle CI, you can set a global environment variable to set the npm_config_yes variable to true to all your jobs

@MylesBorins
Copy link
Contributor

@FezVrasta the change came in the 7.0.0 Semver-Major change. We thought we fixed the CI issue by checking for TTY, but it seems there are edge cases we missed. Looks like #2047 is open to hopefully fix this for more CI environments

@nlf
Copy link
Contributor

nlf commented Oct 27, 2020

we published 7.0.6 today that should hopefully resolve this. we now skip the prompt entirely if you appear to be running in a CI environment.

let us know if you continue to have issues!

@eps1lon
Copy link

eps1lon commented Oct 29, 2020

Works for us now: https://github.com/testing-library/react-testing-library/pull/809/files

Thank you for the quick fix!

@3cp
Copy link

3cp commented Nov 21, 2020

Question: when npx7 prompts and then user installs the missing package,

  1. where is the package stored? Apparently it's not installed as global npm package.
  2. how to force npx makes to check and download latest version of "makes"? We got trouble that npx makes is trapped with local installed old version of "makes".

@ljharb
Copy link
Contributor

ljharb commented Nov 21, 2020

For the second question, npx -p makes or npx makes@latest should do it.

@ruyadorno
Copy link
Contributor

ruyadorno commented Dec 15, 2020

@3cp to answer to the first question: the packages are not installed as a global npm package, each npx install is placed in a separated hash-named folder within the npm cache folder, ref: https://github.com/npm/cli/blob/latest/lib/exec.js#L269

You can look them up with: ls $(npm get cache)/_npx if you ever would want to take a peek at them.

@3cp
Copy link

3cp commented Dec 23, 2020

@ruyadorno thx for the detail. We are able to reset the npx cache now.

Is there a reason for npx to just reuse the cache and not check latest published version (periodically if want to save traffic)? I would love to see npx to eventually bring up the cache to latest published version.

karlhorky added a commit to upleveled/eslint-config-upleveled that referenced this issue Feb 1, 2021
npx prompt content:

```
Need to install the following packages:
  install-peerdeps
Ok to proceed? (y)
```

Ref: npm/cli#1935
@karlhorky
Copy link
Contributor

In case anyone else finds this useful, I've made clearing the npx cache into a gist:

Gist: Clear npx cache

Content:

# Ref: https://github.com/npm/cli/issues/1935#issuecomment-745561262
rm -rf $(npm get cache)/_npx/*

@ruyadorno would there be any negative effects from running this? (aside from having to re-download packages you use with npx)

@ruyadorno
Copy link
Contributor

Is there a reason for npx to just reuse the cache and not check latest published version (periodically if want to save traffic)? I would love to see npx to eventually bring up the cache to latest published version.

I believe that's ultimately a design choice. In the npm arguments standard it's quite easy to specify you want the latest published version by appending @latest rather than the opposite.

@ruyadorno would there be any negative effects from running this? (aside from having to re-download packages you use with npx)

I don't think so 🙃 I believe they won't even necessarily be re-downloaded since the npx algo still shares the same cache as the npm installer, thus it might have the package in the global cache (usually ~/.npm/_cacache/) 😊

@ljharb
Copy link
Contributor

ljharb commented Feb 1, 2021

Designwise, npm typically chooses to automatically assume @latest unless the package is in your package.json or lockfile. none of these cases apply to npx, so i would absolutely assume that it always gets the latest.

A cache is just an implementation detail, and any time deleting the cache provides any different behavior than “it has to repopulate the cache” seems like an objective bug to me.

@karlhorky
Copy link
Contributor

I don't think so 🙃

Ok, we'll go with that! 😅

I believe they won't even necessarily be re-downloaded since the npx algo still shares the same cache as the npm installer, thus it might have the package in the global cache (usually ~/.npm/_cacache/) 😊

Ah, ok - so my "npx cache clear" script needs to run npm cache clean to force npx to get new versions of things?

@ruyadorno
Copy link
Contributor

A cache is just an implementation detail, and any time deleting the cache provides any different behavior than “it has to repopulate the cache” seems like an objective bug to me.

maybe it's the naming that is confusing and this is not a "npx cache" but rather a npx space in which these projects get installed to 🤔

Ah, ok - so my "npx cache clear" script needs to run npm cache clean to force npx to get new versions of things?

no no no, when doing a fresh install npx or npm install will always look up to the registry to find if there are new versions of things, I just mentioned that as a way to differentiate between the real cache from this npx space.

@karlhorky
Copy link
Contributor

karlhorky commented Feb 1, 2021

no no no, when doing a fresh install npx or npm install will always look up to the registry to find if there are new versions of things

Got it.

Just to be sure, what I'm trying to do is make the next run of the same command (after clearing the "cache", or whatever it should be called) install and use install-peerdeps@3.0.3 - the latest version:

$ npx install-peerdeps --yarn --dev @upleveled/eslint-config-upleveled
install-peerdeps v2.0.3

Running rm -rf $(npm get cache)/_npx/* seems to work, and the next run of this command installs and uses the new version:

$ rm -rf $(npm get cache)/_npx/*
$ npx install-peerdeps --yarn --dev @upleveled/eslint-config-upleveled
...
install-peerdeps v3.0.3

@ruyadorno
Copy link
Contributor

cool 😎 yeah, that's the expected result 👍

@ljharb
Copy link
Contributor

ljharb commented Feb 1, 2021

@ruyadorno i don’t think “npx installs to a persistent space” is the mental model of any of its users. The common understanding I’ve heard for years is that it installs it for the life of the command, and then deletes it.

@ruyadorno
Copy link
Contributor

@ruyadorno i don’t think “npx installs to a persistent space” is the mental model of any of its users. The common understanding I’ve heard for years is that it installs it for the life of the command, and then deletes it.

Yeah right, that's a great point - from the pov of someone aware of these internal mechanisms the mental model makes sense but not so much from the userland perspective. Maybe it makes up for a good RFC discussion? Maybe it's just a bug? 🤔

I think @wraithgar has been working on a related fix, maybe he'll have more context to add to this discussion.

@wraithgar
Copy link
Member

Yep I think the related fix I'm working on will mitigate most, if not all of this.

There are three underlying bugs, one is general caching (still debugging this one to see if preferOnline needs to be hard coded for init/exec). The second one is in lib/exec.js which erroneously uses any version of a package if it is running from the cache, regardless of if there is a newer version. The third bug is the one we fixed last week in arborist where an explicit add of a package wouldn't automatically pull in the latest version if you already had it in your tree.

So ... expect fix(es) soon on this. I'll have a PR for npm ready once I suss out where we want to land on the caching issue itself.

@ljharb
Copy link
Contributor

ljharb commented Feb 1, 2021

I consider it a bug, yes, and I'm glad to hear it's likely to be fixed :-)

@wraithgar
Copy link
Member

Here is the fix that handles the outdated versions of things running during npm init and npm exec/npx #2592

This doesn't touch or affect the -y portion of the issue or discussion, which I think has been sufficiently dealt with in other comments.

@Maxim-Mazurok
Copy link

Maxim-Mazurok commented May 21, 2021

There's a problem with env var solution (npm_config_yes=true).
The problem is that I have to support both Linux and Windows. So, to set env variables I'd like to use npx cross-env.
And obviously, npx cross-env npm_config_yes=true doesn't make any sense, because npm_config_yes needs to be set before npx cross-env. And I need npx cross-env to set npm_config_yes. It's a loop :)

Luckily, in my case I can just use --yes since npm@7 is required for the project.

@FossPrime
Copy link

FossPrime commented Mar 17, 2022

Summary on the current state of things: node:lts-alpine bundles NPM v6 for some reason. NVM's lts and node:lts are currently on NPM v8. Your CI and dev environment need to be on v7+ to avoid this regression. NEVER USE -y unless you're 100% sure your CI will never run NPM <=6.

@ljharb
Copy link
Contributor

ljharb commented Mar 17, 2022

node 14 ships with npm 6; node 16+ ships with npm 8. nvm and npm do not have LTS, only node does.

FirePrometheus0109 added a commit to FirePrometheus0109/eslint-config-upleveled that referenced this issue Aug 1, 2024
npx prompt content:

```
Need to install the following packages:
  install-peerdeps
Ok to proceed? (y)
```

Ref: npm/cli#1935
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug thing that needs fixing Release 7.x work is associated with a specific npm 7 release
Projects
None yet
Development

Successfully merging a pull request may close this issue.