Drop transitive dependency on apiguardian

[mkoncek]

Overview

Annotations should stay as static dependencies and not transitive this was explained in https://mail.openjdk.org/pipermail/jigsaw-dev/2021-June/014674.html.

Currently the problem we are facing is that when building assertj-core, we need to have apiguardian as well because assertj-core requires junit5 which transitively requires apiguardian even though assertj-core does not use apiguardian at all.

---

I hereby agree to the terms of the JUnit Contributor License Agreement.

---

Definition of Done

• There are no TODOs left in the code
• Method preconditions are checked and documented in the method's Javadoc
• Coding conventions (e.g. for logging) have been followed
• Change is covered by automated tests including corner cases, errors, and exception handling
• Public API has Javadoc and @API annotations
• Change is documented in the User Guide and Release Notes
• All continuous integration builds pass

[sormuras]

Dropping transitive from:

module org.junit.platform.commons {
	requires java.logging;
	requires java.management; // needed by RuntimeUtils to determine input arguments
	requires static transitive org.apiguardian.api;
	// ...
}

generates warnings like:

/home/runner/work/junit5/junit5
  /junit-platform-commons/build/module/org.junit.platform.commons
  /org/junit/platform/commons/annotation/Testable.java:79:
warning: [exports] class API in module org.apiguardian.api is not indirectly exported using requires transitive
@API(status = STABLE, since = "1.0")
 ^

Suppressing those 30+ warnings doesn't seem to be the correct way, neither. 🤔

[sormuras]

We are talking about module org.assertj.core, right?

• https://github.com/assertj/assertj/blob/main/assertj-core/src/main/java9/module-info.java

module org.assertj.core {
  // AssertJ Core's package API
  exports org.assertj.core.annotations;
  //  [...]
  exports org.assertj.core.util.xml;

  requires static java.logging; // required when printThreadDump is true
  requires static java.management;
  requires static java.xml; // used for XML pretty print
  requires static junit;
  requires static net.bytebuddy;
  requires static org.hamcrest;
  requires static org.junit.jupiter.api;
  requires static org.opentest4j; // to throw AssertionFailedError which is IDE friendly

  // Services loaded by org.assertj.core.configuration.ConfigurationProvider
  uses org.assertj.core.configuration.Configuration;
  uses org.assertj.core.presentation.Representation;
}

From requires static junit; follows:

• requires static org.hamcrest;

From requires static org.junit.jupiter.api; follows:

• requires static org.opentest4j;
• requires static org.apiguardian.api;

[mkoncek]

Yes, org.assertj.core. What you said is true but it shouldn't be that way. Build dependencies are not transitive and apiguardian should not be explicitly transitive.
Also I know you talked about this in #2063. But I would still put more weight on openjdk folks.

[sormuras]

How unfortunate it is, org.apiguardian.api is a compile time and run-time dependency of org.junit.jupiter.api and many (all?) other modules published by the JUnit team. Including modules of the JUnit Platform, JUnit Jupiter, and JUnit Vintage.

This is due to the fact that we added an enum, namely API.Status, to the public API of the org.apiguardian.api.API annotation. If we'd chose type String for the status() accessor back then, we weren't be in such "transitive troubles" as we are now, IIRC.

Also I know you talked about this in #2063.

Great. I knew I wrote something similar before. Technically, nothing changed since 2019.

But I would still put more weight on openjdk folks.

Done. 🏋️

[marcphilipp]

In the context of apiguardian-team/apiguardian#29, I've just verified that transitive is still required to avoid the compiler warning. I agree that it would be great if the Java compiler could be made to not emit it for classes only referenced from annotations. For the time being, we have to assume that's not going to happen. Therefore, I'm closing this PR.