test: verify Cargo cfgs for all supported triples - #205
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a556d3ca2f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| atom_ids = CFG_ATOM_IDS_BY_TRIPLE.get(triple) | ||
| if atom_ids != None: | ||
| return [CFG_ATOMS[atom_id] for atom_id in atom_ids] |
There was a problem hiding this comment.
Use cfg data from the selected Rust toolchain
When a consumer configures a nondefault compiler through toolchains.toolchain(version = ...), this lookup still unconditionally returns the checked-in Rust 1.92 cfg set. These sets vary by compiler version: for example, Rust 1.92 reports bulk-memory and nontrapping-fptoint for wasm32-wasip1, while Rust 1.86 does not, so a dependency guarded by either target_feature is selected even though the configured compiler lacks that baseline cfg. Generate or select the oracle by configured compiler version, or explicitly reject unsupported versions instead of applying the 1.92 table to every toolchain.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I think this is a valid find. My first thoughts are:
- Make the data version-aware: Generate tables keyed by rust version and have each cargo closure use the table matching its selected compiler.
- Enforce the recorded version: If the table was generated with rust 1.92, only use it when the selected compiler is 1.92. For any other version, fail clearly instead of silently calculating the wrong dependency graph.
Any thoughts here @dzbarsky?
There was a problem hiding this comment.
tbh this makes me sad, because I don't think we want to be in the business of per-rust-version data. (What about nightlies? what about someone testing a new compiler that we haven't added data for yet? Fallback to heuristics? Is that better than just using latest? ;) )
Also, what does "have each cargo closure use the table matching its selected compiler" mean? The compiler used at build time is an arbitrary toolchain and the cargo extension has no idea which one will be selected.
Overall it feels weird to me that compiler version affects these, as they should really be immutable properties of the target platform IMO. I suspect (hope?) that it's really only the case for wasm/wasi where so much of it depends on compiler support. I wonder if we can just default to a fairly old/conservative compiler's output. Probably need to do more research, but wdyt?
There was a problem hiding this comment.
Agreed on the research. I'll poke around this week!
| values = {} | ||
| ctx = { | ||
| "_triple": triple, | ||
| "_values": values, |
There was a problem hiding this comment.
why did all of these move into _values isntead of being directly on the ctx?
There was a problem hiding this comment.
The extra map was intended to support repeated rustc values such as target_feature and target_has_atomic, but the nesting was unnecessary. The context now stores a set directly at each cfg key and generic equality uses membership against that set.
| "windows": fam == "windows", | ||
| "wasm": fam == "wasm", | ||
| } | ||
| def triple_to_cfg_attrs(triple): |
There was a problem hiding this comment.
this will run once per-cargo-closure. I wonder if its worth deduping the construciton of these contexts by lifting it higher in the graph and passing those in instead of the triples (since triples can be retrieved from them anyway?)
There was a problem hiding this comment.
The contexts are currently constructed once in resolve_cargo_workspace_members for each Cargo closure, then reused by the resolver and workspace dependency rendering; target-expression results are cached as well. Lifting them further would only deduplicate across closures with the same triple set, so I left that plumbing unchanged pending evidence that cross-closure construction is material.
There was a problem hiding this comment.
I'd expect a typical bazel repo to have 5-15 closures (there are some internal ones for rules_rs, though I should probably merge to a single one...). so yeah maybe not a huge deal
a556d3c to
cb0398a
Compare
|
Sorry for joining this late. I ran into the same question while working on #213. IMHO, per-version tables do not fit the current resolver. The Cargo extension does not know which Rust toolchain Bazel will later select. Using an older table is not safe either -> a newer compiler can enable a cfg missing from that table. I lean toward separating target identity (target_arch, target_os, ABI, and so on) from compiler-dependent capabilities (target_feature, atomic support). #213 only fixes triple-derived identity and leaves compiler-dependent cfg handling unchanged. |
Summary
ALL_TARGET_TRIPLESrather than approximating their cfg valuesThe generated
cfg_target_data.bzlfile is a 14.8 KB interned target-cfg table loaded bycfg_parser.bzlduring normal rule evaluation.Performance
I profiled the same parser query against upstream
mainand this branch with Bazel 9.0.0:I extracted the sole
preloadOrThrowduration from each trace with:jq '[.traceEvents[] | select(.name == "preloadOrThrow") | .dur] | add' profile.jsonBazel reports trace durations in microseconds:
mainThis is a single before/after measurement rather than a benchmark distribution. The percentage increase is large because the baseline is small; the measured absolute difference was 13.8 ms.
Testing
bazel test //rs/private:cfg_parser_tests //rs/private:update_cfg_target_data_testsbazel test //...