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: Flip the default for TargetOptions::executables to true #98622

Merged
merged 1 commit into from
Jul 12, 2022

Conversation

petrochenkov
Copy link
Contributor

This flag is true for most targets and the remaining targets may be mistakes.

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jun 28, 2022
@rust-highfive
Copy link
Collaborator

r? @oli-obk

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive
Copy link
Collaborator

⚠️ Warning ⚠️

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 28, 2022
@@ -9,6 +9,7 @@ pub fn opts() -> TargetOptions {
// don't use probe-stack=inline-asm until rust#83139 and rust#84667 are resolved
stack_probes: StackProbeType::Call,
frame_pointer: FramePointer::Always,
executables: false,
position_independent_executables: true,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@joshtriplett
Is this intentional?
The target doesn't support executables in general, but somehow supports PIC executables?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cc @ojeda

Copy link
Contributor

@ojeda ojeda Jun 29, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the ping Josh! We are not using the builtin target at the moment, so it should be fine.

As for the custom ones we generate, we don't set executables because the default was false (and we don't expect the custom target specs to be used to create executables), so switching it to true should not break anything (and we can always set it back to false on our side in the generator).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As for position_independent_executables, we also don't set it (so it is false for us in the ones we generate). I guess it could be changed to false here (but we don't use the builtin target in any case).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, changed this to executables: true for the target to be self-consistent.

@@ -5,6 +5,7 @@ pub fn opts(kernel: &str) -> TargetOptions {
TargetOptions {
os: format!("solid_{}", kernel).into(),
vendor: "kmc".into(),
executables: false,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @kawadakk
Is this intentional?
The target specifies a linker, but doesn't specify support for executables or dynamic libraries, so the linker is never used, which is weird.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it's intentional. This target requires an external toolchain (linker + post-processor + runtime libraries) to build functional executables or dynamic libraries.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait, what?

That's not the impression I had when that target was first approved?

What is the license on that toolchain?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This target requires an external toolchain (linker + post-processor + runtime libraries) to build functional executables or dynamic libraries.

That's true for majority of our targets though, linker/libc/startup objects are typically not shipped with Rust toolchain.
rustc just calls the linker and passes libraries to it by name and expects them to be there.
Although I'm not sure what the post-processor step means for this target.

Copy link
Contributor

@kawadakk kawadakk Jun 29, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the license on that toolchain?

There's a supported proprietary toolchain (C compiler, linker, etc.) with enhanced functionalities, though it can be substituted by a publicly-available one, such as GNU Arm Embedded Toolchain, to build functional application binaries. ("external" might have given off a wrong connotation.)

The primary deployment mode of this target's operating system is as a library OS. The application binary is built as a static library, which can be done entirely by FOSS components as explained above. Other modes may be possible in the future, and we'd like to leave the executables option for those for now.

Although I'm not sure what the post-processor step means for this target.

Some dynamic executable formats in this operating system use non-standard formats that require post-link conversion.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, keeping executables: false for these targets.

@@ -24,7 +24,7 @@ pub fn opts() -> TargetOptions {
TargetOptions {
abi: "uwp".into(),
vendor: "uwp".into(),
executables: false,
executables: false, // FIXME?
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @ChrisDenton @chouquette
Is this intentional?
Are UWP targets unable to generate executables with mingw specifically?
This flag is true on MSVC UWP targets.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At the time of writing I didn't know of a way to generate an executable from native code when targeting UWP but:

  • This might be a false assumption coming from WinRT times
  • This might not be true anymore, or at all

By all means, if switching this to true is all that's required to generate an executable, please do!

Copy link
Member

@ChrisDenton ChrisDenton Jun 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would check with @mati865 as the resident mingw expert. I would note that these are tier 3 targets so it may not necessarily be a problem if there are extra steps required to make the binary actually work. We also have XP as a tier 3 which won't work out of the box.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely this should be set to true.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed to executables: true.

@rust-log-analyzer

This comment was marked as resolved.

Also change `executables` to true for linux-kernel and windows-uwp-gnu targets
@petrochenkov
Copy link
Contributor Author

@rustbot ready

@oli-obk
Copy link
Contributor

oli-obk commented Jul 12, 2022

Thanks everyone for chiming in!

@bors r+

@bors
Copy link
Contributor

bors commented Jul 12, 2022

📌 Commit 8d9fdb7 has been approved by oli-obk

It is now in the queue for this repository.

@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 Jul 12, 2022
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Jul 12, 2022
rustc_target: Flip the default for `TargetOptions::executables` to true

This flag is true for most targets and the remaining targets may be mistakes.
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 12, 2022
Rollup of 6 pull requests

Successful merges:

 - rust-lang#98622 (rustc_target: Flip the default for `TargetOptions::executables` to true)
 - rust-lang#98633 (Fix last `let_chains` blocker)
 - rust-lang#98972 (Suggest adding a missing zero to a floating point number)
 - rust-lang#99038 (Some more `EarlyBinder` cleanups)
 - rust-lang#99154 (use PlaceRef::iter_projections to fix old FIXME)
 - rust-lang#99171 (Put back UI test regex)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 7615366 into rust-lang:master Jul 12, 2022
@rustbot rustbot added this to the 1.64.0 milestone Jul 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.