fix: put cargo target dir inside the GN build output dir#124
Merged
eseidel merged 1 commit intoshorebird/devfrom Apr 8, 2026
Merged
fix: put cargo target dir inside the GN build output dir#124eseidel merged 1 commit intoshorebird/devfrom
eseidel merged 1 commit intoshorebird/devfrom
Conversation
build_rust_updater previously let cargo write its target/ directly into the source tree at flutter/engine/src/flutter/third_party/updater/target. Two consequences: 1. The engine's existing 'rm -rf out/<config>' clobber on every build doesn't touch it, so compiled rust artifacts silently survived across runs and could mask source/env changes (we just hit this when a stale libzstd_sys.rlib reused on Sandpiper masked the IPHONEOS_DEPLOYMENT_TARGET fix from #123). 2. All GN configs (debug/release/ios/android/...) shared one cargo target dir, stepping on each other's rlibs. Pass --target-dir to cargo, pointing at $root_out_dir/cargo_target, and update build_rust_updater.gni's shorebird_updater_output_lib to match. The cargo workspace now lives next to the rest of the GN build output and is clobbered by the same 'rm -rf out' that has always clobbered the C++ engine artifacts. Tracking: shorebirdtech/_shorebird#2014 Follow-up to: #123
bdero
approved these changes
Apr 8, 2026
eseidel
added a commit
that referenced
this pull request
Apr 8, 2026
The updater's .cargo/config.toml sets '-C target-feature=+crt-static' so rustc compiles the rlib expecting static-CRT linkage. But cargo populates CARGO_CFG_TARGET_FEATURE from the rustc target spec's default features, not from user rustflags, so +crt-static is invisible to build scripts. The cc crate (used by transitive *-sys deps like zstd-sys to compile their C sources) therefore falls back to /MD, producing .obj files full of __imp_* references that the engine's /MT link cannot resolve. Symptom: engine link on Windows fails with LNK2019 unresolved external symbol __imp_clock / __imp__wassert / __imp_qsort_s from zstd-sys's zdict/cover/fastcover/divsufsort translation units, after the updater rolled to a version whose ureq stack pulls in zstd-sys via its compression features. Fix: force /MT via the per-target CFLAGS env that cc honors. cl.exe emits warning D9025 overriding '/MD' with '/MT' and uses the last one specified -- /MT wins, the resulting .obj files reference the static CRT, and the engine link succeeds. This is another instance of the toolchain-consistency problem that #123 solved for iOS deployment target. Every new platform-specific C build setting we discover needs another env var threaded through build_rust_updater.py. Longer term this argues for going rustc-direct (Fuchsia/Chromium style), but for now we have one rust dep and the env-var-per-setting pattern is tractable. Tracking: shorebirdtech/_shorebird#2014 Follow-up to: #123, #124
eseidel
added a commit
that referenced
this pull request
Apr 8, 2026
The updater's .cargo/config.toml sets '-C target-feature=+crt-static' so rustc compiles the rlib expecting static-CRT linkage. But cargo populates CARGO_CFG_TARGET_FEATURE from the rustc target spec's default features, not from user rustflags, so +crt-static is invisible to build scripts. The cc crate (used by transitive *-sys deps like zstd-sys to compile their C sources) therefore falls back to /MD, producing .obj files full of __imp_* references that the engine's /MT link cannot resolve. Symptom: engine link on Windows fails with LNK2019 unresolved external symbol __imp_clock / __imp__wassert / __imp_qsort_s from zstd-sys's zdict/cover/fastcover/divsufsort translation units, after the updater rolled to a version whose ureq stack pulls in zstd-sys via its compression features. Fix: force /MT via the per-target CFLAGS env that cc honors. cl.exe emits warning D9025 overriding '/MD' with '/MT' and uses the last one specified -- /MT wins, the resulting .obj files reference the static CRT, and the engine link succeeds. This is another instance of the toolchain-consistency problem that #123 solved for iOS deployment target. Every new platform-specific C build setting we discover needs another env var threaded through build_rust_updater.py. Longer term this argues for going rustc-direct (Fuchsia/Chromium style), but for now we have one rust dep and the env-var-per-setting pattern is tractable. Tracking: shorebirdtech/_shorebird#2014 Follow-up to: #123, #124
eseidel
added a commit
that referenced
this pull request
Apr 8, 2026
The updater's .cargo/config.toml sets '-C target-feature=+crt-static' so rustc compiles the rlib expecting static-CRT linkage. But cargo populates CARGO_CFG_TARGET_FEATURE from the rustc target spec's default features, not from user rustflags, so +crt-static is invisible to build scripts. The cc crate (used by transitive *-sys deps like zstd-sys to compile their C sources) therefore falls back to /MD, producing .obj files full of __imp_* references that the engine's /MT link cannot resolve. Symptom: engine link on Windows fails with LNK2019 unresolved external symbol __imp_clock / __imp__wassert / __imp_qsort_s from zstd-sys's zdict/cover/fastcover/divsufsort translation units, after the updater rolled to a version whose ureq stack pulls in zstd-sys via its compression features. Fix: force /MT via the per-target CFLAGS env that cc honors. cl.exe emits warning D9025 overriding '/MD' with '/MT' and uses the last one specified -- /MT wins, the resulting .obj files reference the static CRT, and the engine link succeeds. This is another instance of the toolchain-consistency problem that #123 solved for iOS deployment target. Every new platform-specific C build setting we discover needs another env var threaded through build_rust_updater.py. Longer term this argues for going rustc-direct (Fuchsia/Chromium style), but for now we have one rust dep and the env-var-per-setting pattern is tractable. Tracking: shorebirdtech/_shorebird#2014 Follow-up to: #123, #124
eseidel
added a commit
that referenced
this pull request
Apr 8, 2026
mac_build.sh, linux_build.sh, and win_build.sh each had a leftover 'cd $UPDATER_SRC/library && cargo build --target ...' preamble from before bdero's #120 GN/Ninja integration of the rust updater build. mac_build.sh even had a FIXME at the top documenting the intent to delete it. These preambles: 1. Bypass the GN action entirely, so none of the env-var plumbing in build_rust_updater.py applies (no IPHONEOS_DEPLOYMENT_TARGET from #123, no CFLAGS_*_windows_msvc=/MT from #125, no Android NDK env). 2. Write to the source-tree default cargo target dir at third_party/updater/target/, ignoring #124's redirect to $root_out_dir/cargo_target/, so the engine 'rm -rf out' clobber never touches their output. 3. On the persistent Sandpiper runner this means stale rlibs from pre-fix builds keep getting reused, masking #123 entirely and producing the same iOS arm64 zstd_sys deployment-target failure after every fix attempt. Delete the library cargo invocations from all three scripts. The GN build of any engine target that depends transitively on //flutter/shell/common/shorebird:updater (which is most of them) will now invoke build_rust_updater.py via Ninja and produce libupdater.a / updater.lib in the right place with the right env vars. The patch tool's separate cargo build in mac_build.sh / linux_build.sh is a standalone CLI not linked into the engine, so the GN build does not cover it. Left in place with a TODO to migrate it later. Tracking: shorebirdtech/_shorebird#2014 Follow-up to: #120, #123, #124, #125
eseidel
added a commit
that referenced
this pull request
Apr 8, 2026
mac_build.sh, linux_build.sh, and win_build.sh each had a leftover 'cd $UPDATER_SRC/library && cargo build --target ...' preamble from before bdero's #120 GN/Ninja integration of the rust updater build. mac_build.sh even had a FIXME at the top documenting the intent to delete it. These preambles: 1. Bypass the GN action entirely, so none of the env-var plumbing in build_rust_updater.py applies (no IPHONEOS_DEPLOYMENT_TARGET from #123, no CFLAGS_*_windows_msvc=/MT from #125, no Android NDK env). 2. Write to the source-tree default cargo target dir at third_party/updater/target/, ignoring #124's redirect to $root_out_dir/cargo_target/, so the engine 'rm -rf out' clobber never touches their output. 3. On the persistent Sandpiper runner this means stale rlibs from pre-fix builds keep getting reused, masking #123 entirely and producing the same iOS arm64 zstd_sys deployment-target failure after every fix attempt. Delete the library cargo invocations from all three scripts. The GN build of any engine target that depends transitively on //flutter/shell/common/shorebird:updater (which is most of them) will now invoke build_rust_updater.py via Ninja and produce libupdater.a / updater.lib in the right place with the right env vars. The patch tool's separate cargo build in mac_build.sh / linux_build.sh is a standalone CLI not linked into the engine, so the GN build does not cover it. Left in place with a TODO to migrate it later. Tracking: shorebirdtech/_shorebird#2014 Follow-up to: #120, #123, #124, #125
eseidel
added a commit
that referenced
this pull request
Apr 8, 2026
mac_build.sh, linux_build.sh, and win_build.sh each had a leftover 'cd $UPDATER_SRC/library && cargo build --target ...' preamble from before bdero's #120 GN/Ninja integration of the rust updater build. mac_build.sh even had a FIXME at the top documenting the intent to delete it. These preambles: 1. Bypass the GN action entirely, so none of the env-var plumbing in build_rust_updater.py applies (no IPHONEOS_DEPLOYMENT_TARGET from #123, no CFLAGS_*_windows_msvc=/MT from #125, no Android NDK env). 2. Write to the source-tree default cargo target dir at third_party/updater/target/, ignoring #124's redirect to $root_out_dir/cargo_target/, so the engine 'rm -rf out' clobber never touches their output. 3. On the persistent Sandpiper runner this means stale rlibs from pre-fix builds keep getting reused, masking #123 entirely and producing the same iOS arm64 zstd_sys deployment-target failure after every fix attempt. Delete the library cargo invocations from all three scripts. The GN build of any engine target that depends transitively on //flutter/shell/common/shorebird:updater (which is most of them) will now invoke build_rust_updater.py via Ninja and produce libupdater.a / updater.lib in the right place with the right env vars. The patch tool's separate cargo build in mac_build.sh / linux_build.sh is a standalone CLI not linked into the engine, so the GN build does not cover it. Left in place with a TODO to migrate it later. Tracking: shorebirdtech/_shorebird#2014 Follow-up to: #120, #123, #124, #125
eseidel
added a commit
that referenced
this pull request
Apr 8, 2026
The action only declared its stamp file as an output, which worked when shorebird_updater_output_lib lived under //flutter/third_party/... because GN treated source-tree paths as existing files needing no rule. After #124 moved the cargo target dir into $root_out_dir/cargo_target, the lib path became a build-dir path and Ninja correctly demands a rule producing it. Without the lib in outputs: ninja: error: 'cargo_target/aarch64-linux-android/release/libupdater.a', needed by 'libflutter.so', missing and no known rule to make it Add shorebird_updater_output_lib to the action's outputs alongside the stamp file. The cargo invocation already produces the file at this path; this just tells Ninja which rule is responsible. Tracking: shorebirdtech/_shorebird#2014 Follow-up to: #124, #127
eseidel
added a commit
that referenced
this pull request
Apr 8, 2026
…128) The action only declared its stamp file as an output, which worked when shorebird_updater_output_lib lived under //flutter/third_party/... because GN treated source-tree paths as existing files needing no rule. After #124 moved the cargo target dir into $root_out_dir/cargo_target, the lib path became a build-dir path and Ninja correctly demands a rule producing it. Without the lib in outputs: ninja: error: 'cargo_target/aarch64-linux-android/release/libupdater.a', needed by 'libflutter.so', missing and no known rule to make it Add shorebird_updater_output_lib to the action's outputs alongside the stamp file. The cargo invocation already produces the file at this path; this just tells Ninja which rule is responsible. Tracking: shorebirdtech/_shorebird#2014 Follow-up to: #124, #127
eseidel
added a commit
that referenced
this pull request
Apr 8, 2026
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
eseidel
added a commit
that referenced
this pull request
Apr 8, 2026
…130) 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
eseidel
added a commit
that referenced
this pull request
Apr 8, 2026
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
eseidel
added a commit
that referenced
this pull request
Apr 8, 2026
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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
build_rust_updaterpreviously let cargo write itstarget/directly into the source tree atflutter/engine/src/flutter/third_party/updater/target/. Two consequences:rm -rf out/<config>clobber on every build doesn't touch it, so compiled rust artifacts silently survived across runs. This just bit us: a stalelibzstd_sys.rlibreused on Sandpiper masked theIPHONEOS_DEPLOYMENT_TARGETfix from fix: thread Apple deployment target into Rust updater build #123 entirely — the new env var never had a chance to take effect because cargo short-circuited the rebuild.Fix
--target-dirto cargo, pointing at$root_out_dir/cargo_target.build_rust_updater.gni'sshorebird_updater_output_libto match.The cargo workspace now lives next to the rest of the GN build output and is clobbered by the same
rm -rf outthat has always clobbered the C++ engine artifacts. No special-case cleanup incheckout.sh. Each GN config gets its own cargo target dir.Note for reviewer
This is a follow-up to #123 (which threaded
IPHONEOS_DEPLOYMENT_TARGETthrough to cargo) and a follow-up to #120 (your original GN/Ninja integration). Together with #123, this should make the iOS arm64 zstd link error from shorebirdtech/_shorebird#2014 actually go away on the next Sandpiper build.Note for ops
The old
target/directory at~/.engine_checkout/flutter/engine/src/flutter/third_party/updater/target/on the Sandpiper host will become orphaned after this lands (the new code never writes there). It's harmless to leave around but somebody shouldrm -rfit the next time they ssh in.Bigger picture (not in this PR)
We discussed whether to keep cargo-as-opaque-blob or migrate to rustc-direct (Fuchsia/Chromium style, where each crate becomes a GN target compiled by
rustcdirectly with the engine's clang for any*-sysC code). For now we're keeping cargo since this is a one-week-old integration with one Rust dep, but the rustc-direct option becomes worth doing once we have a second crate or hit hermetic-build requirements. There's a// TODOworth adding inbuild_rust_updater.gnilater.Tracking: shorebirdtech/_shorebird#2014
Follow-up to: #120, #123