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

[MNG-8015] Control the type of path where each dependency can be placed #1401

Merged
merged 1 commit into from Mar 1, 2024

Conversation

gnodet
Copy link
Contributor

@gnodet gnodet commented Feb 7, 2024

#1378 merged with master and pushed to maven so that we can actually collaborate on it (as the branch for #1378 is not writable)

@gnodet
Copy link
Contributor Author

gnodet commented Feb 7, 2024

@desruisseaux fwiw, the failing test is an integration test. which belongs to the https://github.com/apache/maven-integration-testing repository and not https://github.com/apache/maven ...

@gnodet gnodet changed the title Explicit module path [MNG-8015] Control the type of path where each dependency can be placed Feb 7, 2024
@cstamas
Copy link
Member

cstamas commented Feb 7, 2024

By looking at this PR, one thing occurred to me: today resolver-only users (those integrating Resolver w/o Maven) are able to fully resolve things as Maven would. If any of these become implemented in maven-core (as opposed to maven-resolver-provider), this "functional equivalence" is gone: JPMS will become handled by Maven, but Resolver integrators will not be able to resolve projects that do use these new functionalities. We need to keep eye on this fact.

@gnodet
Copy link
Contributor Author

gnodet commented Feb 7, 2024

By looking at this PR, one thing occurred to me: today resolver-only users (those integrating Resolver w/o Maven) are able to fully resolve things as Maven would. If any of these become implemented in maven-core (as opposed to maven-resolver-provider), this "functional equivalence" is gone: JPMS will become handled by Maven, but Resolver integrators will not be able to resolve projects that do use these new functionalities. We need to keep eye on this fact.

Ideally, we'd come up with a way to use Maven 4 API outside of maven...

@cstamas
Copy link
Member

cstamas commented Feb 7, 2024

Ideally, we'd come up with a way to use Maven 4 API outside of maven...

That sound awkward to me 😄 like saying "we will make servlet-api usable without servlet engine", so something will need to implement it. And if not Maven, what?

@gnodet
Copy link
Contributor Author

gnodet commented Feb 7, 2024

Ideally, we'd come up with a way to use Maven 4 API outside of maven...

That sound awkward to me 😄 like saying "we will make servlet-api usable without servlet engine", so something will need to implement it. And if not Maven, what?

Maven has all the container side: parsing the projects, analyzing the build and launching all the mojos in sequence. That part is not really in the Maven 4 API which is about using maven model and resolver (well, apart from the Project). But if we can provide a Session, the users should be able to use it, even if not running inside a plugin. I don't see how it's really different from "running maven resolver outside of Maven, but with the model and the resolver-provider", i.e. Maven behaviour... without Maven.

@rmannibucau
Copy link
Contributor

While I understand the technical relevance we should also likely discourage it since one of the goal is to own and not conflict without anything possible.
That said, this PR is way outside the scope of resolver users outside maven IMHO, it is very opiniated so shouldn't be a big blocker we cant compensate by a few standalone impl in practise as it is already the case, no?

@desruisseaux
Copy link
Contributor

I'm not familiar with the relationship between Maven and Resolver. But if the maven-resolver-provider is used outside Maven with the expectation that it provides the same result, maybe we should move the PR there? It is not opinionated, as it is based on the intersection of two supplied information:

  • Which paths a JAR can be placed (information supplied by the JAR producer).
  • Which paths a plugin wants to use (information supplied by the JAR consumer).

The only heuristic part is what to do when the intersection of the above contains 2 or more paths.

@gnodet gnodet force-pushed the explicit-module-path branch 4 times, most recently from 0477c52 to b4caaeb Compare February 10, 2024 10:59
@desruisseaux
Copy link
Contributor

As a real world use case for testing the Maven improvements, there is the following scenario: a project depends on JUnit 5 with compile scope (because the project is a conformance test kit). Everything is JPMS: the project, JUnit 5 and their dependencies. JUnit 5 is itself splitted in many modules, including the junit-jupiter-api, junit-jupiter-engine and junit-platform-commons modules.

For a mysterious reason, Maven 3 places junit-platform-commons dependency on the module path, but junit-jupiter-engine on the class-path (verified with mvn test --debug). The consequence is that JUnit 5 cannot be launch. The JVM refuses to start with the following error message: "class org.junit.platform.engine.discovery.DiscoverySelectors (in unnamed module @0x3b2c72c2) cannot access class org.junit.platform.commons.util.Preconditions (in module org.junit.platform.commons) because module org.junit.platform.commons does not export org.junit.platform.commons.util to unnamed module @0x3b2c72c2". Translation: JUnit 5 cannot access its own internal classes!

The reason why JUnit 5 cannot access its own classes is because junit-platform-commons allows access to its packages only to other JUnit 5 modules. It can be seen with the following command:

jar --describe-module -f junit-platform-commons-1.10.2.jar

Output snippet (reformatted):

qualified exports org.junit.platform.commons.util to
     org.junit.jupiter.api
     org.junit.jupiter.engine
     etc...

Because Maven puts junit-platform-commons on the module-path, access restrictions to that JAR are enforced. But because Maven puts junit-jupiter-engine on the class-path, that module is unnamed and consequently not recognized as a module in above list of modules allowed to access the junit-platform-commons internal packages. Thus the failure to launch JUnit.

A workaround is to add the following lines in Surefire configuration for breaking module encapsulation:

<argLine>
  --add-exports org.junit.platform.commons/org.junit.platform.commons.util=ALL-UNNAMED
  --add-exports org.junit.platform.commons/org.junit.platform.commons.logging=ALL-UNNAMED
</argLine>

We should not be forced to apply such workaround. This real use case is another demonstration of the need to improve JPMS handling compared to what Maven 3 does. For testing if this pull request achieves that goal in Maven 4, we can test if it allows us to remove the above hack from GeoAPI conformance pom.xml file.

@rmannibucau
Copy link
Contributor

@desruisseaux I'm not sure I understand the proposal, the "workaround" is needed per java, do you want surefire to do it implicitly or just use the right path? Isn't it solved by the type work you did and Guillaume reuses in maven 4? (trying to convert the statement in actions)

@desruisseaux
Copy link
Contributor

The workaround is not needed per Java. It is needed because Maven 3 mixes class-path and module-path in an apparently random way. It is an example of case where putting the JAR on the class-path breaks the application. Just putting the junit-jupiter-engine.jar on the module-path, as it should be, fixes the issue and removes completely the need for the workaround.

This problem should indeed by solved by the path work. I reported this story as a real use case for testing that the proposal works. The criterion for considering that test as successful is to be able to remove any hacks in above-cited pom.xml file.

@rmannibucau
Copy link
Contributor

Ok, so no action needed.

side note: it is not random but an heuristic so does not always work indeed - don't ask me why default is not 100% the classpath, it is statically saner for 3rd parties even if it enforces more work for jpms apps, but we see the light so we'll drop it soon 🤞 .

@gnodet
Copy link
Contributor Author

gnodet commented Feb 29, 2024

Should we go ahead and merge that one ? It does not really affects the current behaviour, so if we even find something missing, we can still fix it later...

@desruisseaux
Copy link
Contributor

Fine for me. I added one more commit for allowing plugins to log warning to users when a dependency cannot be placed on any path.

After the merge, I would like to try the new API in maven-compiler-plugin and create a new merge request for it.

@gnodet
Copy link
Contributor Author

gnodet commented Mar 1, 2024

Fine for me. I added one more commit for allowing plugins to log warning to users when a dependency cannot be placed on any path.

After the merge, I would like to try the new API in maven-compiler-plugin and create a new merge request for it.

Sounds good. You'll need to build the following branches:

The above maven-compiler-plugin PR can be used as a basis, as it is already has all the migration to the v4 api. If the underlying plexus-compiler API does not support the needed features to fully leverage the new jpms support, a possibility would be to change the m-compiler-p to simply wrap the javac compiler instead of trying to abstract all implementations. The other one is the eclipse compiler, not sure they are really others. If needed, we'd end up with two plugins, but they may ease the mapping of various options... We discussed that with @cstamas ...

@gnodet gnodet merged commit 9780ca1 into master Mar 1, 2024
31 of 34 checks passed
@cstamas
Copy link
Member

cstamas commented Mar 1, 2024

WooHoo! 🎆

@cstamas cstamas deleted the explicit-module-path branch March 1, 2024 17:29
@desruisseaux
Copy link
Contributor

Thanks! I will try to start playing with the compiler plugin this weekend.

@desruisseaux
Copy link
Contributor

If the underlying plexus-compiler API does not support the needed features to fully leverage the new jpms support, a possibility would be to change the m-compiler-p to simply wrap the javac compiler instead of trying to abstract all implementations.

I have not explored the Plexus compiler capabilities, but indeed I propose to use javax.tools.JavaCompiler instead. It is an interface, so it would hopefully be possible to create a wrapper around the Eclipse compiler. Maybe Eclipse would even do that themselves, since javax.tools.JavaCompiler is standard API.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants