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

print option to dump target spec as JSON #38061

Merged
merged 3 commits into from
Dec 3, 2016
Merged

Conversation

cardoe
Copy link
Contributor

@cardoe cardoe commented Nov 28, 2016

This lets the user dump out the target spec 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. It is also potentially useful for Cargo to determine if something has changed with a target and it needs to rebuild things.

@rust-highfive
Copy link
Collaborator

r? @arielb1

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

@cardoe
Copy link
Contributor Author

cardoe commented Nov 29, 2016

/cc @alexcrichton Based on our chat on IRC today.

For some background this is a rebased version of #32847. For some background please see discussions at rust-lang/cargo#2616 and #34980.

@alexcrichton alexcrichton assigned alexcrichton and unassigned arielb1 Nov 29, 2016
@alexcrichton
Copy link
Member

Thanks for the PR @cardoe!

@rust-lang/tools, thoughts about exposing this? I continue to be perpetually uneasy about the stability and such of target specs, and placing them so front-and-center may make it harder to make tweaks to them in the future. Despite that, however, it's a reality that they exist and they're expected to work, so there may not be much debate about that.

@nrc
Copy link
Member

nrc commented Nov 29, 2016

I feel like its necessary to use target specs, so unless we have a plan for an alternative we need to expose this sort of thing. I don't have an opinion on whether target specs are actually a good scheme or not, so I can't say whether we should think of an alternative

@cardoe
Copy link
Contributor Author

cardoe commented Nov 30, 2016

So I don't want this PR to become whether target specs are a good idea or not and whether they are implemented in the best way or not. I just want to make that clear because I feel like the discussion on my PRs in this area devolve into that.

The best documentation for the target specs is really the code. There's RFC 131 but that doesn't enumerate all the options. Each version of Rust has iteratively changed the target specs and tracking those changes is made harder due to having to read the code to know what has changed. This would allow people to dump a built-in spec as JSON and see the differences.

@brson
Copy link
Contributor

brson commented Nov 30, 2016

Please feature gate this.

@alexcrichton
Copy link
Member

Discussed at tools triage today and yeah the conclusion is that with a feature gate this should be good to go. Could you also perhaps rename this to "target-spec-json" or "json-target-spec" to emphasize the json aspect as well?

@cardoe
Copy link
Contributor Author

cardoe commented Dec 1, 2016

Will do to both requests. Not quite sure how to feature gate something but I'm sure I can find an example in the code base. Looking at the existing options most of them start with "target-" so maybe "target-spec-json" is the best?

@alexcrichton
Copy link
Member

Sure yeah "target-spec-json" sounds good. You can check for unstable features by checking for -Z unstable-options and such. I think there's a few other examples of this throughout the repo (that we should likely centralize...)

@cardoe
Copy link
Contributor Author

cardoe commented Dec 1, 2016

I'm not sure how I'd leave the flag out of the getopt parser since that string of the valid values to --print is passed to the function that gives matches that I would have to check. I'll push what I have and people can give feedback.

To allow manipulation of the options that appear in --print, convert it
to a vector.

Signed-off-by: Doug Goldstein <cardoe@cardoe.com>
@alexcrichton
Copy link
Member

Oh we'd always have it defined as an option, and then right before we actually take this action we'd assert that we're on the nightly channel.

@cardoe
Copy link
Contributor Author

cardoe commented Dec 2, 2016

Oh, well I made it not visible on non-nightly versions. I can drop that first patch if you'd prefer.

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 <cardoe@cardoe.com>
This is a very basic test of the --print target-spec feature.

Signed-off-by: Doug Goldstein <cardoe@cardoe.com>
@alexcrichton
Copy link
Member

@bors: r+

Looks good to me, thanks!

@bors
Copy link
Contributor

bors commented Dec 2, 2016

📌 Commit 7151b5a has been approved by alexcrichton

@bors
Copy link
Contributor

bors commented Dec 3, 2016

⌛ Testing commit 7151b5a with merge 8900854...

bors added a commit that referenced this pull request Dec 3, 2016
print option to dump target spec as JSON

This lets the user dump out the target spec 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. It is also potentially useful for Cargo to determine if something has changed with a target and it needs to rebuild things.
@bors bors merged commit 7151b5a into rust-lang:master Dec 3, 2016
@cardoe cardoe deleted the target-spec branch December 3, 2016 21:22
@cardoe
Copy link
Contributor Author

cardoe commented Dec 12, 2016

So what do I need to do to propose this flag as a stable flag in the future? Wait a certain number of releases?

@alexcrichton
Copy link
Member

@cardoe oh oops we try to have a tracking issue for all unstable features (I forgot to request on here) which tracks stabilization. We move that to "fcp for merge" when we feel it's ready and then once the tools team signs off it'll go into stable.

Do you want to open an issue to track this? I'll tag it appropriately.

steveklabnik pushed a commit to steveklabnik/rust that referenced this pull request Jan 4, 2017
Since 8285ab5, which was merged in with rust-lang#38061, the help for the
--print option is missing the surrounding [ ] around the possible
options.

Signed-off-by: Doug Goldstein <cardoe@cardoe.com>
steveklabnik added a commit to steveklabnik/rust that referenced this pull request Jan 4, 2017
fix help for the --print option

Since 8285ab5, which was merged in with rust-lang#38061, the help for the
--print option is missing the surrounding [ ] around the possible
options.

Signed-off-by: Doug Goldstein <cardoe@cardoe.com>
nikomatsakis pushed a commit to nikomatsakis/rust that referenced this pull request Jan 6, 2017
Since 8285ab5, which was merged in with rust-lang#38061, the help for the
--print option is missing the surrounding [ ] around the possible
options.

Signed-off-by: Doug Goldstein <cardoe@cardoe.com>
bors added a commit that referenced this pull request Jan 6, 2017
fix help for the --print option

Since 8285ab5, which was merged in with #38061, the help for the
--print option is missing the surrounding [ ] around the possible
options.

Signed-off-by: Doug Goldstein <cardoe@cardoe.com>
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.

7 participants