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

Only use objcopy in Linux environments #20297

Merged
merged 1 commit into from
Sep 28, 2021
Merged

Conversation

galderz
Copy link
Member

@galderz galderz commented Sep 21, 2021

Addresses macOS issues in #13856.

  • objcopy invocations in macOS make the executable crash on startup.
  • Debug info is only available for Linux,
    so avoid objcopy altogether on macOS,
    even if present in PATH.
  • Updated documentation to avoid confusion.

@quarkus-bot
Copy link

quarkus-bot bot commented Sep 21, 2021

Thanks for your pull request!

The title of your pull request does not follow our editorial rules. Could you have a look?

  • title should not contain an issue number (use Fix #1234 in the description instead)

This message is automatically generated by a bot.

@galderz galderz changed the title Only use objcopy in Linux environments #13856 Only use objcopy in Linux environments Sep 21, 2021
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.

Thanks @galderz , the patch looks good but I am afraid the warning message in

if (!debugSymbolsEnabled) {
log.warn(
"objcopy executable not found in PATH. Debug symbols will therefore not be separated from the executable.");
log.warn("That also means that resulting native executable is larger as it embeds the debug symbols.");
}
might still be misleading.

What do you think about applying something like the following as well?

diff --git a/core/deployment/src/main/java/io/quarkus/deployment/pkg/steps/NativeImageBuildRunner.java b/core/deployment/src/main/java/io/quarkus/deployment/pkg/steps/NativeImageBuildRunner.java
index d299db7a66..fa0703e27b 100644
--- a/core/deployment/src/main/java/io/quarkus/deployment/pkg/steps/NativeImageBuildRunner.java
+++ b/core/deployment/src/main/java/io/quarkus/deployment/pkg/steps/NativeImageBuildRunner.java
@@ -12,6 +12,7 @@ import java.util.concurrent.CountDownLatch;
 import java.util.concurrent.ExecutorService;
 import java.util.concurrent.Executors;
 
+import org.apache.commons.lang3.SystemUtils;
 import org.jboss.logging.Logger;
 
 import io.quarkus.deployment.util.ProcessUtil;
@@ -75,12 +76,10 @@ public abstract class NativeImageBuildRunner {
                     // Strip debug symbols regardless, because the underlying JDK might contain them
                     objcopy("--strip-debug", resultingExecutableName);
                 }
-            } else {
-                if (!debugSymbolsEnabled) {
-                    log.warn(
-                            "objcopy executable not found in PATH. Debug symbols will therefore not be separated from the executable.");
-                    log.warn("That also means that resulting native executable is larger as it embeds the debug symbols.");
-                }
+            } else if (SystemUtils.IS_OS_LINUX){
+                log.warn(
+                        "objcopy executable not found in PATH. Debug symbols will therefore not be separated from the executable.");
+                log.warn("That also means that resulting native executable is larger as it embeds the debug symbols.");
             }
             return new Result(0, objcopyExists);
         } finally {
diff --git a/core/deployment/src/main/java/io/quarkus/deployment/pkg/steps/NativeImageBuildStep.java b/core/deployment/src/main/java/io/quarkus/deployment/pkg/steps/NativeImageBuildStep.java
index 75ba060b0e..a251cc9733 100644
--- a/core/deployment/src/main/java/io/quarkus/deployment/pkg/steps/NativeImageBuildStep.java
+++ b/core/deployment/src/main/java/io/quarkus/deployment/pkg/steps/NativeImageBuildStep.java
@@ -228,12 +228,7 @@ public class NativeImageBuildStep {
                     final Path finalSources = outputTargetBuildItem.getOutputDirectory().resolve(sources);
                     IoUtils.copy(generatedSources, finalSources);
                     IoUtils.recursiveDelete(generatedSources);
-                } else {
-                    log.warn(
-                            "objcopy executable not found in PATH. Debug symbols therefore cannot be placed into the dedicated directory.");
-                    log.warn("That also means that resulting native executable is larger as it embeds the debug symbols.");
                 }
-
             }
             System.setProperty("native.image.path", finalExecutablePath.toAbsolutePath().toString());

@quarkus-bot
Copy link

quarkus-bot bot commented Sep 21, 2021

This workflow status is outdated as a new workflow run has been triggered.

Failing Jobs - Building d682129

Status Name Step Failures Logs Raw logs
Gradle Tests - JDK 11 Build Failures Logs Raw logs
✔️ JVM Tests - JDK 11
JVM Tests - JDK 11 Windows Build Failures Logs Raw logs
✔️ JVM Tests - JDK 16
MicroProfile TCKs Tests Verify ⚠️ Check → Logs Raw logs

