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

Add module descriptor #535

Closed
XakepSDK opened this issue Apr 13, 2021 · 39 comments · Fixed by #540
Closed

Add module descriptor #535

XakepSDK opened this issue Apr 13, 2021 · 39 comments · Fixed by #540

Comments

@XakepSDK
Copy link

Switch to module descriptor from automatic module name, so this library will have full support of java module system

@ben-manes
Copy link
Owner

ben-manes commented Apr 13, 2021

Until Gradle 7.0, the build system wasn't compatible and external plugins like bnd were not either. That blocked prior attempts to add the descriptor.

The current blocker is that adding it in breaks Eclipse, which is where I do development. I don't know if this is a Gradle Buildship issue or Eclipse's. It is unable to handle a split project (main, test, jmh).

I believe that the library's module-info.java would be,

module com.github.benmanes.caffeine {
  exports com.github.benmanes.caffeine.cache.stats;
  exports com.github.benmanes.caffeine.cache;

  requires transitive org.checkerframework.checker.qual;
  requires transitive com.google.errorprone.annotations;
}

However until there is proper support in the toolchain I don't know how to include it. One approach would be package into the jar as a generated resource, which is what I've seen Maven users do, but that seems error prone.

@ben-manes
Copy link
Owner

If anyone wants to try and make this work a PR is welcome.

@XakepSDK
Copy link
Author

Should annotations with runtime retention be transitive? Wouldn't be better to hide as much as possible?

@ben-manes
Copy link
Owner

requires static would be fine.

A248 added a commit to A248/caffeine that referenced this issue Apr 24, 2021
* Fixes ben-manes#535
* Uses Gradle Eclipse plugin to define add-reads options which
  satisfy Eclipse and make the project buildable through it in
  Eclipse 2021-03

Only the core caffeine module was eligible for a full module
  descriptor considering the circumstances:
* A descriptor was not added to caffeine-guava because some of
  its tests rely on split packages in order to acccess Guava's
  package-private types. Ensuring a smooth experience in
  Eclipse IDE requires resolving these split packages.
* The other modules still have some dependencies on filename-based
  modules, so it would be unwise to add module descriptors for
  them.
A248 added a commit to A248/caffeine that referenced this issue Apr 29, 2021
* Fixes ben-manes#535
* Uses Gradle Eclipse plugin to exclude module-info from
  compilation in Eclipse. Prevents hordes of issues.
A248 added a commit to A248/caffeine that referenced this issue Apr 29, 2021
* Fixes ben-manes#535
* Uses Gradle Eclipse plugin to exclude module-info from
  compilation in Eclipse. Prevents hordes of issues.
ben-manes pushed a commit that referenced this issue Apr 29, 2021
* Fixes #535
* Uses Gradle Eclipse plugin to exclude module-info from
  compilation in Eclipse. Prevents hordes of issues.
@ben-manes
Copy link
Owner

Released in 3.0.2 (thanks @A248!)

@soc
Copy link
Contributor

soc commented May 21, 2021

I'm seeing build failures caused by this, as we explicitly exclude these dependencies in Maven:

[ERROR] module not found: org.checkerframework.checker.qual
[ERROR] module not found: com.google.errorprone.annotations

@ben-manes suggested using requires static, but the patch by @A248 that was merged uses requires.

Could this be fixed?

@ben-manes
Copy link
Owner

Can you provide a reproduction? The static is in the descriptor. I’m not sure if this is a Maven bug or our misunderstanding.

module com.github.benmanes.caffeine {
exports com.github.benmanes.caffeine.cache;
exports com.github.benmanes.caffeine.cache.stats;
requires static transitive com.google.errorprone.annotations;
requires static transitive org.checkerframework.checker.qual;
}

@ben-manes
Copy link
Owner

This explains that it is needed during application compilation, but not runtime. That’s unlike Maven’s provided scope where the dependency needed it at compile time only but consumers did not. It seems there is no equivalent. Maybe you can change scoping rather than exclude?

@soc
Copy link
Contributor

soc commented May 21, 2021

