fix cargo test of dylib projects for end user runs too
#4006
Conversation
This file contains 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
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
|
Nah I think this seems reasonable to land in Cargo regardless, thanks for the PR! My only worry here is the extra invocation of |
|
Sure. I had to fold it in with a run using --cfg because it would not work in the same run as -vV. --print sysroot appears to predate --cfg (Dec 2014) so this should work whenever --cfg does. |
|
ping @alexcrichton |
|
oh, sorry - it's only the force updates that don't notify you, right? |
|
@bors: r+ Thanks! |
|
📌 Commit d773d7b has been approved by |
bors
added a commit
that referenced
this pull request
May 9, 2017
fix `cargo test` of dylib projects for end user runs too Fixes running `cargo test` and `cargo test --target <target>` for dylib projects. Moves the logic just landed in #3996 into cargo itself, so cargo sets the dylib path for anyone running `cargo test` or `cargo run`. Current master sets the dylib path only for `cargo test` on cargo itself. This PR pins to rustup 1.2.0 for the purposes of testing. If rust-lang/rustup#1093 ends up working out, then this PR would only be important for non-rustup users and people doing cross testing, `cargo test --target <target>`. Arguably https://github.com/mcgoo/cargo/blob/ed273851f8bc76f726eda4a2e2a7bb470c3718bc/src/cargo/ops/cargo_rustc/context.rs#L249-L253 should point to lib/rustlib/\<host triple\>/lib instead of sysroot/lib, because I think if the libs are different, you will never be able to compile a working plugin anyway, and for the host=target case you get the lib/rustlib/\<host triple\>/lib anyhow. Is there ever a case where the lib/rustlib/\<host triple\>/lib and sysroot/lib versions of the libs would be expected to differ? This is not a huge deal for me one way or the other - it doesn't impact my workflow at all. I nearly dropped it when I saw @alexcrichton had made it all work in 3996, but I think it's worth doing because it removes a surprise. It certainly would have saved me a couple of days of confusion. Either way, thanks for looking it over.
|
☀️ Test successful - status-appveyor, status-travis |
weihanglo
added a commit
to weihanglo/cargo
that referenced
this pull request
Feb 20, 2024
Copy from <rust-lang#10469 (comment)>: > I've never been entirely clear why it does this. rust-lang#4006 didn't really > explain why it added the corresponding host_dylib_path. I can't envision > a scenario where it matters. I think compiler plugins and proc-macros > should load just fine, since libstd.so should already be loaded by the > compiler. Also, rustc uses rpath these days, and on Windows libstd.so is > placed in the bin directory which will be searched first anyways. > > On balance, I think it should be safe to just remove sysroot_host_libdir. > I can't come up with a scenario where it matters, at least on > windows/macos/linux. One issue is that this is most likely to affect > plugins, but those are deprecated and I think only Servo was the real > holdout. A concern is that nobody is going to test this use case before > it hits stable. Also, * compiler plugins were removed rust-lang/rust#116412 * servo has moved off from plugins: servo/servo#30508 So should generally be fine.
weihanglo
added a commit
to weihanglo/cargo
that referenced
this pull request
Feb 20, 2024
Copy from <rust-lang#10469 (comment)>: > I've never been entirely clear why it does this. rust-lang#4006 didn't really > explain why it added the corresponding host_dylib_path. I can't envision > a scenario where it matters. I think compiler plugins and proc-macros > should load just fine, since libstd.so should already be loaded by the > compiler. Also, rustc uses rpath these days, and on Windows libstd.so is > placed in the bin directory which will be searched first anyways. > > On balance, I think it should be safe to just remove sysroot_host_libdir. > I can't come up with a scenario where it matters, at least on > windows/macos/linux. One issue is that this is most likely to affect > plugins, but those are deprecated and I think only Servo was the real > holdout. A concern is that nobody is going to test this use case before > it hits stable. Also, * compiler plugins were removed rust-lang/rust#116412 * servo has moved off from plugins: servo/servo#30508 So should generally be fine.
stupendoussuperpowers
pushed a commit
to stupendoussuperpowers/cargo
that referenced
this pull request
Feb 28, 2024
Copy from <rust-lang#10469 (comment)>: > I've never been entirely clear why it does this. rust-lang#4006 didn't really > explain why it added the corresponding host_dylib_path. I can't envision > a scenario where it matters. I think compiler plugins and proc-macros > should load just fine, since libstd.so should already be loaded by the > compiler. Also, rustc uses rpath these days, and on Windows libstd.so is > placed in the bin directory which will be searched first anyways. > > On balance, I think it should be safe to just remove sysroot_host_libdir. > I can't come up with a scenario where it matters, at least on > windows/macos/linux. One issue is that this is most likely to affect > plugins, but those are deprecated and I think only Servo was the real > holdout. A concern is that nobody is going to test this use case before > it hits stable. Also, * compiler plugins were removed rust-lang/rust#116412 * servo has moved off from plugins: servo/servo#30508 So should generally be fine.
charmitro
pushed a commit
to charmitro/cargo
that referenced
this pull request
Sep 13, 2024
Copy from <rust-lang#10469 (comment)>: > I've never been entirely clear why it does this. rust-lang#4006 didn't really > explain why it added the corresponding host_dylib_path. I can't envision > a scenario where it matters. I think compiler plugins and proc-macros > should load just fine, since libstd.so should already be loaded by the > compiler. Also, rustc uses rpath these days, and on Windows libstd.so is > placed in the bin directory which will be searched first anyways. > > On balance, I think it should be safe to just remove sysroot_host_libdir. > I can't come up with a scenario where it matters, at least on > windows/macos/linux. One issue is that this is most likely to affect > plugins, but those are deprecated and I think only Servo was the real > holdout. A concern is that nobody is going to test this use case before > it hits stable. Also, * compiler plugins were removed rust-lang/rust#116412 * servo has moved off from plugins: servo/servo#30508 So should generally be fine.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Fixes running
cargo testandcargo test --target <target>for dylib projects.Moves the logic just landed in #3996 into cargo itself, so cargo sets the dylib path for anyone running
cargo testorcargo run. Current master sets the dylib path only forcargo teston cargo itself.This PR pins to rustup 1.2.0 for the purposes of testing. If rust-lang/rustup#1093 ends up working out, then this PR would only be important for non-rustup users and people doing cross testing,
cargo test --target <target>.Arguably https://github.com/mcgoo/cargo/blob/ed273851f8bc76f726eda4a2e2a7bb470c3718bc/src/cargo/ops/cargo_rustc/context.rs#L249-L253 should point to lib/rustlib/<host triple>/lib instead of sysroot/lib, because I think if the libs are different, you will never be able to compile a working plugin anyway, and for the host=target case you get the lib/rustlib/<host triple>/lib anyhow. Is there ever a case where the lib/rustlib/<host triple>/lib and sysroot/lib versions of the libs would be expected to differ?
This is not a huge deal for me one way or the other - it doesn't impact my workflow at all. I nearly dropped it when I saw @alexcrichton had made it all work in 3996, but I think it's worth doing because it removes a surprise. It certainly would have saved me a couple of days of confusion. Either way, thanks for looking it over.