-
Notifications
You must be signed in to change notification settings - Fork 482
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
Parse rustc
target names
#1413
base: main
Are you sure you want to change the base?
Parse rustc
target names
#1413
Conversation
4d0ce6c
to
c9d5ddd
Compare
c9d5ddd
to
08cab75
Compare
08cab75
to
2529f3c
Compare
But keep using a generated LLVM/Clang triple mapping for now.
2529f3c
to
2e98453
Compare
Hmmm the solution here is more complex than I'd imagined. I was thinking of simply splitting target, and replace alpine/* with unknown if os is linux |
Yeah, and that approach would solve the Alpine/Chimera issues, but won't solve the problem people have otherwise been having regarding Rust bootstrapping and custom/new targets, see rust-lang/rust#137064 / rust-lang/rust#137460 (bootstrap has downgraded to Personally, I'm still a little bit on the fence about this, it'd be really nice if we could just only support upstream targets. But ultimately, I don't think that's a good decision for the Rust project as a whole. |
WDYT @ChrisDenton? I get the feeling you're more in touch with other |
For completeness, I'll note that there are other options, I just dislike them even more than this:
|
I got it, but it feels like this PR has hard coded so many stuff and make maintaining harder I still prefer some of the information to be generated from rustc or somewhere else instead of hardcoding everything, maybe we can modify gen-target-info to check env/abi etc for non-straight forward mappings and put them in a sorted array? |
From a rustc perspective, we do want cc-rs to work with new/custom targets as well as possible. Though that doesn't necessarily mean accepting any arbitrary target string. There could be at least some rules if it helps. Also there are perhaps some optimizations here. How much of the target info do we actually use? I suspect we only care about the targets we have special cases for, e.g. Discerning the llvm target is a bit trickier. Though I think in part it's made worse by rustc itself not always being consistent (or even accurate, particularly for tier 3 targets). That is something that could be improved on the rustc side. I'd also add that llvm deals with parts of the triple it doesn't understand by replacing them with "unknown". I'm not sure if we can/should take advantage of that but I thought I should note it. |
Co-authored-by: Jiahao XU <30436523+NobodyXu@users.noreply.github.com>
Co-authored-by: Jiahao XU <30436523+NobodyXu@users.noreply.github.com>
I don't like it either, but it really seems like the only option to me.
Hmm, not a bad idea, though it wouldn't help to reduce the majority of the work here, it would only really help for this part of the parsing (e.g. The bigger problem is that "non-straight forward mappings" is hard to define, since the mapping isn't defined at all!
I doubt
Probably true, I think
Indeed, and I plan to do that in the future (but wanted to land this PR first). |
It has as much sway as the rest of the project, The best thing would be to make an MCP (or maybe it'd require an RFC) to specify some constraints we'd want or otherwise define how to choose target tuples for new targets. Otherwise it would be a case of monitoring new target MCPs and raising objections if their naming isn't great (which would be more hassle at the end of the day). |
CC @workingjubilee, you've been fairly involved with Core assumptions about
|
Parse target names instead of using a pre-generated list. Allows target names like those used by certain Linux distributions, even when compiling outside
build.rs
.I feel fairly confident that the target parsing is complete, and that the parsing approach taken (described in the code, but roughly "parse arch, then env/ABI, then OS, and lastly vendor") is correct.
Unfortunately, since this information is now parsed instead of pre-generated, we cannot just update the information automatically with a PR. Instead, I've added a CI step that opens an issue comment in #1426 whenever a nightly
rustc
target names fails to parse correctly. This isn't as nice, but it's at least workable.Note that ensuring we construct the same LLVM triples as
rustc
is a lot more work; I've started on it a little bit, but it will have to be postponed (it probably also requires a lot of tweaks torustc
to really be nice). So for now, I opted to keep a pre-generated list ofrustc
to LLVM triples.Fixes #1297.
Fixes #1317.
Fixes #1405.
Fixes #1407.