Convert built-in targets to JSON #34980

cardoe · 2016-07-22T20:32:49Z

Convert the built-in targets to JSON to ensure that the JSON parser is always fully featured. This follows on #32988 and #32847. The PR includes a number of extra commits that are just intermediate changes necessary for bisectibility and the ability to prove correctness of the change.

rust-highfive · 2016-07-22T20:33:03Z

r? @nrc

(rust_highfive has picked a reviewer for you, use r? to override)

cardoe · 2016-07-22T20:33:22Z

CC @alexcrichton since we've talked about this a few times.

cardoe · 2016-07-22T20:45:52Z

I can delete commits or squash things together but @jcreekmore and I wanted to provide all the pieces along the way for the review process.

alexcrichton · 2016-07-22T23:32:53Z

src/librustc_back/target/mod.rs

-
-    ("le32-unknown-nacl", le32_unknown_nacl),
-    ("asmjs-unknown-emscripten", asmjs_unknown_emscripten)
+    ("x86_64-unknown-linux-gnu", "json/x86_64-unknown-linux-gnu.json"),


Now that these are just strings and all have the same name, you may be able to change the include to:

include_str!(concat!("json/", $target, ".json"))

And that way each target only needs to be listed here once

brson · 2016-07-22T23:47:26Z

Yeah, losing the comments seems pretty bad, but switching to TOML kinda defeats the motivation of making sure the JSON parser works. Unless they share sufficient code to give us confidence in both. If that's the case then I'm fine doing them in TOML, as long as TOML target specs aren't exposed publicly.

Another approach here that is quite simple might be to just roundtrip every built-in target spec through JSON before using it.

alexcrichton · 2016-07-23T00:00:11Z

@brson I like the idea of round tripping all built-in specs, that way we get to keep the commends and in essence always exercise the json parser.

@cardoe does that make sense?

jcreekmore · 2016-07-23T00:03:05Z

That makes sense and should be fairly straightforward to do.

On Jul 22, 2016, at 7:00 PM, Alex Crichton [email protected] wrote:

@brson I like the idea of round tripping all built-in specs, that way we get to keep the commends and in essence always exercise the json parser.

@cardoe does that make sense?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

cardoe · 2016-07-23T01:22:45Z

@alexcrichton That works. We'll get that rebased in.

nagisa · 2016-07-23T12:14:52Z

I wanted to write up an RFC on this. I feel like this PR as it is currently is lacking somewhat. Most notably:

It introduces repetition. There should be some way to share parts of spec between targets – some form of composition;
Now that built-in targets are not rust code anymore, I feel like having a hardcoded list is very sub-optimal. Instead I would expect us to put targets into some well known location such as $(rustc --print=sysroot)/share/rustc/targets/ and have rustc discover them.
- At least I would prefer such scheme primarily for my target_build_utils crate.

JSON doesn't have comments, but we could perhaps vendor a TOML parser or something like that?

IMO the thing to do here is to deprecate JSON target specs in favour of whatever better format over a long period of time.

jcreekmore · 2016-07-23T12:56:42Z

@alexcrichton and @brson, I rebased the branch so that it adds unit tests for encoding and decoding all of the builtin targets as well as pushing the builtin targets through the JSON codec at runtime. Is this closer to what you wanted?

jcreekmore · 2016-07-23T16:49:43Z

Huh. Looks like the tests failed on travis because of a lack of ios simulator when I am trying to instantiate the Target for checking the encoding. Thoughts?

failures:
---- target::test_json_encode_decode::aarch64_apple_ios stdout ----
    thread 'target::test_json_encode_decode::aarch64_apple_ios' panicked at 'failed to get iphoneos SDK path: No such file or directory (os error 2)', src/librustc_back/target/apple_ios_base.rs:59
---- target::test_json_encode_decode::armv7_apple_ios stdout ----
    thread 'target::test_json_encode_decode::armv7_apple_ios' panicked at 'failed to get iphoneos SDK path: No such file or directory (os error 2)', src/librustc_back/target/apple_ios_base.rs:59
---- target::test_json_encode_decode::armv7s_apple_ios stdout ----
    thread 'target::test_json_encode_decode::armv7s_apple_ios' panicked at 'failed to get iphoneos SDK path: No such file or directory (os error 2)', src/librustc_back/target/apple_ios_base.rs:59
