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 #7677 Exclude findbugs jsr305 jar from plugin #7682

Merged
merged 3 commits into from
Mar 28, 2022

Conversation

janbartel
Copy link
Contributor

Related to #7677.

Exclude findbugs jsr305 jar from maven core dependency.

@janbartel janbartel self-assigned this Mar 2, 2022
@janbartel janbartel requested review from olamy and joakime March 2, 2022 10:56
@joakime
Copy link
Contributor

joakime commented Mar 2, 2022

We need an IT test for #7677 and this proposed PR.

@janbartel
Copy link
Contributor Author

@joakime I don't really think that's strictly necessary: we don't need to test the maven exclusion mechanism do we? If we were doing this via jetty server/system classes settings then maybe we should test that, but as it is, we're using a mechanism provided and tested by maven.

@olamy
Copy link
Member

olamy commented Mar 2, 2022

@joakime I don't really think that's strictly necessary: we don't need to test the maven exclusion mechanism do we? If we were doing this via jetty server/system classes settings then maybe we should test that, but as it is, we're using a mechanism provided and tested by maven.

there is something I don't understand because maven-core is already marked as provided so it should be enough already.

@joakime
Copy link
Contributor

joakime commented Mar 2, 2022

@joakime I don't really think that's strictly necessary: we don't need to test the maven exclusion mechanism do we? If we were doing this via jetty server/system classes settings then maybe we should test that, but as it is, we're using a mechanism provided and tested by maven.

I would expect an IT test that ensures that our jetty-maven-plugin configuration does not pollute the classpath of a typical webapp unduly.

I can easily see someone later adding a new dependency which introduces a transitive dependency that will break their deployments.
The point in #7677 was that our dependencies broke something fundamental like javax.annotation usage in a webapp. (what about other Jakarta EE specs? javax.xml is an obvious one that could have similar issues.)

Since our WebAppClassloader prevents replacing javax.* classes this turned out to be a big deal for that user.

@janbartel
Copy link
Contributor Author

@joakime I don't really think that's strictly necessary: we don't need to test the maven exclusion mechanism do we? If we were doing this via jetty server/system classes settings then maybe we should test that, but as it is, we're using a mechanism provided and tested by maven.

there is something I don't understand because maven-core is already marked as provided so it should be enough already.

Marking it as provided just says that the execution environment should supply it and all its dependencies doesn't it? Certainly I've checked and unless jsr305 is explicitly an <exclusion> maven puts it on the runtime classpath. So jsr305 is now an exclusion in just the same way as the copy of javax.annotations is that comes along from maven core.

@olamy
Copy link
Member

olamy commented Mar 3, 2022

@joakime I don't really think that's strictly necessary: we don't need to test the maven exclusion mechanism do we? If we were doing this via jetty server/system classes settings then maybe we should test that, but as it is, we're using a mechanism provided and tested by maven.
there is something I don't understand because maven-core is already marked as provided so it should be enough already.

Marking it as provided just says that the execution environment should supply it and all its dependencies doesn't it? Certainly I've checked and unless jsr305 is explicitly an <exclusion> maven puts it on the runtime classpath. So jsr305 is now an exclusion in just the same way as the copy of javax.annotations is that comes along from maven core.

it only means it will not be in the plugin classloader (using a forked jetty run would probably not trigger such issue).
But now I remember some issue or feature of maven plugin with transitive dependencies of a provided artifact will still be in the plugin classrealm.... (I'm looking after a issue link)
if we go this way we should exclude a lot more such guava, guice, plexus-* etc....
When we start jetty:run per default as embedded it means we using the maven plugin classloader so it comes with the maven plugin clasloader which include a lot
A better fix could be using our own classloader and list of artifacts we want to include in.

olamy
olamy previously approved these changes Mar 3, 2022
Copy link
Member

@olamy olamy left a comment

Choose a reason for hiding this comment

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

not something perfect as we may exclude a lot more.
but it fix the OP issue and btw maven core will not change the behaviour soon :) (would break so many plugins..)

started a branch jetty-10.0.x-limit-maven-plugin-classloader but some issues with ServiceLoader

