Skip to content

Commit 9b29847

Browse files
committed
darwin: harden reconcile_framework_install_names (fail-loud + all-slice ids)
Two robustness fixes to the #223 reconcile pass (both from code review): 1. Stop swallowing real failures. The install_name_tool/-id/-change and codesign calls used `2>/dev/null || true`, which also hid genuine failures — most importantly install_name_tool's 'larger updated load commands do not fit' (a Mach-O with no header space to grow a load command). A swallowed -change leaves a bare @rpath ref and reproduces the exact dyld launch crash this pass exists to prevent, silently. Now: -id, a *present-dep* -change (absent deps are still a rc-0 no-op), and codesign failures are fatal — the function returns non-zero with a clear message, and sync_site_packages.sh propagates it (`|| exit 1`) so `flet build` fails loudly instead of shipping a broken app. stderr is captured and only printed on error, so the expected 'will invalidate the code signature' warning stays off the build log. 2. Record every slice's old install-id, not just the first. Pass 1 read oldid only from the first slice; a lib whose slices carry divergent install names (e.g. an arch-specific absolute build path instead of an @rpath id) would leave the other slices' deps unrewritten. Now each distinct old id is mapped (deduped) to the same new framework id. Verified: happy path (padded dylibs) + the real llama ggml->llama chain both reconcile to rc 0 with no bare refs; a divergent-slice-id pair rewrites both slices; an unpadded binary now fails loudly (rc 1) instead of silently.
1 parent cc28d13 commit 9b29847

2 files changed

Lines changed: 48 additions & 21 deletions

File tree

src/serious_python_darwin/darwin/sync_site_packages.sh

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -55,7 +55,12 @@ if [[ -n "$SERIOUS_PYTHON_SITE_PACKAGES" && -d "$SERIOUS_PYTHON_SITE_PACKAGES" ]
5555
# resolve e.g. @rpath/libarrow.dylib at launch and the app crashes
5656
# before Python starts. Exclude the python/stdlib xcframeworks copied
5757
# in above -- they are already correct.
58-
reconcile_framework_install_names "$dist/site-xcframeworks" "$dist/python-xcframeworks"
58+
# A reconcile failure (e.g. a Mach-O with no header space to grow a load
59+
# command, or a signing error) would otherwise ship an app that crashes
60+
# at launch -- abort the build instead. sync_site_packages.sh has no
61+
# set -e; prepare_spm.sh runs it under set -euo pipefail, so a non-zero
62+
# exit here fails `flet build` loudly.
63+
reconcile_framework_install_names "$dist/site-xcframeworks" "$dist/python-xcframeworks" || exit 1
5964

6065
rm -rf $dist/site-packages
6166
mkdir -p $dist/site-packages

src/serious_python_darwin/darwin/xcframework_utils.sh

Lines changed: 42 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -132,34 +132,51 @@ reconcile_framework_install_names() {
132132
local -a map_old=()
133133
local -a map_new=()
134134

135-
# Pass 1: fix each framework's own install-id; record old->new for deps.
136-
local xcf fw newid bin oldid raw
135+
# Pass 1: set each framework's own install-id to @rpath/<fw>.framework/<fw>,
136+
# and record old-id -> new-id for the dep rewrite below. Read the old id from
137+
# EVERY slice, not just the first: a lib whose slices carry divergent install
138+
# names (e.g. an upstream build that baked an arch-specific absolute path
139+
# instead of an @rpath id) would otherwise leave the other slices' deps
140+
# unrewritten. Distinct old ids all map to the same new id.
141+
local xcf fw newid bin oldid raw err j found
137142
for xcf in "$xcframeworks_dir"/*.xcframework; do
138143
[ -d "$xcf" ] || continue
139144
fw=$(basename "$xcf" .xcframework)
140145
[ -n "$exclude_dir" ] && [ -e "$exclude_dir/$fw.xcframework" ] && continue
141146
newid="@rpath/$fw.framework/$fw"
142-
oldid=""
143147
for bin in "$xcf"/*/"$fw.framework/$fw"; do
144148
[ -f "$bin" ] || continue
145-
if [ -z "$oldid" ]; then
146-
# Buffer otool output before filtering: piping otool straight
147-
# into `head -1` lets head close the pipe early, and the SIGPIPE
148-
# race intermittently drops the first read (empty oldid), which
149-
# cascades into a shifted/short map.
150-
raw=$(otool -D "$bin" 2>/dev/null)
151-
oldid=$(printf '%s\n' "$raw" | grep -v ':$' | grep -vi 'Architectures in' | head -1 | sed 's/^[[:space:]]*//')
149+
# Buffer otool output before filtering: piping otool straight into
150+
# `head -1` lets head close the pipe early, and the SIGPIPE race
151+
# intermittently drops the first read (empty oldid).
152+
raw=$(otool -D "$bin" 2>/dev/null)
153+
oldid=$(printf '%s\n' "$raw" | grep -v ':$' | grep -vi 'Architectures in' | head -1 | sed 's/^[[:space:]]*//')
154+
# -id must succeed; a failure means the binary is unwritable/corrupt
155+
# and the app would crash at launch, so surface it instead of hiding
156+
# it behind `|| true` (stderr is captured and only printed on error,
157+
# to keep the expected "will invalidate the code signature" warning
158+
# off the build log).
159+
if ! err=$(install_name_tool -id "$newid" "$bin" 2>&1); then
160+
echo "reconcile_framework_install_names: install_name_tool -id failed for $bin: $err" >&2
161+
return 1
162+
fi
163+
if [ -n "$oldid" ] && [ "$oldid" != "$newid" ]; then
164+
found=0
165+
for j in ${map_old[@]+"${map_old[@]}"}; do
166+
[ "$j" = "$oldid" ] && { found=1; break; }
167+
done
168+
[ "$found" -eq 0 ] && { map_old+=("$oldid"); map_new+=("$newid"); }
152169
fi
153-
install_name_tool -id "$newid" "$bin" 2>/dev/null || true
154170
done
155-
if [ -n "$oldid" ] && [ "$oldid" != "$newid" ]; then
156-
map_old+=("$oldid")
157-
map_new+=("$newid")
158-
fi
159171
done
160172

161-
# Pass 2: rewrite interdependent refs to framework paths, then re-sign
162-
# (install_name_tool invalidates the ad-hoc signature).
173+
# Pass 2: rewrite each binary's deps that point at a sibling's old id to that
174+
# sibling's framework path, then re-sign (install_name_tool invalidates the
175+
# ad-hoc signature). `install_name_tool -change` is a no-op returning 0 when
176+
# the dep is absent, so a NON-zero rc means a dep that IS present could not be
177+
# rewritten -- usually no Mach-O header space to grow the load command. That
178+
# is fatal: it would leave a bare @rpath ref and reproduce the exact launch
179+
# crash this pass exists to prevent, so fail the build rather than ship it.
163180
local i n=${#map_old[@]}
164181
for xcf in "$xcframeworks_dir"/*.xcframework; do
165182
[ -d "$xcf" ] || continue
@@ -169,11 +186,16 @@ reconcile_framework_install_names() {
169186
[ -f "$bin" ] || continue
170187
i=0
171188
while [ $i -lt $n ]; do
172-
# no-op if this binary does not reference map_old[i]
173-
install_name_tool -change "${map_old[$i]}" "${map_new[$i]}" "$bin" 2>/dev/null || true
189+
if ! err=$(install_name_tool -change "${map_old[$i]}" "${map_new[$i]}" "$bin" 2>&1); then
190+
echo "reconcile_framework_install_names: install_name_tool -change '${map_old[$i]}' -> '${map_new[$i]}' failed for $bin (no Mach-O header space?): $err" >&2
191+
return 1
192+
fi
174193
i=$((i+1))
175194
done
176-
codesign --force --sign - "$bin" >/dev/null 2>&1 || true
195+
if ! err=$(codesign --force --sign - "$bin" 2>&1); then
196+
echo "reconcile_framework_install_names: codesign failed for $bin: $err" >&2
197+
return 1
198+
fi
177199
done
178200
done
179201
}

0 commit comments

Comments
 (0)