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

output target json files #32847

Closed
wants to merge 3 commits into from
Closed

output target json files #32847

wants to merge 3 commits into from

Conversation

cardoe
Copy link
Contributor

@cardoe cardoe commented Apr 9, 2016

This lets the user dump out the target that the compiler is using. This is useful to people defining their own target.json to compare it against existing targets or understand how different targets change internal settings. While there is an existing --print cfg it is much easier for users to have the output be equal to the input.

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Aatch (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@Aatch
Copy link
Contributor

Aatch commented Apr 9, 2016

r? @alexcrichton

Feel free to re-assign, I just don't feel comfortable reviewing this change.

@rust-highfive rust-highfive assigned alexcrichton and unassigned Aatch Apr 9, 2016
@cardoe
Copy link
Contributor Author

cardoe commented Apr 9, 2016

I'm playing around with #16351 and #31117 trying to come up with an improvement to RFC131 and just thought this would be useful.

@cardoe
Copy link
Contributor Author

cardoe commented Apr 9, 2016

Not sure if doing a reduce against the default set and subtracting the ones that where the same as the default. Additionally the null values should probably be dropped?

@cardoe
Copy link
Contributor Author

cardoe commented Apr 9, 2016

Looks like this should be updated when #32823 lands.

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]>
This option provides the user the ability to dump the configuration that
is in use by rustc for the target they are building for.

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

cardoe commented Apr 10, 2016

Since this hasn't had any reviews yet, I went ahead and squashed in the change that removes values that are equal to the defaults and added support for the change in #32823.

This is a very basic test of the --print target-spec feature.

Signed-off-by: Doug Goldstein <[email protected]>
@alexcrichton
Copy link
Member

Thanks for the PR @cardoe! This is a relatively weighty option to add as it's a new stable flag to the compiler (cc @rust-lang/tools). Custom target specs are also relatively unstable today (but unfortunately not gated), so changing them is kinda tough because there's no complete vision for them.

That being said this seems like reasonable functionality if we're going to support target specs at all I think!

@cardoe
Copy link
Contributor Author

cardoe commented Apr 11, 2016

Sure. Let me know how I can help improve this for possible inclusion. I've been looking at some of the other changes around the custom target specs and trying to improve corner cases. I'll be working on a RFC to update 131 to provide a reasonable default for RUST_TARGET_PATH.

@alexcrichton
Copy link
Member

Implementation-wise the reading-from-json and writing-to-json for custom target specs is pretty unfortunate. It's easy to forget to either read (and now perhaps write) a field as it requires a bunch of duplication. Would it be possible to centralize all of that so there's one source of truth for what's read/written from a custom target spec?

@cardoe
Copy link
Contributor Author

cardoe commented Apr 11, 2016

My Rust skills aren't strong enough to know how to do that but if you've got some kind of examples in the docs or point me in the right direction I'll gladly do it.

@brson
Copy link
Contributor

brson commented Apr 19, 2016

I've always thought our own target definitions should be derived from json target specs. So an alternative way to expose this information would be to convert our own specs to json and publish them for use. I slightly prefer that as a first step since it's an internal architecture improvement and doesn't need a new feature.

@cardoe
Copy link
Contributor Author

cardoe commented Apr 19, 2016

I've been thinking about the same thing but haven't wanted to rock the boat a lot. I pitched out #32988 to expose all the members of the two different structures via JSON so that all the specs can be defined. Along those lines I wanted to extend RFC131 to provide an actual default location for rustc to load spec files from.

@alexcrichton
Copy link
Member

Ok, the tools team discussed this PR and #32988 at triage the other day and @brson's thoughts were basically our conclusion. Could we start out by just reading the standard targets from JSON instead of only custom targets? That should ensure that they're all equally robust at least!

We probably don't want to literally read JSON from the filesystem at compile time, so the default targets would still be included in the librustc_back crate by default (e.g. via include_str!).

@alexcrichton
Copy link
Member

An interesting use case for this came up in rust-lang/cargo#2616 which is that it would help Cargo detect changes in the target spec to trigger a recompile of the whole world.

@alexcrichton
Copy link
Member

Closing due to inactivity, but feel free to resubmit with the standard targets read as JSON as well!

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.

5 participants