search

repaint-io / maven-tiles

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

maven 3.3.9 compatibility #89

Closed bvella closed 5 years ago

bvella commented 5 years ago

Extracted from #84. Fixes #77.

bvella commented 5 years ago

I left a comment on #91 that it requires this to be merged as well for it to work. The classloading workd differently when the extension is loaded from .mvn/extensions.xml file and without these changes, it will fail. Do you need anything else for this merge request?

talios commented 5 years ago

@bvella I can't find it now, but I remember making a comment on one of the tickets about maybe making the build fail ( able to relax to a warning maybe ) if the reactor contains a tile, and you DON'T have .mvn/extensions.xml defined - we could do that as a new issue/merge/release tho.

bvella commented 5 years ago

Comment is here https://github.com/repaint-io/maven-tiles/issues/63#issuecomment-455849594. Referring to #91, the extension is really only useful if you have a tile in the reactor AND it is used by another maven project in the reactor. I will submit a separate pr with documentation update and a warning about this situation.

talios commented 5 years ago

@bvella I remember my suggestion ( can't remember where it was ) was to add that warning in code as well if those conditions are met - if you have a tile in the reactor, AND you use it, and we know it's not going to work as expected without .mvn/extensions.xml - maybe fail the build.

talios commented 5 years ago

Well that's released now - and breaks horribly on my projects :( Trust me not to test it on real world projects.

bvella commented 5 years ago

How is it breaking? The tests and its run fine and I have been using these patches in a big real world project for about 2 months. Working both in command line maven and in intellij. Can you run maven with -X and post any exceptions?

talios commented 5 years ago

@bvella Looks to a combination of the fact we're not including tiles as dependencies ( even when there's no reactor, so kinda pointless ) and the fact we have [1.2] square-bracketed locked-down versions, somewhere in the chain we're including multiple instances of the same tile as a dependency with different versions ranges, which trigger unresolvable ranges.

bvella commented 5 years ago

If you can give me a more specific scenario that I can replicate, I will see if I can fix it. I don't use version ranges in my tile dependencies, so I did not encounter this problem. It might be worth adding an integration test for such a scenario as well.