[#MNG-3595] Changes made to project resolved artifacts are overriden when a plugin uses @requiresDependencyResolution

Maven 2

Changes made to project resolved artifacts are overriden when a plugin uses @requiresDependencyResolution

Created: 25/May/08 03:35 PM Updated: 12/Jun/08 09:10 PM

Component/s:

Plugins and Lifecycle

Affects Version/s:

None

Fix Version/s:

3.0

Time Tracking:

Not Specified

File Attachments:

MNG-3595-test-project.tar.bz2 (13 kB)
2.

MNG-3595.diff (2 kB)
3.

MNG-3595.diff (5 kB)

Complexity:

Intermediate

Description

« Hide

clover:instrument mojo creates instrumented artifacts and attaches them with a 'clover' classifier.

It forks a lifecycle [1][2] and which triggers clover:intrumentInternal which tries to change the project direct and transitive dependencies after 'swizzling' them [3] (replacing normal artifacts with their clovered ones). This in theory would allow to build clovered WAR and EAR, i.e. WAR/EAR containing all available clovered dependencies.

Unfortunately maven handles direct and transitive dependencies differently [4]. While direct dependencies are somewhat preserved (the code comments references clover), transitive dependencies are re-resolved, and thus the results of the swizzling operation are lost as soon as a plugin requiresDependencyResolution in a further part of the lifecycle.

I've managed to hack maven and clover:instrument to work together by allowing a plugin to attach some sort of "dependency resolution post processing" operation [5].
In the case of clover:instrument, the swizzling is then registered in the maven project and re-performed each time the artifacts are re-resolved.

I am not very fond of this solution, but it seems to work for us on the attached example. I will further test the patch this week on a large scale project.

I would like to discuss ways to solve this interaction, whether the clover plugin should be implemented differently or if maven should have some sort of support for this use case.

[1] http://svn.atlassian.com/svn/public/contrib/clover/maven-clover-plugin/trunk/src/main/java/com/atlassian/maven/plugin/clover/CloverInstrumentMojo.java
[2] http://svn.atlassian.com/svn/public/contrib/clover/maven-clover-plugin/trunk/src/main/resources/META-INF/maven/lifecycle.xml
[3] http://svn.atlassian.com/svn/public/contrib/clover/maven-clover-plugin/trunk/src/main/java/com/atlassian/maven/plugin/clover/CloverInstrumentInternalMojo.java
[4] http://maven.apache.org/ref/2.0.9/maven-core/xref/org/apache/maven/plugin/DefaultPluginManager.html#1406
[5] http://www.mail-archive.com/dev@maven.apache.org/msg74636.html

All

Comments

Work Log

Change History

Sort Order:

[ Permlink | « Hide ]

Jerome Lacoste added a comment - 25/May/08 05:58 PM

a simple test project that contains a log produced after patching maven and cloved plugin with the hack .

Notice how the expected-mvn.log contains:

[INFO] Building war: /tmp/clover-war-hello-world-trunk/webapp/target/clover/sayHello-1.0-clover.war
[DEBUG] adding directory META-INF/
[...]
[DEBUG] adding entry WEB-INF/lib/a-1.0-clover.jar
[DEBUG] adding entry WEB-INF/lib/clover-2.1.0.jar
[DEBUG] adding entry WEB-INF/lib/b-1.0-clover.jar

while maven will today include b-1.0-clover.jar and a-1.0.jar

[ Permlink | « Hide ]

Jerome Lacoste added a comment - 25/May/08 06:12 PM

The patch I've applied to maven before modifying the clover plugin

[ Permlink | « Hide ]

Jerome Lacoste added a comment - 25/May/08 06:19 PM

I forgot to say that the patch I've made above applies to 2.0.6. I expect the same patch to work more or less on 2.0.9. Also the patch isn't cleaned up as the real change should be on the MavenProject class. Will attach a cleaned up patch. Doesn't really matter as the solution isn't going to be accepted as is

[ Permlink | « Hide ]

Jerome Lacoste added a comment - 26/May/08 02:48 PM

We've tested the maven patch + pacthed clover on a large scale project (30 modules) and all WAR/EARs ended up generated with full clovered dependencies. Now I just need some input from the maven developers.

For into the clover issue is registered here:http://developer.atlassian.com/jira/browse/CLMVN-6

[ Permlink | « Hide ]

John Casey added a comment - 28/May/08 03:43 PM

I'd prefer to see this as preventing re-resolution of project dependencies, unless the scope changes upward (compile to test, NOT test to compile)...then, you could impose a rule saying that any pre-existing dependency artifacts in the project are preferred over those new ones just resolved...that way, you'd preserve any swizzled artifacts for the duration of your forked execution.

This does get into some interesting side effects for plugins that don't use @requiresDependencyResolution, probably, so it's probably not appropriate for 2.0.x (who knows what sort of regressions this sort of change could introduce).

I'd prefer to do a full audit of the mutability of MavenProject for 2.1 from a plugin's point of view, to make sure that modifications like this are preserved for successive plugin-executions.

[ Permlink | « Hide ]

Jerome Lacoste added a comment - 28/May/08 04:00 PM

#maven
<lacostej> yo ! Is there anything I can do in exchange for some comments regarding MNG-3595 ?

at least to know if this problem is something that is worth addressing. Thanks
<Brian> lacostej, i don't think we should make that change in 2.0.x, but perhaps 2.1
<Brian> it's a little scary to make changes in this area anymore because it will inevitably break someone's build
<lacostej> Brian: OK. I am willing to provide a cleaner patch for 2.1, I just need some comments on what approach to follow.
<lacostej> the current patch cannot build any existing build as it provides a new hook for adding the functionality. But I don't like changing MavenProject
<Brian> jdcasey, wdyt?
<jdcasey> looking
<jdcasey> lacostej: I'm not sure I get it, the clover plugin would register a post-processor for dependencies? or, would it attach a post-processor to the project for use ahead of successive plugin executions?
<jdcasey> lacostej, Brian: why not simply remember when/what scope we've resolved to for a given project, and avoid re-resolving? then swizzled artifacts would remain intact for the forked project instance
<Brian> jdcasey, that would be the preferred technique performance wise also, but i'm afraid of consequences in 2.0.x
<lacostej> jdcasey: the way I solved the problem was just the quickest solution I found. The problem is that successive plugin executions affect the list of transitive dependencies, because of the requireDependencyResolution tag. The idea was then to post process this dependency resolution operation, if it is to happen again. Another solution would be to prevent from the re-resolution to happen again,...
<lacostej> ...but I wasn't sure if it is possible to achieve this without locking maven into one corner. I wasn't sure why we were not already re-resolving for each plugin that required it
<jdcasey> Brian: I understand, but this should be relatively tame
<jdcasey> lacostej: I think as long as we never prune the list of transitive artifacts (i.e. going from test scope to compile scope only) in the project instance, we should be able to avoid re-resolving pretty safely
<Brian> there's a lot of sketchy stuff in there, like you may still get artifacts even if you didn't request resolution, depending on what plugin(s) ran before
<jdcasey> IIRC, there is another long-standing issue with re-resolving the artifacts...jesse filed it, but I'd have to dig around to find it again
<lacostej> jdcasey: what happens if we start adding a new scope ?
<jdcasey> Brian: true, we'd almost need a masked project instance for things that didn't request dependency resolution
<Brian> yeah which is why this scares me in 2.o
<jdcasey> yeah
<jdcasey> Brian: you don't think we could hem it in with a bunch of tests and get the possible pathways covered?
<jdcasey> I mean, it'd be a lot of work, and this is sort of an edge case (if you don't mind my saying so), but still...
<jdcasey> 2.1 is definitely safest
<Brian> i'm risk averse after the mess we went through with .9
<Brian> we're trying to reduce regressions not make more

<jdcasey> it's probably worth mentioning in connection that we ought to do a full audit of the mutability of the MavenProject instance, to make sure things are intuitive across the board...fwiw
<jdcasey> heh
<lacostej> jdcasey: that's a good think. In fact what I was missing when developing is a contract between the plugins and maven core
<jdcasey> Brian: that pretty much means just shoring up the behavior and adopting a sunset clause for 2.0.x asap, then?
<jdcasey>

<lacostej> think->thing
<jdcasey> lacostej: yeah, the formal contract thing keeps coming up
<lacostej> e.g. what should a plugin expect after using setArtifacts()
<jdcasey> I started looking at that awhile back, something outside the core that was fluent in both the 2.0.x core and the 2.1 core, but ran out of time
<jdcasey> sure
<Brian> jdcasey, that's essentially what i had in mind, continue to fix bugs and regressions only
<jdcasey> that's cool
<jdcasey> Brian: maybe it's worth talking about a less scary 2.1, then? something that will do a new feature-set in between the current plan and the 2.0.x features?
<jdcasey> I mean, we have enough code now to talk about killing bugs and doing a release

jdcasey shrugs
<Brian> yes but 2.1 hardly works without the artifact stuff
<Brian> i've been pushing to close the scope on 2.1 for a while now
<lacostej> what's the rough idea behind 2.1 ? E.g. do you plan on enforcing maximum compatibility or can new features be favored over compatiblity ?
<jdcasey> Brian: there isn't that much change in the way it uses artifact...we could probably backport it for a quick release, then move forward to the new artifact stuff in 2.2
<jdcasey> lacostej: addressing some of the design flaws in 2.0.x, like extension loading for instance
<jdcasey> and plugin-versioning/-configuration differences
<lacostej> ok.
<lacostej> jdcasey: back to the issue, the reason I didn't try to lazy resolve the dependencies (and opted for a post-processing instead), is that I wasn't sure what the contract between the plugin and MavenProject would end up if one plugin uses setArtifacts() and at the same time, the plugin manager only resolves once. That could create issues for plugins that work later on in the lifecycle. That's...
<lacostej> ...why I wondered if there should be some guidelines on how operations that for example use setArtifacts() should be implemented in order to avoid plugin conflicts
<lacostej> Anyway. thanks for the comments. I will copy paste part of this chatlog in the issue. I will keep using my patched maven for now (it's only going to be used on our build server anyway) and see if I can get try the resolution caching instead.
<jdcasey> lacostej: sure, I understand. In any case, Brian has an excellent point in that this could be a major source of regressions unless we managed it perfectly and got very lucky...I guess I'd prefer to put it off until 2.1 as well...
<lacostej> jdcasey: I am still running 2.0.6 on one project due to regressions
<jdcasey> lacostej: I'm not sure I'm against the postProcessor idea, but it does seem a little hackish to continue re-resolving and just put in a longer resolution process, rather than figuring out how to limit re-resolution

[ Show » ]

Jerome Lacoste added a comment - 28/May/08 04:00 PM #maven <lacostej> yo ! Is there anything I can do in exchange for some comments regarding MNG-3595 ?

at least to know if this problem is something that is worth addressing. Thanks <Brian> lacostej, i don't think we should make that change in 2.0.x, but perhaps 2.1 <Brian> it's a little scary to make changes in this area anymore because it will inevitably break someone's build <lacostej> Brian: OK. I am willing to provide a cleaner patch for 2.1, I just need some comments on what approach to follow. <lacostej> the current patch cannot build any existing build as it provides a new hook for adding the functionality. But I don't like changing MavenProject <Brian> jdcasey, wdyt? <jdcasey> looking <jdcasey> lacostej: I'm not sure I get it, the clover plugin would register a post-processor for dependencies? or, would it attach a post-processor to the project for use ahead of successive plugin executions? <jdcasey> lacostej, Brian: why not simply remember when/what scope we've resolved to for a given project, and avoid re-resolving? then swizzled artifacts would remain intact for the forked project instance <Brian> jdcasey, that would be the preferred technique performance wise also, but i'm afraid of consequences in 2.0.x <lacostej> jdcasey: the way I solved the problem was just the quickest solution I found. The problem is that successive plugin executions affect the list of transitive dependencies, because of the requireDependencyResolution tag. The idea was then to post process this dependency resolution operation, if it is to happen again. Another solution would be to prevent from the re-resolution to happen again,... <lacostej> ...but I wasn't sure if it is possible to achieve this without locking maven into one corner. I wasn't sure why we were not already re-resolving for each plugin that required it <jdcasey> Brian: I understand, but this should be relatively tame <jdcasey> lacostej: I think as long as we never prune the list of transitive artifacts (i.e. going from test scope to compile scope only) in the project instance, we should be able to avoid re-resolving pretty safely <Brian> there's a lot of sketchy stuff in there, like you may still get artifacts even if you didn't request resolution, depending on what plugin(s) ran before <jdcasey> IIRC, there is another long-standing issue with re-resolving the artifacts...jesse filed it, but I'd have to dig around to find it again <lacostej> jdcasey: what happens if we start adding a new scope ? <jdcasey> Brian: true, we'd almost need a masked project instance for things that didn't request dependency resolution <Brian> yeah which is why this scares me in 2.o <jdcasey> yeah <jdcasey> Brian: you don't think we could hem it in with a bunch of tests and get the possible pathways covered? <jdcasey> I mean, it'd be a lot of work, and this is sort of an edge case (if you don't mind my saying so), but still... <jdcasey> 2.1 is definitely safest <Brian> i'm risk averse after the mess we went through with .9 <Brian> we're trying to reduce regressions not make more

<jdcasey> it's probably worth mentioning in connection that we ought to do a full audit of the mutability of the MavenProject instance, to make sure things are intuitive across the board...fwiw <jdcasey> heh <lacostej> jdcasey: that's a good think. In fact what I was missing when developing is a contract between the plugins and maven core <jdcasey> Brian: that pretty much means just shoring up the behavior and adopting a sunset clause for 2.0.x asap, then? <jdcasey>

<lacostej> think->thing <jdcasey> lacostej: yeah, the formal contract thing keeps coming up <lacostej> e.g. what should a plugin expect after using setArtifacts() <jdcasey> I started looking at that awhile back, something outside the core that was fluent in both the 2.0.x core and the 2.1 core, but ran out of time <jdcasey> sure <Brian> jdcasey, that's essentially what i had in mind, continue to fix bugs and regressions only <jdcasey> that's cool <jdcasey> Brian: maybe it's worth talking about a less scary 2.1, then? something that will do a new feature-set in between the current plan and the 2.0.x features? <jdcasey> I mean, we have enough code now to talk about killing bugs and doing a release

jdcasey shrugs <Brian> yes but 2.1 hardly works without the artifact stuff <Brian> i've been pushing to close the scope on 2.1 for a while now <lacostej> what's the rough idea behind 2.1 ? E.g. do you plan on enforcing maximum compatibility or can new features be favored over compatiblity ? <jdcasey> Brian: there isn't that much change in the way it uses artifact...we could probably backport it for a quick release, then move forward to the new artifact stuff in 2.2 <jdcasey> lacostej: addressing some of the design flaws in 2.0.x, like extension loading for instance <jdcasey> and plugin-versioning/-configuration differences <lacostej> ok. <lacostej> jdcasey: back to the issue, the reason I didn't try to lazy resolve the dependencies (and opted for a post-processing instead), is that I wasn't sure what the contract between the plugin and MavenProject would end up if one plugin uses setArtifacts() and at the same time, the plugin manager only resolves once. That could create issues for plugins that work later on in the lifecycle. That's... <lacostej> ...why I wondered if there should be some guidelines on how operations that for example use setArtifacts() should be implemented in order to avoid plugin conflicts <lacostej> Anyway. thanks for the comments. I will copy paste part of this chatlog in the issue. I will keep using my patched maven for now (it's only going to be used on our build server anyway) and see if I can get try the resolution caching instead. <jdcasey> lacostej: sure, I understand. In any case, Brian has an excellent point in that this could be a major source of regressions unless we managed it perfectly and got very lucky...I guess I'd prefer to put it off until 2.1 as well... <lacostej> jdcasey: I am still running 2.0.6 on one project due to regressions <jdcasey> lacostej: I'm not sure I'm against the postProcessor idea, but it does seem a little hackish to continue re-resolving and just put in a longer resolution process, rather than figuring out how to limit re-resolution