tools: zlib update by checking latest commit by fasenderos · Pull Request #48054 · nodejs/node · GitHub

Zlib rarely creates tags or releases, so now we use the latest commit on the main branch to check if an update is needed.

Refs: nodejs/security-wg#973

I wrote the other script (and the comments therein) based on the Abseil Live at Head policy, which Google recommends to follow for GoogleTest. I don't think this is true for zlib.

What's the motivation behind tracking Chromium's copy of zlib instead of the official zlib repository on GitHub? And if there is a good reason to track Chromium's copy, wouldn't it make more sense to track Chromium releases (assuming they still have a somewhat reasonable release schedule) than their HEAD?

Does zlib really publish releases too infrequently? There's not a lot of activity in the repository. It's a project from 1995 and only 50 commits or so have been pushed to the official development branch over the last five years.

(However, those few bug fixes that are in the official zlib repository do not seem to get pulled into Chromium's copy.)

What's the motivation behind tracking Chromium's copy of zlib instead of the official zlib repository on GitHub?

Performance. There's a number of improvements that upstream zlib hasn't merged (see this open upstream issue and this example upstream PR). I believe there are also other zlib forks that have supposedly implemented different performance-related changes as well but have not been merged back.

@tniessen we are using the Chromium fork https://chromium.googlesource.com/chromium/src/third_party/zlib. They don't have a release schedule.

LGTM, it's a bit cumbersome but I cant think of a better way
@nodejs/security-wg

I think we also want to skip if we end up with echo 1 above, no?

We want to skip only if we don't end up with echo 1. git diff --exit-code exit with code 0 and no output when there's no diff. That being said, we don't gain much from the --exit-code flag, I was suggesting it so we can use it directly as the test, but currently it doesn't give us much expect an additional line with the number 1 🤷‍♂️

Yes, my point is that if it fails we should exit, but currently even if it fails we end up with a non empty string and continue.

Here's how I would write it:

# Revert zconf.h changes before checking diff
perl -i -pe 's|^//#include "chromeconf.h"|#include "chromeconf.h"|' "$DEPS_DIR/zlib/zconf.h"
git stash -- "$DEPS_DIR/zlib/zconf.h"

git fetch https://chromium.googlesource.com/chromium/src/third_party/zlib.git HEAD

(\
  git diff --exit-code --diff-filter=d stash@{0}:deps/zlib FETCH_HEAD -- ':!zconf.h' && \
  echo "Skipped because zlib is on the latest version." && \
  git stash drop && \
  exit 0 \
) || git stash drop

The double git stash drop is a bit unsettling, but I feel that's a more straightforward way of thinking about what this code is doing (but maybe that's just me).

If we don't use the --exit-code flag:

# Revert zconf.h changes before checking diff
perl -i -pe 's|^//#include "chromeconf.h"|#include "chromeconf.h"|' "$DEPS_DIR/zlib/zconf.h"
git stash -- "$DEPS_DIR/zlib/zconf.h"

git fetch https://chromium.googlesource.com/chromium/src/third_party/zlib.git HEAD

DIFF_TREE=$(git diff --diff-filter=d stash@{0}:deps/zlib FETCH_HEAD -- ':!zconf.h')
git stash drop

if [ -z "$DIFF_TREE" ]; then
  echo "Skipped because zlib is on the latest version."
  exit 0
fi

Without --exit-code seems cleaner and easier to understand to me.

DIFF_TREE=$(git diff --diff-filter=d stash@{0}:deps/zlib FETCH_HEAD -- ':!zconf.h')

With this check, we didn't get the diff of other possible changes to zconf.h

I think we still need two checks. One for checking the entire lib without zconf, and one for checking only the re-patched zconf

DIFF_TREE=$(
  git diff 'stash@{0}:deps/zlib' FETCH_HEAD -- zconf.h
  git diff --diff-filter=d HEAD:deps/zlib FETCH_HEAD -- ':!zconf.h'
)

Shouldn’t it be DIFF_TREE=$(git diff stash@{0}:deps/zlib FETCH_HEAD)? stash@{0} is supposed to be exactly the same as upstream if there are no updates, no?

Yes you are right

@fasenderos can you please remove the merge commit? Thank you.

LGTM once conflicts are resolved.

