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

Is org.apache.jasper.glassfish actually required? #1251

Closed
merks opened this issue Aug 13, 2023 · 16 comments
Closed

Is org.apache.jasper.glassfish actually required? #1251

merks opened this issue Aug 13, 2023 · 16 comments

Comments

@merks
Copy link
Contributor

merks commented Aug 13, 2023

I've been trying to eliminate bundles in the target platform from old cvs-based orbit builds.

Looking at SimRel, the only strict requirement on org.apache.jasper.glassfish is the inclusion in the help feature:

image

Looking at what the bundle provides we see two dependencies on the exported packages:

image

But if we look at that package requirement

image

it's also satisfied by org.glassfish.web.javax.servlet.jsp /2.3.4

And that's already in the target platform:

			  <dependency>
				  <groupId>org.glassfish.web</groupId>
				  <artifactId>javax.servlet.jsp</artifactId>
				  <version>2.3.4</version>
				  <type>jar</type>
			  </dependency>

So I think we can remove this now and remove the inclusion in the feature

      <!-- In process of being replaced by org.glassfish.web.javax.servlet.jsp from Central -->
      <unit id="org.apache.jasper.glassfish" version="2.2.2.v201501141630"/>
      <unit id="org.apache.jasper.glassfish.source" version="2.2.2.v201501141630"/>

@akurtakov

Or is there something I've overlooked?

@akurtakov
Copy link
Member

If the help system still works without org.apache.jasper.glassfish (aka jsp files are getting compiled and render after that) you can remove it. IIRC @mickaelistria tried to replace it but it was not working.

@merks
Copy link
Contributor Author

merks commented Aug 15, 2023

The short answer is that currently it is required.

The long answer is that with some small effort it can be replaced the following:

https://repo1.maven.org/maven2/org/eclipse/jetty/apache-jsp/10.0.15/apache-jsp-10.0.15.pom

But firstly we have to make sure the requirements of that are satisfied and that's not the case with the current contents of the target platform We need these:

			  <dependency>
				  <groupId>org.eclipse.jetty.toolchain</groupId>
				  <artifactId>jetty-servlet-api</artifactId>
				  <version>4.0.6</version>
				  <type>jar</type>
			  </dependency>
			  <dependency>
				  <groupId>org.mortbay.jasper</groupId>
				  <artifactId>apache-jsp</artifactId>
				  <version>9.0.52</version>
				  <type>jar</type>
			  </dependency>

We also have to be wary of all the places the name bundle symbolic name is hard coded:

image

To get it working with the newer dependencies, we have to allow newer versions and com.sun.el is needed:

image

The specialized class loader needs to be able to load com.sun.el.EpxressionFactory:

image

And finally the servlet needs to initialize Jasper

image

The older include can be removed from here:

/org.eclipse.help-feature/feature.xml

And then the help system works:

image

@akurtakov

I'm not sure if it's appropriate to make these changes immediately. Perhaps we'll have to wait for 2023-12 m1?

@akurtakov
Copy link
Member

