-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Conversation
r? @oli-obk (rust-highfive has picked a reviewer for you, use r? to override) |
|
@@ -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, |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cc @ojeda
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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, |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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? |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to executables: true
.
This comment was marked as resolved.
This comment was marked as resolved.
Also change `executables` to true for linux-kernel and windows-uwp-gnu targets
483bae0
to
8d9fdb7
Compare
@rustbot ready |
Thanks everyone for chiming in! @bors r+ |
rustc_target: Flip the default for `TargetOptions::executables` to true This flag is true for most targets and the remaining targets may be mistakes.
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
This flag is true for most targets and the remaining targets may be mistakes.