-
Notifications
You must be signed in to change notification settings - Fork 65
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
Replace embedded service.http sources in org.eclipse.osgi.services by original bundles and deprecate o.e.osgi.services #403
Conversation
;version:List<Version>="2.6,3.0,3.1,4.0" | ||
;uses:="javax.servlet,javax.servlet.http,javax.servlet.descriptor,javax.servlet.annotation" | ||
Export-Package: | ||
javax.servlet;version="2.999", |
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 real provider has
javax.servlet;version="2.999", | |
javax.servlet;version="2.999";uses:="javax.servlet.annotation,javax.servlet.descriptor", |
But the two used packages do not exist in javax.servlet 2.2.
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.
Applied this now.
Additionally used packages should not harm consumers of the servlet version 2 package since they can't use it.
Automatic-Module-Name: org.eclipse.osgi.service.http.servlet.contract | ||
Bundle-RequiredExecutionEnvironment: JavaSE-1.8 | ||
Provide-Capability: osgi.contract;osgi.contract=JavaServlet | ||
;version:List<Version>="2.6,3.0,3.1,4.0" |
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.
According to https://docs.osgi.org/reference/portable-java-contracts.html the osgi.contract=JavaServlet
is specified with version 2.5
, 3.0
, 3.1
and 4.0
.
But org.apache.felix:org.apache.felix.http.servlet-api:1.2.0
defines 2.6 instead of 2.5.
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.
Changed the list to have the official version. Since o.o.service.http.whiteboard only uses 3.1 all the others doesn't matter for our use-case anyway.
jakarta.servlet-api;bundle-version="[4.0.0,5.0.0)", | ||
javax.servlet-api;bundle-version="[3.0.0,5.0.0)", | ||
org.eclipse.jetty.servlet-api;bundle-version="[4.0.0,5.0.0)" |
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.
Should we support only javax/jakarta.servlet-api
or also jetty-servlet-api?
7e24d3e
to
281033a
Compare
In general to discuss the path forward we should also consider @tjwatson remarks from eclipse-platform/eclipse.platform.releng.aggregator#239 (comment):
|
281033a
to
bb50da9
Compare
The build has problems to resolve the javax.servlet package exported by the fragment, but also in the IDE workspace (at least after a restart) the import Maybe it would just be simpler to use Another alternative @laeubi suggested (in a private chat), to leverage the missing metadata of javax.servlet:servlet-api:2.5 and just generate them suitably using the new Orbit SimRel infrastructure created by Ed. The instructions could be prototyped in the sdk-target. |
@HannesWell thanks for the efforts and detailed analysis. I think it makes no sense to use an "official" artifact if when we need extra efforts to use it like additional fragments and alike, this defeats the purpose of it. For me the proposal made here to reuse the felix bundled version: seem much more appropriate because of the following reasons:
WDYT @akurtakov @tjwatson ? |
from Maven-Central and remove jakarta.servlet-api in favor of that. Prerequisite of eclipse-equinox/equinox#403 and therefore part of eclipse-equinox/equinox#18
Yes, with all the issues that arose on the way (the CI and WS build is still failing/has issues a.t.m.), I'm more and more inclined towards that solution as well. Especially, as you said, the two specs have probably not a big future. |
from Maven-Central and remove jakarta.servlet-api in favor of that. Prerequisite of eclipse-equinox/equinox#403 and therefore part of eclipse-equinox/equinox#18
from Maven-Central and remove jakarta.servlet-api in favor of that. Prerequisite of eclipse-equinox/equinox#403 and therefore part of eclipse-equinox/equinox#18
@tjwatson could you please share your assessment on how we should proceed here? |
I think migrating to the felix http/whiteboard implementation sounds interesting. But it has a myriad of issues with respect to extensions our implementation has on top of the spec that we either have to deprecate and eventually remove (before we can use the felix imple) or we have to figure out how to implement them on top of the felix implemetnation. I think we should first and foremost focus on how to replace the service.http sources from the osgi.services bundle. Perhaps we just add the capability |
I just wanted to note that |
We do provide an implementation of |
I don't want to and cannot judge about replacing the implementation, but this PR is only about replacing the embedded http/whiteboard API sources with the OSGi bundles from bundles from Maven-Central. But since the original http/whiteboard OSGi bundles have some difficult dependencies (as desribed in the initial comment of this PR), the question is how to provide them. This PR initially suggested to add a fragment to equinox that adds the required capabilities to one of the 'original' |
Fragment-Host: | ||
jakarta.servlet-api;bundle-version="[4.0.0,5.0.0)", | ||
javax.servlet-api;bundle-version="[3.0.0,5.0.0)", | ||
org.eclipse.jetty.servlet-api;bundle-version="[4.0.0,5.0.0)" |
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.
A fragment is only allowed to specify a single host.
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, somehow I assume this is possible. At least the runtime didn't complain, but looking in the spec it does not look like this is permitted.
But ok, then we would have to select one of these three, in case we continue with adding this fragment. In that case it is probably the most future-proof to choose jakarta.servlet-api
.
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.
@tjwatson are you sure? I also though this but remember I have seen such "multi-host" already somewhere else (sadly I can't remember where/when) and was surprised there as well...
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.
@tjwatson are you sure? I also though this but remember I have seen such "multi-host" already somewhere else (sadly I can't remember where/when) and was surprised there as well...
Yes, I'm sure the spec Fragment-Host
syntax only allows for a single host symbolic name to be defined:
Fragment-Host ::= bundle-description
bundle-description ::= symbolic-name
( ';' parameter )* // See [1.3.2](https://docs.osgi.org/specification/osgi.core/8.0.0/framework.introduction.html#framework.general.syntax)
It is true a fragment may be attached to multiple host bundles, but that is only when there are multiple versions of a bundle are installed (with the same symbolic name):
Fragments can be attached to multiple hosts with the same symbolic name but different versions. If multiple fragments with the same symbolic name match the same host, then the Framework must only select one fragment, this must be the fragment with the highest version.
Ok, somehow I assume this is possible. At least the runtime didn't complain, but looking in the spec it does not look like this is permitted.
This is because Equinox just uses the first one listed:
Line 501 in 0582356
ManifestElement fragmentHost = fragmentHosts[0]; |
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.
@tjwatson is osgi.support.multipleHosts=true
gone already?
https://www.eclipse.org/forums/index.php?t=msg&th=276829&goto=1052923&#msg_1052923
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.
Regardless of my answer (which I don't recall off the top of my head), that was only about resolve against multiple host versions (with same BSN).
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 see.. then I maybe has wrong memories here...
How do we ensure what bundle is providing So if we move to use the |
from Maven-Central and remove jakarta.servlet-api and jetty-servlet-api in favor of that. Prerequisite of eclipse-equinox/equinox#403 and therefore part of eclipse-equinox/equinox#18
That's a good question, I cannot fully answer. For the record, the jetty servlet
I cannot answer that with confidence either.
which are only present in jetty's servlet api jar. But just adding the contract is not sufficient since |
I honestly don't know. What I have always cared for is have latest jetty and have help webapp working - whatever satisfies it is good enough. |
As @akurtakov suggests, I think we can only test that help works. Generally I test that by launching runtime workspace and seeing in Help -> Contents works, checking which bundles are installed... |
This implies we need a single bundle exporting two versions of the same package. While possible, it is not truthful. I am not sure I agree that we should move in that direction. To that end, I really think we should repackage the deprecated |
Actually, we should import with no version and do the contract dependency like the OSGi provided |
I also want to note that it seems the
what is quite annoying it would be great to get a solution for this as this currently hinder us from using the Quality gate as this is reported as new warnings on maven problem parser. |
Should I just place the API sources into the Implementation bundle and export+import them or create a separate bundle? |
Thanks Ed for the analysis The requirements on the exported packages will automatically vanish respectively will be redirected, once this PR is completed and the http packages are not contained in this Plugin/Bundle anymore. Correspondingly the inclusion in features should just be removed. I would only keep it in at least one equinox feature so that it is still included in the sdk repo and in the TP of consumers that still use it. I'll reach out to the affected projects ones this is completed.
I assumed that we have the usual two years deprecation period? |
@tjwatson could you please share your assessment of the approach to use a fragment for javax/jakarta.servlet-api, that is currently implemented by this PR? |
I'll refer back to my comment #403 (comment)
I still stand by this approach and I will refer to my previous comment #403 (comment)
I've not seen any arguments against this approach that involves no hacking, n repackaging, and no confusing multi-version exports. |
2c0a43a
to
01e4077
Compare
OK, I just wanted to make sure that this suggestion is not lost because I had the impression that it was confused with a fragment for I have update this PR to move the classes into a new |
How does that work with the projects imported into the IDE? Does PDE and M2E handle this and get everything compiled correctly in the workspace? |
Yes. Of course m2e has to be installed. But IIRC it is already part of the Oomph development setup for the SDK. |
Then I am fine with this approach, but if there are any issues we should simply place the |
I'm such a non-fan to not checking in code, but that's just my opinion. Please be 100% sure that the Oomph setup actually works out-of-the box from a freshly cloned clone. |
Not just yours, that is mine also. It always seems more simple to just have the source checked into the source repo vs. trying to pull it out of some pre-existing artifact. |
One advantage I can think of is that it would stop others from opening PRs trying to "correct" the formatting of the source one way or another :) |
Part of eclipse-equinox#18 Move the sources from org.osgi.service.http to the new org.eclipse.equinox.http.service.api Plug-in project, because o.o.service.http imports javax.servlet packages in versions that are not available as OSGi bundles.
01e4077
to
258a200
Compare
Great. I'm glad we can finally complete this. Thank you all for the discussion and your patience.
I verified that and for me it worked well to import this and fetch the sources just with the Oomph setup. I'm never 100% sure about anything but I'm very confident it works if m2e is installed. :)
That's right. And it is also simpler to upgrade to a new version because only the version number in the pom (and the manifest in Manifest-First plugins) needs to be updated. For this abandoned spec that's not so interesting, but for example for the embedded sources in |
With eclipse-equinox/equinox#403 the sources of org.osgi.service.http are provided in a re-packed bundle from Equinox, that also provides the OSGi capabilities which where temporarily provided by felix.http.servlet-api
With eclipse-equinox/equinox#403 the sources of org.osgi.service.http are provided in a re-packed bundle from Equinox, that also provides the OSGi capabilities which were temporarily provided by felix.http.servlet-api.
I would rather not. There are cases where we modify the internals of the |
With eclipse-equinox/equinox#403 the sources of org.osgi.service.http are provided in a re-packed bundle from Equinox, that also provides the OSGi capabilities which were temporarily provided by felix.http.servlet-api.
This replaces the remaining embedded
org.osgi.service.http*
packages inorg.eclipse.osgi.services
by the original bundles from Maven-Central and then marksorg.eclipse.osgi.services
as deprecated.Fixes #18.
As suggested in eclipse-platform/eclipse.platform.releng.aggregator#239 (comment):
This adds a fragment
org.eclipse.osgi.service.http.servlet.contract
to supply the capability:osgi.contract;osgi.contract=JavaServlet
, required byorg.osgi.service.http.whiteboard
to the knownjavax.servlet
providing OSGi bundles from Maven-Central:javax.servlet:javax.servlet-api
jakarta.servlet:jakarta.servlet-api:[4.0.2-4.0.4]
org.eclipse.jetty.toolchain/jetty-servlet-api
javax.servlet:servlet-api
does not provide OSGi metadata.One non-optimal thing with the different
javax.servlet
provides (except the ones from jetty) is that in the version 4 stream all artifacts have version 4.0 regardless of the BSN or their Maven-version change.However after a quick/coarse check it looks like they only differences are white-space or legal text changes.
Background about OSGi contracts: https://docs.osgi.org/reference/portable-java-contracts.html
Another greater problem is that
org.osgi.service.http
has in its Manifest:In order to fix that I also added corresponding exports of
javax.servlet(.http)
in version2.999
toorg.eclipse.osgi.service.http.servlet.contract
that are served by the host. I compared all of thejavax.servlet
providing artifacts mentioned above and all content injavax.servlet
orjavax.servlet.http
contained in javax.servlet:servlet-api:2.2 is also provided by the other providers.