Skip to content

Fallback to unknown vendor with vendored target triples #55368

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

Closed
wants to merge 1 commit into from

Conversation

zachreizner
Copy link
Contributor

When a target triple is searched for, only exact matches with be
returned. This change will fallback to an "unknown" vendor in the
target triple if one with the specific vendor can not be found.

This helps with #41402.

@rust-highfive
Copy link
Contributor

r? @matthewjasper

(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 Oct 25, 2018
@zachreizner
Copy link
Contributor Author

r? @japaric

@TimNN
Copy link
Contributor

TimNN commented Nov 6, 2018

Ping from triage @japaric: This PR requires your review.

@zachreizner
Copy link
Contributor Author

r? @dtolnay

David has volunteered to take a look at this.

@rust-highfive rust-highfive assigned dtolnay and unassigned japaric Nov 15, 2018
When a target triple is searched for, only exact matches with be
returned. This change will fallback to an "unknown" vendor in the
target triple if one with the specific vendor can not be found.
},
)+
_ => Err(format!("Unable to find target: {}", target))
Copy link
Member

Choose a reason for hiding this comment

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

I believe this would break anyone currently successfully using a non-unknown vendor with their own JSON target file. The code before would use their JSON file. The new code would prefer the corresponding builtin target with unknown vendor while ignoring their JSON file.

// check if triple is in list of supported targets
if let Ok(t) = load_specific(target_triple) {
return Ok(t)
}
// search for a file named `target_triple`.json in RUST_TARGET_PATH
let path = {
let mut target = target_triple.to_string();
target.push_str(".json");
PathBuf::from(target)
};
let target_path = env::var_os("RUST_TARGET_PATH").unwrap_or_default();
// FIXME 16351: add a sane default search path?
for dir in env::split_paths(&target_path) {
let p = dir.join(&path);
if p.is_file() {
return load_file(&p);
}
}

I think the fallback to unknown would be more appropriate if applies only after an exact match JSON file has not been found.

}
let mut triple: Vec<&str> = target.splitn(3, '-').collect();
if triple.len() >= 3 {
if triple[1] != "unknown" {
Copy link
Member

Choose a reason for hiding this comment

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

I would write this as:

if triple.len() >= 3 && triple[1] != "unknown" {

let mut triple: Vec<&str> = target.splitn(3, '-').collect();
if triple.len() >= 3 {
if triple[1] != "unknown" {
triple[1] = "unknown";
Copy link
Member

@dtolnay dtolnay Nov 15, 2018

Choose a reason for hiding this comment

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

Please add an explanation of what triple[1] represents and why this fallback makes sense. Right now it requires reading the PR description and linked issue to understand what is going on. For example just from the code it isn't clear why there wouldn't also be a fallback for triple[2] being unknown.

if triple.len() >= 3 {
if triple[1] != "unknown" {
triple[1] = "unknown";
return load_specific(&triple.join("-"))
Copy link
Member

Choose a reason for hiding this comment

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

Please add a test. Possibly as simple as:

#[test]
fn test_load_specific() {
    let cros_target = load_specific("x86_64-cros-linux-gnu").unwrap();
    assert_eq!(cros_target.llvm_target, "x86_64-unknown-linux-gnu");
}

@dtolnay dtolnay 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-review Status: Awaiting review from the assignee but also interested parties. labels Nov 25, 2018
@TimNN
Copy link
Contributor

TimNN commented Dec 4, 2018

Ping from triage @zachreizner: It looks like some changes have been requested to this PR.

@zachreizner
Copy link
Contributor Author

I've been talking it over with an expert in the murky field of target triples and I'm not sure the solution I've started in this PR is the best way forward. It's a band-aid solution that isn't quite right, but happens to work. The proper solution would be to replicate the behavior from config.sub of the gnu config project. The approach there is to parse the target tuple, itself an organic accretion of decades of behavior, and then make intelligent choices based on os, vendor, arch, and abi. It is that latter half which would be hard to adapt to rustc's dictionary based approach to target configuration. It isn't possible to use a dictionary of target tuples in any intelligent fashion that doesn't look like band-aid solutions like in this PR. My disposition would be to abandon this PR as I don't have the time to refactor how rustc deals with target tuples, and it would seem rustc's maintainers are unlikely to accept such a refactor anyways.

@dtolnay dtolnay closed this Dec 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants