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.

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: zlib update by checking latest commit #48054

Merged
merged 1 commit into from
May 24, 2023

Conversation

fasenderos
Copy link
Contributor

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

@nodejs-github-bot nodejs-github-bot added the tools Issues and PRs related to the tools directory. label May 18, 2023
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 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.)

@mscdex
Copy link
Contributor

mscdex commented May 18, 2023

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.

@lpinca
Copy link
Member

lpinca commented May 18, 2023

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

tools/dep_updaters/update-zlib.sh Outdated Show resolved Hide resolved
tools/dep_updaters/update-zlib.sh Outdated Show resolved Hide resolved
tools/dep_updaters/update-zlib.sh Outdated Show resolved Hide resolved
tools/dep_updaters/update-zlib.sh Outdated Show resolved Hide resolved
tools/dep_updaters/update-zlib.sh Outdated Show resolved Hide resolved
tools/dep_updaters/update-zlib.sh Show resolved Hide resolved
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, it's a bit cumbersome but I cant think of a better way
@nodejs/security-wg

if [ "$NEW_VERSION" = "$CURRENT_VERSION" ]; then
git stash drop

if [ -z "$DIFF_TREE" ]; then
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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 🤷‍♂️

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@aduh95 aduh95 May 20, 2023

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@fasenderos fasenderos May 21, 2023

Choose a reason for hiding this comment

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

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'
)

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes you are right

@fasenderos fasenderos requested review from lpinca and aduh95 May 22, 2023 07:48
@lpinca
Copy link
Member

lpinca commented May 22, 2023

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

@lpinca 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
Copy link
Member

@lpinca lpinca left a comment

Choose a reason for hiding this comment

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

LGTM once conflicts are resolved.

@fasenderos fasenderos closed this May 22, 2023
@fasenderos fasenderos reopened this May 22, 2023
@marco-ippolito marco-ippolito added the commit-queue Add this label to land a pull request using GitHub Actions. label May 24, 2023
@nodejs-github-bot 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
@nodejs-github-bot
Copy link
Collaborator

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

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.

First commit message does not adhere to guidelines.

Comment on lines 27 to 34
# 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.
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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>
@aduh95
Copy link
Contributor

aduh95 commented May 24, 2023

Landed in 860d7e3

@aduh95 aduh95 merged commit 860d7e3 into nodejs:main May 24, 2023
@fasenderos fasenderos deleted the zlib-update branch May 25, 2023 07:05
targos pushed a commit that referenced this pull request May 30, 2023
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>
@targos targos mentioned this pull request Jun 4, 2023
danielleadams pushed a commit that referenced this pull request Jul 6, 2023
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>
MoLow pushed a commit to MoLow/node that referenced this pull request Jul 6, 2023
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>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
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>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
commit-queue-failed An error occurred while landing this 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. tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants