Make Dart native builds reproducible - #1815
Conversation
Coverage Report for CI Build 32894300291Coverage remained the same at 86.718%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
Jolah1
left a comment
Jolah1
left a comment
There was a problem hiding this comment.
Concept ACK — tracking the production lockfile and remapping build paths is the right shape, and the .pubignore interaction holds up: .cargo/ is excluded as a directory, so renaming the overlay doesn't leak it into the
archive, and native/Cargo.lock isn't excluded, so it ships. The lockfile itself is clean (384 packages, only payjoin-ffi-wrapper sourceless).
The hook has four issues that stack, and none of them can surface individually because the first one masks the rest. In order: the env-var opt-in never reaches the hook, so _test-utils is always off — that's the 9 failing Dart
tests. Fix that and --config errors on a path that doesn't exist. Fix that and --locked rejects the patch overlay. And once rebased, the tracked lock's pinned rev no longer matches master's manifest, so --locked fails on the
production path too.
Details and repro commands inline. Also: nix fmt clears both the Formatting and flake-check (maintenance) failures — same treefmt diff. And given the repo's commit conventions this is probably five commits rather than one
(rename overlay, track lock, remap paths, add scripts, docs).
| final rustFlagEnvironment = hasEncodedRustFlags | ||
| ? {'CARGO_ENCODED_RUSTFLAGS': rustFlags} | ||
| : {'RUSTFLAGS': rustFlags}; | ||
| final enableTestUtils = |
There was a problem hiding this comment.
This is why the Dart job fails. hooks_runner spawns build hooks with includeParentEnvironment: false and an allowlisted environment — a static name list plus the prefixes CARGO_, RUSTUP_, NIX_, CCACHE_, DOTNET_, NUGET_, CONAN_. PAYJOIN_FFI_ENABLE_TEST_UTILS matches none of them, so it's always null here regardless of the export in contrib/test.sh:25, and enableTestUtils is always false.
Result in this PR's CI: 9 tests fail on Failed to lookup symbol 'uniffi_payjoin_ffi_fn_func_example_url', ..._original_psbt, ..._testservices_initialize — all _test-utils-gated. The bindings are still generated with _test-utils, so they declare symbols the library doesn't export.
Worth noting the mechanism you replaced worked precisely because it needed no environment. Options: a CARGO_-prefixed variable name, a file marker, or hooks user-defines. Same applies to RUSTFLAGS at line 17 — also not in the allowlist, so that branch is unreachable.
| extraCargoBuildArgs: [ | ||
| if (enableTestUtils && localCargoConfig.existsSync()) ...[ | ||
| '--config', | ||
| '../.cargo/config.local.toml', |
There was a problem hiding this comment.
Cargo resolves a relative --config file path against its own cwd, which hooks_runner sets to input.packageRoot = payjoin-ffi/dart. So ../.cargo/config.local.toml points at payjoin-ffi/.cargo/config.local.toml, which doesn't exist — and cargo doesn't skip it, it hard-errors:
error: failed to parse value from --config argument ../.cargo/config.local.toml as a dotted key expression Should be .cargo/config.local.toml, or an absolute path off input.packageRoot.
| '--config', | ||
| '../.cargo/config.local.toml', | ||
| ], | ||
| '--locked', |
There was a problem hiding this comment.
--locked and the [patch] overlay above are mutually exclusive. The overlay redirects payjoin-ffi/payjoin from the pinned rev to workspace paths, which changes resolution, and --locked forbids the resulting lock update:
$ cargo build --offline --locked --config .cargo/patch.toml
error: cannot update the lock file ... because --locked was passed to prevent this
Same command without --config succeeds. --locked belongs on the production path only — inside an if (!enableTestUtils).
| } | ||
|
|
||
| final inheritedPlain = _nonEmpty(rustFlags); | ||
| return [if (inheritedPlain != null) inheritedPlain, ...remaps].join(' '); |
There was a problem hiding this comment.
In the normal consumer case neither RUSTFLAGS nor CARGO_ENCODED_RUSTFLAGS is inherited, so this joins the remaps with spaces — and cargo splits RUSTFLAGS on whitespace. input.outputDirectory lives under the consumer app's .dart_tool/, so a project at /Users/jane/My App yields --remap-path-prefix=/Users/jane/My plus a stray App/... argument, and rustc fails. Emitting CARGO_ENCODED_RUSTFLAGS unconditionally avoids the ambiguity — it's \x1f-separated.
| rm -f native/Cargo.lock | ||
| echo "==> Checking production native Cargo.lock..." | ||
| test -f native/Cargo.lock | ||
| if grep -n 'path = ' native/Cargo.lock; then |
There was a problem hiding this comment.
This guard can never fire. Cargo.lock has no path key — a path dependency is the absence of source. I reproduced the case it's meant to catch (lock resolved through a [patch] to a local path):
[[package]]
name = "once_cell"
version = "1.21.4" # no source line
grep -c 'path = ' → 0. Second data point: this repo's Cargo-recent.lock has six path members and zero path = . So it returns 0 matches whether the lock is correct or poisoned. smoke_consumer.sh:54's grep -Fq '...rust-payjoin.git?rev=' is the check with teeth — that, plus asserting no non-root package lacks a source.
| artifacts=() | ||
| for copy in first second; do | ||
| matches=( | ||
| "$work_dir/consumer-$copy/.dart_tool/hooks_runner/shared"/*/build/*/target/"$rust_target"/release/*.so |
There was a problem hiding this comment.
This glob matches nothing, so the script exits "found 0" on the first consumer and can never pass. native_toolchain_rust builds into input.outputDirectory/target, and hooks_runner sets outputDirectory to .dart_tool/hooks_runner///out/. The shared/ tree is a different directory, used for outputDirectoryShared.
Two more in the same block: CARGO_TARGET_DIR at line 90 is inert because RustBuildRunner passes --target-dir explicitly and CLI wins — which contradicts the CONTRIBUTING claim that the two runs use separate target dirs. And
line 111 passes a Dart version string where check_reproducible.sh expects a target triple, so the report prints Target: dart-3.x.y while $rust_target is only used in the glob.
| await build(args, (input, output) async { | ||
| final environment = Platform.environment; | ||
| final rustFlags = composeRustFlags( | ||
| packageRoot: input.packageRoot.toFilePath(), |
There was a problem hiding this comment.
input.packageRoot is a directory Uri, so toFilePath() keeps the trailing separator and rustc's plain prefix substitution yields /payjoin/packagenative/src/lib.rs rather than the documented /payjoin/package/native/.... Still deterministic, so reproducibility holds — but not the prefix the docs describe. The unit tests pass '/package' without a trailing slash, which is why they miss it; worth a case with one.
| cp "$BINDING_SOURCE" "$package_copy/lib/payjoin.dart" | ||
|
|
||
| test -f "$package_copy/native/Cargo.lock" | ||
| test ! -d "$package_copy/.cargo" |
There was a problem hiding this comment.
vacuous: line 43 just rm -rf'd this path, so it checks the script's own rm, not the archive.
| ( | ||
| cd "$consumer" | ||
| "$DART_BIN" pub get --offline | ||
| ) > "$work_dir/$copy-pub-get.log" 2>&1 |
There was a problem hiding this comment.
shfmt wants >"$work_dir/$copy-pub-get.log" (no space); same at line 93. That plus dart format on build.dart is the entire Formatting / flake-check failure.
| exit 1 | ||
| fi | ||
|
|
||
| echo "==> Generating FFI bindings..." |
There was a problem hiding this comment.
check_reproducible_test.sh isn't called from anywhere, so the new checker's tests never run in CI.
The Dart package carries a .cargo/config.toml that patches payjoin-ffi to workspace paths. Cargo discovers that file by walking up from the crate, so every command run under payjoin-ffi/dart silently inherits the overlay, and the hook could only tell a checkout from a published archive by the same file's existence without ever saying so. Rename it to config.local.toml, which Cargo does not pick up on its own, and have the build hook pass it explicitly with --config. The presence of the file stays the signal for a workspace build, but it is now the hook that decides when the overlay applies. Pass the absolute path: Cargo resolves a relative --config path against its own working directory, which is the package root, and errors out when nothing is there. Nothing here can move to an environment variable. hooks_runner spawns build hooks with an allowlisted environment, so a project specific name never reaches the hook.
The native wrapper records absolute paths from the machine that built it: the package root, the hook output directory, and the Cargo and Rustup homes all end up in the artifact. Two consumers building the same release therefore produce different bytes, which makes the library impossible to verify. Pass --remap-path-prefix for each of those roots so the recorded paths depend on the release rather than on the machine. Emit them through CARGO_ENCODED_RUSTFLAGS rather than RUSTFLAGS. Cargo splits RUSTFLAGS on whitespace, and the output directory lives under the consumer's project: a path such as /Users/jane/My App would become two arguments and fail the build. Flags inherited from the environment are re-encoded argument by argument for the same reason. Directory URIs render with a trailing separator, and rustc substitutes the prefix literally, so drop it: otherwise native/src/lib.rs would be recorded as /payjoin/packagenative/src/lib.rs.
Dart consumers resolve the native wrapper's Cargo graph themselves. Every install therefore picks whatever versions are current, so two users of the same package version can compile different code, and the release is never tested against what they actually build. Track native/Cargo.lock and have the hook build it with --locked, but only when the local overlay is absent: that overlay redirects payjoin-ffi to workspace paths, and Cargo refuses to update a locked file to match. The workspace test run keeps the tracked lockfile aside, since the graph it pins does not resolve against the patched workspace. Check the lockfile in CI and before publishing. It cannot be checked by grepping for a `path` key, which is what an earlier revision of this branch did: a Cargo lockfile has no such key, and a dependency resolved through the overlay is simply a package that lost its `source`. Assert instead that the wrapper is the only sourceless package, and that the pinned revision matches the manifest, which nothing else catches until a consumer tries to build. The checker ships with tests over both cases. prepare-publish.sh no longer deletes the lockfile, and asserts on the dry run's file listing that the archive carries it and still withholds the development overlay.
51da8ed to
071cb61
Compare
Nothing in the repository verifies that the remapped native build is actually reproducible, or that a consumer installing the published package gets a library it can compile at all. check_reproducible.sh compares two artifacts byte-for-byte and reports the toolchain versions that produced them, so a divergence names the inputs that have to be pinned. smoke_consumer.sh drives the whole consumer path: it stands in two isolated Dart projects on a copy of the package with everything .pubignore withholds removed, builds each through the hook with its own Cargo home, and feeds both artifacts to the checker. The artifact is located by searching the hook output tree rather than by a fixed glob, because hooks_runner has moved that directory between releases and the path is not part of its contract. Nothing sets CARGO_TARGET_DIR: the hook passes --target-dir explicitly and the command line wins, and the two runs already build under separate project directories. contrib/test.sh runs the checker's own tests, which need no Rust build.
The release notes did not say how to regenerate the tracked lockfile, how the hook decides between a workspace and a consumer build, or how to check that two builds of a release match. Record the lockfile step with the command that reproduces it, describe the hook's marker file and its remapped prefixes, and give the two-build reproducibility procedure and the consumer smoke test their own sections. Point Running Tests at contrib/test.sh, which is what CI runs and what keeps the tracked lockfile out of the workspace build's way.
071cb61 to
fddecc8
Compare
|
Thanks @Jolah1 |
3 participants
Dart consumers currently resolve the native wrapper's Cargo graph and inherit build paths from each machine. That makes published-package builds sensitive to local dependency resolution and prevents byte-for-byte reproducibility.
This PR:
native/Cargo.lockand builds it with--locked;Deliberate non-goals: