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

Create source cache when building native in container #12283 #12294

Merged
merged 1 commit into from Sep 25, 2020

Conversation

galderz
Copy link
Member

@galderz galderz commented Sep 23, 2020

  • Let Mandrel/GraalVM decide where to put the source cache, which naturally leads to a folder that's stored within the module shared with container.
  • Copy the application sources in advance, to avoid the need to make src/main/java available to the container.
  • Update documentation.

Closes #12283.

* Let Mandrel/GraalVM decide where to put the source cache,
which naturally leads to a folder
that's stored within the module shared with container.
* Copy the application sources in advance,
to avoid the need to make src/main/java available to the container.
@geoand
Copy link
Contributor

geoand commented Sep 24, 2020

Seems like CI failed... But it does look unrelated - I'll restart the job just to be on the safe side

@geoand
Copy link
Contributor

geoand commented Sep 24, 2020

@rsvoboda can you verify this one?

@rsvoboda
Copy link
Member

rsvoboda commented Sep 24, 2020

I don't have much context for this, I can take a look tmr, I need to tackle few things before looking into this. @Karm / @jerboaa, would it be possible to take a look?

@geoand
Copy link
Contributor

geoand commented Sep 24, 2020

Thanks 👍

@jerboaa
Copy link
Contributor

jerboaa commented Sep 24, 2020

I'll try to take a look tomorrow.

@geoand
Copy link
Contributor

geoand commented Sep 25, 2020

Any news on validating this?

@rsvoboda
Copy link
Member

It's weird, like if Quarkus master with this patch is not providing debug info at all

I have sample app created using:

mvn io.quarkus:quarkus-maven-plugin:1.8.1.Final:create \
    -DprojectGroupId=org.acme \
    -DprojectArtifactId=getting-started \
    -DclassName="org.acme.getting.started.GreetingResource" \
    -Dpath="/hello"
cd getting-started

Debug enabled:

cat src/main/resources/application.properties
quarkus.native.debug.enabled=true

When running mvn clean package -Dnative -Dquarkus.native.container-build=true I can see -H:DebugInfoSourceSearchPath=../../src/main/java -H:DebugInfoSourceCacheRoot=/Users/rsvoboda/tmp/getting-started/target/sources-cache in docker run command

But when running mvn clean package -Dnative -Dquarkus.native.container-build=true -Dquarkus.platform.artifact-id=quarkus-bom -Dquarkus.platform.version=999-SNAPSHOT -Dquarkus-plugin.version=999-SNAPSHOT there is no such info in docker run command, like if debug is not enabled at all

@geoand
Copy link
Contributor

geoand commented Sep 25, 2020

@galderz can you shine some light here please?

@zakkak
Copy link
Contributor

zakkak commented Sep 25, 2020

When running mvn clean package -Dnative -Dquarkus.native.container-build=true I can see -H:DebugInfoSourceSearchPath=../../src/main/java -H:DebugInfoSourceCacheRoot=/Users/rsvoboda/tmp/getting-started/target/sources-cache in docker run command

But when running mvn clean package -Dnative -Dquarkus.native.container-build=true -Dquarkus.platform.artifact-id=quarkus-bom -Dquarkus.platform.version=999-SNAPSHOT -Dquarkus-plugin.version=999-SNAPSHOT there is no such info in docker run command, like if debug is not enabled at all

That's expected, judging by the diff of this PR.

Testing locally...

@rsvoboda
Copy link
Member

rsvoboda commented Sep 25, 2020

JIRA says: This value should work for DebugInfoSourceCacheRoot: -H:DebugInfoSourceCacheRoot=/project/source-cache
That's why I expected to see this property to be set.

So this PR removes command.add("-H:DebugInfoSourceSearchPath=" + javaSourcesPath.toString()); and command.add("-H:DebugInfoSourceCacheRoot=" + sourcesCachePath.toString());

and copies .jar bits and sources into target/getting-started-1.0-SNAPSHOT-native-image-source-jar/ in case of my application

Why is -H:DebugInfoSourceSearchPath and -H:DebugInfoSourceCacheRoot no longer needed to be set?

@jerboaa
Copy link
Contributor

jerboaa commented Sep 25, 2020

It's weird, like if Quarkus master with this patch is not providing debug info at all

I have sample app created using:

mvn io.quarkus:quarkus-maven-plugin:1.8.1.Final:create \
    -DprojectGroupId=org.acme \
    -DprojectArtifactId=getting-started \
    -DclassName="org.acme.getting.started.GreetingResource" \
    -Dpath="/hello"
cd getting-started

Debug enabled:

cat src/main/resources/application.properties
quarkus.native.debug.enabled=true

When running mvn clean package -Dnative -Dquarkus.native.container-build=true I can see -H:DebugInfoSourceSearchPath=../../src/main/java -H:DebugInfoSourceCacheRoot=/Users/rsvoboda/tmp/getting-started/target/sources-cache in docker run command

But when running mvn clean package -Dnative -Dquarkus.native.container-build=true -Dquarkus.platform.artifact-id=quarkus-bom -Dquarkus.platform.version=999-SNAPSHOT -Dquarkus-plugin.version=999-SNAPSHOT there is no such info in docker run command, like if debug is not enabled at all

After the patch, you should see -g added for an image which actually supports debug symbols. AFAIK, the default image is a Graal CE one, so try using -Dquarkus.native.builder-image=quay.io/quarkus/ubi-quarkus-mandrel:20.1-java11 That one should enter the path this patch touches.

@jerboaa
Copy link
Contributor

jerboaa commented Sep 25, 2020

Why is -H:DebugInfoSourceSearchPath and -H:DebugInfoSourceCacheRoot no longer needed to be set?

Default for DebugInfoSourceCacheRoot is sources in $PWD. So the copy of app sources to the rightly mapped container folder would cover the application sources bits. I'm trying to verify this myself too now. First time building quarkus, though. It'll be slow.

The expectation would be for the source cache (Graal tries to produce a directory of all sources which make up the native image) to contain relevant JDK classes (from src.zip), Graal classes, from *src.zip files, and application classes.

@zakkak
Copy link
Contributor

zakkak commented Sep 25, 2020

Why is -H:DebugInfoSourceSearchPath and -H:DebugInfoSourceCacheRoot no longer needed to be set?

Default for DebugInfoSourceCacheRoot is sources in $PWD, not sure about the former. I'm trying to verify this myself too now. First time building quarkus, though. It'll be slow.

Default DebugInfoSourceSearchPath is empty, and Graal checks $PWD in this case. So by copying the sources @galderz makes sure native-image will have access to them without the need of -H:DebugInfoSourceSearchPath

@galderz
Copy link
Member Author

galderz commented Sep 25, 2020

@rsvoboda They're not set any more because they're not needed, given the preferred location for container environments. To be more precise:

  1. Not using -H:DebugInfoSourceCacheRoot the source cache falls within the folder that's volumed across to the container. I had used it to bring the sources cache folder to be right under target/ but as seen, that causes issues on containers. So, as also indicated by the documentation update, I've gone for the default location.

  2. Not using -H:DebugInfoSourceSearchPath any more since it was referencing a location outside of of the volumed folder. Instead, I manually copied the sources across before native-image invocation and then the sources are located in the right place.

@zakkak
Copy link
Contributor

zakkak commented Sep 25, 2020

Non-blocking issue I see with the PR at its current state.

The user experience is not ideal. One has to run

(gdb) set directories getting-started-1.0-SNAPSHOT-native-image-source-jar/sources/jdk:getting-started-1.0-SNAPSHOT-native-image-source-jar/sources/src

for it to work.

This is happening because the binary is placed in a different folder than the sources directory using -H:DebugInfoSourceCacheRoot to place the sources next to the binary would fix that.

Other than that it worked fine for me for both a container-base and a graalvm-home-based build.

@galderz
Copy link
Member Author

galderz commented Sep 25, 2020

This is happening because the binary is placed in a different folder than the sources directory using -H:DebugInfoSourceCacheRoot to place the sources next to the binary would fix that.

Hmmm, you mean the sources dir has to be next to the executable? If so, that might be hard to achieve due to the original issue, which sets a volume starting in getting-started-1.0-SNAPSHOT-native-image-source-jar/ and that's a level up from the binary... I'd have to run it again to see how it manages to set the binary output a folder below.

@zakkak
Copy link
Contributor

zakkak commented Sep 25, 2020

The transfer happens after the build:

[INFO] [io.quarkus.deployment.pkg.steps.NativeImageBuildStep] Execute [objcopy, --only-keep-debug, /tmp/getting-started/target/getting-started-1.0-SNAPSHOT-runner, /tmp/getting-started/target/getting-started-1.0-SNAPSHOT-runner.debug]
[INFO] [io.quarkus.deployment.pkg.steps.NativeImageBuildStep] Execute [objcopy, --add-gnu-debuglink=/tmp/getting-started/target/getting-started-1.0-SNAPSHOT-runner.debug, /tmp/getting-started/target/getting-started-1.0-SNAPSHOT-runner]
[INFO] [io.quarkus.deployment.pkg.steps.NativeImageBuildStep] Execute [objcopy, --strip-debug, /tmp/getting-started/target/getting-started-1.0-SNAPSHOT-runner]

So we can do another copy and bring the sources next to the binary :)

@zakkak
Copy link
Contributor

zakkak commented Sep 25, 2020

Sorry the copying actually happens here

So I would expect something like:

if (nativeConfig.debug.enabled) {
    IoUtils.copy(outputDir.resolve("sources"), outputTargetBuildItem.getOutputDirectory().resolve("sources"));
}

to do the trick.

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.

This seems to be working fine for me. Bug is fixed. The UX improvement can be implemented as a follow-up, IMO.

Copy link
Member

@rsvoboda rsvoboda left a comment

Choose a reason for hiding this comment

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

Approving, the main trouble is solved. User experience can be improved in follow-up PR.

@rsvoboda
Copy link
Member

@jerboaa / @zakkak / @galderz thanks for explaining the details.
Can you please create follow-up issue to improve user experience (bring the sources next to the binary)?

@rsvoboda
Copy link
Member

@geoand I think PR is ready for merge

@geoand
Copy link
Contributor

geoand commented Sep 25, 2020

Great!

@geoand geoand merged commit 80d9d46 into quarkusio:master Sep 25, 2020
@geoand
Copy link
Contributor

geoand commented Sep 25, 2020

I'll attempt a release with these fixes over the weekend

@geoand geoand added this to the 1.9.0 - master milestone Sep 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

No source cache when native executable created in container
6 participants