-
Notifications
You must be signed in to change notification settings - Fork 49
Fix major Java version detection. #93
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
Conversation
cc @hboutemy |
fc2b023
to
9794be8
Compare
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 think the bigger issue here is that the Zip entries are with wrongly rounded timestamps - the entry may end up with timestamp bigger than the original file and thus causing the entry to appear newer than the original and as a result causing problems (probably unlikely but still). And there are no tests to verify that the proper rounding for the Zip entries timestamps is used. Would you like to add tests for that? If you don't have time I can add some tests.
@@ -136,15 +136,18 @@ | |||
|
|||
private static int getJavaVersion() | |||
{ | |||
String javaSpecVersion = System.getProperty( "java.specification.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.
Looks like this code proves to cause more trouble than it is worth. Once Java becomes the minimum required version isJava7OrLower
will be deleted. I'm not sure it is worth to try to keep up with the Java versioning changes just for that. We can just change:
private static final boolean isJava7OrLower = getJavaVersion() <= 7;
to
private static final boolean isJava7 = System.getProperty( "java.specification.version" ).startsWith( "1.7" );
As the Java 7 is the minimum required version we know that if it is not Java 7 then it is newer version. Also whenever the Java version string schema will be for Java version higher than 7 it will not begin with "1.7". So this code will work as long "java.specification.version" is not an empty string. We can add check for that and throw an exception - you know just in case.
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.
Given Java 7 is the exception, I would be reluctant to throw an exception if it's empty, but instead to try and detect 1.7 and if that fails fallback to the normal code path.
@johnou This is quite tricky to catch issue as with |
I'll close this as the Java version does not matter (see #95). Also the version detection seems to work on Java 10. |
Unfortunately the old version detection code returns 0 for 10.0.2 and then causes problems, see spotify/dockerfile-maven#200 for more information.