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

tools: add version to maintaining-dependencies.md dependencies list and automate the update #48081

Merged
merged 2 commits into from
Jun 6, 2023
Merged

tools: add version to maintaining-dependencies.md dependencies list and automate the update #48081

merged 2 commits into from
Jun 6, 2023

Conversation

fasenderos
Copy link
Contributor

@fasenderos fasenderos commented May 19, 2023 •
edited
Loading

Add version to doc/contributing/maintaining/maintaining-dependencies.md dependencies list and automate the update
Refs: nodejs/security-wg#973

Most of the duplicated code in all the dependencies updaters has been moved in the utils.sh file

This is the list of all the dependencies:

  • acorn
  • ada
  • base64
  • brotli
  • c-ares
  • cjs-module-lexer
  • corepack
  • googletest
  • histogram
  • icu-small
  • llhttp
  • minimatch
  • nghttp2
  • ngtcp2
  • npm
  • openssl
  • postject
  • simdutf
  • undici
  • uv
  • uvwasi
  • V8
  • zlib

@nodejs-github-bot nodejs-github-bot added the tools Issues and PRs related to the tools directory. label May 19, 2023
@fasenderos
Copy link
Contributor Author

fasenderos commented May 20, 2023 •
edited
Loading

  1. For corepack and histogram I was not able to locate the update script.
  2. The postject, undici and V8 update scripts seems to differ from the others because they don't commit any changes, so probably the dependency update is done in another way. Any hint?
  3. The zlib update script is currently under review of another PR that will change the update routine

@fasenderos fasenderos marked this pull request as ready for review May 20, 2023 13:13
tniessen
tniessen reviewed May 20, 2023
View reviewed changes
Copy link
Member

@tniessen tniessen left a comment

Choose a reason for hiding this comment

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

I know that the existing update scripts are already not pretty, but I still don't think we should add duplicate code to all of them. Could you please DRY the logic?

@fasenderos
Copy link
Contributor Author

@tniessen I'm agree with you. I started recently to collaborate on node and I wasn't sure if I could (or not) create new files. Anyway if this refactor is ok, than I can continue with the other dependencies.

@fasenderos fasenderos marked this pull request as draft May 21, 2023 12:57
@fasenderos fasenderos marked this pull request as ready for review May 21, 2023 23:47
@fasenderos fasenderos requested review from tniessen and RaisinTen May 22, 2023 07:44
@marco-ippolito
Copy link
Member

marco-ippolito commented May 22, 2023 •
edited
Loading

why do you say they dont commit any change? The commit is done in the action that triggers all these scripts:

node/.github/workflows/tools.yml

Line 295 in 959142a

commit-message: ${{ env.COMMIT_MSG }}

the version of undici is currently 5.22.1 https://github.com/nodejs/node/blob/main/deps/undici/src/package.json
you can update it with NEW_VERSION https://github.com/nodejs/node/blob/main/tools/dep_updaters/update-undici.sh
For zlib we might want to use latest version + commit hash.
For v8 we have automated only patch update so if it's a major update it has to be done manually.
postject should be like every other dependency

@fasenderos fasenderos force-pushed the deps-version branch 2 times, most recently from 7bf0db3 to ce7111a Compare May 23, 2023 10:01
@fasenderos
Copy link
Contributor Author

@marco-ippolito I have added version to undici, postject and V8 and refactored all these scripts by moving most of the duplicated code in the utils.sh file.

@marco-ippolito
Copy link
Member

marco-ippolito commented May 25, 2023 •
edited
Loading

pr for histogram dep_updater : #48171 landed

@fasenderos fasenderos force-pushed the deps-version branch 3 times, most recently from af728dd to 86e0c9d Compare May 26, 2023 08:24
@fasenderos fasenderos mentioned this pull request May 27, 2023
Closed
9 tasks
fasenderos added 2 commits May 30, 2023 23:36
@fasenderos
tools: add version update to external dependencies
d5bb5c7 
Refs: nodejs/security-wg#973
Most of the duplicated code in all the dependencies updaters
has been moved in the `utils.sh` file
@fasenderos
tools: fix lint V8
b6c9e2f
@fasenderos
Copy link
Contributor Author

@marco-ippolito added version for histogram.

marco-ippolito
marco-ippolito approved these changes May 31, 2023
View reviewed changes
Copy link
Member

@marco-ippolito marco-ippolito left a comment

Choose a reason for hiding this comment

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

LGTM

@fasenderos fasenderos mentioned this pull request May 31, 2023
Merged
@marco-ippolito marco-ippolito added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Jun 6, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 6, 2023
@nodejs-github-bot nodejs-github-bot merged commit c541153 into nodejs:main Jun 6, 2023
@nodejs-github-bot
Copy link
Collaborator

Landed in c541153

@marco-ippolito marco-ippolito mentioned this pull request Jun 6, 2023
Merged
@fasenderos fasenderos deleted the deps-version branch June 6, 2023 09:19
@richardlau richardlau mentioned this pull request Jun 23, 2023
Closed
3 tasks
RafaelGSS pushed a commit that referenced this pull request Jul 3, 2023
@fasenderos @RafaelGSS
tools: add version update to external dependencies
10385c8 
Refs: nodejs/security-wg#973
Most of the duplicated code in all the dependencies updaters
has been moved in the `utils.sh` file

PR-URL: #48081
Refs: nodejs/security-wg#973
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
@RafaelGSS RafaelGSS mentioned this pull request Jul 3, 2023
Merged
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
@fasenderos @Ceres6
tools: add version update to external dependencies
18cae43 
Refs: nodejs/security-wg#973
Most of the duplicated code in all the dependencies updaters
has been moved in the `utils.sh` file

PR-URL: nodejs#48081
Refs: nodejs/security-wg#973
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
@fasenderos @Ceres6
tools: add version update to external dependencies
dc55b7a 
Refs: nodejs/security-wg#973
Most of the duplicated code in all the dependencies updaters
has been moved in the `utils.sh` file

PR-URL: nodejs#48081
Refs: nodejs/security-wg#973
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
@ruyadorno ruyadorno added the backport-requested-v18.x PRs awaiting manual backport to the v18.x-staging branch. label Aug 29, 2023
@richardlau richardlau mentioned this pull request Oct 11, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@targos targos targos left review comments

@marco-ippolito marco-ippolito marco-ippolito approved these changes

@tniessen tniessen Awaiting requested review from tniessen

@RaisinTen RaisinTen Awaiting requested review from RaisinTen

Assignees
No one assigned
Labels
backport-requested-v18.x PRs awaiting manual backport to the v18.x-staging branch. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. tools Issues and PRs related to the tools directory.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@fasenderos @marco-ippolito @nodejs-github-bot @targos @tniessen @RaisinTen @ruyadorno