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
Make the requirement on "osgi.serviceloader.processor" optional #2377
Conversation
This allows Eclipse to continue to make use of jetty-* bundles without requiring the presence of Apache Aries. This fixes a problem introduced by the resolution to #2164 and the same solution is used as in 10cdf16 See also the discussion at Eclipse: https://bugs.eclipse.org/bugs/show_bug.cgi?id=532294 Signed-off-by: Mat Booth <mat.booth@redhat.com>
|
Why only There's many more Jetty Artifacts that have ServiceLoader requirements.
|
|
@joakime I just fixed the artifacts used by Eclipse. If you prefer I can submit a bigger patch to make all such cases optional. WDYT? |
|
I don't know TBH. |
| @@ -50,7 +50,7 @@ | |||
| <extensions>true</extensions> | |||
| <configuration> | |||
| <instructions> | |||
| <Require-Capability>osgi.serviceloader; filter:="(osgi.serviceloader=org.eclipse.jetty.util.security.CredentialProvider)";resolution:=optional;cardinality:=multiple, osgi.extender; filter:="(osgi.extender=osgi.serviceloader.processor)"</Require-Capability> | |||
| <Require-Capability>osgi.serviceloader; filter:="(osgi.serviceloader=org.eclipse.jetty.util.security.CredentialProvider)";resolution:=optional;cardinality:=multiple, osgi.extender; filter:="(osgi.extender=osgi.serviceloader.processor)";resolution:=optional</Require-Capability> | |||
| </instructions> | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess if we've made the osgi.serviceloader optional, we should indeed also make the osgi.extender also optional for consistency. So of itself these 2 changes are fine.
However, there are many jetty modules where we haven't made the osgi.serviceloader optional. One of them is the jetty-http module, which is quite core, so just these 2 changes probably aren't going to be enough for Eclipse to work. If we were to make the osgi.serviceloader optional for jetty-http module, I'm not sure that jetty would work properly. Other modules, such as jetty-annotations, are dependent on ServiceLoader to work, so strictly speaking making them optional isn't really correct. The servlet spec makes us use the ServiceLoader for annotations, but beyond that, java is increasingly using the ServiceLoader mechanism - I think Eclipse needs to address this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The requirement on osgi.serviceloader is already optional in the jetty-http module, AFAICT:
https://github.com/eclipse/jetty.project/blob/jetty-9.4.x/jetty-http/pom.xml#L40
I don't believe Eclipse uses jetty-annotations, so I did not check that module.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mbooth101 yes, you're right it does look like it has been made optional in jetty-http - my mistake (note: we need to remove the extraneous commented out duplicate Require-Capability line).
However, my broader point still holds: while I have no problem trying to help out Eclipse, the difficulty is that making our bundles osgi.serviceloader resolution "optional" will break other users: in general, wherever jetty uses the ServiceLoader, we are relying on it for correct functioning. Therefore, the osgi headers should reflect that we do need osgi.serviceloader to be present in order for the bundle to function correctly - so in fact we should remove all of the "optional" clauses from our bundles!
Is there not some other solution that you can suggest for your use-case of jetty in osgi?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
However, at Eclipse we only see breakage when these requirements are made not-optional.
We have a fairly basic use-case that shows jetty is perfectly useful within an OSGi runtime without the need for osgi.serviceloader. We are currently also successfully shipping jetty 9.4.9 with this patch applied in Fedora Linux, so from my point of view, that shows that this requirement really is optional :-) And upstream at Eclipse, we want to avoid growing dependencies on bundles whose presence are unnecessary for the correct operation of Eclipse.
For more advanced use-cases, users would need to add a bundle that provides the osgi.serviceloader capability whether the requirement is optional or not but I don't think jetty should disregard the simple (and IMO valid) use-cases by making osgi.serviceloader mandatory.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, here's what we'll do: we will examine the use of ServiceLoader in each jetty module. Where the use of the ServiceLoader is mandatory for functionality, the osgi.serviceloader will also be made mandatory. For those modules where ServiceLoader can be used optionally to provide functionality, then we will make the osgi.serviceloader correspondingly optional. That way, Eclipse's current usecase should be covered without breaking any other users. Of course, if Eclipse ventures further in the future in their use of jetty then you may find you'll need to install an osgi.serviceloader :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Of course, if Eclipse ventures further in the future in their use of jetty then you may find you'll need to install an osgi.serviceloader :)
Noted, thanks :-)
This allows Eclipse to continue to make use of jetty-* bundles
without requiring the presence of Apache Aries.
This fixes a problem introduced by the resolution to #2164 and
the same solution is used as in 10cdf16
See also the discussion at Eclipse:
https://bugs.eclipse.org/bugs/show_bug.cgi?id=532294
Signed-off-by: Mat Booth mat.booth@redhat.com