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

Remove unused dependencies to o.o.service.http and javax.servlet #443

Conversation

HannesWell
Copy link
Member

The PDE dependencies analysis says these dependencies are not used and I also could not find any textual reference to the packages removed (e.g. for an reflective class-loading).
If the build succeeds we should remove the dependencies, especially to problematic packages like org.osgi.service.http.

@merks
Copy link
Contributor

merks commented Dec 16, 2023

There is a non-zero chances that the help will stop working. But I can't seem to fetch the changes:

image

I guess the test failures are a problem too...

Copy link

github-actions bot commented Dec 16, 2023

Test Results

     24 files  ±0       24 suites  ±0   10m 57s ⏱️ -28s
2 150 tests ±0  2 105 ✔️ ±0  45 💤 ±0  0 ±0 
2 194 runs  ±0  2 149 ✔️ ±0  45 💤 ±0  0 ±0 

Results for commit 67c0af0. ± Comparison against base commit a72b8ec.

♻️ This comment has been updated with latest results.

@HannesWell
Copy link
Member Author

There is a non-zero chances that the help will stop working. But I can't seem to fetch the changes:

Did you select the right remote, i.e. the one that points to this repo and not your fork when fetching?

@HannesWell
Copy link
Member Author

I guess the test failures are a problem too...

I investigate them, luckily they are reproducible in the IDE.

@merks
Copy link
Contributor

merks commented Dec 16, 2023

The Help -> Help Contents comes up like this in a debug launch workspace:

image

Lots of logged exception.

The JSP support is rather fragile.

@HannesWell HannesWell force-pushed the remove-unused-sevlet-http-dependencies branch from a3724a5 to 2bece93 Compare December 16, 2023 07:35
@merks
Copy link
Contributor

merks commented Dec 16, 2023

Fetching again fails like this now:

image

@HannesWell
Copy link
Member Author

HannesWell commented Dec 16, 2023

The removal of the optional dependencies of org.eclipse.equinox.http.servlet to javax.servlet.descriptor was what let many test cases fail.
Lets see if this already fixes all.

Thanks Ed for the help, I try to understand the stack-traces.

@merks
Copy link
Contributor

merks commented Dec 16, 2023

FYI, the BundleProxyClassLoader is partly what makes this stuff fragile. When migrating to newer dependencies, I had to hack this class...

@HannesWell
Copy link
Member Author

@merks thanks for your help in testing the help.
Can you check if adding back the optional dependency javax.servlet.descriptor to org.eclipse.equinox.jsp.jasper fixes the problem?

@laeubi
Copy link
Member

laeubi commented Dec 16, 2023

Can you check if adding back the optional dependency javax.servlet.descriptor to org.eclipse.equinox.jsp.jasper

Obviously it is not that optional at all, can we make it a real dependency?

@HannesWell HannesWell force-pushed the remove-unused-sevlet-http-dependencies branch from 2bece93 to 0a26e9e Compare December 16, 2023 07:44
@HannesWell
Copy link
Member Author

I think I understood the stack-trace and hope/am confident that adding back javax.servlet.descriptor should fix it.

Can you check if adding back the optional dependency javax.servlet.descriptor to org.eclipse.equinox.jsp.jasper

Obviously it is not that optional at all, can we make it a real dependency?

With what has been noticed here yes, but before I just described what the Manifest said. :)
But I agree it seems to be wrong.

And make javax.servlet.descriptor a non-optional dependency.
@HannesWell HannesWell force-pushed the remove-unused-sevlet-http-dependencies branch from 0a26e9e to 67c0af0 Compare December 16, 2023 07:47
@HannesWell
Copy link
Member Author

All tests pass now and if I start an Eclipse from my workspace and open the help via Help -> Help Contents everything seems to work without errors.
@merks can you confirm this?

Can you check if adding back the optional dependency javax.servlet.descriptor to org.eclipse.equinox.jsp.jasper

Obviously it is not that optional at all, can we make it a real dependency?

Made the dependency non-optional now.

@HannesWell HannesWell requested a review from merks December 16, 2023 08:15
@merks
Copy link
Contributor

merks commented Dec 16, 2023

Yes, that works for me. 👍

@HannesWell HannesWell merged commit 9658936 into eclipse-equinox:master Dec 16, 2023
22 of 25 checks passed
@HannesWell HannesWell deleted the remove-unused-sevlet-http-dependencies branch December 16, 2023 09:12
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.

3 participants