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

jetty-http-spi does not properly provide SPI for modules #11778

Closed
bowbahdoe opened this issue May 11, 2024 · 5 comments · Fixed by #11840
Closed

jetty-http-spi does not properly provide SPI for modules #11778

bowbahdoe opened this issue May 11, 2024 · 5 comments · Fixed by #11840
Assignees
Labels
Bug For general bugs on Jetty side

Comments

@bowbahdoe
Copy link

bowbahdoe commented May 11, 2024

Jetty version(s)
12.0.9

Jetty Environment
core

Java version/vendor (use: java -version)
21.0.1"

OS type/version
Mac OS / Ventura 13.3.1 (Apple Silicon)

Description

How to reproduce?

I made a reproducer repo here

https://github.com/bowbahdoe/jdk-httpserver-todobackend

./mvnw clean compile jlink:jlink
./target/maven-jlink/classifiers/image/bin/server

If you go to localhost:7777 in a browser you'll see that /favicon.ico is requested and that produces an exception. Jetty is not in the stacktrace, so I conclude that its implementation is not being used.

My guess is that its due to not having a provides here https://github.com/jetty/jetty.project/blob/jetty-12.0.x/jetty-core/jetty-http-spi/src/main/java/module-info.java

in addition, that artifact does not depend on jetty-server so i have to add it manually - not sure if thats intentional or not.

@bowbahdoe bowbahdoe added the Bug For general bugs on Jetty side label May 11, 2024
@sbordet
Copy link
Contributor

sbordet commented May 12, 2024

@bowbahdoe is there a reason you closed #11768?

@bowbahdoe
Copy link
Author

@sbordet Yes. I tried to go through the ECA and got to the end but the email that agreed wasn't the same as the email I had configured for the commit; though both are on my github account. Then i couldn't figure out how to add an email to the agreement.

So i got frustrated and figured opening an issue would be easier.

@sbordet
Copy link
Contributor

sbordet commented May 12, 2024

Sorry about the ECA experience 😞

It was a good change.

Let us know if you want to try again, so you'll have your contribution registered.
Otherwise, we'll do it.

Thanks!

@bowbahdoe
Copy link
Author

Let us know if you want to try again, so you'll have your contribution registered.
Otherwise, we'll do it.

If I have something else I will. I only looked at this one usage, but it might be worth doing a scan to see if there are other service providers that are in META-INF/services but not the module-infos.

@sbordet
Copy link
Contributor

sbordet commented May 12, 2024

Ok, we will do it.
Thanks!

@sbordet sbordet self-assigned this May 12, 2024
sbordet added a commit that referenced this issue May 24, 2024
Fixed all module-info.java files that did not have a "provides" declaration but had META-INF/services files.

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
sbordet added a commit that referenced this issue May 27, 2024
Fixed all module-info.java files that did not have a "provides" declaration but had META-INF/services files.

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug For general bugs on Jetty side
Projects
Status: ✅ Done
Development

Successfully merging a pull request may close this issue.

2 participants