[MNG-7160] Ability to customize core extensions classloaders

[gnodet]

JIRA issue: https://issues.apache.org/jira/browse/MNG-7160

Following this checklist to help us incorporate your
contribution quickly and easily:

• Make sure there is a JIRA issue filed
  for the change (usually before you start working on it). Trivial changes like typos do not
  require a JIRA issue. Your pull request should address just this issue, without
  pulling in other changes.
• Each commit in the pull request should have a meaningful subject line and body.
• Format the pull request title like [MNG-XXX] - Fixes bug in ApproximateQuantiles,
  where you replace MNG-XXX with the appropriate JIRA issue. Best practice
  is to use the JIRA issue title in the pull request title and in the first line of the
  commit message.
• Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
• Run mvn clean verify to make sure basic checks pass. A more thorough check will
  be performed on your pull request automatically.
• You have run the Core IT successfully.

If your pull request is about ~20 lines of code you don't need to sign an
Individual Contributor License Agreement if you are unsure
please ask on the developers list.

To make clear that you license your contribution under
the Apache License Version 2.0, January 2004
you have to acknowledge this by using the following check-box.

• I hereby declare this contribution to be licenced under the Apache License Version 2.0, January 2004

• In any other case, please file an Apache Individual Contributor License Agreement.

[cstamas]

To me this change looks kinda okay, but also not okay, here is why:

• before change it was delegated, but as core extension realm has no imports applied to it (!), it really has no effect is it "base" realm (parent class-loader in java lingo, no filtering) or "parent" realm (delegates via strategy, filtering applies if set).
• but CoreExports (and corresponding XML file) seems was never applied/used in core extension realm?

This looks something either I am missing, or is simply missing?

[gnodet]

To me this change looks kinda okay, but also not okay, here is why:

• before change it was delegated, but as core extension realm has no imports applied to it (!), it really has no effect is it "base" realm (parent classload in java lingo) or "parent" realm.

Not really, when no imports are specified for the parent realm, the realm has full visibility on its parent.

• but CoreExports (and corresponding XML file) seems was never applied/used in core extension realm?

The xml extension is not used, that's right.

This looks something either I am missing, or is simply missing?

Yes, what this commit does is to simply change from self-first to parent-first class loading, that's all. This avoids the ClassCastException because there will be a single Xpp3Dom class loaded.

If this change is not acceptable for compatibility reason, one possibility would be to extend the extension model to be able to specify self-first or parent-first strategy as an xml attribute on the extension.

[cstamas]

This makes sense to me, just to repeat: core extension (had) have "unfiltered" access to maven being extended, along with all the classes being present in maven-core realm. Essentially, this is the case even today (no CoreExports applied), but due SelfFirst strategy CCEx happend? And after this change will not.

[gnodet]

This makes sense to me, just to repeat: core extension (had) have "unfiltered" access to maven being extended, along with all the classes being present in maven-core realm. Essentially, this is the case even today (no CoreExports applied), but due SelfFirst strategy CCEx happend? And after this change will not.

Yes, that's it.

[rmannibucau]

Change looks good to me but can be great to test some more real world extensions (the ones I tested don't abuse of dependencies enough).

[jvanzyl]

Where are the projects you're testing that demonstrate the Xpp3Dom loading issue? For any extensions I've made that used core dependencies I've marked as provided and I don't have any issues. But in your case you are not marking them as provided and this is when you see the CCE? That's why I just wanted to look at your setup, as it seems like it's the same thing being done in Provisio where there's no issue with 3.x.

[gnodet]

Where are the projects you're testing that demonstrate the Xpp3Dom loading issue? For any extensions I've made that used core dependencies I've marked as provided and I don't have any issues. But in your case you are not marking them as provided and this is when you see the CCE? That's why I just wanted to look at your setup, as it seems like it's the same thing being done in Provisio where there's no issue with 3.x.

Ah, I think the problem comes from the way the extension's artifacts are resolved. When I put the plexus-utils in scope provided, this is not sufficient and I also have to explicitly exclude it from transitive dependencies to get it working.

[jvanzyl]

Using the assembly plugin?

[gnodet]

Using the assembly plugin?

No, the artifacts list comes from https://github.com/apache/maven/blob/master/maven-embedder/src/main/java/org/apache/maven/cli/internal/BootstrapCoreExtensionManager.java#L139

[gnodet]

I've rewritten the PR to allow the user to choose the class loading strategy and added an integration test (apache/maven-integration-testing#125) showing the 4 possibilities and the differences in class loading.

I'm not sure if the fact that even when a dependency with an explicit provided scope is defined, the same dependency leaks through as a transitive dependency is a problem or not. Thoughts ?

[jvanzyl]

I know you've gone through the effort here to get the caching extension to work from lib/ext but is that how you envision it working? If you have a large organization with many developers I think it would be preferable to configure it from the POM no? Or does this extension do some capabilities earlier than an extension in the form of a plugin?

I realize the way extensions in lib/ext versus being defined in a plugin have always worked differently, and I can't off the the top of my head remember why we did that, but maybe they should be aligned so that artifacts are resolved in the same way.

[gnodet]

I know you've gone through the effort here to get the caching extension to work from lib/ext but is that how you envision it working? If you have a large organization with many developers I think it would be preferable to configure it from the POM no? Or does this extension do some capabilities earlier than an extension in the form of a plugin?

I realize the way extensions in lib/ext versus being defined in a plugin have always worked differently, and I can't off the the top of my head remember why we did that, but maybe they should be aligned so that artifacts are resolved in the same way.

Currently, they do not work exactly the same. Some beans are discovered a bit too early in the process. For example EventSpy implementations can't be registered inside an extension discovered from the POM afaik. This is also true for a few other beans.

The new plugin class loading strategy that I propose aims at reconciling the class loaders difference. I also plan to raise a PR so that some beans are discovered later in the process inside the @SessionScoped, such as MojoExecutor and related beans. This would allow the caching extension from #607 to be registered in the POM, which is not currently possible.

[gnodet]

This would allow the caching extension from #607 to be registered in the POM, which is not currently possible.

Registering the extension in the POM is also blocked by #618 and the ability to use org.eclipse.sisu.Priority.

[michael-o]

If this is going into 3.9.0, make sure to adjust the IT too.