-
Notifications
You must be signed in to change notification settings - Fork 30k
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
tools: automate zlib update #47417
tools: automate zlib update #47417
Conversation
Review requested:
|
6f313cd
to
c9f1101
Compare
|
||
CURRENT_VERSION=$(grep "#define ZLIB_VERSION" "$DEPS_DIR/zlib/zlib.h" | sed -n "s/^.*VERSION \"\(.*\)\"/\1/p") | ||
|
||
NEW_VERSION_ZLIB_H=$(curl -s "https://chromium.googlesource.com/chromium/src/+/refs/heads/main/third_party/zlib/zlib.h?format=TEXT" | base64 --decode) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This works but it might remain the same for a very long time. It is not very useful. We refer to the tip of the remote main branch to check if an update is needed. However, afaik, we store the current commit SHA only in the commit message.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I havent found any other solution to check what's the latest version, I think this might be a reasonable compromise and its not very expensive
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's fine but it probably won't find a new version for years. See https://github.com/madler/zlib/tags.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lpinca are you saying that we typically update to something that is not a new release? If so then an option in the script/action to say generate PR based on commit X instead of a new version might make sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are you saying that we typically update to something that is not a new release?
Yes, exactly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
c9f1101
to
210c3bd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
b02200c
to
ef67324
Compare
Commit Queue failed- Loading data for nodejs/node/pull/47417 ✔ Done loading data for nodejs/node/pull/47417 ----------------------------------- PR info ------------------------------------ Title tools: automate zlib update (#47417) Author Marco Ippolito (@marco-ippolito) Branch marco-ippolito:feat/automate-zlib-update -> nodejs:main Labels meta, tools, author ready Commits 1 - tools: automate zlib update Committers 1 - Marco Ippolito PR-URL: https://github.com/nodejs/node/pull/47417 Refs: https://github.com/nodejs/security-wg/issues/828 Reviewed-By: Paolo Insogna Reviewed-By: Luigi Pinca ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/47417 Refs: https://github.com/nodejs/security-wg/issues/828 Reviewed-By: Paolo Insogna Reviewed-By: Luigi Pinca -------------------------------------------------------------------------------- ⚠ Commits were pushed since the last review: ⚠ - tools: automate zlib update ℹ This PR was created on Wed, 05 Apr 2023 09:59:59 GMT ✔ Approvals: 2 ✔ - Paolo Insogna (@ShogunPanda): https://github.com/nodejs/node/pull/47417#pullrequestreview-1376059006 ✔ - Luigi Pinca (@lpinca): https://github.com/nodejs/node/pull/47417#pullrequestreview-1376413213 ⚠ This PR has conflicts that must be resolved ✔ Last GitHub CI successful ℹ Green GitHub CI is sufficient -------------------------------------------------------------------------------- ✔ Aborted `git node land` session in /home/runner/work/node/node/.ncuhttps://github.com/nodejs/node/actions/runs/4678748046 |
ef67324
to
7194af1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Rebase to resolve the conflicts
7194af1
to
4c1a1a5
Compare
Commit Queue failed- Loading data for nodejs/node/pull/47417 ✔ Done loading data for nodejs/node/pull/47417 ----------------------------------- PR info ------------------------------------ Title tools: automate zlib update (#47417) Author Marco Ippolito (@marco-ippolito) Branch marco-ippolito:feat/automate-zlib-update -> nodejs:main Labels meta, tools, author ready Commits 1 - tools: automate zlib update Committers 1 - Marco Ippolito PR-URL: https://github.com/nodejs/node/pull/47417 Refs: https://github.com/nodejs/security-wg/issues/828 Reviewed-By: Paolo Insogna Reviewed-By: Luigi Pinca Reviewed-By: Rafael Gonzaga ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/47417 Refs: https://github.com/nodejs/security-wg/issues/828 Reviewed-By: Paolo Insogna Reviewed-By: Luigi Pinca Reviewed-By: Rafael Gonzaga -------------------------------------------------------------------------------- ⚠ Commits were pushed since the last review: ⚠ - tools: automate zlib update ℹ This PR was created on Wed, 05 Apr 2023 09:59:59 GMT ✔ Approvals: 3 ✔ - Paolo Insogna (@ShogunPanda): https://github.com/nodejs/node/pull/47417#pullrequestreview-1376059006 ✔ - Luigi Pinca (@lpinca): https://github.com/nodejs/node/pull/47417#pullrequestreview-1376413213 ✔ - Rafael Gonzaga (@RafaelGSS) (TSC): https://github.com/nodejs/node/pull/47417#pullrequestreview-1386302187 ✔ Last GitHub CI successful ℹ Green GitHub CI is sufficient -------------------------------------------------------------------------------- ✔ Aborted `git node land` session in /home/runner/work/node/node/.ncuhttps://github.com/nodejs/node/actions/runs/4723740689 |
Commit Queue failed- Loading data for nodejs/node/pull/47417 ✔ Done loading data for nodejs/node/pull/47417 ----------------------------------- PR info ------------------------------------ Title tools: automate zlib update (#47417) Author Marco Ippolito (@marco-ippolito) Branch marco-ippolito:feat/automate-zlib-update -> nodejs:main Labels meta, tools, author ready Commits 1 - tools: automate zlib update Committers 1 - Marco Ippolito PR-URL: https://github.com/nodejs/node/pull/47417 Refs: https://github.com/nodejs/security-wg/issues/828 Reviewed-By: Paolo Insogna Reviewed-By: Luigi Pinca Reviewed-By: Rafael Gonzaga ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/47417 Refs: https://github.com/nodejs/security-wg/issues/828 Reviewed-By: Paolo Insogna Reviewed-By: Luigi Pinca Reviewed-By: Rafael Gonzaga -------------------------------------------------------------------------------- ⚠ Commits were pushed since the last review: ⚠ - tools: automate zlib update ℹ This PR was created on Wed, 05 Apr 2023 09:59:59 GMT ✔ Approvals: 3 ✔ - Paolo Insogna (@ShogunPanda): https://github.com/nodejs/node/pull/47417#pullrequestreview-1376059006 ✔ - Luigi Pinca (@lpinca): https://github.com/nodejs/node/pull/47417#pullrequestreview-1376413213 ✔ - Rafael Gonzaga (@RafaelGSS) (TSC): https://github.com/nodejs/node/pull/47417#pullrequestreview-1386302187 ✔ Last GitHub CI successful ℹ Green GitHub CI is sufficient -------------------------------------------------------------------------------- ✔ Aborted `git node land` session in /home/runner/work/node/node/.ncuhttps://github.com/nodejs/node/actions/runs/4730739601 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with a question.
Landed in 8f879de |
PR-URL: #47417 Refs: nodejs/security-wg#828 Reviewed-By: Paolo Insogna <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
PR-URL: #47417 Refs: nodejs/security-wg#828 Reviewed-By: Paolo Insogna <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
PR-URL: nodejs#47417 Refs: nodejs/security-wg#828 Reviewed-By: Paolo Insogna <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
Refs: nodejs/security-wg#828
@nodejs/security-wg