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

[MRRESOURCES-126] Get rid of legacy #26

Merged
merged 18 commits into from
May 3, 2023
Merged

Conversation

cstamas
Copy link
Member

@cstamas cstamas commented Mar 6, 2023

Get rid of MAT, Maven 3.9.1 warning about localRepository, etc. Make Maven resolve and not Mojo, this gets rid of quite a lot code. For runOnlyAtExecutionRoot introduce new mojo aggregate-process that is aggregator Mojo.

So, with this PR:

  • the existing process mojo does all as before, but is much simpler, but LOOSES runOnlyAtExecutionRoot parameter and functionality
  • added new aggregator-process Mojo that does what process w/ runOnlyAtExecutionRoot before did with slight changes (in bulk output, that is IMHO acceptable).

https://issues.apache.org/jira/browse/MRRESOURCES-126

And with 3.9.0 suddenly all the ITs suffer from "change packaging to pom"
error.
@cstamas cstamas self-assigned this Mar 6, 2023
@cstamas cstamas changed the title Get rid of legacy [MRRESOURCES-126] Get rid of legacy Mar 8, 2023
Plain and an aggregator
Copy link
Contributor

@elharo elharo left a comment

Choose a reason for hiding this comment

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

Sounds like a good idea. It should be a major version bump in the plugin's own version.

go back to good ol' stinky mojo logger and replace vs replaceAll
@michael-o michael-o self-requested a review March 24, 2023 08:46
@cstamas
Copy link
Member Author

cstamas commented Mar 24, 2023

Ran maven 3.9,.x build (w/ currently used MRR plugin, then with 3.1.0-SNAPSHOT), diffed outputs:

[cstamas@urnebes maven (maven-3.9.x *)]$ diff -wB maven-compat/target/classes/META-INF/DEPENDENCIES ~/tmp/mrrp/original/compat/DEPENDENCIES 
76,79d53
< 
<   - Maven Compat (https://maven.apache.org/ref/3.9.2-SNAPSHOT/maven-compat/) org.apache.maven:maven-compat:jar:3.9.2-SNAPSHOT
<     License: Apache-2.0  (https://www.apache.org/licenses/LICENSE-2.0.txt)
< 
[cstamas@urnebes maven (maven-3.9.x *)]$ diff -wB maven-compat/target/classes/META-INF/LICENSE ~/tmp/mrrp/original/compat/LICENSE 
[cstamas@urnebes maven (maven-3.9.x *)]$ diff -wB maven-compat/target/classes/META-INF/NOTICE ~/tmp/mrrp/original/compat/NOTICE 
[cstamas@urnebes maven (maven-3.9.x *)]$ diff -wB maven-core/target/classes/META-INF/DEPENDENCIES ~/tmp/mrrp/original/core/DEPENDENCIES 
76,79d53
< 
<   - Maven Core (https://maven.apache.org/ref/3.9.2-SNAPSHOT/maven-core/) org.apache.maven:maven-core:jar:3.9.2-SNAPSHOT
<     License: Apache-2.0  (https://www.apache.org/licenses/LICENSE-2.0.txt)
< 
[cstamas@urnebes maven (maven-3.9.x *)]$ diff -wB maven-core/target/classes/META-INF/LICENSE ~/tmp/mrrp/original/core/LICENSE
[cstamas@urnebes maven (maven-3.9.x *)]$ diff -wB maven-core/target/classes/META-INF/NOTICE ~/tmp/mrrp/original/core/NOTICE 
[cstamas@urnebes maven (maven-3.9.x *)]$ 

Seems in LICENSES the plugin now includes "itself" as well?
And finally, the warnings are gone as well...

@cstamas
Copy link
Member Author

cstamas commented Mar 24, 2023

Last commit fixes:

[cstamas@urnebes maven (maven-3.9.x *)]$ diff -wB maven-compat/target/classes/META-INF/DEPENDENCIES ~/tmp/mrrp/original/compat/DEPENDENCIES 
[cstamas@urnebes maven (maven-3.9.x *)]$ diff -wB maven-compat/target/classes/META-INF/LICENSE ~/tmp/mrrp/original/compat/LICENSE 
[cstamas@urnebes maven (maven-3.9.x *)]$ diff -wB maven-compat/target/classes/META-INF/NOTICE ~/tmp/mrrp/original/compat/NOTICE 
[cstamas@urnebes maven (maven-3.9.x *)]$ diff -wB maven-core/target/classes/META-INF/DEPENDENCIES ~/tmp/mrrp/original/core/DEPENDENCIES 
[cstamas@urnebes maven (maven-3.9.x *)]$ diff -wB maven-core/target/classes/META-INF/LICENSE ~/tmp/mrrp/original/core/LICENSE
[cstamas@urnebes maven (maven-3.9.x *)]$ diff -wB maven-core/target/classes/META-INF/NOTICE ~/tmp/mrrp/original/core/NOTICE 
[cstamas@urnebes maven (maven-3.9.x *)]$ 

So, in Maven 3.9.x build this PR now produces same (sans some extra newlines, I guess due templates?) output as currently used 1.7.0 plugin.

</goals>
<configuration>
<runOnlyAtExecutionRoot>true</runOnlyAtExecutionRoot>
<!-- <runOnlyAtExecutionRoot>true</runOnlyAtExecutionRoot>-->
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe I'm misunderstanding this. Was the runOnlyAtExecutionRoot parameter removed, made a no-op, or still exists in a different plugin but is no longer needed here?

My concern is whether this change requires dependents to update their own POMs or not.

Copy link
Member Author

Choose a reason for hiding this comment

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

The runOnlyAtExecutionRoot has been removed. But for maven plugin, this is same as no-op as build will not break even if present. Maybe we need to break the build and redirect users to new mojo?

See https://lists.apache.org/thread/bhtozf6w2nknvfzbfc1jb76fr6rqpkbx

The truth is, that this plugin was made for ASF purposes, am unsure is anyone outside ASF using it... so this is not a "general public" plugin like compiler or assembly plugin is...

Copy link
Member

Choose a reason for hiding this comment

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

The new goal name should be logically identically to other plugins: "aggregate" at beginning or end:

osipovmi@deblndw011x:~/var/Projekte/maven-sources (submodules =)
$ grep -r --include='*.java' -- '-aggregate"' .
./plugins/reporting/jxr/maven-jxr-plugin/src/main/java/org/apache/maven/plugin/jxr/AggregatorJxrTestReport.java:@Mojo( name = "test-aggregate", aggregator = true )
./plugins/reporting/maven-checkstyle-plugin/src/main/java/org/apache/maven/plugins/checkstyle/CheckstyleAggregateReport.java:@Mojo( name = "checkstyle-aggregate", aggregator = true, requiresDependencyResolution = ResolutionScope.COMPILE,
./plugins/reporting/maven-checkstyle-plugin/src/main/java/org/apache/maven/plugins/checkstyle/CheckstyleAggregateReport.java:        return "checkstyle-aggregate";
./plugins/reporting/maven-javadoc-plugin/src/main/java/org/apache/maven/plugins/javadoc/AggregatorTestJavadocReport.java:@Mojo( name = "test-aggregate", aggregator = true, requiresDependencyResolution = ResolutionScope.TEST )
./plugins/tools/maven-pdf-plugin/src/main/java/org/apache/maven/plugins/pdf/PdfAggregateMojo.java:    @Parameter( defaultValue = "${project.build.directory}/pdf-aggregate", required = true )
./plugins/tools/maven-pdf-plugin/src/main/java/org/apache/maven/plugins/pdf/PdfAggregateMojo.java:    @Parameter( defaultValue = "${project.build.directory}/pdf-aggregate", required = true )
osipovmi@deblndw011x:~/var/Projekte/maven-sources (submodules =)
$ grep -r --include='*.java' '"aggregate-' .
./plugins/reporting/jxr/maven-jxr-plugin/src/test/java/org/apache/maven/plugin/jxr/stubs/AggregateSubmodule1MavenProjectStub.java:        setArtifactId( "aggregate-test-submodule1" );
./plugins/reporting/jxr/maven-jxr-plugin/src/test/java/org/apache/maven/plugin/jxr/stubs/AggregateSubmodule2MavenProjectStub.java:        setArtifactId( "aggregate-test-submodule2" );
./plugins/reporting/maven-javadoc-plugin/src/main/java/org/apache/maven/plugins/javadoc/AggregatorJavadocJar.java:@Mojo( name = "aggregate-jar", defaultPhase = LifecyclePhase.PACKAGE, aggregator = true,
./plugins/reporting/maven-javadoc-plugin/src/main/java/org/apache/maven/plugins/javadoc/AggregatorJavadocNoForkReport.java:@Mojo( name = "aggregate-no-fork", requiresDependencyResolution = ResolutionScope.COMPILE )
./plugins/reporting/maven-javadoc-plugin/src/test/java/org/apache/maven/plugins/javadoc/AggregatorJavadocReportTest.java:        File testPom = new File( unit, "aggregate-test/aggregate-test-plugin-config.xml" );
./plugins/reporting/maven-javadoc-plugin/src/test/java/org/apache/maven/plugins/javadoc/AggregatorJavadocReportTest.java:        File testPom = new File( unit, "aggregate-resources-test/aggregate-resources-test-plugin-config.xml" );
./plugins/reporting/maven-javadoc-plugin/src/test/java/org/apache/maven/plugins/javadoc/AggregatorJavadocReportTest.java:      File testPom = new File( unit, "aggregate-modules-not-in-subfolders-test/all/pom.xml");
./plugins/reporting/maven-javadoc-plugin/src/test/java/org/apache/maven/plugins/javadoc/stubs/AggregateProject1TestMavenProjectStub.java:        setArtifactId( "aggregate-test-project1" );
./plugins/reporting/maven-javadoc-plugin/src/test/java/org/apache/maven/plugins/javadoc/stubs/AggregateProject1TestMavenProjectStub.java:        build.setFinalName( "aggregate-test-project1" );
./plugins/reporting/maven-javadoc-plugin/src/test/java/org/apache/maven/plugins/javadoc/stubs/AggregateProject2TestMavenProjectStub.java:        setArtifactId( "aggregate-test-project2" );
./plugins/reporting/maven-javadoc-plugin/src/test/java/org/apache/maven/plugins/javadoc/stubs/AggregateProject2TestMavenProjectStub.java:        build.setFinalName( "aggregate-test-project2" );
./plugins/reporting/maven-javadoc-plugin/src/test/java/org/apache/maven/plugins/javadoc/stubs/AggregateResourcesTestMavenProjectStub.java:                "aggregate-resources-test-plugin-config.xml",
./plugins/reporting/maven-javadoc-plugin/src/test/java/org/apache/maven/plugins/javadoc/stubs/AggregateTestMavenProjectStub.java:        readModel( new File( getBasedir(), "aggregate-test-plugin-config.xml" ) );
./plugins/reporting/maven-pmd-plugin/src/main/java/org/apache/maven/plugins/pmd/AggregatorCpdReport.java:@Mojo( name = "aggregate-cpd", aggregator = true, threadSafe = true )
./plugins/reporting/maven-pmd-plugin/src/main/java/org/apache/maven/plugins/pmd/AggregatorCpdViolationCheckMojo.java:@Mojo( name = "aggregate-cpd-check", defaultPhase = LifecyclePhase.VERIFY, threadSafe = true, aggregator = true )
./plugins/reporting/maven-pmd-plugin/src/main/java/org/apache/maven/plugins/pmd/AggregatorCpdViolationCheckMojo.java:@Execute( goal = "aggregate-cpd" )
./plugins/reporting/maven-pmd-plugin/src/main/java/org/apache/maven/plugins/pmd/AggregatorPmdNoForkReport.java:@Mojo( name = "aggregate-pmd-no-fork", aggregator = true, threadSafe = true,
./plugins/reporting/maven-pmd-plugin/src/main/java/org/apache/maven/plugins/pmd/AggregatorPmdReport.java:@Mojo( name = "aggregate-pmd", aggregator = true, threadSafe = true,
./plugins/reporting/maven-pmd-plugin/src/main/java/org/apache/maven/plugins/pmd/AggregatorPmdViolationCheckMojo.java:@Mojo( name = "aggregate-pmd-check", defaultPhase = LifecyclePhase.VERIFY, aggregator = true, threadSafe = true )
./plugins/reporting/maven-pmd-plugin/src/main/java/org/apache/maven/plugins/pmd/AggregatorPmdViolationCheckMojo.java:@Execute( goal = "aggregate-pmd" )

pom.xml Show resolved Hide resolved
</goals>
<configuration>
<runOnlyAtExecutionRoot>true</runOnlyAtExecutionRoot>
<!-- <runOnlyAtExecutionRoot>true</runOnlyAtExecutionRoot>-->
Copy link
Member

Choose a reason for hiding this comment

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

The new goal name should be logically identically to other plugins: "aggregate" at beginning or end:

osipovmi@deblndw011x:~/var/Projekte/maven-sources (submodules =)
$ grep -r --include='*.java' -- '-aggregate"' .
./plugins/reporting/jxr/maven-jxr-plugin/src/main/java/org/apache/maven/plugin/jxr/AggregatorJxrTestReport.java:@Mojo( name = "test-aggregate", aggregator = true )
./plugins/reporting/maven-checkstyle-plugin/src/main/java/org/apache/maven/plugins/checkstyle/CheckstyleAggregateReport.java:@Mojo( name = "checkstyle-aggregate", aggregator = true, requiresDependencyResolution = ResolutionScope.COMPILE,
./plugins/reporting/maven-checkstyle-plugin/src/main/java/org/apache/maven/plugins/checkstyle/CheckstyleAggregateReport.java:        return "checkstyle-aggregate";
./plugins/reporting/maven-javadoc-plugin/src/main/java/org/apache/maven/plugins/javadoc/AggregatorTestJavadocReport.java:@Mojo( name = "test-aggregate", aggregator = true, requiresDependencyResolution = ResolutionScope.TEST )
./plugins/tools/maven-pdf-plugin/src/main/java/org/apache/maven/plugins/pdf/PdfAggregateMojo.java:    @Parameter( defaultValue = "${project.build.directory}/pdf-aggregate", required = true )
./plugins/tools/maven-pdf-plugin/src/main/java/org/apache/maven/plugins/pdf/PdfAggregateMojo.java:    @Parameter( defaultValue = "${project.build.directory}/pdf-aggregate", required = true )
osipovmi@deblndw011x:~/var/Projekte/maven-sources (submodules =)
$ grep -r --include='*.java' '"aggregate-' .
./plugins/reporting/jxr/maven-jxr-plugin/src/test/java/org/apache/maven/plugin/jxr/stubs/AggregateSubmodule1MavenProjectStub.java:        setArtifactId( "aggregate-test-submodule1" );
./plugins/reporting/jxr/maven-jxr-plugin/src/test/java/org/apache/maven/plugin/jxr/stubs/AggregateSubmodule2MavenProjectStub.java:        setArtifactId( "aggregate-test-submodule2" );
./plugins/reporting/maven-javadoc-plugin/src/main/java/org/apache/maven/plugins/javadoc/AggregatorJavadocJar.java:@Mojo( name = "aggregate-jar", defaultPhase = LifecyclePhase.PACKAGE, aggregator = true,
./plugins/reporting/maven-javadoc-plugin/src/main/java/org/apache/maven/plugins/javadoc/AggregatorJavadocNoForkReport.java:@Mojo( name = "aggregate-no-fork", requiresDependencyResolution = ResolutionScope.COMPILE )
./plugins/reporting/maven-javadoc-plugin/src/test/java/org/apache/maven/plugins/javadoc/AggregatorJavadocReportTest.java:        File testPom = new File( unit, "aggregate-test/aggregate-test-plugin-config.xml" );
./plugins/reporting/maven-javadoc-plugin/src/test/java/org/apache/maven/plugins/javadoc/AggregatorJavadocReportTest.java:        File testPom = new File( unit, "aggregate-resources-test/aggregate-resources-test-plugin-config.xml" );
./plugins/reporting/maven-javadoc-plugin/src/test/java/org/apache/maven/plugins/javadoc/AggregatorJavadocReportTest.java:      File testPom = new File( unit, "aggregate-modules-not-in-subfolders-test/all/pom.xml");
./plugins/reporting/maven-javadoc-plugin/src/test/java/org/apache/maven/plugins/javadoc/stubs/AggregateProject1TestMavenProjectStub.java:        setArtifactId( "aggregate-test-project1" );
./plugins/reporting/maven-javadoc-plugin/src/test/java/org/apache/maven/plugins/javadoc/stubs/AggregateProject1TestMavenProjectStub.java:        build.setFinalName( "aggregate-test-project1" );
./plugins/reporting/maven-javadoc-plugin/src/test/java/org/apache/maven/plugins/javadoc/stubs/AggregateProject2TestMavenProjectStub.java:        setArtifactId( "aggregate-test-project2" );
./plugins/reporting/maven-javadoc-plugin/src/test/java/org/apache/maven/plugins/javadoc/stubs/AggregateProject2TestMavenProjectStub.java:        build.setFinalName( "aggregate-test-project2" );
./plugins/reporting/maven-javadoc-plugin/src/test/java/org/apache/maven/plugins/javadoc/stubs/AggregateResourcesTestMavenProjectStub.java:                "aggregate-resources-test-plugin-config.xml",
./plugins/reporting/maven-javadoc-plugin/src/test/java/org/apache/maven/plugins/javadoc/stubs/AggregateTestMavenProjectStub.java:        readModel( new File( getBasedir(), "aggregate-test-plugin-config.xml" ) );
./plugins/reporting/maven-pmd-plugin/src/main/java/org/apache/maven/plugins/pmd/AggregatorCpdReport.java:@Mojo( name = "aggregate-cpd", aggregator = true, threadSafe = true )
./plugins/reporting/maven-pmd-plugin/src/main/java/org/apache/maven/plugins/pmd/AggregatorCpdViolationCheckMojo.java:@Mojo( name = "aggregate-cpd-check", defaultPhase = LifecyclePhase.VERIFY, threadSafe = true, aggregator = true )
./plugins/reporting/maven-pmd-plugin/src/main/java/org/apache/maven/plugins/pmd/AggregatorCpdViolationCheckMojo.java:@Execute( goal = "aggregate-cpd" )
./plugins/reporting/maven-pmd-plugin/src/main/java/org/apache/maven/plugins/pmd/AggregatorPmdNoForkReport.java:@Mojo( name = "aggregate-pmd-no-fork", aggregator = true, threadSafe = true,
./plugins/reporting/maven-pmd-plugin/src/main/java/org/apache/maven/plugins/pmd/AggregatorPmdReport.java:@Mojo( name = "aggregate-pmd", aggregator = true, threadSafe = true,
./plugins/reporting/maven-pmd-plugin/src/main/java/org/apache/maven/plugins/pmd/AggregatorPmdViolationCheckMojo.java:@Mojo( name = "aggregate-pmd-check", defaultPhase = LifecyclePhase.VERIFY, aggregator = true, threadSafe = true )
./plugins/reporting/maven-pmd-plugin/src/main/java/org/apache/maven/plugins/pmd/AggregatorPmdViolationCheckMojo.java:@Execute( goal = "aggregate-pmd" )

aggregator = true,
requiresDependencyResolution = ResolutionScope.TEST,
threadSafe = true )
public class AggregatorProcessRemoteResourcesMojo
Copy link
Member

Choose a reason for hiding this comment

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

See my precending comment regarding naming

@kwin
Copy link
Member

kwin commented Apr 15, 2023

This is one of the last plugins referenced in ASF parent pom whose latest release version still leads to a WARN with Maven 3.9.x. Therefore it would be nice to soon merge, release and then update in ASF parent.

@slawekjaranowski
Copy link
Member

Conflict resolved.

Tested with m-dependency-p, looks better

diff -wB target/classes/META-INF/DEPENDENCIES target-2/classes/META-INF/DEPENDENCIES 
37,43d29
< 
<   - Plexus Classworlds (http://plexus.codehaus.org/plexus-classworlds/) org.codehaus.plexus:plexus-classworlds:bundle:2.5.2
<     License: The Apache Software License, Version 2.0  (http://www.apache.org/licenses/LICENSE-2.0.txt)
< 
<   - Plexus :: Component Annotations (http://plexus.codehaus.org/plexus-containers/plexus-component-annotations/) org.codehaus.plexus:plexus-component-annotations:jar:1.5.5
<     License: The Apache Software License, Version 2.0  (http://www.apache.org/licenses/LICENSE-2.0.txt)
< 
51c36,39
< 
---
>   - Plexus Classworlds (http://codehaus-plexus.github.io/plexus-classworlds/) org.codehaus.plexus:plexus-classworlds:bundle:2.6.0
>     License: Apache License, Version 2.0  (http://www.apache.org/licenses/LICENSE-2.0.txt)
>   - Plexus :: Component Annotations (http://codehaus-plexus.github.io/plexus-containers/plexus-component-annotations/) org.codehaus.plexus:plexus-component-annotations:jar:2.0.0
>     License: Apache License, Version 2.0  (http://www.apache.org/licenses/LICENSE-2.0.txt)
103,106d74
<   - Logging (http://jakarta.apache.org/commons/${pom.artifactId.substring(8)}/) commons-logging:commons-logging:jar:1.1
<     License: The Apache Software License, Version 2.0  (/LICENSE.txt)
< 
< 
114,117d79
< 
<   - Commons Codec (http://commons.apache.org/codec/) commons-codec:commons-codec:jar:1.6
<     License: The Apache Software License, Version 2.0  (http://www.apache.org/licenses/LICENSE-2.0.txt)
< 
123c84,85
< 
---
>   - Apache Commons Logging (http://commons.apache.org/proper/commons-logging/) commons-logging:commons-logging:jar:1.2
>     License: The Apache Software License, Version 2.0  (http://www.apache.org/licenses/LICENSE-2.0.txt)
135c94,95
< 
---
>   - Apache Commons Codec (http://commons.apache.org/proper/commons-codec/) commons-codec:commons-codec:jar:1.11
>     License: Apache License, Version 2.0  (https://www.apache.org/licenses/LICENSE-2.0.txt)

Artifacts used by project are listed

@slawekjaranowski
Copy link
Member

As I see discussion here is about:

  • what version will be next
  • what name of goal should be

can we take such simple decision?

I hope gola name like aggregate-process will be ok - @michael-o ?

@michael-o
Copy link
Member

As I see discussion here is about:

* what version will be next

* what name of goal should be

can we take such simple decision?

I hope gola name like aggregate-process will be ok - @michael-o ?

I guess so.

@slawekjaranowski
Copy link
Member

Goal aggregator-process renamed to aggregate

But I see one more issue runOnlyAtExecutionRoot != aggregate goal
We need add <inherited>false</inherited> to configuration

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants