-
Notifications
You must be signed in to change notification settings - Fork 442
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
Use a search list to find a compatible toolchain #521
Conversation
3f9778c
to
45446e8
Compare
Also, I have a small test project that I put together. Nothing fancy, but it does show that it works. Is this the sort of thing that would be useful to have in CI, or not (because it's a tier 3 target)? |
Nah I'm always happy to try to add more CI here so long as it doesn't put much of a burden on maintenance! |
b4e50a7
to
e578fba
Compare
src/lib.rs
Outdated
.map(|host| host.contains("windows")) | ||
.unwrap_or(false); | ||
let suffix = if self.cpp { "-g++" } else { "-gcc" }; | ||
let extension = if windows_host { ".exe" } else { "" }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this can be replaced with std::env::consts::EXE_SUFFIX
perhaps?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had no idea this was a thing. Thanks for pointing it out.
src/lib.rs
Outdated
{ | ||
return Some(*prefix); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could the order of the loops be inverted here? That way if /usr/bin
is near the front of PATH
we won't have to search for everything everywhere that doesn't exist.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've made this change as requested.
src/lib.rs
Outdated
.as_ref() | ||
.and_then(|path_entries| { | ||
env::split_paths(path_entries) | ||
.find(|path_entry| path_entry.join(&target_compiler).exists()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.find(..).is_some()
can be replaced with .any(...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I also didn't know about this construct. However, as part of inverting the two loops, this ended up being unnecessary. And in fact the whole thing got collapsed down to a single statement.
Add `find_working_gnu_prefix()` to iterate through a slice of potential compiler prefixes, and return the first one that succeeds. This is useful when there is not yet a single clear toolchain prefix, for example with riscv (riscv64-unknown-none-elf vs riscv32-unknown-none-elf vs riscv-none-embed). Signed-off-by: Sean Cross <sean@xobs.io>
riscv currently has several competing compiler toolchain prefixes. If a toolchain supports multilib, then an "incorrect" target triple may be used. For example, many distributions ship multilib toolchains with a `riscv64` prefix, so a riscv32imac-unknown-none-elf target can successfully use a toolchain with a triple of `riscv64-unknown-none`. Signed-off-by: Sean Cross <sean@xobs.io>
e578fba
to
988b382
Compare
👍 |
This adds toolchain detection support and activates it for riscv embedded targets. This enables
cc
to search for compatible toolchains using a whitelist of targets.For example, the following builds a test project using
riscv64-unknown-elf-gcc
when targetingriscv32imac-unknown-none-elf
:This closes #519