Skip to content
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

Expose target options via JSON #32988

Closed
wants to merge 2 commits into from

Conversation

cardoe
Copy link
Contributor

@cardoe cardoe commented Apr 15, 2016

A number of target options are not exposed via JSON. This updates the code to keep it in sync and re-orders the list to match the default implementation to hopefully make it easier for people to inspect to make sure items are not missing.

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]>
Hopefully the note helps people remember to add any additional fields to
the Target::from_json() function.

Signed-off-by: Doug Goldstein <[email protected]>
@cardoe
Copy link
Contributor Author

cardoe commented Apr 15, 2016

The 2nd commit is optional. I can squash it in or delete it. I'd love to know if there's a way to programmatically enforce that all these options are exposed. This is also related to #32847, #32823, and #32818.

@cardoe
Copy link
Contributor Author

cardoe commented Apr 15, 2016

r? @alexcrichton

Since @rust-highfive shunned this PR and @alexcrichton has reviewed my other PRs in this area.

@alexcrichton
Copy link
Member

Thanks for the PR @cardoe! Like with #32847 the ideal solution here I think is to use a "one source of truth" tactic to ensure we don't forget new features in the future.

I'm gonna tag this with T-tools though so we discuss this during triage next week. We haven't discussed much about the stability story of target specs for now, and providing a guarantee that we expose everything is probably both desirable and needs to be considered carefully!

@bors
Copy link
Contributor

bors commented Apr 20, 2016

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

@alexcrichton
Copy link
Member

Ok, the tools team discussed this during triage the other day and the conclusion was that this seems good to do, but we really do want to use something like derive(RustcEncodable, RustcDecodable) instead of doing this by hand.

Could you make a structure which corresponds to the serialized version of a target specification and then have a conversion to the one that the compiler needs? (or just make them one and the same)

@cardoe
Copy link
Contributor Author

cardoe commented May 6, 2016

@alexcrichton I'm just wondering if it would be acceptable to break compatibility of existing target JSON files and convert the internal structures to Encodable and Decodable? There's 3 fields with slightly different names (prefixed with target_) but then a whole slew of fields that are actually instead of a JSON options object instead of at the top level how they're currently parsed. The other approach would be to convert the internal structures to how the target JSON files are currently defined.

The reason I threw out the converting to the internal structure is that I think those are actually a bit cleaner since they encompass required fields vs optional fields nicely. Additionally Rust does have a history of breaking compatibility with the target JSON fields (it recently happened with the data_layout field) so it's not without precedent.

@cardoe
Copy link
Contributor Author

cardoe commented May 6, 2016

I forgot to mention the obvious safe fallback is to implement a middle man structure that is Encodable and Decodable that follows the existing target JSON spec and converts to the internal structures as you originally mentioned but that seems like a lot of duplicated code.

@alexcrichton
Copy link
Member

@cardoe one useful tactic I've taken in the past is to have an intermediate structure. This temporary structure matches the JSON spec, and then we provide conversions from the official struct to/from this temporary one. That should at least prevent breakage!

@alexcrichton
Copy link
Member

Closing due to inactivity, but feel free to resubmit with a rebase!

@cardoe
Copy link
Contributor Author

cardoe commented Jul 22, 2016

@alexcrichton I've finally had a chance to pick this up again. Unfortunately I think the intermediate struct idea and just putting #[derive(RustcDecodable, RustcEncodable)] on it won't work due to the target spec using fields that rustc-serialize cannot handle (e.g. "llvm-target"). As far as I'm aware serde is not yet allowed in the compiler so the path I'm taking now is to implement Encodable and Decodable by hand on struct Target. Then converting all the internal target specs to JSON and loading them with the include_str!() macro.

@alexcrichton
Copy link
Member

@cardoe sounds good to me!

bors added a commit that referenced this pull request Jul 29, 2016
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants