Make the mojo define old & new classpaths automatically

[jglick]

I tried to use japicmp on jenkinsci/jenkins#4848 but the mojo was unusable as is: it automatically added the classpath, but from the new artifact (current effective pom.xml) even for the old artifact. Since this PR removed some things from the classpath and added other things, and some of the switched JARs had types used as supertypes of types in the source tree, there were numerous errors blocking report generation.

ignoreMissingClasses allowed the report to proceed, but simply omitted actual incompatibilities I wanted to know about, so this was not a workaround. And given the giant classpath, manually declaring it was not an option either. Note that the documentation claims that

Dependencies declared in the enclosing pom.xml and its parents are added automatically.

but this was only true when using <dependencies>; when using <oldClassPathDependencies> and/or <newClassPathDependencies>, the POM dependencies were calculated but then ignored (since in TWO_SEPARATE_CLASSPATHS mode classPathEntries is silently ignored), so it was not even feasible to manually specify the different classpath elements for the old and new versions. This patch fixes that as well.

This patch tries to fix the mojo to behave correctly: the old artifact gets its declared classpath (plus any manual additions), and the new artifact gets its declared classpath (plus any manual additions). There are other modes of looking up the old artifact not covered by this patch. I was also unsure of the best way to add test coverage; should I simply delete the redundant

japicmp/japicmp-testbase/japicmp-test-service-impl-base/japicmp-test-service-test/pom.xml

Lines 97 to 110 in bdf3ce2

	<oldClassPathDependencies>
	<dependency>
	<groupId>com.github.siom79.japicmp</groupId>
	<artifactId>japicmp-test-service-v1</artifactId>
	<version>${project.version}</version>
	</dependency>
	</oldClassPathDependencies>
	<newClassPathDependencies>
	<dependency>
	<groupId>com.github.siom79.japicmp</groupId>
	<artifactId>japicmp-test-service-v2</artifactId>
	<version>${project.version}</version>
	</dependency>
	</newClassPathDependencies>

and get that test to pass?

[jglick]

@siom79?

[siom79]

I did some major refactorings in the Mojo due to the support for Java 16. I had to upgrade to new maven APIs. Hence; I expect that it is difficult to merge this PR. Have you time to try the merge von the current trunk?

[jglick]

I will try to recheck it merged with trunk. Now is a good time to ask again for advice on

the best way to add test coverage; should I simply delete the redundant

japicmp/japicmp-testbase/japicmp-test-service-impl-base/japicmp-test-service-test/pom.xml

Lines 97 to 110 in bdf3ce2

	<oldClassPathDependencies>
	<dependency>
	<groupId>com.github.siom79.japicmp</groupId>
	<artifactId>japicmp-test-service-v1</artifactId>
	<version>${project.version}</version>
	</dependency>
	</oldClassPathDependencies>
	<newClassPathDependencies>
	<dependency>
	<groupId>com.github.siom79.japicmp</groupId>
	<artifactId>japicmp-test-service-v2</artifactId>
	<version>${project.version}</version>
	</dependency>
	</newClassPathDependencies>

and get that test to pass?

[siom79]

No, pelase do not delete these two artifacts. The idea here is that I have two artifacts v1 and v2 and compare them in japicmp-test-service-test with each other. They don't have different versions, because I don't want to have two builds to test. Just one build that creates "two versions" (version in the artifact name).

[jglick]

I have two artifacts v1 and v2 and compare them

Yes, I get that you are comparing different artifactIds for the oldVersion and newVersion. My point is that the test redundantly specifies oldClassPathDependencies and newClassPathDependencies; it should not need to do that because the default should be to scan classpaths of those artifacts automatically. Yet

diff --git japicmp-testbase/japicmp-test-service-impl-base/japicmp-test-service-test/pom.xml japicmp-testbase/japicmp-test-service-impl-base/japicmp-test-service-test/pom.xml
index f7a929c..1d084cb 100644
--- japicmp-testbase/japicmp-test-service-impl-base/japicmp-test-service-test/pom.xml
+++ japicmp-testbase/japicmp-test-service-impl-base/japicmp-test-service-test/pom.xml
@@ -94,20 +94,6 @@
 							<version>${project.version}</version>
 						</dependency>
 					</newVersion>