Help systems jsp support is extremely fragile so please keep it for M1. Also I do believe that an effort to move to Jetty 12 (https://www.eclipse.org/lists/jetty-announce/msg00177.html) with EE 8 should be done prior or toghether with that as that would open the door for switching the EE implementation without changing major Jetty version.

@merks
Copy link
Contributor Author

merks commented Aug 15, 2023

I see!Not sure version of the dependency is specified when no version is specified. The largest I guess:

https://repo1.maven.org/maven2/org/eclipse/jetty/ee10/jetty-ee10-apache-jsp/12.0.0/jetty-ee10-apache-jsp-12.0.0.pom

Oh well, stuff to revisit in September...

Thanks for sharing your insights!

@mickaelistria
Copy link
Contributor

I tried to remove it a while ago in #774 , but gave up. Not sure whether the reasons still apply with newer versions of other deps.

@merks
Copy link
Contributor Author

merks commented Sep 14, 2023

@akurtakov

I think I can get JSP working with newer dependencies as described here:

#1251 (comment)

But I don't have the capacity to try to migrate everything to Jetty 12.x.

So should I do the smaller step for now, or should I just leave the whole thing for someone to do the Jetty 12.x work eventually?

@akurtakov
Copy link
Member

Whatever can be done now is better than waiting for big bang changes IMHO.

merks added a commit to merks/eclipse.platform.releng.aggregator that referenced this issue Sep 14, 2023
merks added a commit that referenced this issue Sep 14, 2023
merks added a commit to merks/eclipse.platform.releng.aggregator that referenced this issue Sep 14, 2023
Replace some with org.mortbay.jasper.apache-jsp.

eclipse-platform#1251
@merks
Copy link
Contributor Author

merks commented Sep 15, 2023

@iloveeclipse @akurtakov

I see the nightly I-Build fails like this:

00:50:31  [ERROR] Cannot resolve project dependencies:
00:50:31  [ERROR]   Software being installed: org.eclipse.equinox.sdk.product 4.30.0.qualifier
00:50:31  [ERROR]   Missing requirement: org.mortbay.jasper.apache-jsp 9.0.52 requires 'java.package; org.eclipse.jdt.core.compiler 0.0.0' but it could not be found
00:50:31  [ERROR]   Cannot satisfy dependency: org.eclipse.equinox.jsp.jasper 1.1.800.v20230914-1441 depends on: java.package; org.apache.jasper.servlet [9.0.0,10.0.0)
00:50:31  [ERROR]   Cannot satisfy dependency: org.eclipse.equinox.sdk.feature.group 3.23.1000.v20230914-1441 depends on: org.eclipse.equinox.p2.iu; org.eclipse.equinox.jsp.jasper [1.1.800.v20230914-1441,1.1.800.v20230914-1441]
00:50:31  [ERROR]   Cannot satisfy dependency: org.eclipse.equinox.sdk.product 4.30.0.qualifier depends on: org.eclipse.equinox.p2.iu; org.eclipse.equinox.sdk.feature.group 0.0.0

This suggests that org.eclipse.jdt.core.compiler.batch is needed in the org.eclipse.equinox.sdk feature:

image

One might have expected Tycho to have resolved that package import...

merks added a commit to merks/equinox that referenced this issue Sep 15, 2023
Add an include of the org.eclipse.jdt.core.compiler.batch plugin to the
org.eclipse.equinox.sdk feature.

eclipse-platform/eclipse.platform.releng.aggregator#1251
@mickaelistria
Copy link
Contributor

Aren't we discussing removal of org.eclipse.equinox.sdk.product in #1235 ?

@merks
Copy link
Contributor Author

merks commented Sep 15, 2023

That may be. But the build is broken and it appears that the Equinox SDK feature is "incomplete", at least as far as inclusion in a product is concern...

@iloveeclipse
Copy link
Member

That may be. But the build is broken and it appears that the Equinox SDK feature is "incomplete", at least as far as inclusion in a product is concern...

Fully agree. I'm going to merge eclipse-equinox/equinox#312 & start an I-Build.

iloveeclipse pushed a commit to eclipse-equinox/equinox that referenced this issue Sep 15, 2023
Add an include of the org.eclipse.jdt.core.compiler.batch plugin to the
org.eclipse.equinox.sdk feature.

eclipse-platform/eclipse.platform.releng.aggregator#1251
merks added a commit that referenced this issue Sep 15, 2023
Replace some with org.mortbay.jasper.apache-jsp.

#1251
@merks
Copy link
Contributor Author

merks commented Sep 15, 2023

@akurtakov @iloveeclipse

I created an installation using the installer based on the just-completed I-Build from today. The help system is working:

image

So I think we're in pretty reasonable shape...

Hopefully there are only test issues to iron out; or better yet, hopefully there are none of those!

@merks
Copy link
Contributor Author

merks commented Sep 15, 2023

For this build I see all the UA tests pass for this:

https://download.eclipse.org/eclipse/downloads/drops4/I20230915-0520/testresults/xml/org.eclipse.ua.tests_ep430I-unit-macM1-java17_macosx.cocoa.aarch64_17.xml

Including the org.eclipse.ua.tests.help.webapp.HelpServerInterrupt and the org.eclipse.ua.tests.help.webapp.HelpServerBinding tests, which were the ones failing for the PR build.

I'll need to key an eye on these when they are done, but it's looking promising:

https://download.eclipse.org/eclipse/downloads/drops4/I20230915-0750/testResults.php

@merks
Copy link
Contributor Author

merks commented Sep 16, 2023

The tests are fine

image

so this is done.

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

No branches or pull requests

4 participants