-
Notifications
You must be signed in to change notification settings - Fork 10
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 support for creating a native-image using -jar option #124
Add support for creating a native-image using -jar option #124
Conversation
…ion of a Main Class Signed-off-by: Juan Bustamante <[email protected]>
This seems totally reasonable. I think the reason that we are building without the
At any rate, when we open up libbs (Maven/Gradle/etc..) to allow multiple output artifacts, the behavior of building from class files is not going to be sufficient. My only ask would be to wait until we've resolved that issue with Maven before merging this PR. It'll make testing/validation easier. Is that OK with you @jjbustamante? |
Yes, I am 100% agree to wait for the fix of multiple artifacts, in fact, I will be adding some unit tests to this PR, but I wanted to share what I had before doing that work. I am almost done to push another PR for the multiple artifacts enhancement. |
I created the PR in |
…Options because of just one parameter doen't worth it\n Signed-off-by: Juan Bustamante <[email protected]>
Thanks @dmikusa-pivotal. Let me fix the unit tests! |
Signed-off-by: Juan Bustamante <[email protected]>
Thanks @jjbustamante The tests are passing. I'll give this a review shortly using the https://github.com/quarkusio/quarkus-quickstarts. Unless you have any other suggestions for demo apps to use. |
Hi @dmikusa-pivotal. Using the getting started guide is fine. Let me put here the command I was using to run it locally. I cloned the Quarkus examples application, then I went into
From the previous command, the only thing is that value of the env variable BP_NATIVE_IMAGE_BUILD_ARGUMENTS, I wanted to take the native-image.args generated by Quarkus but that file include the value of jar to be used to create the native-image, I wanted to use something like |
@jjbustamante - FYI. I've been playing with this today. I have been using your PR as the nucleus for a new PR because there are some things I needed to change for consistency and to make testing easier (I'm also adding in something so you don't have to manually set all those args). I'm just running short of time today, but I can hopefully finish this tomorrow and share the PR I'm proposing. Just wanted to give you a heads up. |
@jjbustamante - Here's the PR -> #136. We released the enhancement for the Maven buildpack so it now supports multiple artifacts. A quick way to test this is to download the branch for the PR and run:
I tested with a couple of Quarkus apps and they all built OK. Let me know your experiences & if you think there's anything else we need before we merge this. Thanks! |
I tested the branch with different projects from the Quarkus examples and they are working great. I found one thing trying to test it with a personal Quarkus project but I managed to work around the problem following guidelines from here. Just to let you know my issue. my application native-source folder looks like this:
The
The problem was that those files were not available in the classpath during the build and I had errors like this:
I tried different ways to do it, and the only one that works was adding the classpath parameter I think the feature is great! just le me know when you merged this PR and I will test it directly with all the buildpacks!! thanks a lot |
@jjbustamante Classpath is an interesting issue here. In your PR, you had been setting the CLASSPATH but in my observation, the JAR file that gets produced by Quarkus has the CLASSPATH set within the MANIFEST.MF file, so it doesn't seem necessary to set this manually. In the PR I submitted, when building from a JAR we don't set anything for the CLASSPATH. That could be why it can't find the files. I suspect that if you wanted to set the classpath, you'd need to completely override the CLASSPATH. I don't know if this would cause other issues though, since you'd be adding the How do those json files end up in that location? Is that a standard convention with a Quarkus app? I could look at handling that more gracefully or at the least documenting that, if it's a common case. Thanks for testing! |
I think it's actually related to Graalvm and native-image creation, in my case, those files end up in the In order to solve my particular case, what I did was to follow the recommendations from here to include the required configuration inside the jar that is going to be used to create the image. The steps I did were these:
When I built using pack I didn't use the I am not 100% sure if it is something we need to take care in Buildpacks, the use case for building a Quarkus app was out of the box with what we did, and these particular cases can also be handled by checking the documentation from Graalvm, but adding some FAQ or similar documentation in some place could be very useful for end-users. |
OK, I'll make a note to add this to our docs. Thanks for sharing the details on how you got that use case working. It seems like we're all set though, so I'm going to close out this PR & merge the one I submitted. This work should be out on Friday when we cut the weekly releases. |
Summary
Extend the current native-image Buildpack to allow building an image using the syntax
native-image [options] -jar jarfile [imagename] [options]
See doc
Use Cases
Right now, if we try to build a native image from a Quarkus project we faced several problems. Quarkus allows us to generate a folder with all the files require to build a native image when we use the variable:
-Dquarkus.package.type=native-sources
for example in Maven, the output of this build is a folder in my build directory with a structure similar to this:Assuming the issue 76 is fixed an these artifacts are restore in the /workspace, this PR is trying to provide the functionality to build the image from those sources
This PR Fixes #109
Checklist