[MPOM-337] load plugins containing lifecycle mappings with extensions

[kwin]

https://issues.apache.org/jira/browse/MPOM-337

[hboutemy]

-1
having every plugin as extension just bloats and takes risk in the Maven core classloader
https://issues.apache.org/jira/browse/MNG-5697 is definitely not ready for general use

Edit: wrong analysis, mixed Core Extensions and Build Extensions, see https://issues.apache.org/jira/browse/MNG-7574

[kwin]

@hboutemy What is the ultimate goal here? Not requiring plugins coming with lifecycle mappings to be loaded as extensions and still consider their mappings? What exactly is the concern you have in case those few are loaded as extensions?

[gnodet]

This issue looks legit to me wrt to MNG-5697. @hboutemy do you mind explaining your position ?

[kwin]

@gnodet I opened https://issues.apache.org/jira/browse/MNG-7574 to address the concern of @hboutemy. Hopefully soon the plugins themselves identifies those classes which should be exposed through the Core Classloader which would make this PR obsolete (but this would require a recent Maven version and an updated plugin version containing the new descriptor exposing the extension classes).

[hboutemy]

This issue looks legit to me wrt to MNG-5697. @hboutemy do you mind explaining your position ?

Robert opened MNG-5697 as a long term target, that shows an ideal situation where plugins can provide their lifecycle mapping themselves instead of having core define everything for them https://maven.apache.org/ref/3.9.1/maven-core/default-bindings.html

perfect objective.

Has Maven core the features required for that? NO.

Why is this "plugin as extension" feature not ok?
Marking a plugin as an extension puts all its classes and dependencies in Maven core classloader: this obviously covers the wanted lifecycle binding from the plugin, I understand the idea from this PR.
But this also covers every dependency from the plugin: classloaders from plugins marked as extension are not isolated any more, Maven core classloader mixes dependencies from everything; This is a recipe for conflicts: per-plugin classloader was exactly done to avoid such issue https://maven.apache.org/guides/mini/guide-maven-classloading.html

for MNG-5697 to work, there are multiple Maven core feature required:

1. contributing only partial content of a plugin to core classloader
2. a way to load this early in the build lifecycle: IIRC, the current class loading of a plugin happens late, but for contributing a new lifecycle binding requires to happen very early: This is what is described in the Jira issue in bold Maven core should then get the lifecycle mappings and artifact handlers configurations from the plugin at the beginning of the build, before begin able to calculate the build plan

For plugins that have their bindings coded in Maven core, this is hard to see because you benefit from default bindings, and it's not to check that it's plugin-provided binding that has been used instead of core one
Try with a plugin that has no default binding and this "early load" aspect will be very visible

[hboutemy]

notice: re-reading https://maven.apache.org/guides/mini/guide-maven-classloading.html#build-extension-classloaders , perhaps I'm confused on "Build Extension Classloaders" vs "Core Classloader": that would be a good thing, because that would solve one of my concerns
Edit: confirmed in https://issues.apache.org/jira/browse/MNG-7574

the concern on early load remains

[hboutemy]

one case to test is maven-archetype packaging: it's not in Maven core, then it's visible if it's available in a build context or not

our doc on declaring it is https://maven.apache.org/archetype/archetype-packaging/ : the packaging is a separate artifact declared as extension

you can try to add the packaging artifact as an additional dependency of m-archetype-p and declare the plugin as an extension, and you should see that the packaging is not available when the build plan is calculated

[kwin]

the concern on early load remains

But this only affects components and I fail to see the negative impact as most (if not all components) are actually necessary to register custom types.

declare the plugin as an extension, and you should see that the packaging is not available when the build plan is calculated

Sorry, I cannot follow here. Can you elaborate a bit how this is related to this PR?

[hboutemy]

ok, I've not been clear, sorry. I'll try to explain it another way.
I suppose that by marking the plugins as extensions you expect that the bindings defined in the plugin will be used: this is not the case, the bindings defined in Maven core remain used to calculate the buildplan.

simply put, MNG-5697 does not work yet, and this PR does as if it worked (with a confusion between bindings provided by Maven core vs bindings that should be provided by the plugin)

