sous-chefs / java

Development repository for the java cookbook
https://supermarket.chef.io/cookbooks/java
Apache License 2.0
386 stars 637 forks source link

Extract correct JAVA_HOME from custom URLs #632

Closed detjensrobert closed 4 years ago

detjensrobert commented 4 years ago

Description

This fixes the logic that pulls the JAVA_HOME envvar from the install url. Previously this only worked for the default AdoptOpenJDK Github release urls.

Issues Resolved

Fixes #631.

Check List

ramereth commented 4 years ago

@detjensrobert you might want to see if this happens with the other JDK's we support in this cookbook and fix if needed.

detjensrobert commented 4 years ago

The other JDKs set the home based on the version number, not the download link. They don't appear to be affected since the version number is set separately when using a custom URL.

bkonick commented 4 years ago

@detjensrobert I just tried this out and it worked for AdoptOpenJDK 8 but not 11.

bkonick commented 4 years ago

Here's the output of that split:

irb(main):001:0> url = 'OpenJDK11U-jdk_x64_linux_hotspot_11.0.7_10.tar.gz'
irb(main):002:0> url.split('/')[-1].split('.')[0].split('_')[-1]
=> "11"
irb(main):003:0> url = 'OpenJDK8U-jdk_x64_linux_hotspot_8u265b01.tar.gz'
irb(main):004:0> url.split('/')[-1].split('.')[0].split('_')[-1]
=> "8u265b01"
detjensrobert commented 4 years ago

@bkonick What does the filename for 11 look like? OpenJDK11U-jdk_x64_linux_hotspot_11u###b##.tar.gz? I think I know what the problem is with how I handle the filename. Ah, I see. Let me fix that real quick!

detjensrobert commented 4 years ago

@bkonick I've updated the logic and it should work for newer versions. Please try it again!

bkonick commented 4 years ago

Thanks, @detjensrobert! This worked on both 8 and 11 for me. 🎉

Just a heads up that we don't use other variants (openj9 or openj9 large heap) so I haven't tested those.

bkonick commented 4 years ago

@detjensrobert made a couple inline suggestions to support both openj9 and hotspot. It seems that openj9 large heap is not the same as regular openj9 so an additional check would be needed to account for that.

irb(main):003:0> basename.split('_')[4]
=> "linuxXL"
irb(main):004:0> basename.split('_')[5]
=> "11.0.8"
irb(main):005:0> basename.split('_')[6]
=> "10"
detjensrobert commented 4 years ago

The new logic I added also works for the default urls. Should I remove the old logic and just use the new one to simplify the helper?

detjensrobert commented 4 years ago

(failing test is a package install timeout, not related to my changes)

ramereth commented 4 years ago

The new logic I added also works for the default urls. Should I remove the old logic and just use the new one to simplify the helper?

How do you mean exactly?

detjensrobert commented 4 years ago

@ramereth Since the new logic I added to the sub_url() helper extracts the path from the filename and doesn't care about the URL path at all, it works for all files including the default Github release tarballs. Do we need to keep the old way of extracting the home dir from the Github URL path for the default URLs or can I remove the old logic and use the new logic for all URLs (including the defaults)?

ramereth commented 4 years ago

@detjensrobert as long as it works both ways I don't see a reason for keeping the old logic

ramereth commented 4 years ago

@tas50 @Xorima @bkonick can you please take another look?

bkonick commented 4 years ago

@tas50 @Xorima @bkonick can you please take another look?

LGTM 👍

ramereth commented 4 years ago

Released as 8.3.1.