-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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 install
creates unsync package-lock.json
#6787
Comments
Triaged and this is happening on both npm 9 and npm 10. |
I reproduce the bug with this package too: {
"name": "testcase",
"version": "1.0.0",
"dependencies": {
"addons-scanner-utils": "9.3.0",
"htmlhint": "1.1.4"
}
}
|
There is a weird thing that happens within the When you do the first {
"name": "testcase",
"version": "1.0.0",
"devDependencies": {
"addons-linter": "6.13.0",
"htmlhint": "1.1.4"
}
} to {
"name": "testcase",
"version": "1.0.0",
"devDependencies": {
"addons-linter": "6.13.0",
"htmlhint": "1.1.4"
},
"dependencies": {
"npm": "file:.."
}
} I am not sure if the second part is expected and should be added by @lukekarrys can you confirm that this issue happens on your end too? (Modification of the |
I tested with this two {
"name": "testcase",
"version": "1.0.0",
"devDependencies": {
"addons-linter": "6.13.0",
"htmlhint": "1.1.4"
}
} {
"name": "testcase",
"version": "1.0.0",
"dependencies": {
"addons-scanner-utils": "9.3.0",
"htmlhint": "1.1.4"
}
}
This is the first time I've had this problem. I didn't check if I had other dependencies that were also loaded twice with different versions in |
I noticed it is behaving so on my end since I was working on workspaces. In a normal set up it doesn't modify the
I asked because all the examples you have given have |
I only have one case because
My second example is just a version with one less intermediate step. |
TL;DR: this goes back to 7.0.9 which was the first release that caused the locked versions to be ignored on
Thus, the last glitch-free npm version is: 7.0.8 (As for the currently implemented installation error - it would serve much better as a test-case in integration test suite rather than user-facing functionality where it just causes confusion due to config-vs-lock being completely valid when it is thrown). Our steps that get us to the error:
EXPECTED: install still goes through and installs version 1.2.3 (as package-lock.json indicates) Some history: This all goes way back to v7.X which got a bug introduced to it of which there have been attempts on fixing it ever since with varying level of success (the change in 8.4.1 being one of them).
The actual issue had been around longer and that specific release (and the error we have all been seeing) just represents an attempt to rein in the whole situation. The hunt for the cause: Though I initially focused on tracing down where the error originates from, it became since clear that instead of hunting for a release that does not fail the Below then are the findings by just going through the versions one-by-one to identify where the break happened (given in 3 steps where versions represent versions found in: package.json > package-lock.json > node_modules
Thus the issue was somewhere in v7.0.8...v7.0.9. From there, when doing another round of blind pin-pointing leads to this commit: 0e58e6f. Starting from that specific commit, you'll start getting newer version installed than what is specified in the lock (which in turn in 8.4.1 was turned into a throw). Most notably it's something about the change in build-ideal-tree.js - // didn't find a parent for it, but we're filling in external
- // link targets, so go ahead and process it.
- if (this[_follow] && !link.target.parent && !link.target.fsParent) {
+ // didn't find a parent for it or it has not been seen yet
+ // so go ahead and process it.
+ const unseenLink = (link.target.parent || link.target.fsParent)
+ && !this[_depsSeen].has(link.target)
+ if (this[_follow]
+ && !link.target.parent
+ && !link.target.fsParent
+ || unseenLink) {
this.addTracker('idealTree', link.target.name, link.target.location)
this[_depsQueue].push(link.target)
} Now when trying out newer version, I bumped into another "checkpoint" where same thing started to happen again (though this time around instead of getting a silent install, one would actually see the infamous error that is stated at the original report of the issue). The second change was also done to build-ideal-tree.js, this time within the 8.6.0 release. .then(tree => {
+ // search the virtual tree for invalid edges, if any are found add their source to
+ // the depsQueue so that we'll fix it later
+ depth({
+ tree,
+ getChildren: (node) => [...node.edgesOut.values()].map(edge => edge.to),
+ filter: node => node,
+ visit: node => {
+ for (const edge of node.edgesOut.values()) {
+ if (!edge.valid) {
+ this[_depsQueue].push(node)
+ break // no need to continue the loop after the first hit
+ }
+ }
+ },
+ })
// null the virtual tree, because we're about to hack away at it
// if you want another one, load another copy. Sadly enough, neither of the changes have proper test coverage which does not really inject confidence to the reader in terms of knowing that the base functionality of the installer remains the same (second change just modifies existing tests which just shadows potential new bug). As such, would appreciate if someone else would take it from here. Conclusion:
Whatever these two changes claimed to fix - they introduced new issues. So those fixes should be re-introduced with proper test-coverage or re-reported as issues after reverting the broken fixes (I'd rather have those bugs back than the main intent of lock file being broken). |
Perhaps @isaacs @nlf @lukekarrys can comment on the changes mentioned above and what they're actually addressing. Could these pieces be re-imagined with provided unit/integration tests that would both guard the pre-7.0.9 functionality and also prove that the changes fix whatever they were supposed to fix. |
@siemhesda are you still involved in looking into this issue? |
Hi @allanpaiste I have been away for a while but I'm back on this |
…use install dependencies instead of clean build with ci
I am also seeing this issue occuring when installing Repro:
running |
It happens in my project too but strangely happens only in Github Action (linux-based) and not on my Windows machine |
Hey @wraithgar, we just hit this issue. We did some debugging and are pretty sure the detailed comment above is the cause. I am not sure the conclusion in that is right, but just wanted to flag this as an ongoing issue in latest 9.x (have yet to test in 10.x as the app it happened to is not using 10 yet). |
Maybe you can find some useful info from my duplicated issue: #7793 (also with repro repository, few GitHub Actions and another package-lock.json diff (with better Markdown formatting) and workarounds) |
Is there a good workaround for this beyond rerunning |
|
Is there an existing issue for this?
This issue exists in the latest npm version
Current Behavior
When I run
npm install
, the generatedpackage-lock.json
file isn't synchronized with thepackage.json
file. Thenpm ci
command fails. If I runnpm install
a second time: thepackage-lock.json
file is modified (and synchronized).Expected Behavior
npm install
creates apackage-lock.json
file synchronized.Steps To Reproduce
package.json
with this content:npm install
npm ci
cp package-lock.json package-save.json
npm install
diff package-lock.json package-save.json
The directory at the end with the files
package.json
,package-lock.json
,package-save.json
, and the directorynode_modules/
: testcase.zipEnvironment
Related issues / pull request
npm install
installs an invalid tree (as validated bynpm ci
) #5854npm install
twice to fixnpm ci
command #7793The text was updated successfully, but these errors were encountered: