fix: gate shorebird_updater_supported on having a known rust target#130
Conversation
build_rust_updater.gni previously set shorebird_updater_supported based
only on target_os (is_android || is_ios || ...). But GN evaluates BUILD
files under every toolchain in use, including host and sub toolchains
that can have unusual (target_os, target_cpu) combinations we don't
handle. When a toolchain entered the `if (shorebird_updater_supported)`
block but hit none of the os/cpu cases that set
`shorebird_updater_rust_target`, GN blew up:
ERROR at build_rust_updater.gni:72: Undefined identifier in string
expansion.
shorebird_updater_output_lib = "$...$shorebird_updater_rust_target..."
"shorebird_updater_rust_target" is not currently in scope.
This happens on Windows host builds of android engine variants when
the host toolchain(s) land on a combo like (is_win, target_cpu != x64)
or similar, which our is_win branch doesn't cover.
Flip the logic: compute shorebird_updater_rust_target first, default
to empty, and derive `shorebird_updater_supported` from whether it
got a value. Unhandled combos become unsupported, so the consuming
BUILD.gn files' `if (shorebird_updater_supported)` gate cleanly skips
the updater for that toolchain — which is correct, since the updater
only needs to be built for toolchains whose final binary is the engine.
Tracking: shorebirdtech/_shorebird#2014
Follow-up to: #124, #127, #128
bdero
left a comment
There was a problem hiding this comment.
Clearly, my original change to add support for building the Rust updater in GN was not complete. I'd consider just reverting the whole thing to unblock things if continues to give you trouble.
|
No no, this is very good. This is taking us in the right direction. I'm glad for us to finally do this. |
|
The Clankers are extremely good at this. |
|
I asked Claude if we should move to the full "just call the Rust compiler directly" solution. It gave me this long history of how Fuchsia and Chromium had started with Cargo like we are, and then slowly migrated to calling the Rust compiler directly. It recommended that we stay where we were, just calling Cargo for now, even though it added all this extra noise, because the full solution was a lot more work. |
Picks up the updater roll (#122) plus the GN/build fixes needed to build the engine against the new ureq-based updater stack on all three platforms (#123, #124, #125, #127, #128, #130). This is the first engine revision where a full Linux + macOS + Windows build completes against the rolled updater. Build: https://github.com/shorebirdtech/_build_engine/actions/runs/24139880449
Picks up the updater roll (#122) plus the GN/build fixes needed to build the engine against the new ureq-based updater stack on all three platforms (#123, #124, #125, #127, #128, #130). This is the first engine revision where a full Linux + macOS + Windows build completes against the rolled updater. Build: https://github.com/shorebirdtech/_build_engine/actions/runs/24139880449
Summary
build_rust_updater.gnipreviously setshorebird_updater_supportedfromtarget_osalone (is_android || is_ios || ...). But GN evaluates BUILD files under every toolchain in use, including host and sub toolchains that can hit(target_os, target_cpu)combos we don't explicitly handle. When such a toolchain entered theif (shorebird_updater_supported)block but none of the per-cpu cases setshorebird_updater_rust_target, GN blew up:Reproduced on Windows host builds of android engine variants:
win_build.sh'sgn --android --android-cpu=arm64invocation, evaluated under the Windows host toolchain, landed on anis_win/target_cpu combo our is_win branch doesn't cover.Fix
Flip the logic. Compute
shorebird_updater_rust_targetfirst (default empty), then deriveshorebird_updater_supportedfrom whether it got assigned. Unhandled combos become unsupported, so consumingBUILD.gnfiles' existingif (shorebird_updater_supported)gate cleanly skips the updater for that toolchain — which is correct, since the updater only needs to be built for toolchains whose final binary is the engine.No new os/cpu combos added. No behavior change for already-handled combos. Just safer unhandled-combo behavior.
Tracking: shorebirdtech/_shorebird#2014
Follow-up to: #124, #127, #128