@janbartel
Copy link
Contributor Author

@olamy we have historically excluded maven classes from the webapp classpath by setting them up as server classes: https://github.com/eclipse/jetty.project/blob/jetty-10.0.x/jetty-maven-plugin/src/main/java/org/eclipse/jetty/maven/plugin/MavenWebInfConfiguration.java#L37 But, because this is javax.annotations we can't use that mechanism, hence the exclusion.

Having to explicitly exclude packages or jars from the maven classpath is inherently fragile, and I'd prefer there was another way, but that allowed people to add dependencies to their plugin declaration to mimic the jetty container classpath. Starting to do anything clever with making a new server container classloader and selectively populating it seems both good and bad: good in that we can control it, bad that we don't really know what should go on it. For example, should stuff that's in the reactor be available?

@olamy
Copy link
Member

olamy commented Mar 3, 2022

@olamy we have historically excluded maven classes from the webapp classpath by setting them up as server classes: https://github.com/eclipse/jetty.project/blob/jetty-10.0.x/jetty-maven-plugin/src/main/java/org/eclipse/jetty/maven/plugin/MavenWebInfConfiguration.java#L37 But, because this is javax.annotations we can't use that mechanism, hence the exclusion.

Having to explicitly exclude packages or jars from the maven classpath is inherently fragile, and I'd prefer there was another way, but that allowed people to add dependencies to their plugin declaration to mimic the jetty container classpath. Starting to do anything clever with making a new server container classloader and selectively populating it seems both good and bad: good in that we can control it, bad that we don't really know what should go on it. For example, should stuff that's in the reactor be available?

oh that's good!!! even better.
shouldn't we have this hide list configurable (i.e add more packages)?
we didn't have complain yet :) but users could have similar issues with guice, guava etc...

@olamy
Copy link
Member

olamy commented Mar 3, 2022

@janbartel I can add an IT test for the current case with this jsr. if you are happy my tomorrow :)

@olamy
Copy link
Member

olamy commented Mar 9, 2022

@janbartel sorry for delay I was in the middle of water and mud those last days :)
IT added

Signed-off-by: Olivier Lamy <[email protected]>
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 PR that changes the IT testing in jetty-maven-plugin, and that's it?

There's no change to anything that a user would use from the jetty-maven-plugin.

@@ -61,6 +61,7 @@ public void getContentResponse()
}
String contentCheck = System.getProperty("contentCheck");
String pathToCheck = System.getProperty("pathToCheck");
System.out.println("contentCheck: " + contentCheck);
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove System.out

Copy link
Member

Choose a reason for hiding this comment

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

this class has few more.
And you will see them only when using m-invoker-p in debug mode or streamLogs which is not the case per default but only when debugging or when an IT test fail.

@joakime
Copy link
Contributor

joakime commented Mar 21, 2022

I have verified that the jsr jar is not present any more in the dependency tree of the jetty-maven-plugin

[INFO] --- maven-dependency-plugin:3.2.0:tree (default-cli) @ jetty-maven-plugin ---
[INFO] org.eclipse.jetty:jetty-maven-plugin:maven-plugin:10.0.9-SNAPSHOT
[INFO] +- org.apache.maven.shared:maven-artifact-transfer:jar:0.13.1:compile
[INFO] |  +- org.codehaus.plexus:plexus-component-annotations:jar:2.1.1:compile
[INFO] |  +- org.apache.maven.shared:maven-common-artifact-filters:jar:3.1.0:compile
[INFO] |  |  \- org.sonatype.sisu:sisu-inject-plexus:jar:1.4.2:compile
[INFO] |  |     \- org.sonatype.sisu:sisu-inject-bean:jar:1.4.2:compile
[INFO] |  |        \- org.sonatype.sisu:sisu-guice:jar:noaop:2.1.7:compile
[INFO] |  \- org.codehaus.plexus:plexus-utils:jar:3.4.1:compile
[INFO] +- org.apache.maven:maven-plugin-api:jar:3.8.4:provided
[INFO] |  +- org.eclipse.sisu:org.eclipse.sisu.plexus:jar:0.3.5:provided
[INFO] |  \- org.codehaus.plexus:plexus-classworlds:jar:2.6.0:compile
[INFO] +- org.apache.maven:maven-model:jar:3.8.4:provided
[INFO] +- org.apache.maven:maven-settings:jar:3.8.4:provided
[INFO] +- org.apache.maven:maven-artifact:jar:3.8.4:provided
[INFO] |  \- org.apache.commons:commons-lang3:jar:3.12.0:compile
[INFO] +- org.apache.maven:maven-core:jar:3.8.4:provided
[INFO] |  +- org.apache.maven:maven-settings-builder:jar:3.8.4:provided
[INFO] |  |  \- org.codehaus.plexus:plexus-sec-dispatcher:jar:2.0:provided
[INFO] |  |     \- org.codehaus.plexus:plexus-cipher:jar:2.0:provided
[INFO] |  +- org.apache.maven:maven-builder-support:jar:3.8.4:provided
[INFO] |  +- org.apache.maven:maven-repository-metadata:jar:3.8.4:provided
[INFO] |  +- org.apache.maven:maven-model-builder:jar:3.8.4:provided
[INFO] |  +- org.apache.maven:maven-resolver-provider:jar:3.8.4:provided
[INFO] |  +- org.apache.maven.resolver:maven-resolver-impl:jar:1.6.3:provided
[INFO] |  +- org.apache.maven.resolver:maven-resolver-api:jar:1.6.3:provided
[INFO] |  +- org.apache.maven.resolver:maven-resolver-spi:jar:1.6.3:provided
[INFO] |  +- org.apache.maven.resolver:maven-resolver-util:jar:1.6.3:provided
[INFO] |  +- org.apache.maven.shared:maven-shared-utils:jar:3.3.4:compile
[INFO] |  |  \- commons-io:commons-io:jar:2.11.0:compile
[INFO] |  +- org.eclipse.sisu:org.eclipse.sisu.inject:jar:0.3.5:provided
[INFO] |  +- com.google.inject:guice:jar:no_aop:4.2.2:provided
[INFO] |  |  +- aopalliance:aopalliance:jar:1.0:provided
[INFO] |  |  \- com.google.guava:guava:jar:31.1-jre:provided
[INFO] |  |     +- com.google.guava:failureaccess:jar:1.0.1:provided
[INFO] |  |     +- com.google.guava:listenablefuture:jar:9999.0-empty-to-avoid-conflict-with-guava:provided
[INFO] |  |     +- org.checkerframework:checker-qual:jar:3.12.0:provided
[INFO] |  |     +- com.google.errorprone:error_prone_annotations:jar:2.11.0:provided
[INFO] |  |     \- com.google.j2objc:j2objc-annotations:jar:1.3:provided
[INFO] |  \- org.codehaus.plexus:plexus-interpolation:jar:1.26:provided
[INFO] +- org.apache.maven.plugin-tools:maven-plugin-tools-api:jar:3.6.4:compile
[INFO] +- org.apache.maven.plugin-tools:maven-plugin-annotations:jar:3.6.4:provided
[INFO] +- org.eclipse.jetty:jetty-util:jar:10.0.9-SNAPSHOT:compile
[INFO] +- org.eclipse.jetty:jetty-webapp:jar:10.0.9-SNAPSHOT:compile
[INFO] |  \- org.eclipse.jetty:jetty-xml:jar:10.0.9-SNAPSHOT:compile
[INFO] +- org.eclipse.jetty:jetty-quickstart:jar:10.0.9-SNAPSHOT:compile
[INFO] +- org.eclipse.jetty:jetty-jaas:jar:10.0.9-SNAPSHOT:compile
[INFO] |  +- org.eclipse.jetty:jetty-security:jar:10.0.9-SNAPSHOT:compile
[INFO] |  +- org.apache.directory.api:api-ldap-model:jar:2.1.0:compile
[INFO] |  |  +- org.apache.directory.api:api-asn1-ber:jar:2.1.0:compile
[INFO] |  |  +- org.apache.directory.api:api-i18n:jar:2.1.0:compile
[INFO] |  |  +- org.apache.mina:mina-core:jar:2.1.3:compile
[INFO] |  |  +- org.apache.servicemix.bundles:org.apache.servicemix.bundles.antlr:jar:2.7.7_5:compile
[INFO] |  |  +- org.apache.commons:commons-collections4:jar:4.4:compile
[INFO] |  |  \- commons-codec:commons-codec:jar:1.15:compile
[INFO] |  +- org.apache.directory.api:api-util:jar:2.1.0:compile
[INFO] |  \- org.apache.directory.api:api-asn1-api:jar:2.1.0:compile
[INFO] +- org.eclipse.jetty:jetty-plus:jar:10.0.9-SNAPSHOT:compile
[INFO] +- org.eclipse.jetty:jetty-jndi:jar:10.0.9-SNAPSHOT:compile
[INFO] +- org.eclipse.jetty:jetty-server:jar:10.0.9-SNAPSHOT:compile
[INFO] |  \- org.eclipse.jetty.toolchain:jetty-servlet-api:jar:4.0.6:compile
[INFO] +- org.eclipse.jetty:jetty-servlet:jar:10.0.9-SNAPSHOT:compile
[INFO] +- org.eclipse.jetty:jetty-client:jar:10.0.9-SNAPSHOT:compile
[INFO] |  \- org.eclipse.jetty:jetty-alpn-client:jar:10.0.9-SNAPSHOT:compile
[INFO] +- org.eclipse.jetty:jetty-http:jar:10.0.9-SNAPSHOT:compile
[INFO] +- org.eclipse.jetty:jetty-io:jar:10.0.9-SNAPSHOT:compile
[INFO] +- org.eclipse.jetty:jetty-jmx:jar:10.0.9-SNAPSHOT:compile
[INFO] +- org.eclipse.jetty:jetty-annotations:jar:10.0.9-SNAPSHOT:compile
[INFO] |  +- jakarta.annotation:jakarta.annotation-api:jar:1.3.5:compile
[INFO] |  +- org.ow2.asm:asm:jar:9.2:compile
[INFO] |  \- org.ow2.asm:asm-commons:jar:9.2:compile
[INFO] |     +- org.ow2.asm:asm-tree:jar:9.2:compile
[INFO] |     \- org.ow2.asm:asm-analysis:jar:9.2:compile
[INFO] +- org.eclipse.jetty.websocket:websocket-javax-server:jar:10.0.9-SNAPSHOT:compile
[INFO] |  +- org.eclipse.jetty.websocket:websocket-javax-client:jar:10.0.9-SNAPSHOT:compile
[INFO] |  |  +- org.eclipse.jetty.websocket:websocket-javax-common:jar:10.0.9-SNAPSHOT:compile
[INFO] |  |  \- org.eclipse.jetty.websocket:websocket-core-client:jar:10.0.9-SNAPSHOT:compile
[INFO] |  +- org.eclipse.jetty.websocket:websocket-servlet:jar:10.0.9-SNAPSHOT:compile
[INFO] |  |  \- org.eclipse.jetty.websocket:websocket-core-server:jar:10.0.9-SNAPSHOT:compile
[INFO] |  \- org.eclipse.jetty.toolchain:jetty-javax-websocket-api:jar:1.1.2:compile
[INFO] +- org.eclipse.jetty.websocket:websocket-jetty-server:jar:10.0.9-SNAPSHOT:compile
[INFO] |  +- org.eclipse.jetty.websocket:websocket-jetty-api:jar:10.0.9-SNAPSHOT:compile
[INFO] |  \- org.eclipse.jetty.websocket:websocket-jetty-common:jar:10.0.9-SNAPSHOT:compile
[INFO] |     \- org.eclipse.jetty.websocket:websocket-core-common:jar:10.0.9-SNAPSHOT:compile
[INFO] +- org.eclipse.jetty:apache-jsp:jar:10.0.9-SNAPSHOT:compile
[INFO] |  \- org.mortbay.jasper:apache-jsp:jar:9.0.52:compile
[INFO] |     +- org.eclipse.jetty.toolchain:jetty-schemas:jar:4.0.3:compile
[INFO] |     +- org.mortbay.jasper:apache-el:jar:9.0.52:compile
[INFO] |     \- org.eclipse.jdt:ecj:jar:3.26.0:compile
[INFO] +- org.eclipse.jetty:apache-jstl:jar:10.0.9-SNAPSHOT:compile
[INFO] |  +- org.apache.taglibs:taglibs-standard-spec:jar:1.2.5:compile
[INFO] |  \- org.apache.taglibs:taglibs-standard-impl:jar:1.2.5:compile
[INFO] +- org.slf4j:slf4j-api:jar:2.0.0-alpha6:compile
[INFO] +- jakarta.transaction:jakarta.transaction-api:jar:1.3.3:compile
[INFO] +- org.eclipse.jetty:jetty-slf4j-impl:jar:10.0.9-SNAPSHOT:test
[INFO] +- org.eclipse.jetty:jetty-home:zip:10.0.9-SNAPSHOT:test
[INFO] |  +- org.eclipse.jetty:jetty-deploy:jar:10.0.9-SNAPSHOT:test
[INFO] |  +- org.eclipse.jetty:jetty-servlets:jar:10.0.9-SNAPSHOT:test
[INFO] |  +- org.eclipse.jetty.websocket:websocket-jetty-client:jar:10.0.9-SNAPSHOT:test
[INFO] |  +- org.eclipse.jetty:jetty-proxy:jar:10.0.9-SNAPSHOT:test
[INFO] |  +- org.eclipse.jetty:jetty-rewrite:jar:10.0.9-SNAPSHOT:test
[INFO] |  +- org.eclipse.jetty.http3:http3-server:jar:10.0.9-SNAPSHOT:test
[INFO] |  |  +- org.eclipse.jetty.http3:http3-common:jar:10.0.9-SNAPSHOT:test
[INFO] |  |  |  +- org.eclipse.jetty.http3:http3-qpack:jar:10.0.9-SNAPSHOT:test
[INFO] |  |  |  \- org.eclipse.jetty.quic:quic-common:jar:10.0.9-SNAPSHOT:test
[INFO] |  |  |     \- org.eclipse.jetty.quic:quic-quiche-common:jar:10.0.9-SNAPSHOT:test
[INFO] |  |  |        \- org.mortbay.jetty.quiche:jetty-quiche-native:jar:0.11.0.2:test
[INFO] |  |  \- org.eclipse.jetty.quic:quic-server:jar:10.0.9-SNAPSHOT:test
[INFO] |  |     \- org.eclipse.jetty.quic:quic-quiche-jna:jar:10.0.9-SNAPSHOT:test
[INFO] |  |        \- net.java.dev.jna:jna-jpms:jar:5.10.0:test
[INFO] |  \- org.eclipse.jetty:jetty-alpn-java-server:jar:10.0.9-SNAPSHOT:test
[INFO] +- org.eclipse.jetty.toolchain:jetty-test-helper:jar:5.9:test
[INFO] |  \- org.hamcrest:hamcrest:jar:2.2:test
[INFO] \- org.junit.jupiter:junit-jupiter:jar:5.8.2:test
[INFO]    +- org.junit.jupiter:junit-jupiter-api:jar:5.8.2:test
[INFO]    |  +- org.opentest4j:opentest4j:jar:1.2.0:test
[INFO]    |  +- org.junit.platform:junit-platform-commons:jar:1.8.2:test
[INFO]    |  \- org.apiguardian:apiguardian-api:jar:1.1.2:test
[INFO]    +- org.junit.jupiter:junit-jupiter-params:jar:5.8.2:test
[INFO]    \- org.junit.jupiter:junit-jupiter-engine:jar:5.8.2:test
[INFO]       \- org.junit.platform:junit-platform-engine:jar:1.8.2:test

@janbartel janbartel merged commit 761bb3f into jetty-10.0.x Mar 28, 2022
@joakime joakime deleted the jetty-10.0.x-7677-exclude-findbugs branch May 4, 2022 21:44
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