-
Notifications
You must be signed in to change notification settings - Fork 2.7k
[MNG-5102] Add support for POM mixins #1209
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
base: master
Are you sure you want to change the base?
Conversation
gnodet
commented
Jul 20, 2023
•
edited
Loading
edited
- JIRA issue: https://issues.apache.org/jira/browse/MNG-5102
a810482
to
f97c405
Compare
81f4422
to
211e27a
Compare
1570e7f
to
4c87f2a
Compare
c2b6ec7
to
e4cd448
Compare
<mixins> | ||
<mixin> | ||
<groupId>org.apache.maven.its.mng5102</groupId> | ||
<artifactId>mixin-2</artifactId> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this take a classifier? One of the early issues we identified with tiles - which is why we have a pom.xml and a tile.xml - is for publishing to a repo.
The pom.xml would have release plugin information, which tne tile/mixin wouldn't want.
I like what I'm seeing here, and it looks like it should support my own needs - will try find some time to pull/build this locally and give it a renewed test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's now supported !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great news. I looked at the tests quickly and it seemed like the mixins there only contains properties.
What can be included (or not included) in the mixins?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The mixins are merged in their declaration order as if they were parents, so there are no restrictions at this point. I haven't given much thought if there should be yet...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@talios Our organization use your Maven Tiles (thank you!). Trying to think if there is something in Maven Tiles that this current implementation of Maven Mixins does not support. With Maven Tiles we have issues with profiles (the are active but Maven does not alwats acknowledge them) and properties (properties aren't propagated).
From what Guillaume wrote it seems as if mixins, currently not having any restrictions, might function even better that Maven Tiles.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jimisola Depending on how this works in practise - and @gnodet may be able to quickly confirm, one big thing that users of tiles seem to love (which is something I'd gladly give up as I don't overlike it) - is having mixins, that contain mixins.
Tiles also (by default) disallows dependencies (unlocked via the code smells configuration) - but it's often useful to include runtime dependencies to match plugins.
Say an antlr mixin might declared the antlr runtime dep, AND the plugin config.
One thing I don't see here (and also only cause I've not yet had a running version work yet) is logging of mixins being pulled in. That's quite handy for debugging, as is our default behavior of rewriting the execution ids to be prefixed by the mixin.
The rewriting of execution ids could easily just be done IN the mixin, with descriptive ids to begin with tho.
Properties aren't propagated? AFAIK they are as we use them often, the issue we have (which is possible more to Model inheritance) is if a tile references a property, but doesn't declare it a default value THEN it doesn't seem to work.
But having a property defined at the mixin/tile level, and overridden in the top level pom works.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if it is of interest for @gnodet but here is our Maven Tiles setup.
We use tiles and composites where are used to group tiles.
Hence, we have a need for including mixins in mixins for us to be able to group mixins into composites.
Parent Tile
- maven-tiles-parent which is used to set the Maven Tiles version for all tiles (won't be needed with Maven Mixins)
Tiles:
- common-tile
- java-17-tile
- java-21-tile
- lfv-development-environment-tile
- lint-tile
- maven-core-versions-tile
- openapi-tile
- opentelemetry-tile
- reqstool-tile
- soap-tile
- spring-boot-amqp-tile
- spring-boot-application-tile
- spring-boot-camel-tile
- spring-boot-mvc-tile
- spring-boot-oauth-tile
- spring-boot-redis-tile
- spring-boot-tile
- spring-boot-vault-tile
- sysdev-common-tile
- test-compile-tile
@talios As for the properties issue (our use case seem to be different), with respect for this PR we refer to this new ticket in Maven Tiles.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jimisola I saw that ticket, I see what you mean - I always propagate UP into the tile (so overrides in the project).
I've never really been a fan of composites (which are usually dependencies) as BOM imports already exist - composites just remove the need to declare each dependency in each module - which, in our application which is OSGi based can cause issues if we're just adding deps unseen - I guess it also depends on how often you churn releases of tiles, or dependencies ( we use an additional tool I wrote for $work that handles version ranges/lock files) to track and check that.
Out of interesting - do you tiles include dependencies, or just plugins?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@talios Both dependencies and plugins.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Finally got around to testing this (it's been a hectic fortnight). From a fresh archetype:generated
project, and updating the model
version and XML preamble, adding the following maven tile "mixin":
<mixins>
<mixin>
<groupId>com.smxemail.tiles</groupId>
<artifactId>com.smxemail.tiles.enforcements</artifactId>
<version>4.0.176</version>
<extension>xml</extension>
</mixin>
</mixins>
Everything seemed to work as expected!
Compared to the tiles config:
<plugin>
<groupId>io.repaint.maven</groupId>
<artifactId>tiles-maven-plugin</artifactId>
<version>2.40</version>
<extensions>true</extensions>
<configuration>
<tiles>
<tile>com.smxemail.tiles:com.smxemail.tiles.enforcements:[4.0.176]</tile>
</tiles>
</configuration>
</plugin>
I long for the day when we can have some more concise GAV specifications in Maven, but this is looking good as a transition.
The tile does transclude another tile, which obviously doesn't work here. I will craft one with its own <mixin>
element and do some further testing.