OpenFeign / feign

Feign makes writing java http clients easier
Apache License 2.0
9.52k stars 1.93k forks source link

Add module descriptor #1357

Closed C-Otto closed 1 year ago

C-Otto commented 3 years ago

OpenFeign 10.10.1 does not seem to include a valid JPMS module descriptor:

$ jar --file=~/.gradle/caches/modules-2/files-2.1/io.github.openfeign/feign-core/10.10.1/d09c5d478b1cd83e95035539687b9ff4905dc99c/feign-core-10.10.1.jar --describe-module
No module descriptor found. Derived automatic module.

feign.core@10.10.1 automatic
requires java.base mandated
contains feign
contains feign.auth
contains feign.codec
contains feign.optionals
contains feign.querymap
contains feign.stream
contains feign.template

Please add this so that OpenFeign can be used in Java 9+ projects that make use of modules.

kdavisk6 commented 3 years ago

Open Feign can be used with Java 9+ projects, so your request is misleading, as these newer JDKs will derive an automatic module, as you've seen. We do not include module descriptors as our target platform is still Java 8. We will consider adding module descriptors when we update our baseline for the latest Java 11 LTS

Sineaggi commented 3 years ago

Maven supports building older libraries with java 8 while still shipping a java 9+ module-info https://maven.apache.org/plugins/maven-compiler-plugin/examples/module-info.html

aalmiray commented 1 year ago

Alternatively the ModiTect tools may be used to add a full module descriptor while still remaining Java8 compatible

Jetty did it this way before moving their baseline to Java 11.

velo commented 1 year ago

If you willing to send a pull request, I can review, approve, merge and cut a release with this

aalmiray commented 1 year ago

Great! Just did a quick check for split packages on all projects defined by the feign-bom artifact (using https://github.com/AdoptOpenJDK/jsplitpkgscan) and I'm happy to report there are no split packages 🎉! This makes the next step much simpler.

aalmiray commented 1 year ago

Blocked by https://github.com/Netflix/ribbon/issues/387

aalmiray commented 1 year ago

The soap module declares a service /META-INF/services/javax.xml.soap.SAAJMetaFactory with com.sun.xml.messaging.saaj.soap.SAAJMetaFactoryImpl as value. AFAICT full module descriptors are not allowed to expose services found in unexported packages. Generated module feign.soap won't export package com.sun.xml.messaging.saaj.soap thus causing a failure.

Is this service really required? If so, could it be exported by the owning module?

aalmiray commented 1 year ago

Netflix ribbon is in maintenance mode and has not posted a release since Aug 5 2001. Making ribbon modular requires resolving split packages which would result in binary compatibility breakage (prompting a major version bump). That's a lot of effort and there's no guarantee the change will be accepted nor released.

Instead I'd recommend not modularizing feign-ribbon at this time.

aalmiray commented 1 year ago

Hmm why was this marked as completed? I'm waiting on feedback from Feign team members to see if the proposed changes are adequate. Then I can send a PR.

C-Otto commented 1 year ago

I just closed the issue, I don't know why GitHub thinks this is related to some kind of completion. In my understanding it's not worth it modularizing feign-ribbon and, by extension, feign. It seems you still have hopes of doing this, though? I'm not using feign anymore, so I don't really care, but I'll leave this issue open if you prefer.

velo commented 1 year ago

Is this service really required? If so, could it be exported by the owning module?

I would say to modularize what is possible and ignore everything else like soap module.

If you can get a PR over the line shortly I can include on the upcoming releases