Add JRE dependency to Java components #11462
Conversation
ab55914 to
3abf962
Compare
207477f to
a60dc53
Compare
There was a problem hiding this comment.
Wow, thanks so much! This is really a great cleanup with all the packages using Java! 🥳
It now really only is an underlying layer of the actually used packages. As soon as none of the packages are installed by default, the image would become way smaller! And you are also ensuring that every tool is using the exact same version of Java they should use. 💯
I added some suggestions to maybe introduce a bit of a devx improvement when using these packages. And I would suggest to use the default version of Java wherever possible to (a) ease upgrades by having a single value to change and test compatibility, and (b) trying to avoid installing multiple versions of Java whenever possible.
| @@ -18,6 +19,8 @@ | |||
| URL_ASPECTJWEAVER = f"{MAVEN_REPO_URL}/org/aspectj/aspectjweaver/1.9.7/aspectjweaver-1.9.7.jar" | |||
| JAR_URLS = [URL_ASPECTJRT, URL_ASPECTJWEAVER] | |||
|
|
|||
| SFN_JAVA_VERSION = "11" | |||
There was a problem hiding this comment.
Same as for KCL, maybe we could avoid setting the version here completely, because it is compatible with Java 11+?
There was a problem hiding this comment.
@alexrashed By explicitly specifying the required JRE version in each downstream package, we remove the dependency on the value of fallback/default version in java package.
This will ensure that we can change the fallback/default version of java any time, without breaking any of the other packages.
There was a problem hiding this comment.
That's exactly the compromise. Either we can change the default version at any time but it's not really used anywhere (because it's explicitly set everywhere). Or we don't set the version wherever it's not explicitly necessary and we can change it in one place to test the upgrade of the default version everywhere with a minimal change.
This is why I would be in favor of only defining a Java version if we know it won't be compatible with Java 11+.
There was a problem hiding this comment.
I'm not sure I'm fully convinced, but I've removed the pins in c9faf3d. We can revisit this at a later point if need arises.
There was a problem hiding this comment.
I think we should come to an agreement though. Could you please elaborate why you are not convinced? I think we should discuss this and come to a unanimous conclusion for this kind of pattern as it's quite fundamental and would pave the way how we are going to handle this case in the future.
There was a problem hiding this comment.
I see these packages not differently from any Linux packaging system. These often have have 'default' version (eg. there might be pre-release versions or older versions of the same application). Yet, downstream packages have exact pinning on its dependency.
There was a problem hiding this comment.
That's an interesting analogy, but I would compare it more with our approach on any other dependency, i.e. our Python dependencies.
We don't really want to pin explicit dependencies, because it would make it harder for us to easily upgrade them. We only limit the range for dependencies where we know that there are incompatibilities. At the same time we do pin an exact version and once a week we run our action to update all lock files, creating a PR where we test the upgrade. If it works, it's great. If not, we can introduce certain version range limits or even exact pins.
For me the current approach here is the same. We don't pin the version in the specific installers (at least in those where we don't know about limitations). But we have the default version set (this would be the version lock in our analogy). We could then create a single PR to update the "lock" / default version and test if the integration works well. If it doesn't work, we can limit the version in certain installers or even move away from the upgrade completely.
In the future, I would also love to see an iteration on the package installer framework to (among other things) implement an automated process to suggest version bumps for package installers.
195aee9 to
c0c1829
Compare
c0c1829 to
06e75df
Compare
|
Currently, only patch changes are allowed on master. Your PR labels (semver: minor) indicate that it cannot be merged into the master at this time. |
Background
#11139 introduced a Java LPM package that can install and manage multiple LTS versions of JRE.
This PR integrates this package across the codebase.
Changes
JavaInstallerMixinclass.