@ben-manes Oh, apologies, I scanned over the module-info incorrectly, the static is indeed there!

Well, I guess then that approach of avoiding the leak of those annotation packages into the project/IDE auto-import won't work naymore. :-(

@soc
Copy link
Contributor

soc commented May 21, 2021

@ben-manes I'll give it a try, but I fear this doesn't provide the desired semantics ... we pretty much depend on annotation declarations being optional in the JVMS and not providing the jar file at all, but I think this is not something that is conceptually supported by Java modules.

@ben-manes
Copy link
Owner

I think you might need to have the exclusions and then also add the provided / optional dependencies explicitly with that scope setting. I don't think Maven's exclusions lets one change the scoping of a transitive, so you need to re-establish it at a lower scope. But maybe there is a neater way, e.g. some plugin that handles scope overriding.

@soc
Copy link
Contributor

soc commented May 21, 2021

The problem is not at the level of Maven dependencies, but at the level of Java modules.
Getting fancy at the Maven level doesn't help if the Java module system simply doesn't accept it.

Both requires and requires static require the dependency to be there at compile-time, so I guess there is not much I can do.
(No worries, this isn't a problem you have to worry about ;-))

@ben-manes
Copy link
Owner

Sorry, I kept thinking that this was a compilation failure due to excluding the dependency, rather than a runtime validation by the Java module system. If it is at runtime then there is no workaround.

@soc
Copy link
Contributor

soc commented May 21, 2021

Yes, it's a compilation failure, but one based on the module system rules.

If it is at runtime then there is no workaround.

Yes, I think this too.

Thanks for your thoughts and sorry for the noise, @ben-manes!

@ben-manes
Copy link
Owner

My naive expectation is that if you could scope the dependencies at compileOnly (which is what provided/optional do, iirc), then javac compilation should pass its module validation and at runtime static should pass by not requiring it on the module graph for class loading. Of course this is probably wrong or there are bugs since Java modules is so rarely used in practice.

@soc
Copy link
Contributor

soc commented May 21, 2021

I think the problem here is that compileOnly doesn't provide the desired semantics: making it impossible (at compile time) that the dependency gets accidentally used by my code (that hasn't been modularized already).

@soc
Copy link
Contributor

soc commented May 25, 2021

Btw, what's the reason for the transitive?

I just realized that not even the modularized parts of my code would be safe from these dependencies due to transitive.

@ben-manes
Copy link
Owner

ben-manes commented May 25, 2021

You’re probably right it’s not needed. I probably had it above just trying to figure out the compatibility issues with Eclipse, before @A248 worked it out. I’m unsure though since it is annotations on the api and if removing would be invalid if called through reflection?

What do you think @A248?

@soc
Copy link
Contributor

soc commented May 25, 2021

I think it shouldn't have any influence on reflection, it only adds an invisible "requires com.google.errorprone.annotations..." to all modules that use caffeine.

@A248
Copy link
Contributor

A248 commented May 25, 2021

I've been following this conversation loosely, however I haven't been able to come up with any stellar solutions. I know IDEs have the ability to filter imports, but I am guessing that you want this handled by the build system? There are tools which can check your build for usage of certain classes. I think ArchUnit can do this for you, though I've never used it. PMD also has this feature.

To the best of my knowledge, using transitive is correct is because Caffeine exposes these annotation types. requires static transitive is the equivalent of Gradle's compileOnlyApi. Like with Maven or Gradle, libraries really should declare their dependencies properly. Unlike Maven or Gradle, however, JPMS has no way to perform dependency exclusions, so I do not see an easy way out of this.

I will ask for clarification on the jigsaw-dev mailing list to see if there's a more appropriate solution.

@soc
Copy link
Contributor

soc commented May 25, 2021

using transitive is correct is because Caffeine exposes these annotation types

I don't get this ... with that logic, wouldn't all dependencies be declared transitive?

The only effect transitive has is that consuming modules don't need to write down the requires for it.

(I would argue that transitive is a misfeature, and should be avoided in pretty much any case.)

@A248
Copy link
Contributor

A248 commented May 25, 2021

