Discussion: Reconsider target parsing

[madsmtm]

I'm beginning to regret the decision we took in #1225 (comment) to pre-generate a static list of target information from the current nightly, instead of trying to guess it from the target name.

The gist of the issue is that static data doesn't help us much when using a custom target - but when using custom targets, we usually still need to default to some value (example: the LLVM target). Also, I don't like that you can sometimes use cc in a build script, but not outside.

And yeah, rustc target names are a mess, and they're impossible to parse correctly in general, but we can do a fairly good job at it (that's what I implemented in that PR initially) - especially if we have CI that verifies the parsing code against the latest rustc specs. Of course, manual parsing code is more tedious to update, i.e. the automated GitHub PRs would've instead been turned into a CI failure that we'd have to fix.

Idk., still don't know how I feel about it, but I do know that I was unhappy with #1315, as I felt I kinda had to get the default relocation model guessing correct, and once it was correct, there wasn't any point in the pre-generated data.

Related issues:

• Better error message when failing on unknown targets #1313
• Support custom target-spec #1120
• New cargo target detection regress *-esp-idf target builds. #1262
• Reduce generated target info compile-time/binary size #1272
• Cross-compilation to Yocto breaks after #1225 (1.1.32) #1297
• rustc custom target compile is broken #1405
• *-alpine-linux-musl custom targets #1407

[NobodyXu]

The gist of the issue is that static data doesn't help us much when using a custom target - but when using custom targets, we usually still need to default to some value (example: the LLVM target).

Maybe we can require environment variables to be set for customs?

We could use cc::Build::envs to avoid unsafe set_var

[tronical]

As one of the affected parties, I think what surprised me most is that this came in a minor version and broke existing builds (#1297). I think the change itself makes a lot of sense, but it would have been a lot easier if this was in a new major version so that the transition can be controlled.

Just my two cents. Absolutely love the crate and how responsive you all are as maintainers.

[lasiotus]

Yes, please reconsider target parsing!

Context: Motor OS is a custom Rust target at the moment (not an official Tier-3 target), so it is not part of the pre-generated list of "targets known to rustc". The switch to pre-generated targets keeps us on an older Rust version, which is somewhat unfortunate.

Maybe do target parsing for targets not found in the static/pregenerated list?

Related issue: moturus/motor-os#18

[NobodyXu]

Maybe do target parsing for targets not found in the static/pregenerated list?

I think that'd be reasonable, but shouldn't cargo already pass environment variables that we could use https://docs.rs/cc/latest/src/cc/target/parser.rs.html

cc @lasiotus

[lasiotus]

[...] shouldn't cargo already pass environment variables that we could use https://docs.rs/cc/latest/src/cc/target/parser.rs.html

At the moment rustc/stdlib bootstrap calls get_compiler() which kills the process if the target is not recognized.

Essentially we have here a circular dependency: Rust's target bootstrap depends on cc recognizing the target, which does so only for "targets recognized by rustc", if I understand the logic here correctly.

So unofficial targets are now a bit more painful to maintain/work on.

[NobodyXu]

At the moment rustc/stdlib bootstrap calls get_compiler() which kills the process if the target is not recognized.

Aha so it's not a build-script issue...

Maybe you could set CARGO_* manually?

https://docs.rs/cc/latest/src/cc/target/parser.rs.html

[lasiotus]

Maybe you could set CARGO_* manually?

It seems this should work - I'll try a bit later; thanks for the suggestion!

[lasiotus]

Maybe you could set CARGO_* manually?

It seems this should work - I'll try a bit later; thanks for the suggestion!

Doesn't work: rustc bootstrap code sets Build::target string triple prior to calling get_compiler(); but get_compiler() -> get_target() parses cargo env vars only if the target string is not set. The latest cc-rs version even explicitly advises forking itself for unrecognized targets. So now new targets need to fork both rust and cc-rs. Again, seems like a circular dependency...

Probably the bootstrap code can be hacked to not set the target string triple before calling get_compiler(), but this is becoming increasingly fragile...

[NobodyXu]

Doesn't work: rustc bootstrap code sets Build::target string triple prior to calling get_compiler();

As long as value set by Build::target matches the env variable TARGET it would take the CARGO_* env parsing step

[lasiotus]

As long as value set by Build::target matches the env variable TARGET it would take the CARGO_* env parsing step

That worked, thanks!

As it works only at head, and not at 1.2.0 to which Rust bootstrap is pinned, I created a PR to bump the pin to 1.2.4.

[sunshowers]

In case it helps, my former coworker wrote (and I maintain) https://docs.rs/target-spec which has support for builtin, heuristic, and custom (JSON-based) target parsing. Feel free to use it or grab code from there as required.

[RalfJung]

The entire problem here is that rustc --print target-spec-json is unstable, right? If that was stable you could just run rustc with suitable flags and parse the JSON to get the required info in a structured way?

But ofc we don't really want to commit to that JSON format so this is unstable for a good reason...

[NobodyXu]

The entire problem here is that rustc --print target-spec-json is unstable, right?

If we are running cc-rs on a nightly toolchain, perhaps we could run this instead of using pre-parsed information?

The only problem with this is that we would need a json parser now

[madsmtm]

The entire problem here is that rustc --print target-spec-json is unstable, right?

rustc --print cfg would probably be an option for most of what we need today.

[RalfJung]

Ah, that does work on stable and should also be easier to parse than JSON. :)

[madsmtm]

My current preference would be to do target parsing, and not call rustc --print cfg except for in tests. Possibly with a script / GitHub action that periodically opens a new issue if the test doesn't work.

I'll volunteer to implementing that, but I'll let @NobodyXu make a decision here first.

[NobodyXu]

My current preference would be to do target parsing, and not call rustc --print cfg except for in tests.

Looks good to me, this would fix the -alpine-linux-