Skip to content

Fix some bugs that cause minor annoyances… #22

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

Open
savetheclocktower wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Fix some bugs that cause minor annoyances… #22

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
322e8c0
Fix some bugs that cause minor annoyances…
savetheclocktower Sep 7, 2026
7b73c6c
Tweak the workflow file
savetheclocktower Sep 7, 2026
3de8534
Further changes (pressing my luck)
savetheclocktower Sep 8, 2026
5924dfe
Bump service versions
savetheclocktower Sep 8, 2026
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
19 changes: 12 additions & 7 deletions .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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:
Expand All @@ -13,26 +18,26 @@ jobs:
os:
- ubuntu-latest
- macos-latest
- windows-latest
- windows-2022
node_version:
- 16
- 18
- 20
- 22
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 }}

Expand Down
73 changes: 26 additions & 47 deletions binding.gyp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,27 +28,24 @@
"conditions": [
['OS=="mac"', {
"postbuilds": [
{
'postbuild_name': 'Copy vendored libiconv next to the binding',
'action': [
'bash',
'<(module_root_dir)/script/copy-libiconv.sh',
'<(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)'
# ]

}
]
}]
Expand Down Expand Up @@ -120,30 +117,14 @@
"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"
]
}
]
}
# {
# "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"
# ]
# }
# ]
# }
]
}],

Expand Down Expand Up @@ -194,28 +175,26 @@
'MACOSX_DEPLOYMENT_TARGET': '10.12',
},
"postbuilds": [
{
'postbuild_name': 'Copy vendored libiconv next to the binding',
'action': [
'bash',
'<(module_root_dir)/script/copy-libiconv.sh',
'<(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)'
# ]
}
]
]
}]
]
}]
Expand Down
24 changes: 24 additions & 0 deletions script/copy-libiconv.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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"
18 changes: 9 additions & 9 deletions script/fetch-libiconv-61.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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
Expand All @@ -12,16 +13,16 @@
# `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 [ -z "$1" ]; then
if [ -f "$1" ]; then
echoerr "Error: $1 is a file."
usage
exit 1
fi
if [ ! -d "$1" ]; then
mkdir "$1"
mkdir -p "$1"
fi
}

Expand Down Expand Up @@ -50,9 +51,9 @@ 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
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` —
Expand All @@ -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
Expand All @@ -81,10 +82,9 @@ 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 [ ! -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
Expand Down
Loading