-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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 #4683 Fix jetty-slf4j-impl for osgi #4684
Issue #4683 Fix jetty-slf4j-impl for osgi #4684
Conversation
Signed-off-by: Jan Bartel <janb@webtide.com>
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.
It would be nicer if we could configure jetty-slf4j-impl
via a jetty-logging.properties
file instead of hardcoding levels within the source.
jetty-osgi/test-jetty-osgi/src/test/java/org/eclipse/jetty/osgi/test/TestOSGiUtil.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Jan Bartel <janb@webtide.com>
Signed-off-by: Jan Bartel <janb@webtide.com>
Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
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.
Some osgi version range questions.
I reformatted the pom to make the <Import-Package>
and <DynamicImport-Package>
easier to maintain, and we can start to see the what's changed with each commit/diff. (The felix maven-bundle-plugin strips superfluous whitespace before creating the MANIFEST.MF)
com.sun.el.util;resolution:=optional, | ||
javax.el;version="[3.0,3.1)", | ||
javax.servlet;version="[3.1,4.1)", | ||
javax.servlet.resources;version="[3.1,4.1)", |
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.
Shouldn't the start of Servlet range be 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.
For this and the other range issues you've raised we need to address this in another PR, not this one. If you're happy with the changes to enable jetty-slf4j-impl to work in osgi, please give this issue the thumbs up and open new issue to bump up osgi servlet/jsp api versions.
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.
Fair enough.
I opened issue #4736 for this.
org.apache.el.lang;version="[8.0.23,10)";resolution:=optional, | ||
org.apache.el.stream;version="[8.0.23,10)";resolution:=optional, | ||
org.apache.el.util;version="[8.0.23,10)";resolution:=optional, | ||
org.apache.el.parser;version="[8.0.23,10)";resolution:=optional, |
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.
Apache EL start of range is 9.0.29
org.apache.jasper.servlet;version="[8.0.23,10)";resolution:=optional, | ||
org.apache.jasper.tagplugins.jstl;version="[8.0.23,10)";resolution:=optional, | ||
org.apache.jasper.util;version="[8.0.23,10)";resolution:=optional, | ||
org.apache.jasper.xmlparser;version="[8.0.23,10)";resolution:=optional, |
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.
Apache Jasper start of range is 9.0.29
org.apache.taglibs.standard.tag.rt.xml;version="1.2";resolution:=optional, | ||
org.apache.taglibs.standard.tei;version="1.2";resolution:=optional, | ||
org.apache.taglibs.standard.tlv;version="1.2";resolution:=optional, | ||
org.apache.tomcat;version="[8.0.23,10)";resolution:=optional, |
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.
Apache Tomcat start of range is 9.0.29 too
<DynamicImport-Package> | ||
org.eclipse.jetty.jsp.*;version="[$(version;===;${parsedVersion.osgiVersion}),$(version;==+;${parsedVersion.osgiVersion}))", | ||
org.apache.jasper.*;version="8.0.23", | ||
org.apache.el.*;version="8.0.23" |
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.
Ditto, start of range is higher here too.
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.
Merge it
Closes #4683
The new jetty-slf4j-impl had the wrong manifest entries so it didn't work in osgi. Fixed the manifest, and took the opportunity to rationalize the logging that is done by the osgi tests: they now all share common code, and the log levels are configurable from the command line to ease debugging. As part of this work I also upgraded the bndlib version (only used for testing). I've also started a README.txt file in jetty-osgi/test-jetty-osgi to explain how the test work and how to control the logging options.