refactor: extract BaseJRE to eliminate duplication across standard JRE providers#1288
refactor: extract BaseJRE to eliminate duplication across standard JRE providers#1288stokpop wants to merge 6 commits into
Conversation
…E providers Reduces ~1300 lines of near-identical code in 6 JRE files to a single BaseJRE struct (243 lines) plus thin per-JRE wrappers (~11 lines each). BaseJRE uses a template method pattern with injected config/function fields so implementers never need to override Supply/Finalize, eliminating the 'forgot to call base' risk from method-override approaches. Variation points: - dirPrefixes/dirExacts: drive findJavaHome directory matching per JRE - extraFinalizeOpts: hook for JRE-specific JAVA_OPTS (used by IBM JRE) - installErrNote: extra context in install error messages (GraalVM) Zing JRE is intentionally excluded — it has no memory calculator or jvmkill and has different Finalize behaviour. Fixes cloudfoundry#1264
DetermineJavaVersion previously returned 17 silently when the JRE release file was missing. This made it impossible to diagnose broken or incomplete JRE installations. Changes: - DetermineJavaVersion now returns an error for missing/unreadable release files - BaseJRE.Finalize() logs a WARNING when falling back to Java 17 - Tomcat.Supply() logs a WARNING when falling back to Java 17 - Updated tests to reflect the new error behaviour
|
Added a follow-up commit that improves observability when the JRE
This means a broken or incomplete JRE installation will now produce a visible warning in buildpack output rather than silently proceeding with Java 17 assumptions. |
|
a quick ai review. (with my own context/skilss) 🔴 Behavioral Change: tomcat.go — version selection now runs unconditionally when JAVA_HOME is set, even if version detection fails 🔴 Bug: BaseJRE.Supply() now calls WriteEnvFile, AddBinDependencyLink, and LinkDirectoryInDepDir for ALL JREs — previously only OpenJDK did this
🟡 DetermineJavaVersion no longer handles missing release file silently 🟡 Finalize() may use empty b.version 🟡 BaseJRE.Finalize() — WriteJavaOpts with base opts now applies to ALL JREs i' am okay with the things below. i will still do a full review later on |
Previously returned 0 on read/parse errors, which could silently cause incorrect behaviour in callers that don't check the error. Now returns 17 (the current overall default) alongside the error, so callers that check the error can log a warning while callers that ignore it still get a safe value.
…able
When JAVA_HOME is set but the release file cannot be read (e.g. IBM JRE,
non-standard layout, missing file), the previous code silently assumed
Java 17 and selected Tomcat 10.x. This could break Java EE 8 apps running
on Java 8 that expect Tomcat 9.x.
Extract SelectTomcatVersionPattern() and return ("", nil) on detection
failure so the caller falls back to DefaultVersion("tomcat") — matching
the Ruby buildpack behaviour, which always deferred to the manifest default.
An explicitly configured tomcat version is still honoured even without Java
version info.
…buildpack Ruby reference (open_jdk_like_jre.rb): -XX:ActiveProcessorCount is added in the OpenJDK-like JRE release step only. IBM JRE initializer never sets it — IBM JRE already had its own extraFinalizeOpts with -Xtune:virtualized and -Xshareclasses:none (ibm.go, unchanged). Previously -XX:ActiveProcessorCount was in the universal baseOpts, so IBM received it on top of its own opts. Fix: move it out of baseOpts into extraFinalizeOpts on each HotSpot JRE (OpenJDK, Oracle, SapMachine, Zulu, GraalVM). IBM JRE and its existing opts are untouched. Only -Djava.io.tmpdir=$TMPDIR remains in baseOpts — universal in Ruby too.
Add a new section covering three areas found during the BaseJRE refactor review: JAVA_HOME now being set (new feature), Tomcat version auto-selection with fallback to manifest default on error, and -XX:ActiveProcessorCount being HotSpot-only to match the Ruby buildpack (IBM OpenJ9 supports the flag but it is omitted for Ruby compatibility).
|
Thanks for the review! A general note on approach: we tried to match the Ruby 4.x buildpack behaviour as the baseline, only deviating where there are concrete issues or PR comments that justify it. 1. tomcat.go — version detection failure (commit 2c5754b) The Ruby buildpack has no Java-version-aware Tomcat selection — it always defers to the manifest default ( 2. WriteEnvFile / AddBinDependencyLink / LinkDirectoryInDepDir for all JREs I assume this is fine as-is. Ruby never set JAVA_HOME at all — setting it for all JREs is intentional new behaviour in the Go migration (see #1151). I don't think a per-JRE flag is needed here, but open to arguments if there's a specific case where it causes problems. 3. -XX:ActiveProcessorCount for all JREs (commit bf92b62) In Ruby 4. DetermineJavaVersion returns 0 on error (commit 40e4490) Returns Also added some notes in the migration md so things that do change do not go unnoticed. |
Closes #1264
Summary
Reduces ~1300 lines of near-identical code in 6 JRE files to a single
BaseJREstruct (243 lines) plus thin per-JRE wrappers (~11 lines each).Design
BaseJREuses a template method pattern with injected config/function fields so implementers never need to overrideSupply/Finalize, eliminating the 'forgot to call base' risk from method-override approaches.Variation points:
dirPrefixes/dirExacts: drivefindJavaHomedirectory matching per JREextraFinalizeOpts: hook for JRE-specific JAVA_OPTS (used by IBM JRE:-Xtune:virtualized -Xshareclasses:none)installErrNote: extra context in install error messages (GraalVM:ensure repository_root is configured)ZingJREis intentionally excluded — it has no memory calculator or jvmkill and has genuinely differentFinalizebehaviour.Before / After
Tests
Added
standard_jres_test.gocovering all 5 non-OpenJDK standard JREs (OpenJDK already hadopenjdk_test.go). For each JRE:Name()returns the correct display stringDetect()triggers on the correct env var and not on othersVersion()/JavaHome()are empty before installationfindJavaHome(viaFinalize()) locates the JRE-specific subdirectory prefix correctlyAll existing and new tests pass.