Support multiple artifact dependency aliases to the same package#17067
Support multiple artifact dependency aliases to the same package#17067npmccallum wants to merge 5 commits into
Conversation
|
r? @weihanglo rustbot has assigned @weihanglo. Use Why was this reviewer chosen?The reviewer was selected based on:
|
|
@weihanglo are you able to review this? If not, maybe @ehuss could? |
|
@rustbot reroll |
|
Note that in general, there is a lot less review bandwidth at this time. Weihang has the most bandwidth and he said he wouldn't be available. We'll see when/if I have time. |
| authors = [] | ||
| resolver = "2" | ||
|
|
||
| [dependencies.bar] |
There was a problem hiding this comment.
Is this mimicking existing tests? I at least would prefer we only use inline tables for deps, not standard tables.
|
|
||
| p.cargo("build --unit-graph -Zunstable-options -Z bindeps") | ||
| .masquerade_as_nightly_cargo(&["unit-graph", "bindeps"]) | ||
| .with_stdout_contains(r#"[..]"index":0,"extern_crate_name":"bar"[..]"#) |
There was a problem hiding this comment.
Is this mimicking an existing pattern? We try to avoid the _contains functions
This comment has been minimized.
This comment has been minimized.
|
I split this into smaller commits:
I also adjusted the tests from the inline comments to use inline dependency tables and a structured unit-graph stdout assertion. |
|
May have time for this, though let me pick up the RFC first. |
4546761 to
55cb078
Compare
|
This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
|
@joshtriplett This is my next highest priority if you have cycles. With this feature, arma can generate PMIs cross-architecture. |
This comment has been minimized.
This comment has been minimized.
Add regression coverage for the current invariant that multiple dependency declarations to the same package are rejected when they would require distinct dependency names. This makes the existing behavior explicit before relaxing the artifact-dependency cases tracked in rust-lang#12374.
Extract dependency-name calculation so artifact alias handling can use the same extern crate naming rules in build graph lowering and metadata output.
Permit all-artifact aliases to the same package to keep their dependency-specific names when lowering build and unit graph edges. Mixed artifact and non-artifact dependencies still use the existing duplicate-name check so library dependencies have one extern crate name.
Use the shared dependency-name rules when emitting cargo metadata for artifact aliases to the same package. Bin-only artifact aliases emit dependency-specific extern names, while lib aliases continue to require a single library dependency name.
Document declaring separate aliases when building artifacts from the same package for multiple target platforms. Also call out that multiple lib=true aliases must agree on one library extern crate name.
| .unwrap_or_else(|| (to_target.crate_name().into(), None)) | ||
| } | ||
|
|
||
| pub fn lib_dependency_name<'a>( |
There was a problem hiding this comment.
This appears to be dead code within this commit
| Ok((extern_crate_name, dep_name)) | ||
| } | ||
|
|
||
| pub fn dep_extern_crate_name_and_dep_name( |
There was a problem hiding this comment.
Doesn't look like this needs to be pub within this commit
| [build-dependencies.bar] | ||
| path = "bar/" | ||
| artifact = "bin" | ||
| target = "{}" | ||
|
|
||
| [build-dependencies.bar-native] | ||
| package = "bar" | ||
| path = "bar/" | ||
| artifact = "bin" | ||
| target = "{}" | ||
| [build-dependencies] | ||
| bar = {{ path = "bar/", artifact = "bin", target = "{}" }} | ||
| bar-native = {{ package = "bar", path = "bar/", artifact = "bin", target = "{}" }} |
There was a problem hiding this comment.
Was this style change made in the wrong commit? It makes it harder to see what may be intentional changes fthat go with this fix
| .with_status(101) | ||
| .with_stderr_data(str![[r#" | ||
| [LOCKING] 1 package to latest compatible version | ||
| [ERROR] the crate `foo v0.0.0 ([ROOT]/foo)` depends on crate `bar v0.5.0 ([ROOT]/foo/bar)` multiple times with different names | ||
|
|
||
| "#]]) | ||
| .with_stderr_contains( | ||
| "[RUNNING] `rustc --crate-name bar --edition=2015 bar/src/main.rs [..]--target [ALT_TARGET] [..]", | ||
| ) |
There was a problem hiding this comment.
What is the goal with using with_stderr_contains with this line of verbose output?
For context, we try to minimize the use of _contains assertions because we can't do snapshotting with them
| #[cargo_test] | ||
| fn multiple_artifact_aliases_no_op_rebuild() { |
There was a problem hiding this comment.
Why is a new test being added in a non-test commit?
| .with_stdout_contains(r#"[..]"rename":"bar-lib"[..]"#) | ||
| .with_stdout_contains(r#"[..]"rename":"bar-bin"[..]"#) | ||
| .with_stdout_contains(r#"[..]"extern_name":"bar_lib"[..]"#) | ||
| .with_stdout_contains(r#"[..]"extern_name":"bar_bin"[..]"#) |
There was a problem hiding this comment.
The lack of context is another problem with _contains assertions: I was looking to the test to better understand a part of your change and I can't tell what is happening from this
This fills in a behavior described by RFC 3176 that was not implemented when artifact dependencies were initially added.
The RFC allows a package to be declared multiple times under different dependency names, with each declaration selecting a different artifact target. Cargo rejected several of those same-package artifact dependency forms as duplicate renamed dependencies. This PR allows the all-artifact cases while keeping the existing duplicate-name checks for mixed artifact/non-artifact dependency cases.
The series is split into reviewable commits:
cargo metadata.lib = truerestriction.The
lib = truecase still requires Cargo to refer to the package library with one Rust extern crate name. Multiple artifact aliases that would give the library different extern names continue to be rejected.Closes #12374
FYI @joshtriplett @Byron