-					<oldClassPathDependencies>
-						<dependency>
-							<groupId>com.github.siom79.japicmp</groupId>
-							<artifactId>japicmp-test-service-v1</artifactId>
-							<version>${project.version}</version>
-						</dependency>
-					</oldClassPathDependencies>
-					<newClassPathDependencies>
-						<dependency>
-							<groupId>com.github.siom79.japicmp</groupId>
-							<artifactId>japicmp-test-service-v2</artifactId>
-							<version>${project.version}</version>
-						</dependency>
-					</newClassPathDependencies>
 					<parameter>
 						<accessModifier>public</accessModifier>
 						<onlyModified>true</onlyModified>

seems to pass without my patch and fail with my patch

Could not load 'Class not found: japicmp.test.service.InterfaceMethodRemoved': japicmp.test.service.InterfaceMethodRemoved. Please make sure that all libraries have been added to the classpath (OLD CLASSPATH= / NEW CLASSPATH=:/home/jglick/.m2/repository/com/github/siom79/japicmp/japicmp-test-service-v1/0.14.4-SNAPSHOT/japicmp-test-service-v1-0.14.4-SNAPSHOT.jar:/home/jglick/.m2/repository/com/github/siom79/japicmp/japicmp-test-service-impl-v1/0.14.4-SNAPSHOT/japicmp-test-service-impl-v1-0.14.4-SNAPSHOT.jar:) or try the option '--ignore-missing-classes'.

which is the opposite of what I was expecting.

On master, the test also passes with the patch above. I can confirm that the mojo is configuring ONE_COMMON_CLASSPATH, which is wrong, yet the test passes anyway. It seems the test is too weak; in two-classpath mode (no modifications to source tree) japicmp-testbase/japicmp-test-service-impl-base/japicmp-test-service-test/target/japicmp/japicmp.diff shows

***  MODIFIED CLASS: PUBLIC japicmp.test.service.InterfaceMethodAddedImpl  (not serializable)
	===  CLASS FILE FORMAT VERSION: 52.0 <- 52.0
	===* UNCHANGED INTERFACE: japicmp.test.service.InterfaceMethodAdded
	+++  NEW METHOD: PUBLIC(+) void methodAdded()
***! MODIFIED CLASS: PUBLIC japicmp.test.service.InterfaceMethodRemovedImpl  (not serializable)
	===  CLASS FILE FORMAT VERSION: 52.0 <- 52.0
	===! UNCHANGED INTERFACE: japicmp.test.service.InterfaceMethodRemoved
	---! REMOVED METHOD: PUBLIC(-) void methodRemoved()
***! MODIFIED CLASS: PUBLIC japicmp.test.service.SubclassAddsNewStaticField  (not serializable)
	===  CLASS FILE FORMAT VERSION: 52.0 <- 52.0
	+++! NEW FIELD: PUBLIC(+) STATIC(+) int field

whereas in one-classpath mode two lines are missing:

***  MODIFIED CLASS: PUBLIC japicmp.test.service.InterfaceMethodAddedImpl  (not serializable)
	===  CLASS FILE FORMAT VERSION: 52.0 <- 52.0
	+++  NEW METHOD: PUBLIC(+) void methodAdded()
***! MODIFIED CLASS: PUBLIC japicmp.test.service.InterfaceMethodRemovedImpl  (not serializable)
	===  CLASS FILE FORMAT VERSION: 52.0 <- 52.0
	---! REMOVED METHOD: PUBLIC(-) void methodRemoved()
***! MODIFIED CLASS: PUBLIC japicmp.test.service.SubclassAddsNewStaticField  (not serializable)
	===  CLASS FILE FORMAT VERSION: 52.0 <- 52.0
	+++! NEW FIELD: PUBLIC(+) STATIC(+) int field

but MavenPluginTestIT only checks output of SubclassAddsNewStaticField which behaves identically in either case.

I have not tried to resolve merge conflicts yet since it does not look trivial and I would like to sort out the test situation first.

[jglick]

I am now able to reproduce the problem in the mojo integration test, by extending it to use more differing signatures in the service module between v1 and v2, and adjusted the mojo to pass the test. There are however lots of operational modes not covered by the test, and some modes not covered by the mojo (see unimplemented clauses in setUpClassPathUsingMavenProject), and I am not even sure I know what all of these modes would mean in practice. Probably all of setUpClassPath needs to be heavily refactored and simplified; possibly all of the classpath calculations need to be moved to populateArchivesListsFromParameters? Which then means reworking getOptions a bit too.

[jglick]

Oh before I forget, to iterate quickly (~5s):

mvnd install -am -pl japicmp-maven-plugin,japicmp-testbase/japicmp-test-service-impl-base/japicmp-test-service-impl-v2,japicmp-testbase/japicmp-test-service-impl-base/japicmp-test-service-test