Skip to content
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

Support Java 11 or later #30

Merged
merged 1 commit into from
Dec 23, 2024
Merged

Support Java 11 or later #30

merged 1 commit into from
Dec 23, 2024

Conversation

komamitsu
Copy link
Contributor

@komamitsu komamitsu commented Dec 19, 2024

This PR fixes #29.

The current implementation tries to cast jdk.internal.loader.ClassLoaders$AppClassLoader to java.net.URLClassLoader. It works in Java 8, but doesn't work in Java 11 or later since ClassLoaders$AppClassLoader no longer inherit java.net.URLClassLoader.

The PR also upgrades the version of Guice since version 4 doesn't work with Java 17 or later.

I confirmed if this change works as follows:

@komamitsu komamitsu self-assigned this Dec 19, 2024
@yito88
Copy link
Member

yito88 commented Dec 19, 2024

Thank you! Can we test Kelpie with all Java versions we support in the CI?

java-version: 1.8

@komamitsu
Copy link
Contributor Author

@yito88 I'm okay to update the workflow file to execute ./gradlew build with multiple Java versions. But I'm not sure if it makes sense, because:

  • It seems Kelpie has only com.scalar.kelpie.config.ConfigTest. The original issue occurs when com.scalar.kelpie.modules.ModuleLoader#loadModule is evaluated. So, executing ConfigTest even with multiple Java versions doesn't guarantee Kelpie works in Java 11 or later.
  • The original issue occurs when com.scalar.kelpie.modules.ModuleLoader#loadModule is evaluated in Java 11 or later. So, I guess simple unit tests won't be enough. Probably we should add integration tests which builds a Jar file and executes it via Kelpie executable somehow.

What do you think?

@yito88
Copy link
Member

yito88 commented Dec 19, 2024

Right, we could add some integration tests. How about running small ones with scalar-labs/kelpie-test in the CI?

Copy link
Member

@yito88 yito88 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The CI stuff could be separated from this PR

@komamitsu
Copy link
Contributor Author

@yito88 Thanks for the suggestion! It sounds good. I'll try it when I have a time later.

Copy link
Collaborator

@brfrn169 brfrn169 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thank you!

Copy link
Contributor

@feeblefakie feeblefakie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thank you!

@feeblefakie feeblefakie merged commit aeb20eb into master Dec 23, 2024
2 checks passed
@feeblefakie feeblefakie deleted the support-java-11 branch December 23, 2024 12:19
@komamitsu komamitsu mentioned this pull request Dec 25, 2024
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.

Kelpie doesn't work with Java 11
4 participants