Print file paths instead of file:// URLs. #5994

zachlute · 2018-09-08T02:48:09Z

This change ensures cargo will output file paths in the expected format
(C:\foo... on Windows, /foo/... elsewhere). Previously it would output
file:// URLs instead.

To support this change, additional changes were made to the test suite
string processing such that [ROOT] is now replaced with the appropriate
file path root for the platform.

The CWD template was also updated to use [CWD] like other replacement
templates and to do the replacement on the expected value rather than
the actual value to avoid replacing things we don't expect with CWD.

This change ensures cargo will output file paths in the expected format (C:\foo\... on Windows, /foo/... elsewhere). Previously it would output file:// URLs instead. To support this change, additional changes were made to the test suite string processing such that [ROOT] is now replaced with the appropriate file path root for the platform. The CWD template was also updated to use [CWD] like other replacement templates and to do the replacement on the expected value rather than the actual value to avoid replacing things we don't expect with CWD.

rust-highfive · 2018-09-08T02:48:13Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @matklad (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.

zachlute · 2018-09-08T03:36:09Z

Test failures were due to overzealous replacement of file:// for git repositories. Strange that it didn't fail on Windows...

zachlute · 2018-09-08T03:39:02Z

Though I guess that is a question worth asking: I made the decision that git paths should still be file:// URIs, because I think people expect git paths to be URIs, and we tack extra junk on to the end of them that would make it weird as a file path anyway. So please let me know if that seems wrong.

Looks like things fail on my Mac, so I can use that to track down the issues.

dwijnand

LGTM

dwijnand · 2018-09-08T14:51:30Z

Thank you, @zachlute!

alexcrichton · 2018-09-09T17:50:01Z

Indeed looks great to me, thanks @zachlute!

@bors: r+

bors · 2018-09-09T17:50:02Z

📌 Commit a5a95fe65d4d3c207c31b48b1525d3d0f47c5813 has been approved by alexcrichton

bors · 2018-09-09T22:07:11Z

🔒 Merge conflict

This pull request and the master branch diverged in a way that cannot be automatically merged. Please rebase on top of the latest master branch, and let the reviewer approve again.

How do I rebase?

Assuming self is your fork and upstream is this repository, you can resolve the conflict following these steps:

git checkout convert-file-uris (switch to your branch)
git fetch upstream master (retrieve the latest master)
git rebase upstream/master -p (rebase on top of it)
Follow the on-screen instruction to resolve conflicts (check git status if you got lost).
git push self convert-file-uris --force-with-lease (update this PR)

You may also read Git Rebasing to Resolve Conflicts by Drew Blessing for a short tutorial.

Please avoid the "Resolve conflicts" button on GitHub. It uses git merge instead of git rebase which makes the PR commit history more difficult to read.

Sometimes step 4 will complete without asking for resolution. This is usually due to difference between how Cargo.lock conflict is handled during merge and rebase. This is normal, and you should still perform step 5 to update this PR.

Error message

Auto-merging tests/testsuite/workspaces.rs
Auto-merging tests/testsuite/tool_paths.rs
Auto-merging tests/testsuite/search.rs
CONFLICT (content): Merge conflict in tests/testsuite/search.rs
Auto-merging tests/testsuite/rename_deps.rs
Auto-merging tests/testsuite/registry.rs
CONFLICT (content): Merge conflict in tests/testsuite/registry.rs
Auto-merging tests/testsuite/publish.rs
Auto-merging tests/testsuite/patch.rs
CONFLICT (content): Merge conflict in tests/testsuite/patch.rs
Auto-merging tests/testsuite/overrides.rs
CONFLICT (content): Merge conflict in tests/testsuite/overrides.rs
Auto-merging tests/testsuite/install.rs
CONFLICT (content): Merge conflict in tests/testsuite/install.rs
Auto-merging tests/testsuite/git.rs
Auto-merging tests/testsuite/doc.rs
Auto-merging tests/testsuite/directory.rs
Auto-merging tests/testsuite/cross_publish.rs
CONFLICT (content): Merge conflict in tests/testsuite/cross_publish.rs
Auto-merging tests/testsuite/build_script.rs
Auto-merging tests/testsuite/build.rs
Auto-merging tests/testsuite/alt_registry.rs
CONFLICT (content): Merge conflict in tests/testsuite/alt_registry.rs
Auto-merging src/cargo/core/source/source_id.rs
CONFLICT (content): Merge conflict in src/cargo/core/source/source_id.rs
Automatic merge failed; fix conflicts and then commit the result.

bors · 2018-09-09T22:07:44Z

☔ The latest upstream changes (presumably #5990) made this pull request unmergeable. Please resolve the merge conflicts.

zachlute · 2018-09-10T18:52:03Z

@alexcrichton This one should be good to go again if anybody wants to get this on the path to integration before somebody changes all the same strings I did again. :)

alexcrichton · 2018-09-10T18:59:14Z

@bors: r+

Of course!

bors · 2018-09-10T18:59:15Z

📌 Commit 730d07c has been approved by alexcrichton

bors · 2018-09-10T18:59:23Z

⌛ Testing commit 730d07c with merge 43d6198f984f1128bb3b493a08edd2f88f0c81ec...

dwijnand · 2018-09-10T19:37:21Z

Sorry for the clash.

bors · 2018-09-10T20:59:23Z

💥 Test timed out

zachlute · 2018-09-10T22:48:15Z

Oh no, what does that mean? There doesn't seem to be any way to view a log or anything to know what it was doing.

alexcrichton · 2018-09-10T22:56:07Z

@bors: retry

ah we just gotta try again

bors · 2018-09-10T22:56:14Z

⌛ Testing commit 730d07c with merge 32d73f85305ef4681b0dd24beda42263713cd8b0...

bors · 2018-09-11T00:56:14Z

💥 Test timed out

alexcrichton · 2018-09-11T01:24:34Z

@bors: retry

...

bors · 2018-09-11T01:24:42Z

⌛ Testing commit 730d07c with merge 943eedbb3d19bf781270dea00cc090871cfd0617...

bors · 2018-09-11T03:24:42Z

💥 Test timed out

alexcrichton · 2018-09-11T05:11:38Z

@bors: retry

bors · 2018-09-11T05:11:46Z

⌛ Testing commit 730d07c with merge c1c4fc2...

Print file paths instead of file:// URLs. Fixes #4661. This change ensures cargo will output file paths in the expected format (C:\foo\... on Windows, /foo/... elsewhere). Previously it would output file:// URLs instead. To support this change, additional changes were made to the test suite string processing such that [ROOT] is now replaced with the appropriate file path root for the platform. The CWD template was also updated to use [CWD] like other replacement templates and to do the replacement on the expected value rather than the actual value to avoid replacing things we don't expect with CWD.

bors · 2018-09-11T05:49:01Z

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing c1c4fc2 to master...

zachlute · 2018-09-11T05:49:43Z

Woohoo!

rust-highfive assigned matklad Sep 8, 2018

dwijnand approved these changes Sep 8, 2018

View reviewed changes

zachlute added 3 commits September 9, 2018 16:48

Resolve merge conflicts with test string changes.

b020d37

Fixed CWD templates in tool_path tests.

ad6bc8e

Resolve conflicts from upstream.

730d07c

zachlute force-pushed the convert-file-uris branch from a5a95fe to 730d07c Compare September 10, 2018 00:04

bors merged commit 730d07c into rust-lang:master Sep 11, 2018

zachlute deleted the convert-file-uris branch September 11, 2018 14:51

ehuss added this to the 1.31.0 milestone Feb 6, 2022

Print file paths instead of file:// URLs. #5994

Print file paths instead of file:// URLs. #5994

Uh oh!

Conversation

zachlute commented Sep 8, 2018

Uh oh!

rust-highfive commented Sep 8, 2018

Uh oh!

zachlute commented Sep 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zachlute commented Sep 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dwijnand left a comment

Choose a reason for hiding this comment

Uh oh!

dwijnand commented Sep 8, 2018

Uh oh!

alexcrichton commented Sep 9, 2018

Uh oh!

bors commented Sep 9, 2018

Uh oh!

bors commented Sep 9, 2018

Uh oh!

bors commented Sep 9, 2018

Uh oh!

zachlute commented Sep 10, 2018

Uh oh!

alexcrichton commented Sep 10, 2018

Uh oh!

bors commented Sep 10, 2018

Uh oh!

bors commented Sep 10, 2018

Uh oh!

dwijnand commented Sep 10, 2018

Uh oh!

bors commented Sep 10, 2018

Uh oh!

zachlute commented Sep 10, 2018

Uh oh!

alexcrichton commented Sep 10, 2018

Uh oh!

bors commented Sep 10, 2018

Uh oh!

bors commented Sep 11, 2018

Uh oh!

alexcrichton commented Sep 11, 2018

Uh oh!

bors commented Sep 11, 2018

Uh oh!

bors commented Sep 11, 2018

Uh oh!

alexcrichton commented Sep 11, 2018

Uh oh!

bors commented Sep 11, 2018

Uh oh!

bors commented Sep 11, 2018

Uh oh!

zachlute commented Sep 11, 2018

Uh oh!

Uh oh!

zachlute commented Sep 8, 2018 •

edited

Loading

zachlute commented Sep 8, 2018 •

edited

Loading