-
Notifications
You must be signed in to change notification settings - Fork 18
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
Make the compiled package runnable on Java 7 or any later version #38
Conversation
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.
The solution LGTM - based on the few things I know about Maven, profiles are the right thing to use here. A couple of minor suggestions below.
<maven.compiler.source>1.7</maven.compiler.source> | ||
<maven.compiler.target>1.7</maven.compiler.target> |
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.
For anyone else who doesn't remember - we're bumping the minimum Java version from 6 to 7, because the runtime was actually already incompatible with Java 6. See #34 (comment)
11c265c
to
fa31fb3
Compare
@dgelessus Thanks for your feedback! |
fa31fb3
to
b5ef30b
Compare
Can confirm it works as expected on my machine as well. 🙂 Although at first I ran into an error during Full stacktrace
I was able to fix that by updating the |
Fixes a java.lang.ExceptionInInitializerError on Java 17 when running `mvn package`, see #38 (comment)
Thanks for looking into this, I've just did exactly that! |
Fixes a java.lang.ExceptionInInitializerError on Java 17 when running `mvn package`, see #38 (comment)
3924179
to
4111120
Compare
<plugin> | ||
<groupId>org.apache.maven.plugins</groupId> | ||
<artifactId>maven-compiler-plugin</artifactId> | ||
<version>3.10.1</version> |
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.
Just noting why I updated to this version: I had to update maven-compiler-plugin
at least to 3.6 (it was 3.1), see https://maven.apache.org/plugins/maven-compiler-plugin/compile-mojo.html#release:
<release>
The -release argument for the Java compiler, supported since Java9
- Type:
java.lang.String
- Since:
3.6
- Required:
No
- User Property:
maven.compiler.release
At first, I thought I would update to some older version, e.g. 3.6.2
, but I saw this at https://mvnrepository.com/artifact/org.apache.maven.plugins/maven-compiler-plugin/3.6.2:
Vulnerabilities | Vulnerabilities from dependencies:CVE-2022-29599 CVE-2020-15250
|
---|
It may or may not be serious or relevant, but I thought it would be easier and safer to just raise the requirement to a later version. But 3.8.1
has these vulnerabilities still listed, and the oldest version where this is apparently fixed is 3.9.0
. I have finally decided to update to the latest version 3.10.1
(it's not such a leap anymore) and it still works, as far as I can tell (see https://github.com/generalmimon/ks-java-runtime-test/actions), so it seems fine to me.
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 was going to suggest that after merging, we should make a small patch release 0.9.1 to restore Java 7 compatibility for the release version of the compiler. But I'm not sure if we can do that easily - if I'm reading it correctly, commit b83000e is potentially backwards-incompatible with the 0.9 compiler.
I actually wanted to release KS 0.10 in the nearest future (that's why I am hunting down the issues more I am able to publish almost everything myself; however, Maven Central is about the only thing I don't have access to. So perhaps I can do that regardless, and hopefully he'll follow up with that Maven Central repo afterwards (I have no idea when, though). |
Fix #34
I checked that using the new
pom.xml
, the package compiled bymvn package
is compatible with Java 7 and Java 8 (in particular, the problem reported in #34 no longer occurs) regardless of the Java version used for compilation, unlike the oldpom.xml
- here is the summary of test results performed in a GitHub Actions workflow:Before (
generalmimon:set-jdk-7-in-pom-xml
):✔️
✔️
✔️
✔️
✔️
❌
❌
✔️
✔️
✔️
❌
❌
✔️
✔️
✔️
❌
❌
✔️
✔️
✔️
After (
kaitai-io:build-for-java-7
):✔️
✔️
✔️
✔️
✔️
✔️
✔️
✔️
✔️
✔️
✔️
✔️
✔️
✔️
✔️
✔️
✔️
✔️
✔️
✔️