-
Notifications
You must be signed in to change notification settings - Fork 13k
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
Conversation
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]>
Since @rust-highfive shunned this PR and @alexcrichton has reviewed my other PRs in this area. |
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 |
☔ The latest upstream changes (presumably #32939) made this pull request unmergeable. Please resolve the merge conflicts. |
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 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) |
@alexcrichton I'm just wondering if it would be acceptable to break compatibility of existing target JSON files and convert the internal structures to 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 |
I forgot to mention the obvious safe fallback is to implement a middle man structure that is |
@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! |
Closing due to inactivity, but feel free to resubmit with a rebase! |
@alexcrichton I've finally had a chance to pick this up again. Unfortunately I think the intermediate struct idea and just putting |
@cardoe sounds good to me! |
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.
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.