Skip to content

Commit a4fcbe3

Browse files
charliekclaude
andcommitted
review(coderabbit): dedup cargo stages into bundle-lib.sh + doc/test fixes
roost_find_cargo / roost_setup_cargo_profile / roost_cargo_target_dir extracted so bundle-iced.sh and the roostctl embed stage share one copy of cargo discovery, profile mapping, and CARGO_TARGET_DIR anchoring (the comment said the paths must not diverge; a second copy is what permits divergence). bundle-iced.sh's header no longer files shared deferrals (notarize/DMG) under "unlike bundle.sh", and its Developer ID bullet is gone — the script does sign with one when ROOST_DEVELOPER_ID_IDENTITY is set. client.py's app_dock_badge docstring now names the Swift app's unknown-op answer instead of claiming every other UI says not-implemented. tempfile.mktemp replaced with a uuid-based never-created path helper (ruff S306). Verified: shellcheck clean, test-harness 101 OK, bundle rebuilt + e2e-iced-bundle 14 passed. Skipped (replied on the thread): the cross-language bundle-identity consistency check — executable-name drift already fails the eager Contents/MacOS/Roost-Iced validation, and app-id drift fails the identity assertion with the exact wanted line quoted, so drift is loud today; a three-way generator is more machinery than the risk warrants. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AyvCPrbetEKy3iHgKLQmwq
1 parent 800729f commit a4fcbe3

4 files changed

Lines changed: 90 additions & 72 deletions

File tree

mac/scripts/bundle-iced.sh

Lines changed: 10 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -35,8 +35,9 @@
3535
# the two bundles (6c decision, not made here); see the header
3636
# comment in Info-iced.plist.template for why the Sparkle plist
3737
# keys are absent too.
38-
# * Code-sign with a Developer ID certificate (ad-hoc until #83,
39-
# same as bundle.sh).
38+
#
39+
# Deferred the same way bundle.sh defers them (shared posture, not a
40+
# difference):
4041
# * Notarize via `notarytool`.
4142
# * Build a DMG (out of scope — plan 027 scope brief: local build
4243
# only, no release.yml wiring).
@@ -95,42 +96,18 @@ APP_DIR="${OUT_DIR}/${APP_NAME}.app"
9596

9697
roost_check_libghostty_archive "${REPO_ROOT}"
9798

98-
CARGO_BIN="$(command -v cargo || true)"
99-
if [ -z "${CARGO_BIN}" ] && [ -x "${HOME}/.cargo/bin/cargo" ]; then
100-
CARGO_BIN="${HOME}/.cargo/bin/cargo"
101-
fi
102-
if [ -z "${CARGO_BIN}" ]; then
103-
echo "error: cargo not found on PATH or at ~/.cargo/bin/cargo" >&2
104-
exit 1
105-
fi
99+
CARGO_BIN="$(roost_find_cargo)"
100+
roost_setup_cargo_profile "${CONFIG}"
106101

107-
CARGO_PROFILE_FLAG="--release"
108-
CARGO_PROFILE_DIR="release"
109-
if [ "${CONFIG}" = "debug" ]; then
110-
CARGO_PROFILE_FLAG=""
111-
CARGO_PROFILE_DIR="debug"
112-
fi
113-
114-
echo "==> Building roost-iced (cargo build -p roost-iced --${CARGO_PROFILE_DIR})"
102+
echo "==> Building roost-iced (cargo build -p roost-iced --${ROOST_CARGO_PROFILE_DIR})"
115103
(
116104
cd "${REPO_ROOT}"
117-
# shellcheck disable=SC2086 # CARGO_PROFILE_FLAG must word-split (empty => no flag)
118-
"${CARGO_BIN}" build -p roost-iced ${CARGO_PROFILE_FLAG}
105+
# shellcheck disable=SC2086 # ROOST_CARGO_PROFILE_FLAG must word-split (empty => no flag)
106+
"${CARGO_BIN}" build -p roost-iced ${ROOST_CARGO_PROFILE_FLAG}
119107
)
120108

