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

Add optional native flag to build PIE binaries #33931

Merged
merged 1 commit into from
Jul 21, 2023

Conversation

galderz
Copy link
Member

@galderz galderz commented Jun 9, 2023

Closes #33524. Adds quarkus.native.pie configuration option. Usage and verification:

$ mvn package -Dquarkus.native.pie=true -Dnative
...
file getting-started/target/getting-started-1.0.0-SNAPSHOT-runner
getting-started/target/getting-started-1.0.0-SNAPSHOT-runner: ELF 64-bit LSB pie executable, ...

Compared with:

$ mvn package -Dnative
file getting-started/target/getting-started-1.0.0-SNAPSHOT-runner
getting-started/target/getting-started-1.0.0-SNAPSHOT-runner: ELF 64-bit LSB executable, ...

Commits need squashing once review passes.

@zakkak is away so maybe @Karm or @jerboaa can review?

@quarkus-bot

This comment has been minimized.

@galderz
Copy link
Member Author

galderz commented Jun 12, 2023

The JVM test failure is unrelated.

Copy link
Contributor

@jerboaa jerboaa left a comment

Choose a reason for hiding this comment

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

Seems OK to me.

Copy link
Member

@gsmet gsmet left a comment

Choose a reason for hiding this comment

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

I added a suggestion and in any case could you squash the commits?

@@ -198,6 +198,11 @@ public interface NativeConfig {
*/
Optional<Boolean> containerBuild();

/**
* Explicit configuration option to generate PIE native binaries.
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure I understand when PIE will be enabled, especially when this is not set. I think we should make it more explicit in the doc.

Copy link
Contributor

@zakkak zakkak left a comment

Choose a reason for hiding this comment

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

LGTM, other than the documentation part (see my suggestion)

@@ -198,6 +198,11 @@ public interface NativeConfig {
*/
Optional<Boolean> containerBuild();

/**
* Explicit configuration option to generate PIE native binaries.
Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with @gsmet that a more explicit (and consistent with the rest) documentation is needed.

Suggested change
* Explicit configuration option to generate PIE native binaries.
* If this build should build a Position Independent Executable (PIE). Not setting the option results in using the
* underlying system's defaults.

@galderz
Copy link
Member Author

galderz commented Jun 27, 2023

@gsmet @zakkak I've pushed an update for the javadoc option. How does that sound now?

Copy link
Contributor

@zakkak zakkak left a comment

Choose a reason for hiding this comment

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

I still don't find the doc clear enough. Added some suggestions.

@@ -198,6 +198,15 @@ public interface NativeConfig {
*/
Optional<Boolean> containerBuild();

/**
* Explicit configuration option to generate Position Independent Executable (PIE) native executables for Linux.
* If the system supports, the default behaviour is to generate native binaries without PIE for
Copy link
Contributor

Choose a reason for hiding this comment

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

If the system supports what?

I suggest:

Suggested change
* If the system supports, the default behaviour is to generate native binaries without PIE for
* If the system we are building on supports PIE generation, the default behaviour is to disable it

@@ -198,6 +198,15 @@ public interface NativeConfig {
*/
Optional<Boolean> containerBuild();

/**
* Explicit configuration option to generate Position Independent Executable (PIE) native executables for Linux.
Copy link
Contributor

Choose a reason for hiding this comment

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

Avoid redundant "executable":

Suggested change
* Explicit configuration option to generate Position Independent Executable (PIE) native executables for Linux.
* Explicit configuration option to generate a native Position Independent Executable (PIE) for Linux.

Comment on lines 205 to 206
* Some systems only support PIE executables,
* so this option offers the opportunity to generate native executables with PIE instead.
Copy link
Contributor

Choose a reason for hiding this comment

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

Avoid redundant "executable" and reduce words:

Suggested change
* Some systems only support PIE executables,
* so this option offers the opportunity to generate native executables with PIE instead.
* Some systems, however, only support platform independent executables (PIEs),
* this option enables the generation of such native executables.

Copy link
Member Author

Choose a reason for hiding this comment

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

Don't see the need to introduce again what is a PIE.

@quarkus-bot

This comment has been minimized.

@galderz
Copy link
Member Author

galderz commented Jun 28, 2023

Pushed another round of updates for the PIE option javadoc.

Copy link
Contributor

@zakkak zakkak left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @galderz

@quarkus-bot

This comment has been minimized.

@galderz galderz force-pushed the topic.0607.pie-flag branch from 680b1a0 to df25342 Compare June 28, 2023 16:38
@galderz
Copy link
Member Author

galderz commented Jun 28, 2023

Thx @zakkak. Squashed the commits and pushed.

@quarkus-bot

This comment has been minimized.

@galderz galderz force-pushed the topic.0607.pie-flag branch from df25342 to c83272a Compare July 21, 2023 07:52
@galderz
Copy link
Member Author

galderz commented Jul 21, 2023

Rebased to resolve conflict. Can someone integrate this?

@zakkak zakkak added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Jul 21, 2023
@quarkus-bot
Copy link

quarkus-bot bot commented Jul 21, 2023

Failing Jobs - Building c83272a

Status Name Step Failures Logs Raw logs
✔️ Maven Tests - JDK 11
Maven Tests - JDK 11 Windows Build ⚠️ Check → Logs Raw logs

@zakkak zakkak merged commit 2da3206 into quarkusio:main Jul 21, 2023
@quarkus-bot quarkus-bot bot added kind/enhancement New feature or request and removed triage/waiting-for-ci Ready to merge when CI successfully finishes labels Jul 21, 2023
@quarkus-bot quarkus-bot bot added this to the 3.3 - main milestone Jul 21, 2023
@zakkak
Copy link
Contributor

zakkak commented Jul 21, 2023

@galderz do you think it's worth backporting to 3.2?

@galderz
Copy link
Member Author

galderz commented Jul 24, 2023

@zakkak Yeah I think it's worth it. I'll send a PR for it.

@galderz
Copy link
Member Author

galderz commented Jul 24, 2023

See #34944 for the 3.2.x backport.

@zakkak
Copy link
Contributor

zakkak commented Jul 24, 2023

See #34944 for the 3.2.x backport.

I added the triage/backport? label so that it doesn't get accidentally missed

@galderz galderz deleted the topic.0607.pie-flag branch December 19, 2023 05:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/core kind/enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Native Executable --no-pie
4 participants