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] npm ci installs an optional dependency which targets a different os than the host os #558

Closed
JakeChampion opened this issue Dec 5, 2019 · 38 comments
Labels
Bug thing that needs fixing

Comments

@JakeChampion
Copy link
Contributor

JakeChampion commented Dec 5, 2019

What / Why

npm ci seems to install an optional dependency for the linux os when running on a mac and seems to install the optional dependency for mac when running on linux.

When

$ npm init -y; npm i oax@0.5.25; npm ls; npm ci; npm ls
click to view output of above command
Wrote to /private/tmp/d/package.json:

{
"name": "d",
"version": "1.0.0",
"description": "",
"main": "index.js",
"scripts": {
"test": "echo "Error: no test specified" && exit 1"
},
"keywords": [],
"author": "",
"license": "ISC"
}

oax@0.5.25 postinstall /private/tmp/d/node_modules/oax
node ./postinstall.js

npm notice created a lockfile as package-lock.json. You should commit this file.
npm WARN d@1.0.0 No description
npm WARN d@1.0.0 No repository field.
npm WARN optional SKIPPING OPTIONAL DEPENDENCY: oax-windows-64@0.5.25 (node_modules/oax/node_modules/oax-windows-64):
npm WARN notsup SKIPPING OPTIONAL DEPENDENCY: Unsupported platform for oax-windows-64@0.5.25: wanted {"os":"win32","arch":"x64"} (current: {"os":"darwin","arch":"x64"})
npm WARN optional SKIPPING OPTIONAL DEPENDENCY: oax-linux-64@0.5.25 (node_modules/oax/node_modules/oax-linux-64):
npm WARN notsup SKIPPING OPTIONAL DEPENDENCY: Unsupported platform for oax-linux-64@0.5.25: wanted {"os":"linux","arch":"x64"} (current: {"os":"darwin","arch":"x64"})

  • oax@0.5.25
    added 2 packages and audited 4 packages in 1.1s
    found 0 vulnerabilities

d@1.0.0 /private/tmp/d
└─┬ oax@0.5.25
├── oax-darwin@0.5.25
├── UNMET OPTIONAL DEPENDENCY oax-linux-64@0.5.25
└── UNMET OPTIONAL DEPENDENCY oax-windows-64@0.5.25

npm WARN prepare removing existing node_modules/ before installation

oax@0.5.25 postinstall /private/tmp/d/node_modules/oax
node ./postinstall.js

added 3 packages in 0.722s
d@1.0.0 /private/tmp/d
└─┬ oax@0.5.25
├── oax-darwin@0.5.25
├── oax-linux-64@0.5.25
└── UNMET OPTIONAL DEPENDENCY oax-windows-64@0.5.25

Where

  • n/a

How

Current Behavior

currently it looks like npm ci is broken for optional dependencies which use the os and arch fields of package.json

Steps to Reproduce

$ npm init -y; npm i oax@0.5.25; npm ls; npm ci; npm ls

You should see that npm i works correctly and installs a single optional dependency for oax.
You should also see that npm ci works incorrectly and installs two of the optional dependencies for oax, this should never happen as each optional dependency is targeting a different operating system and architecture, it should be impossible to have more than one of the optional dependencies installed.

Expected Behavior

it should install oax-darwin when running on darwin and should install oax-linux when running on linux

Who

  • n/a

References

  • n/a
@coyoteecd
Copy link

I ran into the same problem with fsevents on Windows, when installing two packages that depend on different versions of fsevents.

npm install chokidar --save

npm WARN optional SKIPPING OPTIONAL DEPENDENCY: fsevents@2.1.2 (node_modules\fsevents):
npm WARN notsup SKIPPING OPTIONAL DEPENDENCY: Unsupported platform for fsevents@2.1.2: wanted {"os":"darwin","arch":"any"} (current: {"os":"win32","arch":"x64"})

+ chokidar@3.3.1
added 14 packages from 17 contributors and audited 19 packages in 1.668s
found 0 vulnerabilities

npm install webpack --save-dev

npm WARN optional SKIPPING OPTIONAL DEPENDENCY: fsevents@1.2.11 (node_modules\watchpack\node_modules\fsevents):
npm WARN notsup SKIPPING OPTIONAL DEPENDENCY: Unsupported platform for fsevents@1.2.11: wanted {"os":"darwin","arch":"any"} (current: {"os":"win32","arch":"x64"})

+ webpack@4.41.5
added 327 packages from 195 contributors and audited 4246 packages in 16.581s

Latest webpack depends on watchpack which depends on an older chokidar@2.1.8 which depends on an old fsevents@1.2.11.

While chokidar latest depends on fsevents@2.1.2

But npm install skipped correctly both versions of fsevents as they are OS-incompatible.

However:

 npm ci
npm WARN prepare removing existing node_modules/ before installation

> fsevents@1.2.11 install K:\SWS\test\node_modules\watchpack\node_modules\fsevents
> node-gyp rebuild


K:\SWS\test\node_modules\watchpack\node_modules\fsevents>if not defined npm_config_node_gyp (node "C:\Program Files\nodejs\node_modules\npm\node_modules\npm-lifecycle\node-gyp-bin\\..\..\node_modules\node-gyp\bin\node-gyp.js" rebuild )  else (node "C:\Program Files\nodejs\node_modules\npm\node_modules\node-gyp\bin\node-gyp.js" rebuild )
Traceback (most recent call last):
  File "C:\Program Files\nodejs\node_modules\npm\node_modules\node-gyp\gyp\gyp_main.py", line 50, in <module>
    sys.exit(gyp.script_main())
  File "C:\Program Files\nodejs\node_modules\npm\node_modules\node-gyp\gyp\pylib\gyp\__init__.py", line 554, in script_main
    return main(sys.argv[1:])
  File "C:\Program Files\nodejs\node_modules\npm\node_modules\node-gyp\gyp\pylib\gyp\__init__.py", line 547, in main
    return gyp_main(args)
  File "C:\Program Files\nodejs\node_modules\npm\node_modules\node-gyp\gyp\pylib\gyp\__init__.py", line 532, in gyp_main
    generator.GenerateOutput(flat_list, targets, data, params)
  File "C:\Program Files\nodejs\node_modules\npm\node_modules\node-gyp\gyp\pylib\gyp\generator\msvs.py", line 2033, in GenerateOutput
    root_entries = _GatherSolutionFolders(
  File "C:\Program Files\nodejs\node_modules\npm\node_modules\node-gyp\gyp\pylib\gyp\generator\msvs.py", line 1791, in _GatherSolutionFolders
    return _DictsToFolders('', root, flat)
  File "C:\Program Files\nodejs\node_modules\npm\node_modules\node-gyp\gyp\pylib\gyp\generator\msvs.py", line 1744, in _DictsToFolders
    for folder, contents in bucket.items():
AttributeError: 'MSVSProject' object has no attribute 'items'
gyp ERR! configure error
gyp ERR! stack Error: `gyp` failed with exit code: 1
gyp ERR! stack     at ChildProcess.onCpExit (C:\Program Files\nodejs\node_modules\npm\node_modules\node-gyp\lib\configure.js:351:16)
gyp ERR! stack     at ChildProcess.emit (events.js:210:5)
gyp ERR! stack     at Process.ChildProcess._handle.onexit (internal/child_process.js:272:12)
gyp ERR! System Windows_NT 10.0.17134
gyp ERR! command "C:\\Program Files\\nodejs\\node.exe" "C:\\Program Files\\nodejs\\node_modules\\npm\\node_modules\\node-gyp\\bin\\node-gyp.js" "rebuild"
gyp ERR! cwd K:\SWS\test\node_modules\watchpack\node_modules\fsevents
gyp ERR! node -v v12.14.0
gyp ERR! node-gyp -v v5.0.5
gyp ERR! not ok
added 275 packages in 9.344s

And if I look in node_modules, fsevents is there and it shouldn't be.
Also, npm ci --no-optional doesn't work, this is reported here: #637

I'm using the Node 12 LTS install, npm -v => 6.13.4

@ManfredLange
Copy link

ManfredLange commented Jan 4, 2020

Whether npm ci --no-optional works may depend on additional factors. See #637 (comment)

So while npm ci fails in my environment, npm ci --no-optional works. My environment:

  • Windows 10
  • Nodejs 12.13.1
  • npm 6.13.4

@padinko
Copy link

padinko commented Jan 13, 2020

After updating to latest node and npm have same issue.. all projects where it is fsevent fail to install with npm ci, because it is building fsevents

  • Windows 10 Pro 1909
  • node 12.14.1
  • npm 6.13.6

@ManfredLange
Copy link

After updating to latest node and npm have same issue.. all projects where it is fsevent fail to install with npm ci, because it is building fsevents

* Windows 10 Pro 1909

* node 12.14.1

* npm 6.13.6

@padinko Did you also try the variant with the additional --no-optional flag, i.e. npm ci --no-optional? Any difference?

@padinko
Copy link

padinko commented Jan 13, 2020

After updating to latest node and npm have same issue.. all projects where it is fsevent fail to install with npm ci, because it is building fsevents

* Windows 10 Pro 1909

* node 12.14.1

* npm 6.13.6

@padinko Did you also try the variant with the additional --no-optional flag, i.e. npm ci --no-optional? Any difference?

npm ci fail with this packages:

\node_modules\watchpack\node_modules\fsevents
\node_modules\webpack-dev-server\node_modules\fsevents
\node_modules\jest-haste-map\node_modules\fsevents

npm ci --no-optional have only 2 of them:

\node_modules\webpack-dev-server\node_modules\fsevents
\node_modules\jest-haste-map\node_modules\fsevents

so there it is difference, but still compiling fsevents

@guidobouman
Copy link

@paulmillr @pipobscure My issue (#658) was a duplicate of this ticket. Track this one to stay up to date.

@padinko
Copy link

padinko commented Jan 23, 2020

I can confirm this bug also on Ubuntu. npm ci installs fsevents that should be installed only on MacOS

@guidobouman
Copy link

@mikemimik or @isaacs, do you have any input on this bug? Yes, fsevents changed something why this is now a bigger issue. But the underlying issue is still caused by NPM, and should be resolved here.

@isaacs
Copy link
Contributor

isaacs commented Jan 29, 2020

It looks like the relevant change is that fsevents started using node-pre-gyp to pull a precompiled binary, rather than building it in place, resulting in a postinstall script that exits without an error on all platforms. Since npm ci just tries to lay out whatever is in the lockfile without checking whether it supports the platform, and only removes optional deps whose install scripts fail, it results in installing this dep.

npm v7 won't have this problem. (I'm working in the code that'll do this now.) I haven't checked into how involved it'd be to fix this issue for npm v6, but there's a good chance it'll just end up being "upgrade to v7 for the fix". In the meantime, I'd recommend using npm install instead of npm ci if this is affecting you.

@guidobouman
Copy link

guidobouman commented Jan 30, 2020

Yes, fsevents changed their tactic. But it is actually the other way around. They used to have precompiled binaries. Installation on Windows would give a 404, and skip. Now it tries to build, and then breaks the build. Because the build shouldn't even get started. Regardless: npm ci is designed for CI environments. How should we then use npm i instead? We want the strict package-lock checks in CI.

Also: Would npm@7 land in Node 14, in time before it's LTS? Am I correct to assume we're stuck with npm i or a version lock for a year when in the LTS track?

@pipobscure
Copy link

Sorry for changing tactics on you. However we lost access to the S3 originally used and it was becoming an increasingly serious security issue. That’s why we went back to build as needed. Especially given that v2.x based on NAPI doesn’t need to build at all for node v8.x+

@isaacs
Copy link
Contributor

isaacs commented Jan 30, 2020

Now it tries to build, and then breaks the build. Because the build shouldn't even get started.

Oh, that's weird. I'd expect npm ci to handle a build failure from an optional dep as a non-fatal warning type event, and just remove the offending dep.

Regardless: npm ci is designed for CI environments. Why should we then use npm i instead? We want the strict package-lock checks in CI.

Good question.

"Designed for CI environments" does not always mean "best for this particular CI environment, for this particular application".

In this case, there are two issues that npm ci is running into.

  • It isn't properly handling build failures for optional deps.
  • It isn't able to pre-filter deps based on os/cpu restrictions, because it only looks at the package-lock.json, where that information is not tracked. (That is, it's faster because it skips some usually-unnecessary file reads; including, incorrectly, the only file that contains the information required to properly avoid this failure.)

So, you should use npm i rather than npm ci because it will work, avoiding both bugs.

We want the strict package-lock checks in CI.

If you're talking about the fact that the package-lock provides the authoritative integrity and resolution checks, then good news: npm install does that as well.

If you're talking about the check that the package-lock and package.json are in sync with one another, you can add "scripts": { "prepare": "npx lock-verify" } to your package.json.

Would npm@7 land in Node 14, in time before it's LTS?

That is my expectation, yes.

But, even if it is LTS, the approach in the past has been to have an LTS version of npm in the LTS version of node, even if that means it changes within the LTS "frozen" time frame, so shipping npm v6 for 4 years would be a profoundly bad idea that I don't think Node will do. And as npm is really something of a separate project, rather than a "dependency" in a way that affects the run-time, this is usually fine.

Since npm v7 will have some breaking changes (though we're trying to minimize these as much as possible), it may be an issue if we don't get it there in time, or we may make some concessions to set default configs or do other things so that the npm v7 that ships with node 14 LTS is as close as possible to npm v6.

@isaacs
Copy link
Contributor

isaacs commented Jan 30, 2020

Oh, I just checked the node lts schedule and see that I was mistaken about hte timeframe. Node 14's initial release is in 3 months, but it doesn't go LTS until October.

So yeah, we should be well in the clear. I expect that the initial release of npm v7 will be available in time for node 14, and more than sufficiently stable by the time v14 hits LTS. (Famous last words and all, but the confidence has been steadily rising as we've gotten closer to doing the integration, and I don't have any reason to think that will change soon.)

JakeChampion added a commit to Financial-Times/origami-component-converter that referenced this issue Feb 4, 2020
JakeChampion added a commit to Financial-Times/origami-component-converter that referenced this issue Feb 4, 2020
AviVahl added a commit to wixplosives/test-drive that referenced this issue Feb 4, 2020
@tommck
Copy link

tommck commented Feb 11, 2020

I'm surprised this isn't being treated as more important of a bug that it is. This is making it so that we can't use "npm ci" on our build servers in windows. That's a big deal.

@JakeChampion
Copy link
Contributor Author

Not just windows, I can't use npm ci on any system

@tommck
Copy link

tommck commented Feb 14, 2020

If you're talking about the fact that the package-lock provides the authoritative integrity and resolution checks, then good news: npm install does that as well.

Wait.. it's been my experience that npm install will potentially result in updating the package-lock.json file if new packages are available that still adhere to the rules set in the package.json file.

Has this functionality changed @isaacs ?

@coyoteecd
Copy link

@tommck I think he addresses that with this part:

If you're talking about the check that the package-lock and package.json are in sync with one another, you can add "scripts": { "prepare": "npx lock-verify" } to your package.json.

Guess you can use it as a poor man's implementation of npm ci in your CI environment. You would run npm install; this runs the prepare script that checks if the package-lock is still in sync with your package.json.

If I'm not mistaken, this has two problems:

  • I don't see how this handles updates to indirect dependencies that satisfy the constraints of your direct dependencies. Those would get updated and your build would include newer code.
  • If lock-verify detects changes, you're getting a failed build out of the blue. You'd have to re-generate and commit the updated package-lock.json. I'd like my builds to be predictable, not to fail when some dependency gets released.

So yeah, it's a big issue - luckily in our case we do builds on linux and they still work for our combination of packages... Other people are less lucky.

@tommck
Copy link

tommck commented Feb 14, 2020

@coyoteecd So... assuming the lock file was fine (correct/verified), running "npm install" still might modify the package lock file with new dependencies?

@isaacs
Copy link
Contributor

isaacs commented Feb 14, 2020

running "npm install" still might modify the package lock file with new dependencies?

Incorrect. It will not do that.

Running npm install with no arguments will not add any dependencies that differ from what's in the lockfile.

What npm install will do, which npm ci will not do, is skip downloading dependencies that are in node_modules and already match what's in the lockfile.

@isaacs
Copy link
Contributor

isaacs commented Feb 14, 2020

Wait.. it's been my experience that npm install will potentially result in updating the package-lock.json file if new packages are available that still adhere to the rules set in the package.json file.

Has this functionality changed @isaacs ?

I'd love to see a case where that happens. Unless you're explicitly telling it not to respect the lockfile, or running npm update, or the lockfile is invalid (ie, deps do not have their dependencies met by the tree it defines), the lockfile has locked down npm install ever since it was introduced in npm v5.

@padinko
Copy link

padinko commented Feb 14, 2020

full-icu is package, which is sometimes changing lock file.. but i think it's their issue, not npm

my experiences: when you have node v12, and another developer have v10, full-icu downgrade icu data package for node v10..

when you have lock with everything and no node_modules directory and run npm i it will remove icu data from lock file, you need to run npm i second time to add it again..

we was using npm ci, because of this 2 issues

JakeChampion added a commit to Financial-Times/sass that referenced this issue Mar 11, 2020
…erent to what the package supports

This is a required for npm ci to work. npm/cli#558
JakeChampion added a commit to Financial-Times/sass that referenced this issue Mar 11, 2020
…erent to what the package supports

     This is a required for npm ci to work. npm/cli#558
@jedrzejowski
Copy link

jedrzejowski commented May 27, 2021

Adding fsevents as optional dependency solves issue in latest docker container node:14-alpine.
npm i fsevents@latest -f --save-optional
npm ci --no-optional
But it may have some side effects (I'm only using webpack).

@guidobouman
Copy link

Not installing manually, but updating your dependencies should fix the issue with fsevents as well.

@isaacs
Copy link
Contributor

isaacs commented May 27, 2021

npm install modifies package-lock.json. Even if e.g. package-lock.json has an older reactjs-popup version locked, npm install still modifies package-lock.json.

In the case mentioned, there is a missing dependency, so yes, it will update the tree and modify package-lock.json accordingly to record what it did. (It will do the same for an invalid dependency.)

@darcyclarke
Copy link
Contributor

npm v6 is no longer in active development; We will continue to push security releases to v6 at our team's discretion as-per our Support Policy.

If your bug is preproducible on v7, please re-file this issue using our new issue template.

If your issue was a feature request, please consider opening a new RRFC or RFC. If your issue was a question or other idea that was not CLI-specific, consider opening a discussion on our feedback repo

Closing: This is an automated message.

InManuBytes added a commit to outintech/nbjc-webapp that referenced this issue Apr 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug thing that needs fixing
Projects
None yet
Development

No branches or pull requests