Maven 2 & 3

Improve plugin configuration property merge algorithm

Details

  • Type: Improvement Improvement
  • Status: Closed Closed
  • Priority: Critical Critical
  • Resolution: Fixed
  • Affects Version/s: 2.0-alpha-3
  • Fix Version/s: 2.0-beta-1
  • Component/s: Plugins and Lifecycle
  • Labels:
    None
  • Complexity:
    Expert
  • Number of attachments :
    0

Description

If the property are the same in the parent and the child project, then the parent property is not inherited. This is fine for simple properties but breaks for complex properties such as lists. Here's an example:

My parent POM:

<build>
<pluginManagement>
<plugins>
<plugin>
<artifactId>maven-surefire-plugin</artifactId>
<configuration>
<systemProperties>

<property>
<name>cargo.resin3x.port</name>
<value>8280</value>
</property>
<property>
<name>cargo.resin3x.url</name>
<value>http://www.caucho.com/download/resin-3.0.9.zip</value>
</property>

</systemProperties>
</configuration>
</plugin>
</plugins>
</pluginManagement>
</build>

My child POM:

<build>
<plugins>
<plugin>
<artifactId>maven-surefire-plugin</artifactId>
<configuration>
<systemProperties>

<!-- Default list of containers to run on. If you want to shorten or change the
execution of 'samples', simply specify a shorter list of containers on the
command line or in your settings -->
<property>
<name>cargo.containers</name>
<value>resin3x, orion2x, tomcat5x, jetty4xEmbedded</value>
</property>

<!-- Location where to download and install the containers for the tests -->
<property>
<name>cargo.install.dir</name>
<value>$

{basedir}

/../../target/installs</value>
</property>

</systemProperties>
</configuration>
</plugin>
</plugins>
</build>

It sounds a reasonable expectations that the system properties will get merged.

Issue Links

depends upon

Improvement - An improvement or enhancement to an existing feature or task. PLXUTILS-22 allow disctinction between merge and override semantics on Xpp3Dom merge method

  • Major - Major loss of function.
  • Closed - The issue is considered finished, the resolution is correct. Issues which are not closed can be reopened.
relates to

Bug - A problem which impairs or prevents the functions of the product. MNG-2591 Plugins are merged incorrectly

  • Blocker - Blocks development and/or testing work, production could not run
  • Closed - The issue is considered finished, the resolution is correct. Issues which are not closed can be reopened.

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
Vincent Massol added a comment -

I think I may have traced it to line 278 of ModelUtils in maven-core:

Xpp3Dom childConfiguration = (Xpp3Dom) child.getConfiguration();
Xpp3Dom parentConfiguration = (Xpp3Dom) parent.getConfiguration();

childConfiguration = Xpp3Dom.mergeXpp3Dom( childConfiguration, parentConfiguration );

Which itself leads to the following method from plexus-util/Xpp3Dom.java in Plexus:

private static void mergeIntoXpp3Dom( Xpp3Dom dominant, Xpp3Dom recessive )
{
// TODO: how to mergeXpp3Dom lists rather than override?
// TODO: share this as some sort of assembler, implement a walk interface?
if ( recessive == null )

{ return; }

Xpp3Dom[] children = recessive.getChildren();
for ( int i = 0; i < children.length; i++ )
{
Xpp3Dom child = children[i];
Xpp3Dom childDom = dominant.getChild( child.getName() );
if ( childDom != null )

{ mergeIntoXpp3Dom( childDom, child ); }

else

{ dominant.addChild( new Xpp3Dom( child ) ); }

}
}

Does my use case correspond to the first todo item in the code?

Show
Vincent Massol added a comment - I think I may have traced it to line 278 of ModelUtils in maven-core: Xpp3Dom childConfiguration = (Xpp3Dom) child.getConfiguration(); Xpp3Dom parentConfiguration = (Xpp3Dom) parent.getConfiguration(); childConfiguration = Xpp3Dom.mergeXpp3Dom( childConfiguration, parentConfiguration ); Which itself leads to the following method from plexus-util/Xpp3Dom.java in Plexus: private static void mergeIntoXpp3Dom( Xpp3Dom dominant, Xpp3Dom recessive ) { // TODO: how to mergeXpp3Dom lists rather than override? // TODO: share this as some sort of assembler, implement a walk interface? if ( recessive == null ) { return; } Xpp3Dom[] children = recessive.getChildren(); for ( int i = 0; i < children.length; i++ ) { Xpp3Dom child = children [i] ; Xpp3Dom childDom = dominant.getChild( child.getName() ); if ( childDom != null ) { mergeIntoXpp3Dom( childDom, child ); } else { dominant.addChild( new Xpp3Dom( child ) ); } } } Does my use case correspond to the first todo item in the code?
Hide
Permalink
Brett Porter added a comment -

yes, this is the first TODO.

I guess the reason I haven't jumped in is that we might need to consider whether it needs to be able to override in some cases instead of merge. Got any suggestions for syntax?

maybe:

<excludes override="true">

?

Show
Brett Porter added a comment - yes, this is the first TODO. I guess the reason I haven't jumped in is that we might need to consider whether it needs to be able to override in some cases instead of merge. Got any suggestions for syntax? maybe: <excludes override="true"> ?
Hide
Permalink
Vincent Massol added a comment -

yes, an override attribute sounds good so that merge is the default.

Show
Vincent Massol added a comment - yes, an override attribute sounds good so that merge is the default.
Hide
Permalink
John Casey added a comment -

need fix +tests...this will require fix in plexus-utils, so I'll file an issue there and link to this.

Show
John Casey added a comment - need fix +tests...this will require fix in plexus-utils, so I'll file an issue there and link to this.
Hide
Permalink
John Casey added a comment -

since this is mostly a plexus fix, the hours on this end are basically just for testing...

Show
John Casey added a comment - since this is mostly a plexus fix, the hours on this end are basically just for testing...
Hide
Permalink
John Casey added a comment -

see it0060

Show
John Casey added a comment - see it0060
Hide
Permalink
Vincent Massol added a comment -

John,

I'm trying to use this feature with the example above and when I run my build with -X I get:

[DEBUG] Setting system property [cargo.containers]=[resin3x, orion2x, tomcat5x, jetty4xEmbedded]
[DEBUG] Setting system property [cargo.install.dir]=[C:\dev\cargo\samples\java/../../target/installs]

Unfortunately the other properties (cargo.resin3x.port and cargo.resin3x.url) do not seem to be set.

Any idea?

Thanks

Show
Vincent Massol added a comment - John, I'm trying to use this feature with the example above and when I run my build with -X I get: [DEBUG] Setting system property [cargo.containers] = [resin3x, orion2x, tomcat5x, jetty4xEmbedded] [DEBUG] Setting system property [cargo.install.dir] = [C:\dev\cargo\samples\java/../../target/installs] Unfortunately the other properties (cargo.resin3x.port and cargo.resin3x.url) do not seem to be set. Any idea? Thanks
Hide
Permalink
Brett Porter added a comment -

Vincent, please reopen if you think it is not working. How does your use case differ from it0060? Can you enhance the it?

Show
Brett Porter added a comment - Vincent, please reopen if you think it is not working. How does your use case differ from it0060? Can you enhance the it?
Hide
Permalink
Vincent Massol added a comment -

No, it was just me not knowing how to use it. I had somehow imagine inheritance would have been automatic and you would only need to specify an attribute for not inheriting...

So it's working even though I find it a tad complex to have to specify an <inheritance> object in the parent's POM and an attribute in the child project. But I'm sure you've weighted pros and cons before reaching this conclusion so that's probably fine...

Show
Vincent Massol added a comment - No, it was just me not knowing how to use it. I had somehow imagine inheritance would have been automatic and you would only need to specify an attribute for not inheriting... So it's working even though I find it a tad complex to have to specify an <inheritance> object in the parent's POM and an attribute in the child project. But I'm sure you've weighted pros and cons before reaching this conclusion so that's probably fine...
Hide
Permalink
Brett Porter added a comment -

I thought inherited=true was the default too, but I do remember we discussed the pros and cons and I trust John to have implemented what we agreed at the time and my memory to have failed me (that part was a while ago)

Show
Brett Porter added a comment - I thought inherited=true was the default too, but I do remember we discussed the pros and cons and I trust John to have implemented what we agreed at the time and my memory to have failed me (that part was a while ago)

People

  • Assignee:
    John Casey
    Reporter:
    Vincent Massol
Vote (0)
Watch (0)

Dates

  • Created:
    Updated:
    Resolved:

Time Tracking

Estimated:
3h
Original Estimate - 3 hours
Remaining:
0m
Remaining Estimate - 0 minutes
Logged:
2h
Time Spent - 2 hours Time Not Required