Add support for absolute target.json paths #5228
Conversation
|
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. |
src/cargo/ops/cargo_compile.rs
Outdated
| &Some(ref target) if target.ends_with(".json") => { | ||
| let path = Path::new(target) | ||
| .canonicalize() | ||
| .map_err(|_| format_err!("Target path {:?} is not a valid file", target))?; |
There was a problem hiding this comment.
Could this use chain_err to avoid losing the original error?
src/cargo/ops/cargo_rustc/layout.rs
Outdated
| .file_stem() | ||
| .ok_or_else(|| format_err!("target was empty"))?); | ||
| if triple.ends_with(".json") { | ||
| let json_path = Path::new(triple).canonicalize().map_err(|err| { |
There was a problem hiding this comment.
This can use chain_err to avoid explicitly adding the string of err to the error message
| @@ -235,7 +235,17 @@ pub fn compile_ws<'a>( | |||
| ref target_rustc_args, | |||
| } = *options; | |||
|
|
|||
| let target = target.clone(); | |||
| let target = match target { | |||
| &Some(ref target) if target.ends_with(".json") => { | |||
There was a problem hiding this comment.
Is this clause required here? Ideally there'd only be one location where we'd deal with json targets and currently it's split across two locations
There was a problem hiding this comment.
I'm not sure which other location you mean. The Layout::new implementation?
The motivation for putting it here is that we canonicalize the path that is passed to rustc, to avoid the issue that the path becomes invalid due to the cwd changes.
There was a problem hiding this comment.
Oh sure I didn't look too too closely before, I was mostly just thinking that we should have one location where we recognize .json targets instead of two, but the most recent version only has one so that sounds good to me!
src/cargo/ops/cargo_rustc/layout.rs
Outdated
| json_path.hash(&mut hasher); | ||
| let hash = hasher.finish(); | ||
|
|
||
| path.push(Path::new(&format!("{}-{}", file_stem, hash))); |
There was a problem hiding this comment.
Hm are we sure that this is what we want to do? If we did this I think we may run a risk of not being able to easily discover the output filename, right?
There was a problem hiding this comment.
Oh, good point. I'll push a new version that keeps the old output folder names.
|
I pushed a new version that keeps the output folder name as before. I didn't perform and end-to-end test with xargo yet, as I have to adjust the rustc and xargo PRs first. I will post an update here then.
A test that the path is canonicalized? |
Oh I was moreso thinking of one that uses a |
|
That makes sense. I added two basic tests. |
|
The rustc and xargo PRs are now updated as well and I verified that they work together. |
|
Looks great! I think there's a formatting failure on CI but otherwise r=me |
|
Fixed the formatting error and cleaned up the commits. |
|
@bors: r+ |
|
📌 Commit 6ab7019 has been approved by |
Add support for absolute target.json paths Builds upon rust-lang/rust#49019 with the goal to provide a solution to #4905. This PR does two things: ~~1. It appends a hash of the target path to the target folder name if a `*.json` path is passed as `--target`, like it's done in rust-lang/rust#49019. This helps differentiating targets with the same JSON file name and avoids sysroot clashes in `xargo`.~~ See #5228 (comment) 2. It canonicalizes the passed target path (if it's a `*.json` path), so that the path stays valid when building dependencies and setting the `RUST_TARGET_PATH` environment variable is no longer necessary.
|
☀️ Test successful - status-appveyor, status-travis |
Builds upon rust-lang/rust#49019 with the goal to provide a solution to #4905.
This PR does two things:
1. It appends a hash of the target path to the target folder name if aSee #5228 (comment)*.jsonpath is passed as--target, like it's done in rust-lang/rust#49019. This helps differentiating targets with the same JSON file name and avoids sysroot clashes inxargo.2. It canonicalizes the passed target path (if it's a
*.jsonpath), so that the path stays valid when building dependencies and setting theRUST_TARGET_PATHenvironment variable is no longer necessary.