I don't get this ... with that logic, wouldn't all dependencies be declared transitive?

The only effect transitive has is that consuming modules don't need to write down the requires for it.

transitive is not merely a feature of convenience, but the means of declaring a dependency which should be visible to API consumers, because a type from the dependency is exposed in the module's API.

This may help for explanation: https://nipafx.dev/java-modules-implied-readability/

Ignoring annotations for a moment, suppose you have the following method in an exported package. If Bar is from a dependency, transitive is a necessity here because API consumers will see the Bar return type:

public Bar foo();

@soc
Copy link
Contributor

soc commented May 25, 2021

Ignoring annotations for a moment, suppose you have the following method in an exported package. If Bar is from a dependency, transitive is a necessity here because API consumers will see the Bar return type:

It really isn't – either the consumer refers to type Bar, then he can require it, or the consumer doesn't, then he can elide it.

I see nothing wrong with being explicit. If some module isn't required in the consumer's module-info file, I know it's not being used. That's a huge plus over Maven dependencies, where everything is transitive by default.

Having modularized around 400 modules, I never felt the need to use transitive even once.

@A248
Copy link
Contributor

A248 commented May 25, 2021

Just FYI, use of transitive is further elaborated in the specification: https://openjdk.java.net/projects/jigsaw/spec/sotms/#implied-readability

Regardless, we'll receive helpful clarification from the jigsaw-dev mailing list with respect to handling annotation dependencies as used here. I've already sent them a message. Hopefully that will clear up any issue here.

@ben-manes
Copy link
Owner

@A248 I don't think your message went through. You have to subscribe to the mailing list to post.

@A248
Copy link
Contributor

A248 commented Jun 4, 2021

@soc You are right that we don't need to use transitive here.

The jigsaw-dev members (Alex Buckley and Remi Forax specifically) confirmed that requires static, without transitivity, is sufficient and works best. https://mail.openjdk.java.net/pipermail/jigsaw-dev/2021-June/thread.html

So, the caffeine module can be updated to remove the transitive declaration.

@ben-manes
Copy link
Owner

Thanks @A248. Let's make the change then. You're welcome to send a PR or I'll make the changes at some point soon.

@soc
Copy link
Contributor

soc commented Jun 4, 2021

@ben-manes Does #563 look as desired?

@ben-manes
Copy link
Owner

@soc lgtm

@A248
Copy link
Contributor

A248 commented Jun 4, 2021

All's good from what I see, for what it's worth.

@ben-manes
Copy link
Owner

fyi, there is a warning in the build that I am suppressing using -Xlint:all,-exports.

warning: [exports] class Nullable in module org.checkerframework.checker.qual is not indirectly exported using requires transitive

@XakepSDK
Copy link
Author

XakepSDK commented Jun 7, 2021

Should we ask jigsaw-dev about this?

@ben-manes
Copy link
Owner

Sure, you are welcome to. They explicitly said annotations in the api don’t need to be transitive, so they might view this as ignorable or a bug. It’s certainly confusing and contradictory advice.

@A248
Copy link
Contributor

A248 commented Jun 7, 2021

I've mentioned this on jigaw-dev, so we'll find out what's happening here:
https://mail.openjdk.java.net/pipermail/jigsaw-dev/2021-June/014675.html

@hohonuuli
Copy link

This just bit me. A note for folks coming across the issue. The quickest fix is to downgrade from caffeine 3.0.2 to 3.0.1.

Also, @ben-manes caffeine is awesome! Thanks for your continued work on it.

@ben-manes
Copy link
Owner

Released in 3.0.3

@nipafx
Copy link

nipafx commented Jul 6, 2021

Alex Buckley replied to the thread.

@soc
Copy link
Contributor

soc commented Jul 6, 2021

Also, it seems that requires static without transitive does not break the Maven dependency exclusions, as I previously anticipated. So that's a pretty good outcome, I guess.

@A248
Copy link
Contributor

A248 commented Jul 7, 2021

Released in 3.0.3

It might be valuable to briefly mention this in the 3.0.3 release notes.

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 a pull request may close this issue.

6 participants