Skip to content

Fix major Java version detection.#93

Closed
johnou wants to merge 1 commit into
codehaus-plexus:masterfrom
johnou:fix_major_version_detection
Closed

Fix major Java version detection.#93
johnou wants to merge 1 commit into
codehaus-plexus:masterfrom
johnou:fix_major_version_detection

Conversation

@johnou

@johnou johnou commented Aug 6, 2018

Copy link
Copy Markdown

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

johnou commented Aug 6, 2018

Copy link
Copy Markdown
Author

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

@plamentotev plamentotev left a comment

Copy link
Copy Markdown

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.


private static int getJavaVersion()
{
String javaSpecVersion = System.getProperty( "java.specification.version" );

Copy link
Copy Markdown

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
Copy Markdown
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
Copy Markdown

@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
Copy Markdown

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