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.

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

Closed
wants to merge 1 commit into from
Closed

Fallback to unknown vendor with vendored target triples #55368

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 10 additions & 2 deletions src/librustc_target/spec/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -247,11 +247,19 @@ macro_rules! supported_targets {
// run-time that the parser works correctly
t = Target::from_json(t.to_json())?;
debug!("Got builtin target: {:?}", t);
Ok(t)
return Ok(t)
},
)+
_ => 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.

rust/src/librustc_target/spec/mod.rs

Lines 1101 to 1122 in cc4999e

// 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" {

triple[1] = "unknown";
Copy link
Member

@dtolnay dtolnay Nov 15, 2018 •
edited
Loading

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.

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");
}

}
}
Err(format!("Unable to find target: {}", target))
}

pub fn get_targets() -> Box<dyn Iterator<Item=String>> {
Expand Down