Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Normalize path for cargo vendor output #10668

Merged
merged 4 commits into from
Jul 16, 2022
Merged

Normalize path for cargo vendor output #10668

merged 4 commits into from
Jul 16, 2022

Conversation

weihanglo
Copy link
Member

What does this PR try to resolve?

Normalize path for cargo vendor output

fixes #10652

How should we test and review this PR?

A new test vendor::vendor_path_specified is 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

  1. ops/cargo_add/dependency.rs ↩

  2. ops/vendor.rs ↩

  3. ops/cargo_package.rs ↩

@rust-highfive
Copy link

r? @ehuss

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 13, 2022
@ehuss
Copy link
Contributor

ehuss commented May 18, 2022

I'm having a hard time understanding the issue. directory = 'deps\.third_party' looks valid to me on Windows. Can you say more about what the underlying problem is?

@weihanglo
Copy link
Member Author

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.

@ehuss
Copy link
Contributor

ehuss commented May 18, 2022

Using a more cross-platform path seems reasonable, I just want to check with the author of the issue to make sure I am understanding their situation correctly.

@Zymlex Can you say more about why you filed #10652? The path is valid on Windows. Is the concern that it isn't cross-platform?

@Zymlex
Copy link

Zymlex commented May 19, 2022

@ehuss
image

image

@weihanglo
Copy link
Member Author

@Zymlex
It should work if you use single quote instead. Content surrounded by single quotes is a raw string in TOML.
Both directory = "deps\\.third_party" and directory = 'deps\.third_party' works for me.

Ref: https://toml.io/en/v1.0.0#string

@Zymlex Zymlex mentioned this pull request May 19, 2022
Closed
@Zymlex
Copy link

Zymlex commented May 19, 2022

@weihanglo It seems that you are right. Most likely, the reason was causeв by wrong quotes.
A rather unpleasant feature of the format.

ehuss
ehuss reviewed Jul 16, 2022
View reviewed changes
Copy link
Contributor

@ehuss ehuss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

src/cargo/ops/vendor.rs
@@ -298,7 +298,7 @@ fn sync(
config.insert(
merged_source_name.to_string(),
VendorSource::Directory {
directory: opts.destination.to_path_buf(),
directory: opts.destination.to_string_lossy().replace("\\", "/"),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a comment here explaining why it is replacing the slashes?

@weihanglo
Copy link
Member Author

Thanks for the review!

@bors r=ehuss

@bors
Copy link
Contributor

bors commented Jul 16, 2022

📌 Commit 88e8892 has been approved by ehuss

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 16, 2022
@bors
Copy link
Contributor

bors commented Jul 16, 2022

⌛ Testing commit 88e8892 with merge 576356f...

@bors
Copy link
Contributor

bors commented Jul 16, 2022

☀️ Test successful - checks-actions
Approved by: ehuss
Pushing 576356f to master...

@bors bors merged commit 576356f into rust-lang:master Jul 16, 2022
@weihanglo weihanglo deleted the issue-10652 branch July 17, 2022 05:05
@ehuss ehuss mentioned this pull request Jul 20, 2022
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 20, 2022
@bors
Auto merge of rust-lang#99484 - ehuss:update-cargo, r=ehuss
f426146 
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)
@ehuss ehuss added this to the 1.64.0 milestone Jul 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ehuss ehuss ehuss left review comments

Assignees

@ehuss ehuss

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
1.64.0
Development

Successfully merging this pull request may close these issues.

Invalid vendor path example
5 participants
@weihanglo @rust-highfive @ehuss @Zymlex @bors