-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Fix quarkus.native.debug-build-process #20451
Conversation
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.
Perhaps this should take the "runs-in-container" property into account? I'd think for local debugging this isn't needed and it would only be exposing more than necessary. Thoughts?
@jerboaa You mean that we should be using |
fa3377d
to
747fcce
Compare
Yes. Use the old code to only bind to a specific port locally when not in a container and change to the new |
This workflow status is outdated as a new workflow run has been triggered. Failing Jobs - Building 747fcce
Full information is available in the Build summary check run. Failures⚙️ JVM Tests - JDK 11 #- Failing: integration-tests/container-image/maven-invoker-way
📦 integration-tests/container-image/maven-invoker-way✖ 📦 integration-tests/container-image/maven-invoker-way/target/it/container-build-jib-with-mongo✖
⚙️ JVM Tests - JDK 17 #- Failing: extensions/smallrye-reactive-messaging-kafka/deployment integration-tests/container-image/maven-invoker-way
! Skipped: docs integration-tests/kafka-sasl-elytron integration-tests/kubernetes/quarkus-standard-way-kafka and 2 more 📦 extensions/smallrye-reactive-messaging-kafka/deployment✖
📦 integration-tests/container-image/maven-invoker-way✖ 📦 integration-tests/container-image/maven-invoker-way/target/it/container-build-jib-with-mongo✖
|
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.
Perhaps we could avoid the localhost part? See below.
@@ -620,8 +620,15 @@ public NativeImageInvokerInfo build() { | |||
nativeImageArgs.add("-H:DebugInfoSourceSearchPath=" + APP_SOURCES); | |||
} | |||
if (nativeConfig.debugBuildProcess) { | |||
String debugBuildProcessHost; | |||
if (isContainerBuild) { | |||
debugBuildProcessHost = "0.0.0.0"; |
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.
How about?
String debugAgentAddress = DEBUG_BUILD_PROCESS_PORT;
if (isContainerBuild) {
debugAgentAddress = "0.0.0.0:" + DEBUG_BUILD_PROCESS_PORT;
}
[...]
.add("-J-Xrunjdwp:transport=dt_socket,address=" + debugAgentAddress + ",server=y,suspend=y");
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.
We sure can, but what's the benefit of it?
This way we make clear that we only want the process to listen on localhost when building locally, do you forsee any issues with this?
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.
No it would just be the more backward compatible way, I guess. Looking at the JDK 11 address parsing code "localhost" is handled fine, so feel free to ignore it.
As of JDK 9 `address` needs to specify the host as well. This patch makes JDWP listen on 0.0.0.0 and is compatible with JDK 8 and JDK >= 9
747fcce
to
56fb882
Compare
As of JDK 9
address
needs to specify the host as well. This patchmakes JDWP listen on 0.0.0.0 and is compatible with JDK 8 and JDK >= 9