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

Issue #5830 Remove com.sun.nio.file import. #5838

Merged
merged 2 commits into from Jan 12, 2021

Conversation

janbartel
Copy link
Contributor

Closes #5830

The bnd tool which generates the osgi manifest for jetty-util is including an Import-Package for com.sun.nio.file, even though this class is only speculatively loaded (ie it is not mandatory for it to be present).

By using the 3-arg Class.forName(String name, boolean initialize, ClassLoader loader) method call, we can get bnd to ignore this package.

BTW: the speculative loading of the com.sun.nio.file.SensitivityWatchEventModifier will probably be removed in jetty-10 onwards, as will any references in jetty-util module-info.java.

This is acheived by using the 3-arg Class.forName() method call.
The bnd tool which generates the osgi manifests doesn't seem to
include Import-Package statements for the 3-arg method call, but
does for the 1-arg method call. The speculative loading of the
com.sun.nio.file.SensitivityWatchEventModifier will probably be
removed in jetty-10 onwards.

Signed-off-by: Jan Bartel <janb@webtide.com>
@janbartel janbartel linked an issue Dec 22, 2020 that may be closed by this pull request
@janbartel janbartel added this to In progress in Jetty 9.4.36 via automation Dec 22, 2020
Copy link
Contributor

@joakime joakime left a comment

Choose a reason for hiding this comment

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

👍 - this is a simple enough fix.

Jetty 9.4.36 automation moved this from In progress to Reviewer approved Dec 22, 2020
@olamy
Copy link
Member

olamy commented Dec 22, 2020

note it's a nitpicky comment :)
what will we happen if in the future bnd tools do not ignore anymore the 3-arg method.
It would be better to use the bdn option -noclassforname: true https://bnd.bndtools.org/instructions/noclassforname.html
But not sure how to pass the option...

@janbartel
Copy link
Contributor Author

@olamy I thought of that, but decided not to use it because a) every time I've put in special bnd voodoo it comes back later to bite us on the ass in unexpected ways b) if sometime in the future we added a class that actually wanted the class.forname discovered then by then we would probably have forgotten all about the special flag we added and waste a lot of time diagnosing the problem C) the class.forname in question is going to be deleted on jetty 10 anyway so no point polluting the bnd args for it.

Signed-off-by: Jan Bartel <janb@webtide.com>
@janbartel
Copy link
Contributor Author

@joakime after our offline conversation, I've removed the code that was doing the reflection to use the native classes: can you re-review to make sure I've removed it correctly?

Also, is there any reason to keep the @disabled annotation on the PathWatcherTest now the native code has gone??

Copy link
Contributor

@joakime joakime left a comment

Choose a reason for hiding this comment

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

LGTM

@janbartel janbartel merged commit 1a7c3de into jetty-9.4.x Jan 12, 2021
Jetty 9.4.36 automation moved this from Reviewer approved to Done Jan 12, 2021
@janbartel janbartel deleted the jetty-9.4.x-5830-remove-com-sun-import branch January 12, 2021 10:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Jetty-util contains wrong Import-Package
3 participants