Normalize path for cargo vendor output#10668
Conversation
|
r? @ehuss (rust-highfive has picked a reviewer for you, use r? to override) |
|
I'm having a hard time understanding the issue. |
|
Indeed it is valid on Windows but not on Unix. And I recalled that cargo-add did a trick like this, so I decided to make the output more cross-platform compatible. If you feel uncertain on this, I can close it since the output doesn't actually write to anywhere unless being piped to somewhere. |
|
@Zymlex |
|
@weihanglo It seems that you are right. Most likely, the reason was causeв by wrong quotes. |
ehuss
left a comment
There was a problem hiding this comment.
@weihanglo Given that the original issue was due to a misunderstanding about the syntax for TOML, do you still want to go ahead with this? I think I'm fine with the change. There are some cases where changing the slashes will break the path (like a network path), but I think that is unlikely (I assume these are almost always relative paths).
If you want to go ahead with this, feel free to r=ehuss with the comment added.
| merged_source_name.to_string(), | ||
| VendorSource::Directory { | ||
| directory: opts.destination.to_path_buf(), | ||
| directory: opts.destination.to_string_lossy().replace("\\", "/"), |
There was a problem hiding this comment.
Can you add a comment here explaining why it is replacing the slashes?
|
Thanks for the review! @bors r=ehuss |
|
☀️ Test successful - checks-actions |
Update cargo 9 commits in 8827baaa781b37872134c1ba692a6f0aeb37890e..d8d30a75376f78bb0fabe3d28ee9d87aa8035309 2022-07-14 02:56:51 +0000 to 2022-07-19 13:59:17 +0000 - docs: fixing minor error in the default timing report filename (rust-lang/cargo#10879) - Stabilize `--crate-type` flag for `cargo rustc` (rust-lang/cargo#10838) - Stabilize `-Zmultitarget` (rust-lang/cargo#10766) - Clean up leftover in unstable documentation (rust-lang/cargo#10874) - Normalize path for `cargo vendor` output (rust-lang/cargo#10668) - add a reason to `masquerade_as_nightly_cargo` so it is searchable (rust-lang/cargo#10868) - Allow '.' in workspace.default-members in non-virtual workspaces. (rust-lang/cargo#10784) - remove`.masquerade_as_nightly_cargo()` from various tests the no longer need it (rust-lang/cargo#10867) - remove `.masquerade_as_nightly_cargo()` from build_script_extra_link_arg.rs (rust-lang/cargo#10866)
What does this PR try to resolve?
Normalize path for
cargo vendoroutputfixes #10652
How should we test and review this PR?
A new test
vendor::vendor_path_specifiedis added. Please run under Windows to verify the behaviour.Additional information
There are already three places 123 using the same trick. I guess we are happy to adopt it here.
Footnotes
ops/cargo_add/dependency.rs ↩
ops/vendor.rs ↩
ops/cargo_package.rs ↩