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

Drop transitive dependency on apiguardian #3193

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mkoncek
Copy link

@mkoncek mkoncek commented Mar 15, 2023

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

@sormuras
Copy link
Member

sormuras commented Mar 15, 2023

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
Copy link
Member

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

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;

@sormuras sormuras self-assigned this Mar 15, 2023
@mkoncek
Copy link
Author

mkoncek commented Mar 15, 2023

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
Copy link
Member

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. 🏋️

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

Successfully merging this pull request may close these issues.

None yet

2 participants