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 #1357

Closed
C-Otto opened this issue Jan 24, 2021 · 11 comments · Fixed by #1943
Closed

Add module descriptor #1357

C-Otto opened this issue Jan 24, 2021 · 11 comments · Fixed by #1943
Labels
feign-12 Issues that are related to the next major release

Comments

@C-Otto
Copy link

C-Otto commented Jan 24, 2021

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

kdavisk6 commented Feb 3, 2021

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

@kdavisk6 kdavisk6 added the feign-12 Issues that are related to the next major release label Feb 3, 2021
@Sineaggi
Copy link
Contributor

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

aalmiray commented Feb 3, 2023

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

velo commented Feb 4, 2023

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

@aalmiray
Copy link
Contributor

aalmiray commented Feb 4, 2023

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

aalmiray commented Feb 4, 2023

Blocked by Netflix/ribbon#387

@aalmiray
Copy link
Contributor

aalmiray commented Feb 4, 2023

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

aalmiray commented Feb 6, 2023

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.

@C-Otto C-Otto closed this as completed Feb 18, 2023
@aalmiray
Copy link
Contributor

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

C-Otto commented Feb 18, 2023

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.

@C-Otto C-Otto reopened this Feb 18, 2023
@velo
Copy link
Member

velo commented Feb 18, 2023

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

aalmiray added a commit to aalmiray/feign that referenced this issue Feb 19, 2023
aalmiray added a commit to aalmiray/feign that referenced this issue Feb 20, 2023
aalmiray added a commit to aalmiray/feign that referenced this issue Feb 20, 2023
velo added a commit that referenced this issue Feb 26, 2023
* Adds explicit module descriptors to a subset of modules (#1943)

Fixes #1357

* Configure CI to run with Java 11

* Configure moditect for all modules, enable only on those that required it

* Do not skip moditect when running tests

* Only skip modules that don't work with moditect plugin

---------

Co-authored-by: Marvin Froeder <velo@users.noreply.github.com>
Co-authored-by: Marvin Froeder <velobr@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feign-12 Issues that are related to the next major release
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants