repaint-io / maven-tiles

Injecting maven configurations by composition rather than inheritance
154 stars 32 forks source link

Feature request: inject ${project.version} into POM if inherited from parent #47

Closed skybert closed 8 years ago

skybert commented 8 years ago

Hi there,

I have a feature request: if the POM which you add the maven-tiles plugin to inherits its ${project.version} from its parent, copy it to the POM so that its version isn't inherited from the tile POM.

<project>
  <parent>
    <groupId>com.example.com</groupId>
    <artifactId>parent-artifact</artifactId>
    <version>3</version>
  </parent>
  <artifactId>child-artifact</artifactId>
..
</project>

The point is that child-artifact hasn't explicitly set its version, it inherits it from parent-artifact. Now, when mvn runs, maven-tils injects the tile.xml as the new parent and the version in child-artifact now becomes whatever is defined in the tile's POM. I suggest that maven-tiles sets the version in child-artifact to 3 (if it hasn't specified one) after perfoming the parent POM injection.

I believe this would improve maven-tiles' user friendliness.

Cheers,

-Torstein

Example:

rvowles commented 8 years ago

Why do you have parents? What we are trying to encourage people to do is not use parents at all. Compose POMS for dependencies, Tiles for plugins, so there is never a requirement for a parent?

talios commented 8 years ago

@skybert I had a look at this patch the other day and I have a few concerns/questions:

  1. If your project pom.xml defines a version, and a tile also declares one - the patch looks like it would override and use the one from the tile ( injected as a parent, and overridden by the child project pom ) - that feels wrong ( unless mistaken )
  2. If you have two, three, or four tiles - all of which declare a version, which one is used? It would seem that might be rather magically non-deterministic ( on the surface, I assume the last applied tile's version would be used ) - and what if that tile embedded another tile via a <tiles></tiles> element inside the tile.xml.

As @rvowles says - the aim of tiles is to rid the world of parents, however I do have one project remaining as a multi-module/parent build, but we just redeclare the <version> element in each child.

talios commented 8 years ago

Actually, scratch that - I was thinking this was the pull request for the test/source directory rewriting. I don't believe tiles.xml can set a version, so most of what I said doesn't relate anyway ).

I don't think we parse the parent pom in any fashion to extract the version number, so maybe just do what I suggested and copy the version down. We have the release plugin set to automatically update the child versions automatically so you don't need to enter a value for each and every child.

skybert commented 8 years ago

@rvowles regardless if I agree with you or not, it's nevertheless not an option for my current project to rewrite everything to not use parents. It's a Maven project (or collection of projects if you will) with a total of 708 modules, Rewriting all those POMs to not use parents just to make use of this new tile plugin (instead of an older tile plugin) was out of scope for my task at hand.

Looks like @erwint's change does exactly what I want, thanks!

talios commented 8 years ago

Having looked thru @erwint's patch and his response to some questions I had, I think I'm fine with pulling this in.

In @skybert's case tho, you wouldn't need to remove the parents, but just include the <version> elements in the children in the interim until we merge and release with this ( there's a few other changes in the queue that I'm looking at to see if we want to pull them into this next release as well ).

skybert commented 8 years ago

Thanks @talios, adding <version> to the children works well. No hurry with the release (we've already converted our POMs to use repaint-io:maven-tiles), just happy to see this improvement being merged into a future release.

Cheers!