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 org.apache.felix:org.apache.felix.http.servlet-api:1.2.0 #239

Closed

Conversation

HannesWell
Copy link
Member

@HannesWell HannesWell changed the title Add org.apache.felix:org.apache.felix.http.servlet-api:1.2.0 Add org.apache.felix:org.apache.felix.http.servlet-api:2.0.0 Jun 18, 2022
@HannesWell
Copy link
Member Author

This is currently blocked by eclipse-equinox/p2#64.

Furthermore we should sort out which servlet-api bundles we want to use.
org.apache.felix.http.servlet-api:2.0.0 not only includes the 'old' javax.servlet api but also its successor the new jakarta.servlet-api version 5 (there is already version 6 available).
Correct me if I'm wrong but it looks like felix.http.servlet-api is intended to provide all thesese servlet APIs from one bundle, isn't it. So there should be sufficient to include only felix.http.servlet-api.

@mknauer
Copy link

mknauer commented Nov 17, 2022

For my better understanding, what would be the use case to be solved here by org.apache.felix.http.servlet-api?

It may be appealing to have such a bundle that provides the API in both namespaces, but in the end someone has to make the decision which namespace to use, and here I typically prefer a dedicated decision for exactly one, i.e. something like jakarta.servlet:jakarta.servlet-api:4.0.3 or jakarta.servlet:jakarta.servlet-api:5.0.0.

@HannesWell
Copy link
Member Author

For my better understanding, what would be the use case to be solved here by org.apache.felix.http.servlet-api?

The main reason to choose felix.http.servlet-api was that it contains the JavaServlet osgi.contract capability defined in its Manifest:

Provide-Capability: osgi.contract;osgi.contract=JavaServlet;version:List<Version>="2.6,3.0,3.1,4.0"

@tjwatson can you tell if that was made available in other bundles in the meantime as well?

@tjwatson
Copy link
Contributor

Furthermore we should sort out which servlet-api bundles we want to use.

We should not use the 2.x versions of org.apache.felix.http.servlet-api. We should only use the 1.x versions that only contain the javax.servlet packages. Not until someone takes up the effort to migrate to the jakarta namespace should we pull in the jakarta packages IMO.

@HannesWell
Copy link
Member Author

Furthermore we should sort out which servlet-api bundles we want to use.

We should not use the 2.x versions of org.apache.felix.http.servlet-api. We should only use the 1.x versions that only contain the javax.servlet packages. Not until someone takes up the effort to migrate to the jakarta namespace should we pull in the jakarta packages IMO.

Makes sense.
At the moment it looks like the jakarta.servlet-api:4.0.4 (which constains the 'old' javax packages) is referenced in same features. But since they are (almost) the same respectively the felix-bundle is a super-set, we should not include both.

Btw. the felix-servlet does not include the javax.servlet.http.NoBodyOutputStream/NoBodyResponse compared to the jakarata-servlet 4.0.4.

In general couldn't we ask the Jakarta maintainers to adjust the Manifest and also include the contracts?
They already have a OSGi compliant Manifest, so I would not expect great resistance about such change. At least when we have migrate to Jakarta-servlet we can use the 'official' jars.

@HannesWell
Copy link
Member Author

/request-license-review

@github-actions
Copy link
Contributor

github-actions bot commented Dec 9, 2022

/request-license-review

License review requests:

After all reviews have concluded, re-run the license-vetting check from the Github Actions web-interface to update its status.

Workflow run (with attached summary files):
https://github.com/eclipse-platform/eclipse.platform.releng.aggregator/actions/runs/3653507657

@akurtakov
Copy link
Member

In general couldn't we ask the Jakarta maintainers to adjust the Manifest and also include the contracts?
They already have a OSGi compliant Manifest, so I would not expect great resistance about such change. At least when we have migrate to Jakarta-servlet we can use the 'official' jars.

This sounds like best path forward.

@tjwatson
Copy link
Contributor

tjwatson commented Dec 9, 2022

In general couldn't we ask the Jakarta maintainers to adjust the Manifest and also include the contracts?
They already have a OSGi compliant Manifest, so I would not expect great resistance about such change. At least when we have migrate to Jakarta-servlet we can use the 'official' jars.

This sounds like best path forward.

@rotty3000 Didn't you attempt to do something like this in the past with the Jakarta specification artifacts?

@HannesWell
Copy link
Member Author

Wasn't it also one reason to use the felix-sevlet-api bundles that it exports the servlet packages in multiple versions?

Tom you explained that a while ago already in eclipse-equinox/equinox.framework#44 (comment).

When comparing the Manifests jakarta.servlet:jakarta.servlet-api:4.0.4 exports

Export-Package:
 javax.servlet.annotation;uses:="javax.servlet";version="4.0.0",
 javax.servlet.descriptor";version="4.0.0",
 javax.servlet.descriptor;version="4.0.0",
 javax.servlet.http;uses:="javax.servlet";version="4.0.0",
 javax.servlet;uses:="javax.servlet.annotation

and for org.apache.felix:org.apache.felix.http.servlet-api:1.2.0 the Manifest contains:

Export-Package:
 javax.servlet.annotation;version="2.6";uses:="javax.servlet",
 javax.servlet.annotation;version="3.0",
 javax.servlet.annotation;version="3.1",
 javax.servlet.annotation;version="4.0",
 javax.servlet.descriptor",
 javax.servlet.descriptor;version="2.6",
 javax.servlet.descriptor;version="3.0",
 javax.servlet.descriptor;version="3.1",
 javax.servlet.descriptor;version="4.0",
 javax.servlet.http;version="2.6";uses:="javax.servlet",
 javax.servlet.http;version="3.0",
 javax.servlet.http;version="3.1",
 javax.servlet.http;version="4.0",
 javax.servlet;version="2.6";uses:="javax.servlet.annotation,
 javax.servlet;version="3.0",
 javax.servlet;version="3.1",
 javax.servlet;version="4.0"
Provide-Capability: osgi.contract;osgi.contract=JavaServlet;version:List<Version>="2.6,3.0,3.1,4.0";uses:="javax.servlet,javax.servlet.http,javax.servlet.descriptor,javax.servlet.annotation"

So if we adjust the jakarta artifact to include the contracts, should we do that as well?
Or couldn't we adjust our Package-Imports to work with 4.0?

@HannesWell
Copy link
Member Author

But the good news is that the build succeeds, so eclipse-equinox/p2#64 worked. :)

@HannesWell HannesWell changed the title Add org.apache.felix:org.apache.felix.http.servlet-api:2.0.0 Add org.apache.felix:org.apache.felix.http.servlet-api:1.2.0 Dec 16, 2022
@HannesWell
Copy link
Member Author

/request-license-review

@github-actions
Copy link
Contributor

/request-license-review

License review requests:

After all reviews have concluded, re-run the license-vetting check from the Github Actions web-interface to update its status.

Workflow run (with attached summary files):
https://github.com/eclipse-platform/eclipse.platform.releng.aggregator/actions/runs/3712190962

@HannesWell
Copy link
Member Author

IIRC in order to migrate from the jakarta.servlet-api bundle to org.apache.felix.http.servlet-api, the former has to be replaced in features by the latter.
So if we want to do that, we first have to keep both, then migrate and the remove the jakrata bundle. Or we ask the jakarat maintainers to include the same metadata in another (back-ported) 4.x.0 release.

@HannesWell HannesWell marked this pull request as ready for review January 18, 2023 20:26
@HannesWell
Copy link
Member Author

@tjwatson, @akurtakov how should we proceed here?

@akurtakov
Copy link
Member

What is the status of this one? Overall I prioritize using artifacts from groupIds of the actual developers not "repackaging".

@akurtakov
Copy link
Member

I'm about to abandon this one unless someone says this is still relevant and being worked on in the next week.

@HannesWell
Copy link
Member Author

HannesWell commented Apr 26, 2023

What is the status of this one? Overall I prioritize using artifacts from groupIds of the actual developers not "repackaging".

The reason to use the one repacked by Felix is

For my better understanding, what would be the use case to be solved here by org.apache.felix.http.servlet-api?

The main reason to choose felix.http.servlet-api was that it contains the JavaServlet osgi.contract capability defined in its Manifest:

Provide-Capability: osgi.contract;osgi.contract=JavaServlet;version:List<Version>="2.6,3.0,3.1,4.0"

@tjwatson can you tell if that was made available in other bundles in the meantime as well?

It was suggested to add the missing metadata to the original artifact, but there was no response yet why this was not yet done or should be re-attempted.

In general couldn't we ask the Jakarta maintainers to adjust the Manifest and also include the contracts?
They already have a OSGi compliant Manifest, so I would not expect great resistance about such change. At least when we have migrate to Jakarta-servlet we can use the 'official' jars.

This sounds like best path forward.

@rotty3000 Didn't you attempt to do something like this in the past with the Jakarta specification artifacts?

In general I would like to complete this so we can deprecate and eventually remove o.e.osgi.serivces completely.
@tjwatson can you please advice on how we should proceed best?

@tjwatson
Copy link
Contributor

I would not anticipate Jakarta releasing another version of jakarta.servlet:jakarta.servlet-api for the 4.0.x spec release to include additional OSGi meta-data. You could try opening an issue to see about getting traction on that. But I think it is a big ask for them to release an update to a spec JAR from a couple of spec versions back.

This is a sort of hack, but to use the existing jakarta.servlet:jakarta.servlet-api artifact could we also deploy a fragment to jakarta.servlet:jakarta.servlet-api that adds the osgi.contract capability we need? If I recall correctly that was the missing piece to allow the osgi JARs to work for providing the http service APIs.

@HannesWell
Copy link
Member Author

I would not anticipate Jakarta releasing another version of jakarta.servlet:jakarta.servlet-api for the 4.0.x spec release to include additional OSGi meta-data. You could try opening an issue to see about getting traction on that. But I think it is a big ask for them to release an update to a spec JAR from a couple of spec versions back.

Yes, there probably won't be much enthusiasms about such proposal. :/
Nevertheless I think for Jakarta 6 it is worth a try.

This is a sort of hack, but to use the existing jakarta.servlet:jakarta.servlet-api artifact could we also deploy a fragment to jakarta.servlet:jakarta.servlet-api that adds the osgi.contract capability we need? If I recall correctly that was the missing piece to allow the osgi JARs to work for providing the http service APIs.

Yes I thought about that as well. Sounds like a possible solution.
Should we add such fragment to equinox next to o.e.osgi.services?
In general I wonder why only this capability is important if nothing in the code corresponds to that?

I also wonder how long will Equinox will have to support javax.servlet?
I'm asking because, in general an alternative would be to try to include the mentioned capability in the jakarta-servlet 6 spec bundle, migrate the eclipse platform to jakarta-servlet and just remove the javax-code. If we can do that in an oversee-able time-frame, it might not be worth the mentioned fragment as an intermediate workaround.

@tjwatson
Copy link
Contributor

I also wonder how long will Equinox will have to support javax.servlet?

That is a complicate question.

  1. there is no http service spec that will be updated to the jakarta namespace, only the servlet whiteboard spec is updated to jakarta. So we have to deprecate and remove the implementation of the http service implementation
  2. the javax.servlet API is in other APIs of ours like the http.registry. So we have to deprecate some of that and replace parts that use jakarta namespace
  3. all current users of javax.servlet have to migrate their code to jakarta also

These are not things we can easily do. And that is just from a deprecation breaking POV. I've not mentioned the actual work of changing our own codebase to use jakarta and who will be dedicated to doing that work.

@HannesWell
Copy link
Member Author

HannesWell commented Nov 18, 2023

With eclipse-equinox/equinox#403 this now seems obsolete.
Lets discuss there how to resolve the problems associated with the org.osgi.service.http* bundles.

@@ -216,12 +216,18 @@
<version>3.0.3</version>
<type>jar</type>
</dependency>
<dependency>
<dependency><!-- Duplicated by org.apache.felix.http.servlet-api below -->
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we simply delete this then?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I had the same idea, since jakarta/javax.servlet-api is not contained in the eclipse-sdk update-site (altough jakarta.servlet-api_4.0.0 is in the SimRel repo) this should not cause problems.
I just wanted to rebase this PR first and resolve the conflicts as it is, which took 2 minutes longer. 😄

Copy link
Contributor

@laeubi laeubi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like a good way forward!

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

laeubi commented Dec 16, 2023

I assume this will already solve the maven / ide warnings we see and thing should be merged as a first step, if we later decide to not need it anymore it can be removed / replaced again.

@laeubi
Copy link
Contributor

laeubi commented Dec 16, 2023

@HannesWell I now created one here that only adds the dependency to the target and leave everything else unchanged:

@laeubi laeubi closed this Dec 16, 2023
@HannesWell HannesWell deleted the felixServletAPI branch December 16, 2023 12:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants