From 322e8c0782d11445448987f361aceec08e5d8785 Mon Sep 17 00:00:00 2001 From: Andrew Dupont Date: Mon, 7 Sep 2026 13:43:07 -0700 Subject: [PATCH 1/4] =?UTF-8?q?Fix=20some=20bugs=20that=20cause=20minor=20?= =?UTF-8?q?annoyances=E2=80=A6?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit …during the build process. Claude ferreted these out; the precipitating annoyance was the fact that `yarn install` in the Pulsar repo often somehow forces me to re-run the download-`libiconv` step. Claude doesn't know why the existing version gets removed during that process, but says that `yarn build` doesn't catch the omission because the are-we-still-fresh metadata doesn't know to check for `libiconv.2.dylib`. Also, Claude spotted some bugs in the `fetch-libiconv-61.sh` script. --- binding.gyp | 65 +++++++++++++++++++------------------ script/fetch-libiconv-61.sh | 9 ++--- 2 files changed, 39 insertions(+), 35 deletions(-) diff --git a/binding.gyp b/binding.gyp index 23943893..54d0b07b 100644 --- a/binding.gyp +++ b/binding.gyp @@ -28,27 +28,28 @@ "conditions": [ ['OS=="mac"', { "postbuilds": [ + { + 'postbuild_name': 'Copy vendored libiconv next to the binding', + # `-L` because `ext/lib/libiconv.2.dylib` is a + # symlink to the versioned dylib. We want the real + # file here — a link would still point outside of + # `build/`, which is what we're getting away from. + 'action': [ + 'cp', + '-L', + '<(module_root_dir)/ext/lib/libiconv.2.dylib', + '<(PRODUCT_DIR)/libiconv.2.dylib' + ] + }, { 'postbuild_name': 'Adjust vendored libiconv install name', 'action': [ 'install_name_tool', "-change", "libiconv.2.dylib", - "@loader_path/../../ext/lib/libiconv.2.dylib", + "@loader_path/libiconv.2.dylib", "<(PRODUCT_DIR)/superstring.node" ] - - # NOTE: This version of the post-build action - # should be used if we find it necessary to avoid - # changing the `dylib`’s install name in an earlier - # step. - # - # 'action': [ - # 'bash', - # '<(module_root_dir)/script/adjust-install-name.sh', - # '<(PRODUCT_DIR)' - # ] - } ] }] @@ -120,7 +121,7 @@ "action_name": "Run script", "message": "Building GNU libiconv...", "inputs": [], - "outputs": ["ext"], + "outputs": ["<(module_root_dir)/ext/lib/libiconv.2.dylib"], "action": [ "bash", "script/fetch-libiconv-61.sh" @@ -194,28 +195,30 @@ 'MACOSX_DEPLOYMENT_TARGET': '10.12', }, "postbuilds": [ + { + 'postbuild_name': 'Copy vendored libiconv next to the binding', + # `-L` because `ext/lib/libiconv.2.dylib` is a + # symlink to the versioned dylib. We want the real + # file here — a link would still point outside of + # `build/`, which is what we're getting away from. + 'action': [ + 'cp', + '-L', + '<(module_root_dir)/ext/lib/libiconv.2.dylib', + '<(PRODUCT_DIR)/libiconv.2.dylib' + ] + }, { 'postbuild_name': 'Adjust vendored libiconv install name', 'action': [ - 'install_name_tool', - "-change", - "libiconv.2.dylib", - "@executable_path/../../ext/lib/libiconv.2.dylib", - "<(PRODUCT_DIR)/tests" + 'install_name_tool', + "-change", + "libiconv.2.dylib", + "@executable_path/libiconv.2.dylib", + "<(PRODUCT_DIR)/tests" ] - - # NOTE: This version of the post-build action - # should be used if we find it necessary to avoid - # changing the `dylib`’s install name in an earlier - # step. - # - # 'action': [ - # 'bash', - # '<(module_root_dir)/script/adjust-install-name.sh', - # '<(PRODUCT_DIR)' - # ] } - ] + ] }] ] }] diff --git a/script/fetch-libiconv-61.sh b/script/fetch-libiconv-61.sh index 74c98904..f43cd4db 100644 --- a/script/fetch-libiconv-61.sh +++ b/script/fetch-libiconv-61.sh @@ -1,4 +1,5 @@ #!/bin/bash +set -euo pipefail # When compiling `superstring` on macOS, we used to be able to rely on the # builtin version of `libiconv`. But newer versions of macOS include FreeBSD @@ -15,7 +16,7 @@ echoerr() { echo "$@\n" >&2; } create-if-missing() { - if [ -z "$1" ]; then + if [ -f "$1" ]; then echoerr "Error: $1 is a file." usage exit 1 @@ -50,7 +51,7 @@ dylib_path="$EXT/lib/libiconv.2.dylib" # If this path already exists, we'll assume libiconv has already been fetched # and compiled. Otherwise we'll do it now. -if [ ! -L "$dylib_path" ]; then +if [ ! -e "$dylib_path" ]; then echo "Path $dylib_path is missing; fetching and installing libiconv." cd $SCRATCH # TODO: Instead of downloading this each time, we can check this into source @@ -64,7 +65,7 @@ if [ ! -L "$dylib_path" ]; then make make install - if [ ! -L "$dylib_path" ]; then + if [ ! -e "$dylib_path" ]; then echoerr "Error: expected $dylib_path to be present, but it was not. Installation of libiconv failed. Cannot proceed." usage exit 1 @@ -84,7 +85,7 @@ fi cd $ROOT # We expect this path to exist and be a symbolic link that points to a file. -if [ ! -L "$dylib_path" ]; then +if [ ! -e "$dylib_path" ]; then echoerr "Error: expected $dylib_path to be present, but it was not. Cannot proceed." usage exit 1 From 7b73c6ce27d4de73f768e7e44cfe5445571d8d4f Mon Sep 17 00:00:00 2001 From: Andrew Dupont Date: Mon, 7 Sep 2026 16:23:03 -0700 Subject: [PATCH 2/4] Tweak the workflow file --- .github/workflows/ci.yml | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index b6ae41b8..2d944ae0 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -1,7 +1,12 @@ name: ci on: - - pull_request - - push + push: + branches: [master] + pull_request: + +concurrency: + group: ${{ github.workflow }}-${{ github.ref }} + cancel-in-progress: true jobs: Test: @@ -13,7 +18,7 @@ jobs: os: - ubuntu-latest - macos-latest - - windows-latest + - windows-2022 node_version: - 16 - 18 From 3de853447bad785f0e6ef5f3d38fc8c2b5ce8b6c Mon Sep 17 00:00:00 2001 From: Andrew Dupont Date: Mon, 7 Sep 2026 17:20:10 -0700 Subject: [PATCH 3/4] Further changes (pressing my luck) --- .github/workflows/ci.yml | 2 +- binding.gyp | 32 ++++---------------------------- script/copy-libiconv.sh | 24 ++++++++++++++++++++++++ script/fetch-libiconv-61.sh | 9 ++++----- 4 files changed, 33 insertions(+), 34 deletions(-) create mode 100755 script/copy-libiconv.sh diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 2d944ae0..8f47253a 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -20,9 +20,9 @@ jobs: - macos-latest - windows-2022 node_version: - - 16 - 18 - 20 + - 22 name: Node ${{ matrix.node_version }} on ${{ matrix.os }} steps: diff --git a/binding.gyp b/binding.gyp index 54d0b07b..f4b8fd80 100644 --- a/binding.gyp +++ b/binding.gyp @@ -30,13 +30,9 @@ "postbuilds": [ { 'postbuild_name': 'Copy vendored libiconv next to the binding', - # `-L` because `ext/lib/libiconv.2.dylib` is a - # symlink to the versioned dylib. We want the real - # file here — a link would still point outside of - # `build/`, which is what we're getting away from. 'action': [ - 'cp', - '-L', + 'bash', + '<(module_root_dir)/script/copy-libiconv.sh', '<(module_root_dir)/ext/lib/libiconv.2.dylib', '<(PRODUCT_DIR)/libiconv.2.dylib' ] @@ -129,22 +125,6 @@ } ] } - # { - # "target_name": "find_libiconv", - # "target_type": "none", - # "actions": [ - # { - # "action_name": "Run script", - # "message": "Locating GNU libiconv...", - # "inputs": [], - # "outputs": ["vendor/libiconv/lib/libiconv.2.dylib"], - # "action": [ - # "bash", - # "script/find-gnu-libiconv.sh" - # ] - # } - # ] - # } ] }], @@ -197,13 +177,9 @@ "postbuilds": [ { 'postbuild_name': 'Copy vendored libiconv next to the binding', - # `-L` because `ext/lib/libiconv.2.dylib` is a - # symlink to the versioned dylib. We want the real - # file here — a link would still point outside of - # `build/`, which is what we're getting away from. 'action': [ - 'cp', - '-L', + 'bash', + '<(module_root_dir)/script/copy-libiconv.sh', '<(module_root_dir)/ext/lib/libiconv.2.dylib', '<(PRODUCT_DIR)/libiconv.2.dylib' ] diff --git a/script/copy-libiconv.sh b/script/copy-libiconv.sh new file mode 100755 index 00000000..19deeb9e --- /dev/null +++ b/script/copy-libiconv.sh @@ -0,0 +1,24 @@ +#!/bin/bash +set -euo pipefail + +# Copy the vendored libiconv next to the built product. Resolves the symlink +# (`ext/lib/libiconv.2.dylib` points at the versioned dylib) and installs the +# copy atomically, so two targets postbuilding in parallel can't observe a +# half-written file. + +src="$1" +dest="$2" + +tmp="$(mktemp "${dest}.XXXXXX")" +trap 'rm -f "$tmp"' EXIT + +# `-L` because `ext/lib/libiconv.2.dylib` is a symlink to the versioned dylib. +# We want the real file here — a link would still point outside of `build/`, +# which is what we're getting away from. +cp -L "$src" "$tmp" + +# We _must_ get the permissions right here; a `.dylib` with 0600 would work for +# the user who built it but fail for anyone else. +chmod 755 "$tmp" + +mv -f "$tmp" "$dest" diff --git a/script/fetch-libiconv-61.sh b/script/fetch-libiconv-61.sh index f43cd4db..6cee8b5c 100644 --- a/script/fetch-libiconv-61.sh +++ b/script/fetch-libiconv-61.sh @@ -13,7 +13,7 @@ set -euo pipefail # `libiconv.2.dylib`. For now, letting the user compile their own `libiconv` # has the advantage of very likely matching the system's architecture. -echoerr() { echo "$@\n" >&2; } +echoerr() { printf '%s\n\n' "$*" >&2; } create-if-missing() { if [ -f "$1" ]; then @@ -22,7 +22,7 @@ create-if-missing() { exit 1 fi if [ ! -d "$1" ]; then - mkdir "$1" + mkdir -p "$1" fi } @@ -53,7 +53,7 @@ dylib_path="$EXT/lib/libiconv.2.dylib" # and compiled. Otherwise we'll do it now. if [ ! -e "$dylib_path" ]; then echo "Path $dylib_path is missing; fetching and installing libiconv." - cd $SCRATCH + cd "$SCRATCH" # TODO: Instead of downloading this each time, we can check this into source # control via git subtree. That would allow someone to build this without # needing internet connectivity. But we'd still need to do a `make install` — @@ -82,9 +82,8 @@ else echo "Path $dylib_path is already present; skipping installation of libiconv." fi -cd $ROOT +cd "$ROOT" -# We expect this path to exist and be a symbolic link that points to a file. if [ ! -e "$dylib_path" ]; then echoerr "Error: expected $dylib_path to be present, but it was not. Cannot proceed." usage From 5924dfe5dda46da135b276d503d30964333e00a3 Mon Sep 17 00:00:00 2001 From: Andrew Dupont Date: Mon, 7 Sep 2026 18:00:57 -0700 Subject: [PATCH 4/4] Bump service versions --- .github/workflows/ci.yml | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 8f47253a..973f7476 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -26,18 +26,18 @@ jobs: name: Node ${{ matrix.node_version }} on ${{ matrix.os }} steps: - - uses: actions/checkout@v4 + - uses: actions/checkout@v5 with: submodules: true - name: Cache - uses: actions/cache@v3 + uses: actions/cache@v4 with: path: | node_modules key: ${{ runner.os }}-${{ matrix.node_version }}-${{ hashFiles('package.json') }} - name: Setup node - uses: actions/setup-node@v4 + uses: actions/setup-node@v5 with: node-version: ${{ matrix.node_version }}