121-
# Respect CARGO_TARGET_DIR for artifact discovery, exactly like the
122-
# roostctl embed step in bundle-lib.sh — shared caches (sccache, CI
123-
# matrices fanning out across configs) routinely override the default
124-
# `<repo>/target/` location.
125-
CARGO_TARGET="${CARGO_TARGET_DIR:-${REPO_ROOT}/target}"
126-
# Cargo resolves a relative CARGO_TARGET_DIR from its own CWD (the repo
127-
# root, where the build subshell cd's) — anchor discovery the same way so
128-
# the two can't diverge.
129-
case "${CARGO_TARGET}" in
130-
/*) ;;
131-
*) CARGO_TARGET="${REPO_ROOT}/${CARGO_TARGET}" ;;
132-
esac
133-
ICED_BUILD_BIN="${CARGO_TARGET}/${CARGO_PROFILE_DIR}/roost-iced"
109+
CARGO_TARGET="$(roost_cargo_target_dir "${REPO_ROOT}")"
110+
ICED_BUILD_BIN="${CARGO_TARGET}/${ROOST_CARGO_PROFILE_DIR}/roost-iced"
134111
if [ ! -x "${ICED_BUILD_BIN}" ]; then
135112
echo "error: cargo build did not produce ${ICED_BUILD_BIN}" >&2
136113
exit 1

mac/scripts/bundle-lib.sh

Lines changed: 58 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -142,6 +142,58 @@ roost_install_app_icon() {
142142
fi
143143
}
144144

145+
# roost_find_cargo
146+
#
147+
# Prints the cargo to invoke. Discovers `cargo` on PATH instead of
148+
# hardcoding ~/.cargo/bin/cargo — release runners may have cargo at a
149+
# different prefix (toolchain managed by mise / rustup / system
150+
# package). Falling back to the literal path preserves the prior
151+
# behavior for the common dev case.
152+
roost_find_cargo() {
153+
local cargo_bin
154+
cargo_bin="$(command -v cargo || true)"
155+
if [ -z "${cargo_bin}" ] && [ -x "${HOME}/.cargo/bin/cargo" ]; then
156+
cargo_bin="${HOME}/.cargo/bin/cargo"
157+
fi
158+
if [ -z "${cargo_bin}" ]; then
159+
echo "error: cargo not found on PATH or at ~/.cargo/bin/cargo" >&2
160+
exit 1
161+
fi
162+
echo "${cargo_bin}"
163+
}
164+
165+
# roost_setup_cargo_profile CONFIG
166+
#
167+
# Sets ROOST_CARGO_PROFILE_FLAG (word-split deliberately; empty for
168+
# debug) and ROOST_CARGO_PROFILE_DIR in the caller's scope — the same
169+
# caller-scope convention roost_setup_signing uses.
170+
roost_setup_cargo_profile() {
171+
ROOST_CARGO_PROFILE_FLAG="--release"
172+
ROOST_CARGO_PROFILE_DIR="release"
173+
if [ "$1" = "debug" ]; then
174+
ROOST_CARGO_PROFILE_FLAG=""
175+
ROOST_CARGO_PROFILE_DIR="debug"
176+
fi
177+
}
178+
179+
# roost_cargo_target_dir REPO_ROOT
180+
#
181+
# Prints the artifact root, honoring CARGO_TARGET_DIR — shared caches
182+
# (e.g. sccache + CI matrices that fan out across configs) routinely
183+
# override the default `<repo>/target/` location. Cargo resolves a
184+
# relative CARGO_TARGET_DIR from its own CWD (the repo root, where the
185+
# build subshells cd) — discovery anchors the same way so the two
186+
# can't diverge.
187+
roost_cargo_target_dir() {
188+
local repo_root="$1"
189+
local cargo_target="${CARGO_TARGET_DIR:-${repo_root}/target}"
190+
case "${cargo_target}" in
191+
/*) ;;
192+
*) cargo_target="${repo_root}/${cargo_target}" ;;
193+
esac
194+
echo "${cargo_target}"
195+
}
196+
145197
# roost_build_and_embed_roostctl REPO_ROOT APP_DIR CONFIG
146198
#
147199
# Embed roostctl under Contents/Resources/bin/ so `claude install`
@@ -150,32 +202,16 @@ roost_install_app_icon() {
150202
# fast and tracked through the same Cargo cache as any cargo build
151203
# invocation; rebuilding here keeps the bundle in lockstep with
152204
# whatever roost-cli source the developer has checked out.
153-
#
154-
# Discovers `cargo` on PATH instead of hardcoding ~/.cargo/bin/cargo.
155-
# Release runners may have cargo at a different prefix (toolchain
156-
# managed by mise / rustup / system package). Falling back to the
157-
# literal path preserves the prior behavior for the common dev case.
158205
roost_build_and_embed_roostctl() {
159206
local repo_root="$1"
160207
local app_dir="$2"
161208
local config="$3"
162209

163210
local cargo_bin
164-
cargo_bin="$(command -v cargo || true)"
165-
if [ -z "${cargo_bin}" ] && [ -x "${HOME}/.cargo/bin/cargo" ]; then
166-
cargo_bin="${HOME}/.cargo/bin/cargo"
167-
fi
168-
if [ -z "${cargo_bin}" ]; then
169-
echo "error: cargo not found on PATH or at ~/.cargo/bin/cargo" >&2
170-
exit 1
171-
fi
172-
173-
local cargo_profile_flag="--release"
174-
local cargo_profile_dir="release"
175-
if [ "${config}" = "debug" ]; then
176-
cargo_profile_flag=""
177-
cargo_profile_dir="debug"
178-
fi
211+
cargo_bin="$(roost_find_cargo)"
212+
roost_setup_cargo_profile "${config}"
213+
local cargo_profile_flag="${ROOST_CARGO_PROFILE_FLAG}"
214+
local cargo_profile_dir="${ROOST_CARGO_PROFILE_DIR}"
179215
echo "==> Building roostctl (cargo build -p roost-cli --${cargo_profile_dir})"
180216
(
181217
# shellcheck disable=SC2164 # callers run this lib under `set -e`
@@ -184,17 +220,8 @@ roost_build_and_embed_roostctl() {
184220
"${cargo_bin}" build -p roost-cli ${cargo_profile_flag}
185221
)
186222

187-
# Respect CARGO_TARGET_DIR for the artifact-discovery step. Shared
188-
# caches (e.g. sccache + CI matrices that fan out across configs)
189-
# routinely override the default `<repo>/target/` location.
190-
local cargo_target="${CARGO_TARGET_DIR:-${repo_root}/target}"
191-
# Cargo resolves a relative CARGO_TARGET_DIR from its own CWD (the
192-
# repo root, where the build subshell cd's) — anchor discovery the
193-
# same way so the two can't diverge.
194-
case "${cargo_target}" in
195-
/*) ;;
196-
*) cargo_target="${repo_root}/${cargo_target}" ;;
197-
esac
223+
local cargo_target
224+
cargo_target="$(roost_cargo_target_dir "${repo_root}")"
198225
local roostctl_src="${cargo_target}/${cargo_profile_dir}/roostctl"
199226
if [ ! -x "${roostctl_src}" ]; then
200227
echo "error: cargo build did not produce ${roostctl_src}" >&2

tools/roosttest/client.py

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -565,8 +565,10 @@ def app_dock_badge(self) -> str | None:
565565
the badge write actually landed.
566566
567567
Gated by ROOST_TEST_MODE=1 (raises `RoostError('not-enabled')`
568-
when off) and macOS-iced-only: every other UI, and the iced UI on
569-
Linux, answers `RoostError('not-implemented')`."""
568+
when off) and macOS-iced-only: the GTK UI and the iced UI on
569+
Linux answer `RoostError('not-implemented')`; the Swift Mac app
570+
has no dispatcher case at all and answers
571+
`RoostError('unknown-op')` (same as `tab.feed_ime`)."""
570572
res = self.call("app.dock_badge", {})
571573
# Direct key access: a missing field is a protocol violation, and
572574
# `.get` would read it as "badge cleared" — the exact state the

tools/roosttest_unit/test_iced_bundle_launch.py

Lines changed: 18 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -15,16 +15,24 @@
1515
import sys
1616
import tempfile
1717
import unittest
18+
import uuid
1819
from pathlib import Path
1920
from unittest.mock import Mock, patch
2021

2122
ROOSTTEST_DIR = Path(__file__).resolve().parents[1] / "roosttest"
2223
sys.path.insert(0, str(ROOSTTEST_DIR))
2324

2425
import ui # noqa: E402
26+
2527
from client import RoostError # noqa: E402
2628

2729

30+
def _absent_log_path() -> Path:
31+
"""A unique path that is never created — mocks the UI log location
32+
without `tempfile.mktemp`'s predictable-name hazard (ruff S306)."""
33+
return Path(tempfile.gettempdir()) / f"roost-absent-{uuid.uuid4().hex}.log"
34+
35+
2836
def _make_bundle(root: Path, executable_name: str = "Roost-Iced") -> Path:
2937
app = root / "Roost-Iced.app"
3038
macos_dir = app / "Contents" / "MacOS"
@@ -284,7 +292,7 @@ class BundlePidVerificationTests(unittest.TestCase):
284292

285293
def test_mismatched_process_name_refuses_to_adopt_the_pid(self) -> None:
286294
with (
287-
patch("ui._iced_bundle_ui_log_path", return_value=Path(tempfile.mktemp())),
295+
patch("ui._iced_bundle_ui_log_path", return_value=_absent_log_path()),
288296
patch("ui.subprocess.run"),
289297
patch("ui.wait_alive"),
290298
patch("ui._answering_pid", return_value=4242),
@@ -309,7 +317,7 @@ def test_matching_process_name_adopts_the_pid(self) -> None:
309317
},
310318
clear=True,
311319
),
312-
patch("ui._iced_bundle_ui_log_path", return_value=Path(tempfile.mktemp())),
320+
patch("ui._iced_bundle_ui_log_path", return_value=_absent_log_path()),
313321
patch("ui.subprocess.run") as run,
314322
patch("ui.wait_alive"),
315323
patch("ui._answering_pid", return_value=4242),
@@ -340,8 +348,12 @@ def test_matching_process_name_adopts_the_pid(self) -> None:
340348

341349
def _launch_argv_with_env(self, env: dict[str, str]) -> list[str]:
342350
with (
351+
tempfile.TemporaryDirectory() as temp_dir,
343352
patch.dict(os.environ, env, clear=True),
344-
patch("ui._iced_bundle_ui_log_path", return_value=Path(tempfile.mktemp())),
353+
patch(
354+
"ui._iced_bundle_ui_log_path",
355+
return_value=Path(temp_dir) / "iced-ui.log",
356+
),
345357
patch("ui.subprocess.run") as run,
346358
patch("ui.wait_alive"),
347359
patch("ui._answering_pid", return_value=4242),
@@ -372,7 +384,7 @@ def test_executable_outside_the_bundle_refuses_to_adopt_the_pid(self) -> None:
372384
e.g. a `Roost-Iced` on $PATH from an unrelated build. Must not be
373385
adopted even though the process-name check alone would pass."""
374386
with (
375-
patch("ui._iced_bundle_ui_log_path", return_value=Path(tempfile.mktemp())),
387+
patch("ui._iced_bundle_ui_log_path", return_value=_absent_log_path()),
376388
patch("ui.subprocess.run"),
377389
patch("ui.wait_alive"),
378390
patch("ui._answering_pid", return_value=4242),
@@ -393,7 +405,7 @@ def test_identity_mismatch_pkills_by_name_when_no_pid_was_ever_adopted(
393405
self,
394406
) -> None:
395407
with (
396-
patch("ui._iced_bundle_ui_log_path", return_value=Path(tempfile.mktemp())),
408+
patch("ui._iced_bundle_ui_log_path", return_value=_absent_log_path()),
397409
patch("ui.subprocess.run") as run,
398410
patch("ui.wait_alive"),
399411
patch("ui._answering_pid", return_value=4242),
@@ -411,7 +423,7 @@ def test_canary_failure_after_pid_adoption_pid_kills_the_bundle(self) -> None:
411423
down through the pid-based path, not a name-based `pkill`."""
412424
with (
413425
patch.dict(os.environ, {"ROOST_TEST_MODE": "1"}, clear=True),
414-
patch("ui._iced_bundle_ui_log_path", return_value=Path(tempfile.mktemp())),
426+
patch("ui._iced_bundle_ui_log_path", return_value=_absent_log_path()),
415427
patch("ui.subprocess.run") as run,
416428
patch("ui.wait_alive"),
417429
patch("ui._answering_pid", return_value=4242),

0 commit comments

Comments
 (0)