feature: skip the openjdk install when the JVM is already on the image - #248
Conversation
Problem: every sbt job spends ~44 MB and tens of seconds on `apt install openjdk-N-jre`. The machine images already carry JDK 8, 17 and 21, so apt was only upgrading an installed package (17.0.13 -> 17.0.15 on ubuntu-2004). Fix: install only when the JVM directory is absent, and always switch the default. JDK 11 is the only version genuinely missing from the images.
Up to standards ✅🟢 Issues
|
![codacy-production[bot]](https://avatars.githubusercontent.com/in/56611?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull Request Overview
The PR correctly implements an optimization to skip OpenJDK installation when the target version is already present on the image, which should result in faster build times. Codacy analysis indicates the code is up to standards.
However, a portability issue was identified: the path used for the JVM check is hardcoded to amd64. This will cause the check to fail on ARM-based executors (e.g., CircleCI's arm.medium resource class), leading to unnecessary re-installation attempts and potential pathing errors in the update-alternatives step.
Test suggestions
- Verify that installation is skipped when the target JDK directory exists.
- Verify that 'apt update' and 'apt install' are triggered when the target JDK directory is missing.
- Verify that the default Java version is updated correctly in both the skip and install paths.
Prompt proposal for missing tests
Consider implementing these tests if applicable:
1. Verify that installation is skipped when the target JDK directory exists.
2. Verify that 'apt update' and 'apt install' are triggered when the target JDK directory is missing.
3. Verify that the default Java version is updated correctly in both the skip and install paths.
TIP Improve review quality by adding custom instructions
TIP How was this review? Give us feedback
| sudo apt update | ||
| sudo apt install openjdk-${OJDK_VERSION}-jre | ||
| sudo update-alternatives --set java /usr/lib/jvm/java-${OJDK_VERSION}-openjdk-amd64/bin/java | ||
| JVM_DIR="/usr/lib/jvm/java-${OJDK_VERSION}-openjdk-amd64" |
There was a problem hiding this comment.
🟡 MEDIUM RISK
The JVM path is hardcoded to amd64, which breaks compatibility with ARM-based executors. Use dpkg --print-architecture to dynamically determine the correct path for the host architecture.
| JVM_DIR="/usr/lib/jvm/java-${OJDK_VERSION}-openjdk-amd64" | |
| JVM_DIR="/usr/lib/jvm/java-${OJDK_VERSION}-openjdk-$(dpkg --print-architecture)" |
There was a problem hiding this comment.
Not applying. -amd64 was already on the original line 94; this diff only moves it into a variable.
ARM is unreachable here: machine.yml fixes resource_class: large and sbt.yml has no resource_class param. The orb has 8 other amd64 hardcodes, so fixing one implies ARM works when it does not. Own ticket if we ever want it.
A directory can exist without update-alternatives knowing about it, and the --set below would then fail. Testing the alternative tests the actual precondition.
3 participants
Problem: every sbt job spent ~44 MB and tens of seconds on
apt install openjdk-N-jre.The machine images already carry JDK 8, 17 and 21, so apt was only upgrading a package
that was already installed.
Fix: install only when the requested JVM is not already registered, and always switch
the default. JDK 11 is the only version genuinely missing from the images.
Trade-off: the build JDK now stays at the image's patch level, so CI stops picking up
Java security updates. Acceptable for a build box; runtime images are unaffected and
keep their newer CVE-patched versions. Probe: codacy-events pipeline #971.