---- target::test_json_encode_decode::i386_apple_ios stdout ----
    thread 'target::test_json_encode_decode::i386_apple_ios' panicked at 'failed to get iphonesimulator SDK path: No such file or directory (os error 2)', src/librustc_back/target/apple_ios_base.rs:59
---- target::test_json_encode_decode::x86_64_apple_ios stdout ----
    thread 'target::test_json_encode_decode::x86_64_apple_ios' panicked at 'failed to get iphonesimulator SDK path: No such file or directory (os error 2)', src/librustc_back/target/apple_ios_base.rs:59
failures:
    target::test_json_encode_decode::aarch64_apple_ios
    target::test_json_encode_decode::armv7_apple_ios
    target::test_json_encode_decode::armv7s_apple_ios
    target::test_json_encode_decode::i386_apple_ios
    target::test_json_encode_decode::x86_64_apple_ios

nrc · 2016-07-25T01:22:22Z

r? @alexcrichton

alexcrichton · 2016-07-25T06:44:56Z

Oh that's actually because they just can't be loaded anywhere but somewhere with the xcrun command. It should be ok for now to ignore those targets, but could you file an issue and tag it with a FIXME indicating that the failure to find xcrun should be more robust?

cardoe · 2016-07-25T15:37:07Z

@alexcrichton One idea to fix it would be to change all the fn target() -> Target to fn target() -> Result<Target, String>. The reason for Result<Target, String> is that librustc_back internally uses that type and all the target() calls are always wrapped with Ok().

I've roughly done this in cardoe@d16b7d0 if that sounds reasonable we can rebase that into this branch or clean it up and make it its own PR.

alexcrichton · 2016-07-27T00:54:20Z

@cardoe sounds reasonable to me!

bors · 2016-07-27T17:20:17Z

