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

Added JPMS payload support (#511) #542

Open
wants to merge 13 commits into
base: master
Choose a base branch
from
Open

Added JPMS payload support (#511) #542

wants to merge 13 commits into from

Conversation

DaSpood
Copy link
Collaborator

@DaSpood DaSpood commented Aug 12, 2024

#511

Only the 4 following commits from mag/jpms were identified as actually related to the JPMS changes:

Current problems:

  • In order to properly instantiate JDBC drivers, we need to remove the driverClassName key from the resource definition xml. When it is present, Tomcat will use reflection to instantiate drivers, which will fail if the driver implements JPMS properly. When the key is missing, Tomcat calls the DriverManager which is the wanted behavior. For now this is only an issue with the HSQLDB driver (driverClassName="org.hsqldb.jdbcDriver"), not tested yet for other drivers. This change has not been commited yet.

  • In ModuleManager (in jqm-runner), at the end of the method, resolveAndBind will throw an exception if one of the modules present in paths already existed in parentModuleLayer (for example slf4j), with ResolutionException: Module XXX reads more than one module named YYY. Manually removing the slf4j jar from the paths array solved it but we need to find a way to automatically detect and remove duplicates, which sounds like a lot of ugly code to write because it is hard to match a file system path to a module name without calling moduleFinder.findAll() on each path first.

  • After the above error is fixed, some JPMS tests still fail due to the content of the test jar: ServiceConfigurationError: com.enioka.jqm.tests.jpms.api.TestService: Provider com.enioka.jqm.tests.jpms.services.TestServiceImpl1 not found. It may simply be a module-info.jar mistake, did not have time to investigate further.

  • There is a regression with an existing non-JPMS test: EngineHiddenJavaClassesTest.testHiddenBigInteger, which expected a KO and got an OK result. No time to investigate yet. Because these are integration tests this may be a side effect of other failures related to the errors above.

@DaSpood DaSpood marked this pull request as draft August 12, 2024 15:26
@marcanpilami
Copy link
Contributor

Problem 1: we agree on the solution. It even works outside JPMS (the DriverManager will select the correct driver according to the URL, which contains the codename of the driver). I have corrected the resource file just for the tests.

Problem 2: not really an issue. Indeed we must avoir loading a module inside a layer when the parent already contains it. This is imposed by the JPMS norm. So I have added n attempt at doing so, see commit.

Problem 3: that was because you had not taken some class loader code in the original branch. URLClassLoader by default has no idea how to deal with modules, a method must be overloaded for this.

Problem 4: actually this test cannot exist in a JPMS world - what it does is explicitely what JPMS tries to make impossible. It was commented in the oirignal branch.

@marcanpilami marcanpilami changed the title #511 - Port mag/jpms to JQM v3 Added JPMS payload support (#511) Aug 17, 2024
@DaSpood DaSpood marked this pull request as ready for review September 3, 2024 11:10
@marcanpilami marcanpilami changed the base branch from noosgi/rebase to master October 22, 2024 08:45
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