search

halcyon / asdf-java

A Java plugin for asdf-vm.
MIT License
454 stars 86 forks source link

Add the JVM Implementation after the name of the Vendor #140

Closed delgurth closed 3 years ago

delgurth commented 3 years ago

In order to fix https://github.com/halcyon/asdf-java/issues/46 and https://github.com/halcyon/asdf-java/issues/57 I've added the jvm_impl after the vendor.

Unfortunately this will break the names configured previously in .tools-versions if you had selected one of the openj9 JDK's. Not sure how you can do this backwards compatible.

joschi commented 3 years ago

I'm not a big fan of breaking consumer again (sorry for the last time) but I also see the advantage of putting the JVM implementation into the name tag.

This being said, I think GraalVM releases (e. g. graalvm-graalvm-21.0.0.2+java11) would look a bit weird, wouldn't they?

delgurth commented 3 years ago

I'm not a big fan of breaking consumer again (sorry for the last time) but I also see the advantage of putting the JVM implementation into the name tag.

Unfortunately I do not see a proper way of dealing with #46 and #57 without introducing a breaking change. And my hope is that people will understand the added value of this one.

This being said, I think GraalVM releases (e. g. graalvm-graalvm-21.0.0.2+java11) would look a bit weird, wouldn't they?

Good point, missed that one :( Guess graalvm should be in the excemption list, just as hotspot. So only openj9 needs special treatment at this moment I guess?

delgurth commented 3 years ago

@joschi changed it to only do something for openj9. This makes it fragile in case a new jvm_impl is introduced, but I'm no jq hero unfortunately.

halcyon commented 3 years ago

@delgurth tremendous work, thank you! I'll merge it once you've had a chance to resolve conflicts.

delgurth commented 3 years ago

@halcyon thanks. My git says I'm up to date with upstream/master and GitHub says I'm not. Will check what's going wrong in a few hours.

delgurth commented 3 years ago

I was messing up git history. Fixed the history now for this pull request. 2 more to go.