Commit Queue failed

- Loading data for nodejs/node/pull/48054
✔  Done loading data for nodejs/node/pull/48054
----------------------------------- PR info ------------------------------------
Title      tools: zlib update by checking latest commit (#48054)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     fasenderos:zlib-update -> nodejs:main
Labels     tools, commit-queue-squash
Commits    12
 - tools: zlib update by checking latest commit
 - tools: update zlib with a delay of 2 days
 - Update tools/dep_updaters/update-zlib.sh
 - Update tools/dep_updaters/update-zlib.sh
 - Update tools/dep_updaters/update-zlib.sh
 - Update tools/dep_updaters/update-zlib.sh
 - Update tools/dep_updaters/update-zlib.sh
 - Update tools/dep_updaters/update-zlib.sh
 - Update tools/dep_updaters/update-zlib.sh
 - tools: exclude deleted files from checking diff
 - tools: better zconf checking
 - tools: remove git diff exit-code and lint the code
Committers 1
 - Andrea Fassina 
PR-URL: https://github.com/nodejs/node/pull/48054
Refs: https://github.com/nodejs/security-wg/issues/973
Reviewed-By: Marco Ippolito 
Reviewed-By: Luigi Pinca 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/48054
Refs: https://github.com/nodejs/security-wg/issues/973
Reviewed-By: Marco Ippolito 
Reviewed-By: Luigi Pinca 
--------------------------------------------------------------------------------
   ⚠  Commits were pushed since the last review:
   ⚠  - tools: zlib update by checking latest commit
   ⚠  - tools: update zlib with a delay of 2 days
   ⚠  - Update tools/dep_updaters/update-zlib.sh
   ⚠  - Update tools/dep_updaters/update-zlib.sh
   ⚠  - Update tools/dep_updaters/update-zlib.sh
   ⚠  - Update tools/dep_updaters/update-zlib.sh
   ⚠  - Update tools/dep_updaters/update-zlib.sh
   ⚠  - Update tools/dep_updaters/update-zlib.sh
   ⚠  - Update tools/dep_updaters/update-zlib.sh
   ⚠  - tools: exclude deleted files from checking diff
   ⚠  - tools: better zconf checking
   ⚠  - tools: remove git diff exit-code and lint the code
   ℹ  This PR was created on Thu, 18 May 2023 07:09:19 GMT
   ✔  Approvals: 2
   ✔  - Marco Ippolito (@marco-ippolito): https://github.com/nodejs/node/pull/48054#pullrequestreview-1435422412
   ✔  - Luigi Pinca (@lpinca): https://github.com/nodejs/node/pull/48054#pullrequestreview-1436303379
   ✘  Last GitHub CI failed
   ℹ  Green GitHub CI is sufficient
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu

https://github.com/nodejs/node/actions/runs/5065863953

First commit message does not adhere to guidelines.

I wrote this comment specifically for GoogleTest. Are you sure that this exact wording, in its entirety, is true for the Chromium fork of zlib, too?

@tniessen I left your comment because your english is better than mine and is very well explained what happens in the next lines of code.

However I think that the text thus modified is correct; let me know if that's okay so I can update the comment. Thanks

This is a rather arbitrary restriction. This script is assumed to run on
Sunday, shortly after midnight UTC. This check thus prevents pulling in the
most recent commits if any changes were made on Friday or Saturday (UTC).
Because of Google's own "Live at Head" philosophy, new bugs that are likely to
affect Node.js tend to be fixed quickly, so we don't want to pull in a commit
that was just pushed, and instead rather wait for the next week's update. If
no commits have been pushed in the last two days, we assume that the most
recent commit is stable enough to be pulled in.

Zlib rarely gets new tags or releases, so now we use the latest commit on the upstream default branch to check if an update is available. Refs: nodejs/security-wg#973 PR-URL: nodejs#48054 Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

Landed in 860d7e3

Zlib rarely gets new tags or releases, so now we use the latest commit on the upstream default branch to check if an update is available. Refs: nodejs/security-wg#973 PR-URL: #48054 Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

Zlib rarely gets new tags or releases, so now we use the latest commit on the upstream default branch to check if an update is available. Refs: nodejs/security-wg#973 PR-URL: nodejs#48054 Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

nodejs-github-bot added the tools Issues and PRs related to the tools directory. label May 18, 2023

marco-ippolito requested a review from tniessen May 18, 2023 08:21

aduh95 reviewed May 18, 2023

View reviewed changes

Comment thread tools/dep_updaters/update-zlib.sh Outdated

tniessen reviewed May 18, 2023

View reviewed changes

aduh95 reviewed May 18, 2023

View reviewed changes

Comment thread tools/dep_updaters/update-zlib.sh Outdated

Comment thread tools/dep_updaters/update-zlib.sh Outdated

Comment thread tools/dep_updaters/update-zlib.sh Outdated

Comment thread tools/dep_updaters/update-zlib.sh Outdated

aduh95 reviewed May 19, 2023

View reviewed changes

Comment thread tools/dep_updaters/update-zlib.sh Outdated

Comment thread tools/dep_updaters/update-zlib.sh Outdated

aduh95 reviewed May 19, 2023

View reviewed changes

Comment thread tools/dep_updaters/update-zlib.sh Outdated

lpinca reviewed May 19, 2023

View reviewed changes

Comment thread tools/dep_updaters/update-zlib.sh Outdated

fasenderos mentioned this pull request May 20, 2023

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

Merged

23 tasks

lpinca mentioned this pull request May 20, 2023

tools: fix zconf.h path #48089

Merged

marco-ippolito approved these changes May 20, 2023

View reviewed changes

lpinca reviewed May 20, 2023

View reviewed changes

fasenderos requested review from aduh95 and lpinca May 22, 2023 07:48

fasenderos force-pushed the zlib-update branch from 4010cd2 to e3e7759 Compare May 22, 2023 09:58

lpinca added the commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. label May 22, 2023

lpinca approved these changes May 22, 2023

View reviewed changes

fasenderos force-pushed the zlib-update branch from 04b3960 to e3e7759 Compare May 22, 2023 10:47

fasenderos closed this May 22, 2023

fasenderos force-pushed the zlib-update branch from e3e7759 to f858dc5 Compare May 22, 2023 10:56

fasenderos reopened this May 22, 2023

marco-ippolito added the commit-queue Add this label to land a pull request using GitHub Actions. label May 24, 2023

nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels May 24, 2023

aduh95 approved these changes May 24, 2023

View reviewed changes

aduh95 reviewed May 24, 2023

View reviewed changes

Comment thread tools/dep_updaters/update-zlib.sh Outdated

tniessen reviewed May 24, 2023

View reviewed changes

aduh95 force-pushed the zlib-update branch from c7650e6 to 860d7e3 Compare May 24, 2023 22:41

aduh95 merged commit 860d7e3 into nodejs:main May 24, 2023

fasenderos deleted the zlib-update branch May 25, 2023 07:05

targos mentioned this pull request Jun 4, 2023

v20.3.0 release proposal #48332

Merged

danielleadams mentioned this pull request Jul 10, 2023

v18.17.0 release proposal #48694

Merged

tools: zlib update by checking latest commit by fasenderos · Pull Request #48054 · nodejs/node · GitHub

Uh oh!

Uh oh!

tools: zlib update by checking latest commit - #48054

tools: zlib update by checking latest commit#48054aduh95 merged 1 commit intonodejs:mainnodejs/node:mainfrom fasenderos:zlib-updatefasenderos/node:zlib-updateCopy head branch name to clipboard

Conversation

fasenderos commented May 18, 2023

Uh oh!

Uh oh!

tniessen left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mscdex commented May 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lpinca commented May 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

marco-ippolito left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aduh95 May 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fasenderos May 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lpinca commented May 22, 2023

Uh oh!

lpinca left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented May 24, 2023

Uh oh!

Uh oh!

tniessen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aduh95 commented May 24, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

tools: zlib update by checking latest commit#48054
aduh95 merged 1 commit into
nodejs:mainfrom
fasenderos:zlib-update

tniessen left a comment •
edited
Loading

mscdex commented May 18, 2023 •
edited
Loading

lpinca commented May 18, 2023 •
edited
Loading

marco-ippolito left a comment •
edited
Loading

aduh95 May 20, 2023 •
edited
Loading

fasenderos May 21, 2023 •
edited
Loading