-
Notifications
You must be signed in to change notification settings - Fork 441
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
Adds cross platform architecture compilation for native image #1549
base: main
Are you sure you want to change the base?
Conversation
Hi @kgston, Thank you for your contribution! We really value the time you've taken to put this together. We see that you have signed the Lightbend Contributors License Agreement before, however, the CLA has changed since you last signed it. |
The CI checks seems to be unhappy that I added a new parameter for specifying the target platform architecture. |
@muuki88 Could you take a look at the PR when you find the time? Thanks. |
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 this high quality contribution 🙏
While I can't really give feedback on the graalvm implementation, the changes and tests look very good.
Only the entrypoint vs cmd is what I don't get 🫣 I'll merge if you give a short explanation here that I can reference if someone else wants to change it back 😁
streams.log.info(s"Generating new GraalVM native-image image based on $baseImage: $imageName") | ||
|
||
val dockerContent = Dockerfile( | ||
Cmd("FROM", baseImage), | ||
Cmd("WORKDIR", "/opt/graalvm"), | ||
ExecCmd("RUN", "gu", "install", "native-image"), | ||
ExecCmd("RUN", "sh", "-c", "ln -s /opt/graalvm-ce-*/bin/native-image /usr/local/bin/native-image"), | ||
ExecCmd("ENTRYPOINT", "native-image") | ||
ExecCmd("CMD", "native-image") |
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.
I will never get the difference in my head. Why is this change from entrypoint to cmd needed?
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.
So the difference between ENTRYPOINT
and CMD
is that the former predetermines the executable that will run in the docker container while the latter just sets it as the default, but allows you to override it at runtime.
I gave it a bit more thought and looked at the available options again and I found a way to return to using ENTRYPOINT
instead.
Originally, the reason why I used CMD
is because in the latest GraalVM images that Oracle provides, bundles the native-image
executable out of the box by default, so there is no longer any real need to create a custom docker image that installs the native-image
executable. Instead, we can just directly use the docker image provided by Oracle. However, in order to keep the docker run
command consistent between both the Oracle provided image and the legacy native-packager generated image, I changed the legacy image to use CMD
instead of ENTRYPOINT
.
But on more research, it seems like there are 2 versions of the image that we can use. graalvm-community
is a generic version that uses CMD
and allows you to choose your executable, and a native-image-community
specialized one that uses ENTRYPOINT
. I tweaked the code such that we can support both images while keeping the previous style of using ENTRYPOINT
as the entry to the docker image.
IIRC there's another, private trait where those parameters can be added. If not, I'm fine with adding those binary compatibility breakages to the mima exceptions. |
Hmm, I don't know if its exactly breaking binary compatibility since the old parameters still work the same as the old versions. According to the CI, it just says that the new param that I added doesn't exist on the older version, which is to be expected. Is there a better, more appropriate way to specify the target platform? |
I'm referencing this validation error: https://github.com/sbt/sbt-native-packager/actions/runs/5718905995/job/15495686495#step:7:1 which indicates binary compatibility issues. What are you referencing to? |
eee74af
to
5928aa8
Compare
947439b
to
3161c3c
Compare
I've fixed the binary compat issue and all the subsequent issues that popped up from the graalvm test. Other unrelated tests are failing, and I assume it is a side effect from upgrading SBT. I'll try to debug the issue when I find some time, but taking a quick look at the github action logs, it is not immediately obvious what's the issue. Any hints would be appreciated, otherwise I might have to create a separate PR for upgrading to SBT 1.9.3 to try and isolate the issue. |
triggered a rerun .. the appveyor thing seems super weird. Do the tests fail on your local machine? |
I tried running |
Closes #1443
This attempts to fill in the gap in cross platform architecture native image packaging. Added a new key
graalVMNativeImagePlatformArch: Option[String]
to specify the target platform architecture. Currently it only supports a single platform as doing multi platform would involve a much bigger change in the code base.While this works technically, the performance is still rather lacking. When I crossed compiled (amd64 -> arm64) compile times took ~19x longer. The simple hello world test took over 5 minutes to produce a binary.
The 5+ minutes test case also triggered a bug in SBT #6771 causing the test to be aborted halfway through, but was recently fixed in SBT 1.9.3 hence the upgrade. However that then necessitated an upgrade in Scala to 2.12.13. Also updated the tests to use the
--no-fallback
option as I noticed that it was generating a fallback image when running the tests.Also, it was a little tricky to correctly detect if an existing container was in the correct platform architecture when
graalVMNativeImagePlatformArch
is not defined. I used a docker image label to get around that.Also added support for newer graalvm containers using a different package name.