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

rustc_target: Move some target options from Target to TargetOptions #77729

Merged
merged 7 commits into from
Nov 8, 2020

Conversation

petrochenkov
Copy link
Contributor

@petrochenkov petrochenkov commented Oct 8, 2020

The only reason for Target to TargetOptions to be separate structures is that options in TargetOptions have reasonable defaults and options in Target don't.
(Otherwise all the options logically belong to a single Target struct.)

This PR moves a number of options with reasonable defaults from Target to TargetOptions, so they no longer needs to be specified explicitly for majority of the targets.
The move also allows to inherit the options from rustc_target/src/spec/*_base.rs files in a nicer way.
I didn't change any specific option values here.

The moved options are target_c_int_width (defaults to "32"), target_endian (defaults to "little"), target_os (defaults to "none"), target_env (defaults to ""), target_vendor (defaults to "unknown") and linker_flavor (defaults to LinkerFlavor::Gcc).

Next steps (in later PRs):

@rust-highfive
Copy link
Collaborator

r? @estebank

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 8, 2020
@camelid camelid added A-codegen Area: Code generation A-target-specs Area: Compile-target specifications C-cleanup Category: PRs that clean code up or issues documenting cleanup. labels Oct 8, 2020
@bors

This comment has been minimized.

@bors

This comment has been minimized.

@petrochenkov
Copy link
Contributor Author

ping @estebank, it's been 3 weeks.
The PR is large, but it's mostly mechanical.

@petrochenkov
Copy link
Contributor Author

r? @Mark-Simulacrum, by GitHub suggestion.

@Mark-Simulacrum
Copy link
Member

@bors r+ rollup=never -- I did a lookthrough and didn't spot any obvious mistakes, but of course with such large mechanical diffs I may have missed something. I'm not too worried though, in practice it seems like for tier 1 platforms we'll catch it quickly (perhaps even in CI) if there were accidental typos or whatever.

@bors
Copy link
Contributor

bors commented Nov 6, 2020

📌 Commit 05084ab85537332a0e36853fb8487bd0c9f7e386 has been approved by Mark-Simulacrum

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 6, 2020
@bors
Copy link
Contributor

bors commented Nov 7, 2020

⌛ Testing commit 05084ab85537332a0e36853fb8487bd0c9f7e386 with merge 4ac1d90f32b90b154035fa4f93e22c5627bc87b4...

@bors
Copy link
Contributor

bors commented Nov 7, 2020

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Nov 7, 2020
@petrochenkov
Copy link
Contributor Author

This needs a rebase, new targets were added.

@petrochenkov petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 7, 2020
@petrochenkov
Copy link
Contributor Author

@bors r=Mark-Simulacrum

@bors
Copy link
Contributor

bors commented Nov 7, 2020

📌 Commit c0c0597 has been approved by Mark-Simulacrum

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 7, 2020
@bors
Copy link
Contributor

bors commented Nov 8, 2020

⌛ Testing commit c0c0597 with merge f2ea2f6...

@bors
Copy link
Contributor

bors commented Nov 8, 2020

☀️ Test successful - checks-actions
Approved by: Mark-Simulacrum
Pushing f2ea2f6 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 8, 2020
@bors bors merged commit f2ea2f6 into rust-lang:master Nov 8, 2020
@rustbot rustbot added this to the 1.49.0 milestone Nov 8, 2020
bjorn3 pushed a commit to rust-lang/rustc_codegen_cranelift that referenced this pull request Nov 13, 2020
rustc_target: Further cleanup use of target options

Follow up to rust-lang/rust#77729.

Implements items 2 and 4 from the list in rust-lang/rust#77729 (comment).

The first commit collapses uses of `target.options.foo` into `target.foo`.

The second commit renames some target options to avoid tautology:
`target.target_endian` -> `target.endian`
`target.target_c_int_width` -> `target.c_int_width`
`target.target_os` -> `target.os`
`target.target_env` -> `target.env`
`target.target_vendor` -> `target.vendor`
`target.target_family` -> `target.os_family`
`target.target_mcount` -> `target.mcount`

r? `@Mark-Simulacrum`
@cuviper cuviper mentioned this pull request Nov 12, 2021
workingjubilee added a commit to workingjubilee/rustc that referenced this pull request Nov 15, 2021
Android is not GNU

For a long time, the Android targets had `target_env=""`, but this changed to `"gnu"` in Rust 1.49.0. I tracked this down to rust-lang#77729 which started setting `"gnu"` in the `linux_base` target options, and this was inherited by `android_base`. Then rust-lang#78929 split the env into `linux_gnu_base`, but `android_base` was also changed to follow that. Android was not specifically mentioned in either pull request, so I believe this was an accident. Moving it back to `linux_base` will use an empty `env` again.

r? `@Mark-Simulacrum`
cc `@petrochenkov`
workingjubilee added a commit to workingjubilee/rustc that referenced this pull request Nov 15, 2021
Android is not GNU

For a long time, the Android targets had `target_env=""`, but this changed to `"gnu"` in Rust 1.49.0. I tracked this down to rust-lang#77729 which started setting `"gnu"` in the `linux_base` target options, and this was inherited by `android_base`. Then rust-lang#78929 split the env into `linux_gnu_base`, but `android_base` was also changed to follow that. Android was not specifically mentioned in either pull request, so I believe this was an accident. Moving it back to `linux_base` will use an empty `env` again.

r? ``@Mark-Simulacrum``
cc ``@petrochenkov``
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Nov 15, 2021
Android is not GNU

For a long time, the Android targets had `target_env=""`, but this changed to `"gnu"` in Rust 1.49.0. I tracked this down to rust-lang#77729 which started setting `"gnu"` in the `linux_base` target options, and this was inherited by `android_base`. Then rust-lang#78929 split the env into `linux_gnu_base`, but `android_base` was also changed to follow that. Android was not specifically mentioned in either pull request, so I believe this was an accident. Moving it back to `linux_base` will use an empty `env` again.

r? ```@Mark-Simulacrum```
cc ```@petrochenkov```
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Nov 16, 2021
Android is not GNU

For a long time, the Android targets had `target_env=""`, but this changed to `"gnu"` in Rust 1.49.0. I tracked this down to rust-lang#77729 which started setting `"gnu"` in the `linux_base` target options, and this was inherited by `android_base`. Then rust-lang#78929 split the env into `linux_gnu_base`, but `android_base` was also changed to follow that. Android was not specifically mentioned in either pull request, so I believe this was an accident. Moving it back to `linux_base` will use an empty `env` again.

r? ````@Mark-Simulacrum````
cc ````@petrochenkov````
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-codegen Area: Code generation A-target-specs Area: Compile-target specifications C-cleanup Category: PRs that clean code up or issues documenting cleanup. merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants