Maven 2 & 3
  1. Maven 2 & 3
  2. MNG-732

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

          Activity

          Hide
          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
          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
          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
          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
          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
          John Casey added a comment -

          see it0060

          Show
          John Casey added a comment - see it0060
          Hide
          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
          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
          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
          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
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Time Tracking

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