Full information is available in the Build summary check run.

Failures

⚙️ Gradle Tests - JDK 11 #

- Failing: integration-tests/gradle 

📦 integration-tests/gradle

io.quarkus.gradle.devmode.MultiModuleIncludedBuildTest.main line 24 - More details - Source on GitHub

java.lang.AssertionError: 

Expecting actual:

⚙️ JVM Tests - JDK 11 Windows #

- Failing: extensions/amazon-lambda/deployment 
! Skipped: docs extensions/amazon-lambda-http/deployment extensions/amazon-lambda-rest/deployment and 6 more

📦 extensions/amazon-lambda/deployment

io.quarkus.amazon.lambda.deployment.testing.LambdaDevServicesContinuousTestingTestCase.testLambda line 41 - More details - Source on GitHub

org.opentest4j.AssertionFailedError: expected: <0> but was: <1>
	at org.junit.jupiter.api.AssertionUtils.fail(AssertionUtils.java:55)
	at org.junit.jupiter.api.AssertionUtils.failNotEqual(AssertionUtils.java:62)

@galderz
Copy link
Member Author

galderz commented Sep 22, 2021

@zakkak Fixed the warning messages. See updated commit.

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 Galder

@quarkus-bot
Copy link

quarkus-bot bot commented Sep 22, 2021

This workflow status is outdated as a new workflow run has been triggered.

Failing Jobs - Building 430fce8

Status Name Step Failures Logs Raw logs
Initial JDK 11 Build Build Failures Logs Raw logs

Failures

⚙️ Initial JDK 11 Build #

- Failing: core/deployment 
! Skipped: core/test-extension/deployment core/test-extension/runtime devtools/bom-descriptor-json and 606 more

📦 core/deployment

Failed to execute goal net.revelc.code:impsort-maven-plugin:1.6.2:check (check-imports) on project quarkus-core-deployment: Error reading file /home/runner/work/quarkus/quarkus/core/deployment/src/main/java/io/quarkus/deployment/pkg/steps/NativeImageBuildStep.java

log.warn(
"objcopy executable not found in PATH. Debug symbols therefore cannot be placed into the dedicated directory.");
log.warn("That also means that resulting native executable is larger as it embeds the debug symbols.");
}
Copy link
Contributor

@zakkak zakkak Sep 22, 2021

Choose a reason for hiding this comment

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

Hmmm, it looks like you were a bit more aggressive here, we need that } back :)

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll fix this shortly.

* `objcopy` invocations in macOS make the executable crash on startup.
* Debug info is only available for Linux,
so avoid `objcopy` altogether on macOS,
even if present in PATH.
* Updated documentation to avoid confusion.
* Adjusted warning messages about `objcopy`.
@gsmet
Copy link
Member

gsmet commented Sep 27, 2021

I rebased and fixed the compilation issue. Let's see how CI goes.

@gsmet
Copy link
Member

gsmet commented Sep 27, 2021

@galderz @zakkak I wonder if this should be backported to 2.2?

/cc @geoand

@geoand
Copy link
Contributor

geoand commented Sep 27, 2021

Likely not necessary as @zakkak can best explain

@zakkak
Copy link
Contributor

zakkak commented Sep 27, 2021

@galderz @zakkak I wonder if this should be backported to 2.2?

@gsmet this is not a blocker, but it would be nice to have it backported.

@galderz
Copy link
Member Author

galderz commented Sep 28, 2021

Thx for fixing it @gsmet

@galderz
Copy link
Member Author

galderz commented Sep 28, 2021

It'd be nice to backport it. It'd stop people from asking us what is going with this (Max asked us yesterday).

@geoand
Copy link
Contributor

geoand commented Sep 28, 2021

Backport to 2.2 you mean?

@geoand geoand merged commit a82a082 into quarkusio:main Sep 28, 2021
@quarkus-bot quarkus-bot bot added this to the 2.4 - main milestone Sep 28, 2021
@galderz
Copy link
Member Author

galderz commented Sep 28, 2021

@geoand Yeah backport to 2.2. Let me know if you need a hand with that :)

@geoand
Copy link
Contributor

geoand commented Sep 28, 2021

It should be straightforward, but I'll let you if/when the time comes

@geoand geoand modified the milestones: 2.4 - main, 2.3.0.Final Sep 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants