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

Introduce a TargetTriple enum to support absolute target paths #49019

Merged
merged 3 commits into from
Mar 28, 2018
Merged

Introduce a TargetTriple enum to support absolute target paths #49019

merged 3 commits into from
Mar 28, 2018

Conversation

phil-opp
Copy link
Contributor

This PR replaces target triple strings with a TargetTriple enum, which represents either a target triple or a path to a JSON target file. The path variant is used if the --target argument has a .json extension, else the target triple variant is used.

The motivation of this PR is support for absolute target paths to avoid the need for setting the RUST_TARGET_PATH environment variable (see rust-lang/cargo#4905 for more information). For places where some kind of triple is needed (e.g. in the sysroot folder), we use the file name (without extension).

For compatibility, we keep the old behavior of searching for a file named $(target_triple).json in RUST_TARGET_PATH for non-official target triples.

@rust-highfive
Copy link
Collaborator

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 14, 2018
@oli-obk
Copy link
Contributor

oli-obk commented Mar 15, 2018

Travis failure is legit

@phil-opp
Copy link
Contributor Author

Thanks for the hint, I pushed a new version.

@pnkfelix
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Mar 19, 2018

📌 Commit 4b16597 has been approved by pnkfelix

@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 Mar 19, 2018
@phil-opp
Copy link
Contributor Author

@pnkfelix I added a commit that appends a hash of the target path when a TargetPath is converted to a triple. This avoids collisions when two target files in different folders are named identically. It also allows to differentiate targets with the same name in error messages. For example:

error[E0461]: couldn't find crate `core` with expected target triple stm32f7-11703095894775286769
  |
  = note: the following crate versions were found:
          crate `core`, target triple stm32f7-10929095608748322303

@phil-opp
Copy link
Contributor Author

Rebased and fixed some target triple uses introduced by other PRs.

This was referenced Mar 22, 2018
Merged
Closed
Open
@bors
Copy link
Contributor

bors commented Mar 23, 2018

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

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Mar 23, 2018
@phil-opp
Copy link
Contributor Author

Rebased

bors added a commit to rust-lang/cargo that referenced this pull request Mar 26, 2018
@bors
Auto merge of #5228 - phil-opp:target-spec, r=alexcrichton
715fc78 
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.
@pnkfelix
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Mar 28, 2018

📌 Commit b889f98 has been approved by pnkfelix

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 28, 2018
@bors
Copy link
Contributor

bors commented Mar 28, 2018

⌛ Testing commit b889f98 with merge d87c19d...

bors added a commit that referenced this pull request Mar 28, 2018
@bors
Auto merge of #49019 - phil-opp:target-spec, r=pnkfelix
d87c19d 
Introduce a TargetTriple enum to support absolute target paths

This PR replaces target triple strings with a `TargetTriple` enum, which represents either a target triple or a path to a JSON target file. The path variant is used if the `--target` argument has a `.json` extension, else the target triple variant is used.

The motivation of this PR is support for absolute target paths to avoid the need for setting the `RUST_TARGET_PATH` environment variable (see rust-lang/cargo#4905 for more information). For places where some kind of triple is needed (e.g. in the sysroot folder), we use the file name (without extension).

For compatibility, we keep the old behavior of searching for a file named `$(target_triple).json` in `RUST_TARGET_PATH` for non-official target triples.
@bors
Copy link
Contributor

bors commented Mar 28, 2018

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

@bors bors merged commit b889f98 into rust-lang:master Mar 28, 2018
This was referenced Mar 28, 2018
Merged
Merged
@phil-opp phil-opp deleted the target-spec branch March 28, 2018 15:57
@phil-opp phil-opp mentioned this pull request May 7, 2018
Merged
@ehuss ehuss mentioned this pull request Jul 20, 2019
Open
@Mark-Simulacrum Mark-Simulacrum mentioned this pull request Apr 10, 2020
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@pnkfelix pnkfelix

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
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@phil-opp @rust-highfive @oli-obk @pnkfelix @bors