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

tools: zlib update by checking latest commit #48054

Merged
merged 1 commit into from
May 24, 2023
Merged

tools: zlib update by checking latest commit #48054

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 29 additions & 6 deletions tools/dep_updaters/update-zlib.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,23 +1,46 @@
#!/bin/sh
set -e
# Shell script to update zlib in the source tree to a specific version
# Shell script to update zlib in the source tree to the most recent version.
# Zlib rarely creates tags or releases, so we use the latest commit on the main branch.
# See: https://github.com/nodejs/node/pull/47417

BASE_DIR=$(cd "$(dirname "$0")/../.." && pwd)
DEPS_DIR="$BASE_DIR/deps"

CURRENT_VERSION=$(grep "#define ZLIB_VERSION" "$DEPS_DIR/zlib/zlib.h" | sed -n "s/^.*VERSION \"\(.*\)\"/\1/p")
echo "Comparing latest upstream with current revision"

NEW_VERSION_ZLIB_H=$(curl -s "https://chromium.googlesource.com/chromium/src/+/refs/heads/main/third_party/zlib/zlib.h?format=TEXT" | base64 --decode)
git fetch https://chromium.googlesource.com/chromium/src/third_party/zlib.git HEAD

NEW_VERSION=$(printf '%s' "$NEW_VERSION_ZLIB_H" | grep "#define ZLIB_VERSION" | sed -n "s/^.*VERSION \"\(.*\)\"/\1/p")
# 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"

echo "Comparing $NEW_VERSION with $CURRENT_VERSION"
DIFF_TREE=$(git diff --diff-filter=d 'stash@{0}:deps/zlib' FETCH_HEAD)

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 •
edited
Loading

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 •
edited
Loading

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

echo "Skipped because zlib is on the latest version."
exit 0
fi

# 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).
# 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.
LAST_CHANGE_DATE=$(git log -1 --format=%ct FETCH_HEAD)
TWO_DAYS_AGO=$(date -d 'now - 2 days' '+%s')

if [ "$LAST_CHANGE_DATE" -gt "$TWO_DAYS_AGO" ]; then
echo "Skipped because the latest version is too recent."
exit 0
fi

fasenderos marked this conversation as resolved.
Show resolved Hide resolved
NEW_VERSION=$(git rev-parse --short=7 FETCH_HEAD)

echo "Making temporary workspace..."

WORKSPACE=$(mktemp -d 2> /dev/null || mktemp -d -t 'tmp')
Expand Down