-
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
Copy sources from remote container when quarkus.native.debug.enabled=true #15288
Copy sources from remote container when quarkus.native.debug.enabled=true #15288
Conversation
\cc @jonathan-meier |
core/deployment/src/main/java/io/quarkus/deployment/pkg/steps/NativeImageBuildRunner.java
Outdated
Show resolved
Hide resolved
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.
Looks good to me, though I think we already missed copying some more artefacts, e.g. the dashboard dump:
quarkus/core/deployment/src/main/java/io/quarkus/deployment/pkg/steps/NativeImageBuildStep.java
Lines 796 to 799 in faa48d7
if (nativeConfig.enableDashboardDump) { | |
nativeImageArgs.add("-H:DashboardDump=" + outputTargetBuildItem.getBaseName() + "_dashboard.dump"); | |
nativeImageArgs.add("-H:+DashboardAll"); | |
} |
quarkus/core/deployment/src/main/java/io/quarkus/deployment/pkg/steps/NativeImageBuildStep.java
Line 83 in faa48d7
outputDir = buildSystemTargetBuildItem.getOutputDirectory().resolve("native-sources"); |
NativeImageBuildStep
and hand that list over to the NativeImageBuildRunner
.
Also, thanks for catching the issue with the user. I missed that because in my tests the local user had the same UID as the quarkus user in the container (UID 1001). Sorry about that!
...ent/src/main/java/io/quarkus/deployment/pkg/steps/NativeImageBuildRemoteContainerRunner.java
Outdated
Show resolved
Hide resolved
7319acc
to
7975a6b
Compare
Using them results in files being copied back to host to be owned by the guest user instead of the host user. e.g. $ podman create --name temp --user 1000:1000 --userns=keep-id -it \ quay.io/quarkus/ubi-quarkus-native-image:21.0.0-java11 $ podman cp temp:/opt/graalvm/bin/native-image remote-native-image $ ls -la remote-native-image -rwxr-xr-x. 1 100000 100000 14641161 Feb 14 03:28 remote-native-image* $ id -u 1000
b2904f0
to
63fec3d
Compare
core/deployment/src/main/java/io/quarkus/deployment/pkg/steps/NativeImageBuildLocalRunner.java
Show resolved
Hide resolved
63fec3d
to
bff39cf
Compare
if (workingDirectory != null) { | ||
processBuilder.directory(workingDirectory); | ||
} | ||
process = processBuilder.start(); |
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.
Looks like you are you are not redirecting the output streams like ProcessUtil#launchProcess
was doing
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.
Indeed, but that's how things where before (except for cleanupServer).
runCommand
is used to run native-image --shutdown-server
, docker rm container-name
and docker cp src dest
. IMO it's OK to not redirect output for these commands (as discussed in https://github.com/quarkusio/quarkus/pull/15288/files#r582745324). WDYT?
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.
Okay
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 George
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 fixing this
I just realized that #14635 has been backported to 1.12.1. Would be nice if this PR could be backported as well, mainly because of the docker user fix on Linux. |
Oh, you are right. Even though the conflicting changes in |
Sorry to bring this up once more, it is actually enough to just cherry-pick the second commit of this PR (5d4f39d) onto 1.12. Without this fix, the remote container build is hardly usable on 1.12 as it is. I opened PR #15420 against 1.12 with this change. If this is undesirable or goes against your backporting protocols, feel free to just close it! |
It's up to @gsmet to decide. |
When using
-Dquarkus.native.remote-container-build=true
in combination with-Dquarkus.native.debug.enabled=true
native image generation fails with:This PR fixes this by copying the sources from the container back to the host.