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

PoC/experiment: new scope "import-plugins" #1190

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

cstamas
Copy link
Member

@cstamas cstamas commented Jun 28, 2023

Introduces new scope very similar to "import" but this one is "import-plugins" and as it's name says, imports pluginManagement.

To test it:

Enjoy!

Introduces new scope very similar to "import" but this one is
"import-plugins" and as it's name says, imports pluginManagement.

To test it:
* build this PR
* use this POM https://gist.github.com/cstamas/8668205f68b0eaf42c8c34bba7d080a4
* invoke `mvn help:effective-pom`
@cstamas cstamas self-assigned this Jun 28, 2023
@talios
Copy link

talios commented Jun 28, 2023

@cstamas This is interesting and could almost be a migration path for tiles-maven-plugin - I wonder if having a new scope is good tho, rather than reusing the existing import scope, but included in the <pluginManagement/> section itself.

I'll pull the PR in the morning and play with it.

@hboutemy
Copy link
Member

hboutemy commented Jun 28, 2023

see https://issues.apache.org/jira/browse/MNG-5588
like @talios : why not simplly use import for pluginManagement like it is done for dependencyManagement?
answer from MNG-5588: there is no scope in pluginManagement to abuse
in MNG-5588, I abused instead the inherited scope... (I can't find my implementation for now...) https://issues.apache.org/jira/browse/MNG-5588?focusedCommentId=17318534&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-17318534

@cstamas
Copy link
Member Author

cstamas commented Jun 28, 2023

Um, inherited=import does not look intuitive at all to me :) while scope import-plugins does, and tells at first sight what it does IMHO. Also, as existing "import", the new scope is special is same way: is valid only where other "import" is.

@hboutemy
Copy link
Member

putting a plugin-related scope on a dependencyManagement does not look intuitive either

@cstamas
Copy link
Member Author

cstamas commented Jun 28, 2023

"plugin-related scope"? Plugins does not have scopes 😄

OTOH, if you look at original definition of "import" scope: https://maven.apache.org/guides/introduction/introduction-to-dependency-mechanism.html#dependency-scope

"This scope is only supported on a dependency of type pom in the <dependencyManagement> section. It indicates the dependency is to be replaced with the effective list of dependencies in the specified POM's <dependencyManagement> section. Since they are replaced, dependencies with a scope of import do not actually participate in limiting the transitivity of a dependency."

The new "import-plugins" works exactly the same, except just replace dependencyManagement with pluginManagement. So, is even intuitive in this way as well, and devs will understand it easier:

"This scope is only supported on a dependency of type pom in the <dependencyManagement> section. It indicates the dependency is to be REMOVED and the effective list of plugins in the specified POM's <pluginManagement> section ADDED. Since they are REMOVED, dependencies with a scope of import do not actually participate in limiting the transitivity of a dependency."

@cstamas
Copy link
Member Author

cstamas commented Jun 28, 2023

@talios @hboutemy also, what is the problem with adding new scope (that is special,in very same way as existing "import" scope is special)?

@cstamas
Copy link
Member Author

cstamas commented Jun 28, 2023

And an idea from comment on MNG-5588: IF you are importing a POM, why not import everything? (And then need for new scope really goes away). Users can prepare POMs with parts they want (depMgt only, pluginMgt only, or both) and import.

(but this carries potential of breaking builds, that do import some POM for it's depMgt but are not interested, or worse, would break them if pluginMgt is imported...)

@rmannibucau
Copy link
Contributor

The issue with "import" magic is that very quickly you will also need the conflict resolution management until you import a single piece which is basically a parent.

Think extensions are a nice way to configure plugins (bad for dependencies since this part must always load in an IDE without code execution but it is not the case for plugins) but for the case you want to inherit from something done outside we could just use xml references, which would enable to import a template and customize it inline instead of importing as such which often leads to issues.

So from my window we shouldn't enable much on dependencies but we can go quite far now we have the consumer model transformation pipeline by enabling to reference a plugin from an imported resource (can need an import block but not a big deal) and customize it inline, even with required placeholders if needed.
I'd really love to see it done within an extension and using the xml transformer feature and without hacking the core yet to validate the usage since this part is quite hard to design right upfront.

@talios
Copy link

talios commented Jun 29, 2023

And an idea from comment on MNG-5588: IF you are importing a POM, why not import everything? (And then need for new scope really goes away). Users can prepare POMs with parts they want (depMgt only, pluginMgt only, or both) and import.

That's essentially what tiles-maven-plugin does - only it does it by synthetically injected the tile-pom as a parent (and has its own transitive tiles, which I don't like - but users do).

Because it's pulling in everything - you get the plugin definition and the executions, so you don't need to repeat all the plugins in the child pom (but that I could probably accept).

@talios
Copy link

talios commented Jun 29, 2023

@talios @hboutemy also, what is the problem with adding new scope (that is special,in very same way as existing "import" scope is special)?

No real problem - more questioning the naming (which may be too early in the change to quibble over naming), tho reading the reasoning of 'plugins don't have scope' that means this is possibly more a trade-off due to current limitations of the POM format.

@rmannibucau's comment:

Think extensions are a nice way to configure plugins

makes me think back to the current tiles implementation which operates as an extension, hooking into the model builder - but with the immutable model in M4 doesn't work - an new extension point for model manipulation would be nice.

One nice thing with tiles, by pulling as parents - we also pull in any ` defined, which can also then be overridden/customised.

@rmannibucau
Copy link
Contributor

Well, if v4 does not allow tiles to be implemented it means the plugin API is not yet sufficient and can't be used so we must fix it, can be nice to work on that, intent is to copy instead of mutating, not breaking use cases.

@cstamas
Copy link
Member Author

cstamas commented Jun 29, 2023

[...] tho reading the reasoning of 'plugins don't have scope' that means this is possibly more a trade-off due to current limitations of the POM format.

Plugins does not and will not have "scope". Why would a plugin need "scope"? IMHO this is not a deficiency but was a deliberate design goal.

@cstamas
Copy link
Member Author

cstamas commented Dec 16, 2023

As we all know, the "import" scope is not really a scope, it is a hack. And we do already have that hack in place, and it works. So, am really unsure why we either:

  • continue on that trail of hacks and implement other (very similar) hacks, or,
  • get rid of the original hack altogether (due all the issues it brings in)

So, why are we sitting comfy and not doing anything about it? This PR does 1st bullet, but for sure we can get a stab on 2nd bullet as well, as we decide...

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