☔ The latest upstream changes (presumably #33363) made this pull request unmergeable. Please resolve the merge conflicts.

Not all TargetOptions are exposed via the JSON interface to create different targets. This exposes all the missing items and reorders them to match the structure so that it is easier in the future to identify missing items. Signed-off-by: Doug Goldstein <[email protected]>

Target's can already be built up from JSON files as well as built into librustc_back so this adds the ability to convert any Target back into JSON. Signed-off-by: Doug Goldstein <[email protected]>

Change all the target generation functions to return a Result<Target, String> so that targets that are unable to be instantiated can be expressed as an Err instead of a panic!(). This should improve rust-lang#33497 as well.

Expand the supported_targets!() macro to also generate a set of JSON encode/decode tests to verify that the parser will encode and decode all of the fields needed for all of the builtin targets. Additionally, add PartialEq to Target and TargetOptions in support of the tests.

Since we can know which targets are instantiable on a particular host, it does not make sense to list invalid targets in the target print code. Filter the list of targets to only include the targets that can be instantiated.

cardoe · 2016-07-27T20:17:34Z

@alexcrichton This should be good to go now. It should also resolve the issues that people reported in the referenced issues as well.

alexcrichton · 2016-07-28T15:56:15Z

@bors: r+ 54c61ff

Awesome, thanks @cardoe and @jcreekmore!

bors · 2016-07-29T06:18:53Z

⌛ Testing commit 54c61ff with merge 1523a54...

Convert built-in targets to JSON Convert the built-in targets to JSON to ensure that the JSON parser is always fully featured. This follows on #32988 and #32847. The PR includes a number of extra commits that are just intermediate changes necessary for bisectibility and the ability to prove correctness of the change.

bors · 2016-07-29T10:27:59Z

…henkov Defer Apple SDKROOT detection to link time. This defers the detection of the SDKROOT for Apple iOS/tvOS targets to link time, instead of when the `Target` is defined. This allows commands that don't need to link to work (like `rustdoc` or `rustc --print=target-list`). This also makes `--print=target-list` a bit faster. This also removes the note in the platform support documentation about these targets being missing. When I wrote it, I misunderstood how the SDKROOT stuff worked. Notes: * This means that JSON spec targets can't explicitly override these flags. I think that is probably fine, as I believe the value is generally required, and can be set with the SDKROOT environment variable. * This changes `x86_64-apple-tvos` to use `appletvsimulator`. I think the original code was wrong (it was using `iphonesimulator`). Also, `x86_64-apple-tvos` seems broken in general, and I cannot build it locally. The `data_layout` does not appear to be correct (it is a copy of the arm64 layout instead of the x86_64 layout). I have not tried building Apple's LLVM to see if that helps, but I suspect it is just wrong (I'm uncertain since I don't know how the tvOS simulator works with its bitcode-only requirements). * I'm tempted to remove the use of `Result` for built-in target definitions, since I don't think they should be fallible. This was added in rust-lang#34980, but that only relates to JSON definitions. I think the built-in targets shouldn't fail. I can do this now, or not. Fixes rust-lang#36156 Fixes rust-lang#76584

rust-highfive assigned nrc Jul 22, 2016

alexcrichton reviewed Jul 22, 2016
View reviewed changes

jcreekmore force-pushed the expose-target-options branch from 8c54a6a to e1e95a4 Compare July 23, 2016 12:55

rust-highfive assigned alexcrichton and unassigned nrc Jul 25, 2016

cardoe force-pushed the expose-target-options branch from e1e95a4 to 436c467 Compare July 27, 2016 15:17

cardoe and others added 5 commits July 27, 2016 10:24

librustc_back: add ToJson trait to Target

c7b6ed2

Target's can already be built up from JSON files as well as built into librustc_back so this adds the ability to convert any Target back into JSON. Signed-off-by: Doug Goldstein <[email protected]>

librustc_back: convert fn target() to return Result

eafecbf

Change all the target generation functions to return a Result<Target, String> so that targets that are unable to be instantiated can be expressed as an Err instead of a panic!(). This should improve rust-lang#33497 as well.

librustc_back: filter targets for only valid ones

54c61ff

Since we can know which targets are instantiable on a particular host, it does not make sense to list invalid targets in the target print code. Filter the list of targets to only include the targets that can be instantiated.

cardoe force-pushed the expose-target-options branch from 436c467 to 54c61ff Compare July 27, 2016 17:31

cardoe mentioned this pull request Jul 27, 2016

ICE with *-*-ios targets without ios developer stuff installed #29812

Closed

This was referenced Jul 27, 2016

Misleading error caused by incorrect target specification #34806

Closed

compiler ICE when unable to find iOS SDK #32752

Closed

Compiler panics when 'data-layout' field is missing in LLVM target json #33497

Closed

bors merged commit 54c61ff into rust-lang:master Jul 29, 2016

cardoe deleted the expose-target-options branch July 30, 2016 18:32

TimNN mentioned this pull request Aug 31, 2016

Targets filtered out too early #36156

Closed

cardoe mentioned this pull request Nov 29, 2016

print option to dump target spec as JSON #38061

Merged

ehuss mentioned this pull request Sep 25, 2020

Defer Apple SDKROOT detection to link time. #77202

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Convert built-in targets to JSON #34980

Convert built-in targets to JSON #34980

cardoe commented Jul 22, 2016

rust-highfive commented Jul 22, 2016

cardoe commented Jul 22, 2016

cardoe commented Jul 22, 2016

alexcrichton Jul 22, 2016

brson commented Jul 22, 2016

alexcrichton commented Jul 23, 2016

jcreekmore commented Jul 23, 2016

cardoe commented Jul 23, 2016

nagisa commented Jul 23, 2016

jcreekmore commented Jul 23, 2016

jcreekmore commented Jul 23, 2016

nrc commented Jul 25, 2016

alexcrichton commented Jul 25, 2016

cardoe commented Jul 25, 2016

alexcrichton commented Jul 27, 2016

bors commented Jul 27, 2016

cardoe commented Jul 27, 2016

alexcrichton commented Jul 28, 2016

bors commented Jul 29, 2016

bors commented Jul 29, 2016

Convert built-in targets to JSON #34980

Convert built-in targets to JSON #34980

Conversation

cardoe commented Jul 22, 2016

rust-highfive commented Jul 22, 2016

cardoe commented Jul 22, 2016

cardoe commented Jul 22, 2016

alexcrichton Jul 22, 2016

Choose a reason for hiding this comment

brson commented Jul 22, 2016

alexcrichton commented Jul 23, 2016

jcreekmore commented Jul 23, 2016

cardoe commented Jul 23, 2016

nagisa commented Jul 23, 2016

jcreekmore commented Jul 23, 2016

jcreekmore commented Jul 23, 2016

nrc commented Jul 25, 2016

alexcrichton commented Jul 25, 2016

cardoe commented Jul 25, 2016

alexcrichton commented Jul 27, 2016

bors commented Jul 27, 2016

cardoe commented Jul 27, 2016

alexcrichton commented Jul 28, 2016

bors commented Jul 29, 2016

bors commented Jul 29, 2016