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.

Sign up for GitHub

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

Jump to bottom

[v18.x backport] deps: upgrade npm to 9.2.0 #46230

Closed
wants to merge 2 commits into from
Closed

[v18.x backport] deps: upgrade npm to 9.2.0 #46230

wants to merge 2 commits into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Jan 17, 2023 •
edited
Loading

Backport of #45780 and #45693.

Hopefully I'm not jumping the gun here. @nodejs/releasers

This is just a cherry-pick of the relevant commits. No merge conflicts needed to be resolved or anything like that.

@nodejs/lts @nodejs/releasers @nodejs/npm

@npm-cli-bot @Trott
deps: upgrade npm to 9.1.3
0a6c54a 
PR-URL: nodejs#45693
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruy Adorno <ruyadorno@google.com>
@npm-cli-bot @Trott
deps: upgrade npm to 9.2.0
5648ed1 
PR-URL: nodejs#45780
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
@nodejs-github-bot nodejs-github-bot added dont-land-on-v14.x fast-track PRs that do not need to wait for 48 hours to land. needs-ci PRs that need a full CI run. npm Issues and PRs related to the npm client dependency or the npm registry. v18.x Issues that can be reproduced on v18.x or PRs targeting the v18.x-staging branch. labels Jan 17, 2023
@github-actions

This comment was marked as resolved.

Sign in to view
@Trott
Copy link
Member Author

Trott commented Jan 17, 2023

Removing "fast-track" label because I don't think we want to do that on an LTS backport.

@Trott Trott removed the fast-track PRs that do not need to wait for 48 hours to land. label Jan 17, 2023
@MylesBorins
Copy link
Contributor

@Trott could we please also get 66b1356 included 🙏🏼

@Trott
Copy link
Member Author

Trott commented Jan 17, 2023

@Trott could we please also get 66b1356 included 🙏🏼

Done.

@Trott Trott changed the title [v18.x backport] deps: upgrade npm to 9.2.0 [v18.x backport] deps: upgrade npm to 9.3.0 Jan 17, 2023
@ruyadorno
Copy link
Member

thank you @Trott! 🙏

@nlf
Copy link
Contributor

nlf commented Jan 17, 2023

#46242 fixes a critical bug with npm ci that we should really land here too

@saquibkhan
Copy link
Contributor

with above change you can also change the title of this PR - [v18.x backport] deps: upgrade npm to 9.3.1

@MylesBorins MylesBorins changed the title [v18.x backport] deps: upgrade npm to 9.3.0 [v18.x backport] deps: upgrade npm to 9.2.0 Jan 18, 2023
@MylesBorins
Copy link
Contributor

MylesBorins commented Jan 18, 2023 •
edited
Loading

I'm rolling back this PR to npm 9.2.0 as that is the latest version of npm 9 currently shipping in Node.js 19. npm 9.3.1 is currently on main awaiting a release. I'd like to propose that we work on testing with what has shipped in 19.x, agree to land it, get the backport landed, then independently decide if we want to upgrade to 9.3.1 prior to the LTS release.

CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/3076/
CITGM v18.13.0: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/3072/ from #46025

@nlf
Copy link
Contributor

nlf commented Jan 18, 2023

fwiw, our own citgm suite is not as diverse but i did run it as an exercise: https://github.com/npm/cli/actions/runs/3943506886

notably the tests for mkdirp are broken in their own repo, so that one seems safe to ignore

@saquibkhan
Copy link
Contributor

I'm rolling back this PR to npm 9.2.0 as that is the latest version of npm 9 currently shipping in Node.js 19.

@nlf will this have #46242 , do you see any concerns here?

@MylesBorins MylesBorins added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 18, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 18, 2023
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/49045/

@nlf
Copy link
Contributor

nlf commented Jan 18, 2023

I'm rolling back this PR to npm 9.2.0 as that is the latest version of npm 9 currently shipping in Node.js 19.

@nlf will this have #46242 , do you see any concerns here?

the bug was introduced in 9.3.0 and resolved in 9.3.1, backporting 9.2.0 should be fine. the only version we explicitly do not want to backport is 9.3.0

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/49053/

@MylesBorins
Copy link
Contributor

TL;DR There are 2 new CITGM failures in this PR as opposed to the CITGM run for the release of v18.13.0 but neither are indications of an problems with npm 9, this PR should be good to go 👍🏼

Failure 1: node-gyp

We are unable to reproduce the node-gyp failure with a production build of Node.js and the current working theory is that the failure is due to the process.version being a prerelease... we can see the exact same failure in other CITGM runs unrelated to this change. just to be sure we ran CITGM no build and everything passed with 18.13.0 + npm 9.2.0

https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker-nobuild/1489/

Failure 2: npm

The npm 9 failures appear to be related to the npm test suite and appear to already be broken on Node.js 19 CITGM as can be seen in the runs for v19.4.0. We need to investigate how to fix this but after reviewing the logs with @nlf we've confirmed that this is a mismatch in snapshots due to system configuration in CITGM (changing tmp directories via env vars). We got a copy of the env vars and @nlf is going to take a look and get the test suite in npm 9 fixed so that it doesn't break in the CitGM environment.

@MylesBorins MylesBorins mentioned this pull request Jan 18, 2023
Closed
@Trott Trott changed the title [v18.x backport] deps: upgrade npm to 9.2.0 [v18.x backport] deps: upgrade npm to 9.3.1 Jan 18, 2023
@Trott
Copy link
Member Author

Trott commented Jan 18, 2023

Uh, oops, I updated this to 9.3.1. Feel free to push out the last two commits if you want to roll it back to 9.2.0 and apologies in advance. But I think 9.3.1 should be good?

@MylesBorins
Copy link
Contributor

MylesBorins commented Jan 18, 2023 •
edited
Loading

@Trott since 9.3.1 hasn't made it to current yet I wanted to land 9.2.0 and have a separate conversation about updating to 9.3.1 after it's been out in curent

@Trott Trott changed the title [v18.x backport] deps: upgrade npm to 9.3.1 [v18.x backport] deps: upgrade npm to 9.2.0 Jan 19, 2023
BethGriggs
BethGriggs approved these changes Jan 20, 2023
View reviewed changes
Copy link
Member

@BethGriggs BethGriggs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 to landing npm@9 in 18.

I tried skimming CITGM - it's reassuring there are no net new widespread or consistent failures. Although, we do have a high number of baseline failures (~140) in CITGM at the moment. But, I'm hopeful the baking time of npm@9 in Node.js 19 would have surfaced anything problematic.

juanarbol pushed a commit that referenced this pull request Jan 22, 2023
@npm-cli-bot @juanarbol
deps: upgrade npm to 9.1.3
4844935 
PR-URL: #45693
Backport-PR-URL: #46230
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruy Adorno <ruyadorno@google.com>
juanarbol pushed a commit that referenced this pull request Jan 22, 2023
@npm-cli-bot @juanarbol
deps: upgrade npm to 9.2.0
7599b91 
PR-URL: #45780
Backport-PR-URL: #46230
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
@azu azu mentioned this pull request Jan 24, 2023
Closed
@MylesBorins
Copy link
Contributor

landed in 7599b91

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RafaelGSS RafaelGSS RafaelGSS approved these changes

@juanarbol juanarbol juanarbol approved these changes

@BethGriggs BethGriggs BethGriggs approved these changes

@danielleadams danielleadams danielleadams approved these changes

@richardlau richardlau richardlau approved these changes

@targos targos targos approved these changes

@ruyadorno ruyadorno ruyadorno approved these changes

@bengl bengl bengl approved these changes

Assignees
No one assigned
Labels
needs-ci PRs that need a full CI run. npm Issues and PRs related to the npm client dependency or the npm registry. v18.x Issues that can be reproduced on v18.x or PRs targeting the v18.x-staging branch.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.