that's why I say that once we prove that it works for a packaging plugin that is not provided by Maven core (like archetype), we'll see how we transition for the currently Maven core provided ones
But you can also try with a custom build of Maven core where jar packaging bindings have been removed: it's just harder

[kwin]

But before removing bindings from core they should be loaded through plugin extensions (not the other way around). This PR prepares for future Maven version which do no longer include those bindings in core.

[hboutemy]

yes, we agree: removing is far future
first step is proving with archetype (or another if you have another) that marking a plugin that has default bindings for a packaging as extension works

[kwin]

I use maven-bundle plugin and https://jackrabbit.apache.org/filevault-package-maven-plugin/. Both use custom types and bindings and everything works fine when they are loaded with extensions. Don’t know why your example with archetype is different.

[ctubbsii]

I'm not really sure I understand the purpose of the extension element here, or how that relates to future changes to lifecycle mappings. However, I only expect the Apache parent POM to do version management for my plugins. I don't expect it to be responsible for execution decisions. Shouldn't these go in the child project's POM's build/plugins/ sections, and not in this parent POM's build/pluginManagement/plugins/ section?

[kwin]

I don't expect it to be responsible for execution decisions.

The loading with extensions has no impact on goal execution.

[ctubbsii]

I don't expect it to be responsible for execution decisions.

The loading with extensions has no impact on goal execution.

I wasn't referring to goal executions. I was referring to the explicit choice to execute whatever the <extension /> is causing to be executed. If that element has any execution (or effect, if you prefer) whatsoever, then it is a choice being made on behalf of users. That's new. Currently the only choice being made for users is the version. This is the same reason I wouldn't put <scope>test</scope> on a dependency in <dependencyManagement> (because that forces users to explicitly specify <scope>compile</scope> to override, when that's usually understood to be the default). The scope should be declared where the plugin is executed, not where its version is being managed. It seems like a similar principle is applicable here.

[kwin]

For me the decision whether a plugin should be loaded with or without extensions should primarily be driven by the plugin developer (not by the consumer). In general plugins usually don't work correctly if they ship with extensions but they aren't loaded. Compare also with https://issues.apache.org/jira/browse/MNG-7572. The fact that this needs to be specified during loading and is not yet given by the plugin descriptor I consider a flaw in the design.

Do you have a use case where it makes sense that a plugin is loaded without extensions although it provides one?

[ctubbsii]

For me the decision whether a plugin should be loaded with or without extensions should primarily be driven by the plugin developer (not by the consumer).

I agree.

The fact that this needs to be specified during loading and is not yet given by the plugin descriptor I consider a flaw in the design.

I agree with that, too.

Do you have a use case where it makes sense that a plugin is loaded without extensions although it provides one?

Not off hand, but if it's configurable by a user, I imagine that there's use cases where a plugin's execution is functionally altered by that configuration. Otherwise, why is this even made available for configuration by users? So long as it's configurable by users, I just seems sensible that configuration should be determined by the user in their <build><plugins> section and not in the parent POM's <build><pluginManagement><plugins> section. But, as I said originally, I don't really understand the need for this flag at all. Right now, my understanding is that it doesn't really do anything. So I'm not even sure why it's being added.

[gnodet]

Otherwise, why is this even made available for configuration by users?

I think https://issues.apache.org/jira/browse/MNG-598 has the answer. It seems this decision has been made as a workaround to avoid having to scan all plugins for packaging handlers. It seems to me that this was a technical decision rather than a design one.

[kwin]

It seems to me that this was a technical decision rather than a design one.

For me the underlying design principle is that all plugins not being loaded with extensions true only ever load mojo components but nothing else (kind of a poor man's sandbox :-)).

[hboutemy]

as written in https://issues.apache.org/jira/browse/MNG-5697 , I think packaging and artifact handlers (aka dependency types) are 2 separate topics that have been conflated until now but should really be decoupled

[kwin]

I think packaging and artifact handlers (aka dependency types) are 2 separate topics

This is fine, but doesn't have any impact on this PR, does it?

[kwin]

@hboutemy Do you still have concerns with merging this? If so can you please elaborate on the why?