Fix passing of links-overrides with target-applies-to-host and an implicit target #14205
Conversation
rustbot
added
A-build-scripts
|
@rustbot label: Z-target-applies-to-host r? @weihanglo if you're willing to look at this too |
|
Well, the test doesn't work on windows, converting to draft until I fix that. |
|
Marking this as ready for review, but leaving the fix for the test on windows as a separate commit for now because I'm hoping that there's a better way to link to a cdylib built with cargo on windows that I'm missing. See the last commit for my current understanding of what you have to do to do that. If there isn't a better way, I should probably squash the last commit into the first commit that introduces the test before this is merged. |
| @@ -61,7 +61,7 @@ const OLD_CARGO_WARNING_SYNTAX: &str = "cargo:warning="; | |||
| /// [the doc]: https://doc.rust-lang.org/nightly/cargo/reference/build-scripts.html#cargo-warning | |||
| const NEW_CARGO_WARNING_SYNTAX: &str = "cargo::warning="; | |||
| /// Contains the parsed output of a custom build script. | |||
| #[derive(Clone, Debug, Hash, Default)] | |||
| #[derive(Clone, Debug, Hash, Default, PartialEq, Eq, PartialOrd, Ord)] | |||
There was a problem hiding this comment.
Also realized that there is an implication that these newly added fields would affect the order of any sorted units list. I don't think that would cause real issues though.
There was a problem hiding this comment.
True... if this is a significant concern we could move the new fields to the end.
Ultimately I don't think I have the context to decide if it is. I think it is almost certainly publicly exposed given comments like this one, and where these fields currently are in the struct is probably high enough to change the order in the cases described in this comment. I wouldn't have thought that the ordering of units was something that was guaranteed to be stable between cargo versions.
I'll hold off on updating the rustflags Arc to Rc until we've decided if we need to move it to the bottom since that might as well be one PR.
…-host and an implicit target
gmorenz
force-pushed
the
links_overrides_in_unit
branch
from
0943b5c
to
9098072
Compare
|
Made promised changes, rebased on master. Removed part about test from the initial github comment since it's no longer relevant. |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions |
Yeah, also I need to incorporate the Arc -> Rc feedback. Will get to this later today, I've just been a bit busy and was putting it off (though I don't anticipate it taking more than 10 minutes). When you say "haven't got time" are you looking forwards and saying I should ask someone else to review it, or just "not yet/right now"? |
|
It is in my queue, and I am happy to review when ready :) |
Use `Rc` instead of `Arc` for storing rustflags # What does this PR try to resolve? There's no reason that these reference counted pointers (which I introduced in #13900) need to be atomic, so let's use non-atomic pointers. # Additional information First noticed by `@weihanglo` [here](#14205 (comment)). r? `@weihanglo`
Update cargo 9 commits in a2b58c3dad4d554ba01ed6c45c41ff85390560f2..5f6b9a92201d78af75dc24f14662c3e2dacbbbe1 2024-07-16 00:52:02 +0000 to 2024-07-19 18:09:17 +0000 - Add `TomlPackage::new`, `Default` for `TomlWorkspace` (rust-lang/cargo#14271) - fix(test): Move 'cargo_home' from 'install' to 'paths' (rust-lang/cargo#14270) - fix(test)!: Clarify extension trait role with rename (rust-lang/cargo#14269) - feat(test): Re-export ProcessBuilder (rust-lang/cargo#14268) - fix(test): Move path2url to CargoPathExt::to_url (rust-lang/cargo#14266) - Fix passing of links-overrides with target-applies-to-host and an implicit target (rust-lang/cargo#14205) - fix(toml): Improve error on missing package and workspace (rust-lang/cargo#14261) - Migrate global_cache_tracker snapbox (rust-lang/cargo#14244) - make summary sync by using Arc not Rc (rust-lang/cargo#14260)
Update cargo 9 commits in a2b58c3dad4d554ba01ed6c45c41ff85390560f2..5f6b9a92201d78af75dc24f14662c3e2dacbbbe1 2024-07-16 00:52:02 +0000 to 2024-07-19 18:09:17 +0000 - Add `TomlPackage::new`, `Default` for `TomlWorkspace` (rust-lang/cargo#14271) - fix(test): Move 'cargo_home' from 'install' to 'paths' (rust-lang/cargo#14270) - fix(test)!: Clarify extension trait role with rename (rust-lang/cargo#14269) - feat(test): Re-export ProcessBuilder (rust-lang/cargo#14268) - fix(test): Move path2url to CargoPathExt::to_url (rust-lang/cargo#14266) - Fix passing of links-overrides with target-applies-to-host and an implicit target (rust-lang/cargo#14205) - fix(toml): Improve error on missing package and workspace (rust-lang/cargo#14261) - Migrate global_cache_tracker snapbox (rust-lang/cargo#14244) - make summary sync by using Arc not Rc (rust-lang/cargo#14260)
What does this PR try to resolve?
This fixes the link-overrides half of #14195, both the panic, and the fact that the field is being discarded, the latter of which caused the former as discussed in the issue.
It does so following the blueprint laid out in #13900 - which is also in my opinion the current best summary of the broader context.
How should we test and review this PR?
For reviewing, comparing to the changes in #13900 might be useful.
Additional information
I'm pushing a PR for the other half of #14195 simultaneously. I thought it better to keep the PRs small since they're independent, though if merged simultaneously there will be a conflict over the ordering of fields in
Unit.