Skip to content

Use the existing path when removing the prefix fails #41170

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.

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

Merged
merged 1 commit into from
Apr 19, 2017

Conversation

Nashenas88
Copy link
Contributor

This allows the use of out-of-tree paths to be specified. I found this while trying to build with a modified version of rls-data, which is currently pointing to a version on crates.io.

cc @alexcrichton

Also, it wasn't clear if I needed to add a test for this (or how). I didn't see any tests that took paths into consideration.

@rust-highfive
Copy link
Contributor

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

@alexcrichton
Copy link
Member

Thanks for the PR! This looks right to me but I'm not quite sure how it came up. Can you elaborate as to the use case that caused this to panic?

@Nashenas88
Copy link
Contributor Author

Sure thing. I'm working on modifications to rls-data, which is currently specified through a version on crates.io. My approach was to download rls-data locally, make changes and point the compiler to my local copy. In the Cargo.toml in librustc_save_analysis, I specified this new dependency as rls-data = { path = "../../../rls-data" }. Building with this caused the panic. I don't think these should be supported for official builds, but for dev it seemed important, especially as more crates come from crates.io.

@Nashenas88
Copy link
Contributor Author

On IRC I was asking whether or not this is a change that was needed or not. While it's helpful for me, I wanted to make sure you guys also agreed.

@Mark-Simulacrum Mark-Simulacrum added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 14, 2017
@carols10cents
Copy link
Member

@alexcrichton (I know you're traveling this week, just keeping this on your radar)

On IRC I was asking whether or not this is a change that was needed or not. While it's helpful for me, I wanted to make sure you guys also agreed.

Wdyt alex?

@alexcrichton
Copy link
Member

@Nashenas88 gah sorry for the delay, got caught up in a lot of travel! In any case that sounds like a perfect use case to me, thanks for the PR!

@bors: r+

@bors
Copy link
Collaborator

bors commented Apr 19, 2017

📌 Commit a6f7628 has been approved by alexcrichton

@alexcrichton alexcrichton 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 Apr 19, 2017
@bors
Copy link
Collaborator

bors commented Apr 19, 2017

⌛ Testing commit a6f7628 with merge 5f22d46...

bors added a commit that referenced this pull request Apr 19, 2017
Use the existing path when removing the prefix fails

This allows the use of out-of-tree paths to be specified. I found this while trying to build with a modified version of `rls-data`, which is currently pointing to a version on crates.io.

cc @alexcrichton

Also, it wasn't clear if I needed to add a test for this (or how). I didn't see any tests that took paths into consideration.
@bors
Copy link
Collaborator

bors commented Apr 19, 2017

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

@bors bors merged commit a6f7628 into rust-lang:master Apr 19, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants