Skip to content

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

Closed

Conversation

johnou
Copy link

@johnou johnou commented Aug 6, 2018

Unfortunately the old version detection code returns 0 for 10.0.2 and then causes problems, see spotify/dockerfile-maven#200 for more information.

@johnou
Copy link
Author

johnou commented Aug 6, 2018

cc @hboutemy

@johnou johnou force-pushed the fix_major_version_detection branch from fc2b023 to 9794be8 Compare August 7, 2018 07:56
@plamentotev plamentotev added the bug label Aug 8, 2018
Copy link
Member

@plamentotev plamentotev left a 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" );
Copy link
Member

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.

Copy link
Author

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.

@plamentotev
Copy link
Member

@johnou This is quite tricky to catch issue as with 10.0.2 the build is green although the timestamps are wrong. Many thanks for reporting this.

@plamentotev
Copy link
Member

I'll close this as the Java version does not matter (see #95).

Also the version detection seems to work on Java 10. System.getProperty( "java.specification.version" ) returns 10 even if the JDK version is 10.0.2. The dockerfile plugin just uses old version of Plexus Archiver that uses System.getProperty( "java.version" )) and that is why it does not properly detects the Java version.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants