Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[MNG-7596] Upgrade to plexus 3.5.0 #866

Merged
merged 1 commit into from
Dec 1, 2022
Merged

Conversation

gnodet
Copy link
Contributor

@gnodet gnodet commented Nov 9, 2022

JIRA issue: https://issues.apache.org/jira/browse/MNG-7596
Upgrade to plexus 3.5.0 and change the xml merge accordingly

Note that I had to hack a bit the order to keep the current ordering, see https://issues.apache.org/jira/browse/MNG-5474

@gnodet gnodet marked this pull request as draft November 9, 2022 21:13
@gnodet gnodet force-pushed the plexus-3.5.0 branch 2 times, most recently from 4db03d3 to df6ffaf Compare November 10, 2022 05:31
@gnodet gnodet changed the title Upgrade to plexus 3.5.0 [MNG-7596] Upgrade to plexus 3.5.0 Nov 10, 2022
@gnodet gnodet marked this pull request as ready for review November 10, 2022 05:32
@gnodet gnodet requested a review from kwin November 10, 2022 05:32
@gnodet gnodet added this to the 4.0.0-alpha-3 milestone Nov 10, 2022
@gnodet
Copy link
Contributor Author

gnodet commented Nov 22, 2022

@cstamas @kwin would you mind having a look at this PR ?

@gnodet gnodet requested a review from cstamas November 28, 2022 12:52
Copy link
Member

@kwin kwin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The double effort on maintenance is really nasty. We should indeed deprecate the original Xpp3Dom and Xpp3DomBuilder then.

@gnodet gnodet merged commit 615390f into apache:master Dec 1, 2022
Comment on lines -183 to -184
* <li> if the dominant root node's value is empty, set it to the recessive root node's value</li>
* <li> For each attribute in the recessive root node which is not set in the dominant root node, set it.</li>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gnodet Why has this behavior been removed? Is there a migration path that you can share?

The following example has worked before that change where we have a parent pom and a child pom and a configuration that we'd like to merge. Here the parent and child pom definitions:

<!--parent pom-->
  ...
<pluginManagement>
  <plugins>
    <plugin>
      <groupId>foo.bar</groupId>
      <artifactId>foo-bar-plugin</artifactId>
      <configuration>
        <plugins>
          <plugin>
            <groupId>org.apache.maven.plugins</groupId>
            <artifactId>maven-compiler-plugin</artifactId>
            <bar>
              <value>foo</value>
            </bar>
          </plugin>
          <plugin>
            <groupId>org.apache.maven.plugins</groupId>
            <artifactId>maven-surefire-plugin</artifactId>
            <foo>
              <properties>
                <property>
                  <name>prop1</name>
                  <value>value1</value>
                </property>
              </properties>
            </foo>
          </plugin>
        </plugins>
      </configuration>
    </plugin>
  </plugins>
</pluginManagement>
  ...
  <!--child pom-->
  ...
<pluginManagement>
<plugins>
  <plugin>
    <groupId>foo.bar</groupId>
    <artifactId>foo-bar-plugin</artifactId>
    <configuration>
      <plugins>
        <plugin>
          <groupId>org.apache.maven.plugins</groupId>
          <artifactId>maven-compiler-plugin</artifactId>
        </plugin>
        <plugin>
          <groupId>org.apache.maven.plugins</groupId>
          <artifactId>maven-surefire-plugin</artifactId>
          <foo>
            <properties combine.children="append">
              <property>
                <name>prop2</name>
                <value>value2</value>
              </property>
            </properties>
          </foo>
        </plugin>
      </plugins>
    </configuration>
  </plugin>
</plugins>
</pluginManagement>
  ...

And the expected effective child pom that is used after the merge:

  <!--expected effective child pom-->
  ...
<pluginManagement>
<plugins>
  <plugin>
    <groupId>foo.bar</groupId>
    <artifactId>foo-bar-plugin</artifactId>
    <configuration>
      <plugins>
        <plugin>
          <groupId>org.apache.maven.plugins</groupId>
          <artifactId>maven-compiler-plugin</artifactId>
          <bar>
            <value>foo</value>
          </bar>
        </plugin>
        <plugin>
          <groupId>org.apache.maven.plugins</groupId>
          <artifactId>maven-surefire-plugin</artifactId>
          <foo>
            <properties combine.children="append">
              <property>
                <name>prop1</name>
                <value>value1</value>
              </property>
              <property>
                <name>prop2</name>
                <value>value2</value>
              </property>
            </properties>
          </foo>
        </plugin>
      </plugins>
    </configuration>
  </plugin>
</plugins>
</pluginManagement>
  ...

The problem we see now after this change, is that it will be merged the following way:

<!--expected effective child pom after this change-->
  ...
<pluginManagement>
<plugins>
  <plugin>
    <groupId>foo.bar</groupId>
    <artifactId>foo-bar-plugin</artifactId>
    <configuration>
      <plugins>
        <plugin>
          <groupId>org.apache.maven.plugins</groupId>
          <artifactId>maven-compiler-plugin</artifactId>
          <foo>
            <properties>
              <property>
                <name>prop1</name>
                <value>value1</value>
              </property>
            </properties>
          </foo>
        </plugin>
        <plugin>
          <groupId>org.apache.maven.plugins</groupId>
          <artifactId>maven-surefire-plugin</artifactId>
          <foo>
            <properties combine.children="append">
              <property>
                <name>prop2</name>
                <value>value2</value>
              </property>
            </properties>
          </foo>
        </plugin>
      </plugins>
    </configuration>
  </plugin>
</plugins>
</pluginManagement>
  ...

The problems are the following:

  1. On the maven-compiler-plugin configuration the <bar> node is not merged from the parent pom configuration but rather the one from the maven-surefire-plugin configuration from the parent pom.
  2. The maven-surefire-plugin configuration not as expected as the children of the <properties> node are not appended as configured via the combine.children="append" property.

Is that changed something that stays for Maven 4 ? I assume that then a migration path should be given as there are projects that use the behavior before this change and it would result in different effective pom files, which will change how the build is configured.

I'm also open to chat about it on the ASF Slack if required or if you have further questions.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if that can help, but big efforts were made in the past on maven-shared-utils reimplementation, with its test suite: https://github.com/apache/maven-shared-utils/blob/master/src/test/java/org/apache/maven/shared/utils/xml/pull/Xpp3DomTest.java
perhaps that can be reused

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@guylabs I'm not really the author of the change, I simply back ported it because the code is duplicated somehow from plexus-utils. The original change is codehaus-plexus/plexus-utils@67ac243

guylabs referenced this pull request in codehaus-plexus/plexus-utils Jan 4, 2023
@gnodet gnodet deleted the plexus-3.5.0 branch January 25, 2023 13:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants