[SPARK-12734][BUILD] Fix Netty exclusion and use Maven Enforcer to prevent future bugs #10672
Conversation
|
I'd like to backport the |
There was a problem hiding this comment.
@pwendell, this was from your original PR but I think it's no longer necessary because we don't run the compile phase.
There was a problem hiding this comment.
Also, note that we need to install dummy JARs and test JARs for all modules so that mvn validate doesn't fail during dependency resolution.
|
LGTM. Other candidates for banning: the servlet API jars in all its packagings |
|
Test build #49041 has finished for PR 10672 at commit
|
|
I've opened two pull requests to backport the I agree about the servlet API JARs, that's a good case. We should also exclude the ASM classes, since pulling in old versions of ASM is a leading cause of Java 8 compatibility issues. I'll update this in a little bit in order to cover those two obvious cases. There might be a couple of other candidates that are worth banning, but I'd like to cover them in followups. |
|
Actually, I might choose to handle those separately in a followup next week in order to have more time to think through a few additional cases, including the exclusion of certain Jetty versions. I want to get the enforcer infra and the Netty fix in first to unblock a 1.5 doc building problem. |
|
LGTM. |
|
Thanks for reviewing. I'm going to merge this into master and will open separate PRs to test the backports, which might possibly require a different set of exclusion rules. |
…event future bugs Netty classes are published under multiple artifacts with different names, so our build needs to exclude the `io.netty:netty` and `org.jboss.netty:netty` versions of the Netty artifact. However, our existing exclusions were incomplete, leading to situations where duplicate Netty classes would wind up on the classpath and cause compile errors (or worse). This patch fixes the exclusion issue by adding more exclusions and uses Maven Enforcer's [banned dependencies](https://maven.apache.org/enforcer/enforcer-rules/bannedDependencies.html) rule to prevent these classes from accidentally being reintroduced. I also updated `dev/test-dependencies.sh` to run `mvn validate` so that the enforcer rules can run as part of pull request builds. /cc rxin srowen pwendell. I'd like to backport at least the exclusion portion of this fix to `branch-1.5` in order to fix the documentation publishing job, which fails nondeterministically due to incompatible versions of Netty classes taking precedence on the compile-time classpath. Author: Josh Rosen <[email protected]> Author: Josh Rosen <[email protected]> Closes apache#10672 from JoshRosen/enforce-netty-exclusions.
…event future bugs Netty classes are published under multiple artifacts with different names, so our build needs to exclude the `io.netty:netty` and `org.jboss.netty:netty` versions of the Netty artifact. However, our existing exclusions were incomplete, leading to situations where duplicate Netty classes would wind up on the classpath and cause compile errors (or worse). This patch fixes the exclusion issue by adding more exclusions and uses Maven Enforcer's [banned dependencies](https://maven.apache.org/enforcer/enforcer-rules/bannedDependencies.html) rule to prevent these classes from accidentally being reintroduced. I also updated `dev/test-dependencies.sh` to run `mvn validate` so that the enforcer rules can run as part of pull request builds. /cc rxin srowen pwendell. I'd like to backport at least the exclusion portion of this fix to `branch-1.5` in order to fix the documentation publishing job, which fails nondeterministically due to incompatible versions of Netty classes taking precedence on the compile-time classpath. Author: Josh Rosen <[email protected]> Author: Josh Rosen <[email protected]> Closes apache#10672 from JoshRosen/enforce-netty-exclusions.
|
I discovered that the now-removed |
There was a problem hiding this comment.
Yeah, so this exclude is likely to break Akka remote now that the Netty 3.x classes are completely missing from the classpath.
|
I've opened #10693 to hotfix the exclusions. In the 1.5 and 1.6 backport branches, I've re-scoped the changes to only exclude |
This is a hotfix for a build bug introduced by the Netty exclusion changes in #10672. We can't exclude `io.netty:netty` because Akka depends on it. There's not a direct conflict between `io.netty:netty` and `io.netty:netty-all`, because the former puts classes in the `org.jboss.netty` namespace while the latter uses the `io.netty` namespace. However, there still is a conflict between `org.jboss.netty:netty` and `io.netty:netty`, so we need to continue to exclude the JBoss version of that artifact. While the diff here looks somewhat large, note that this is only a revert of a some of the changes from #10672. You can see the net changes in pom.xml at 3119206...5211ab8#diff-600376dffeb79835ede4a0b285078036 Author: Josh Rosen <[email protected]> Closes #10693 from JoshRosen/netty-hotfix.
…to branch-1.6 This patch backports the Netty exclusion fixes from #10672 to branch-1.6. Author: Josh Rosen <[email protected]> Closes #10691 from JoshRosen/netty-exclude-16-backport.
…to branch-1.5 This patch backports the Netty exclusion fixes from #10672 to branch-1.5. Author: Josh Rosen <[email protected]> Closes #10690 from JoshRosen/netty-exclude-15-backport.
…er install in dep tests This patch fixes a build/test issue caused by the combination of #10672 and a latent issue in the original `dev/test-dependencies` script. First, changes which _only_ touched build files were not triggering full Jenkins runs, making it possible for a build change to be merged even though it could cause failures in other tests. The `root` build module now depends on `build`, so all tests will now be run whenever a build-related file is changed. I also added a `clean` step to the Maven install step in `dev/test-dependencies` in order to address an issue where the dummy JARs stuck around and caused "multiple assembly JARs found" errors in tests. /cc zsxwing Author: Josh Rosen <[email protected]> Closes #10704 from JoshRosen/fix-build-test-problems.
…er install in dep tests This patch fixes a build/test issue caused by the combination of #10672 and a latent issue in the original `dev/test-dependencies` script. First, changes which _only_ touched build files were not triggering full Jenkins runs, making it possible for a build change to be merged even though it could cause failures in other tests. The `root` build module now depends on `build`, so all tests will now be run whenever a build-related file is changed. I also added a `clean` step to the Maven install step in `dev/test-dependencies` in order to address an issue where the dummy JARs stuck around and caused "multiple assembly JARs found" errors in tests. /cc zsxwing Author: Josh Rosen <[email protected]> Closes #10704 from JoshRosen/fix-build-test-problems. (cherry picked from commit a449914) Signed-off-by: Josh Rosen <[email protected]>
…er install in dep tests This patch fixes a build/test issue caused by the combination of #10672 and a latent issue in the original `dev/test-dependencies` script. First, changes which _only_ touched build files were not triggering full Jenkins runs, making it possible for a build change to be merged even though it could cause failures in other tests. The `root` build module now depends on `build`, so all tests will now be run whenever a build-related file is changed. I also added a `clean` step to the Maven install step in `dev/test-dependencies` in order to address an issue where the dummy JARs stuck around and caused "multiple assembly JARs found" errors in tests. /cc zsxwing Author: Josh Rosen <[email protected]> Closes #10704 from JoshRosen/fix-build-test-problems. (cherry picked from commit a449914) Signed-off-by: Josh Rosen <[email protected]>
4 participants
Netty classes are published under multiple artifacts with different names, so our build needs to exclude the
io.netty:nettyandorg.jboss.netty:nettyversions of the Netty artifact. However, our existing exclusions were incomplete, leading to situations where duplicate Netty classes would wind up on the classpath and cause compile errors (or worse).This patch fixes the exclusion issue by adding more exclusions and uses Maven Enforcer's banned dependencies rule to prevent these classes from accidentally being reintroduced. I also updated
dev/test-dependencies.shto runmvn validateso that the enforcer rules can run as part of pull request builds./cc @rxin @srowen @pwendell. I'd like to backport at least the exclusion portion of this fix to
branch-1.5in order to fix the documentation publishing job, which fails nondeterministically due to incompatible versions of Netty classes taking precedence on the compile-time classpath.