[BEAM-14117] Unvendor bytebuddy dependency #17317

cushon · 2022-04-07T22:22:25Z

This change unvendors bytebuddy, as discussed in BEAM-14117

Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:

Choose reviewer(s) and mention them in a comment (R: @username).
Format the pull request title like [BEAM-XXX] Fixes bug in ApproximateQuantiles, where you replace BEAM-XXX with the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.
Update CHANGES.md with noteworthy changes.
If this contribution is large, please file an Apache Individual Contributor License Agreement.

See the Contributor Guide for more tips on how to make review process smoother.

To check the build health, please visit https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md

GitHub Actions Tests Status (on master branch)

See CI.md for more information about GitHub Actions CI.

asf-ci · 2022-04-07T22:22:27Z

Can one of the admins verify this patch?

asf-ci · 2022-04-07T22:22:27Z

Can one of the admins verify this patch?

codecov · 2022-04-07T23:00:36Z

Codecov Report

Merging #17317 (7dcc6d8) into master (7dcc6d8) will not change coverage.
The diff coverage is n/a.

❗ Current head 7dcc6d8 differs from pull request most recent head ee7bf73. Consider uploading reports for the commit ee7bf73 to get more accurate results

@@           Coverage Diff           @@
##           master   #17317   +/-   ##
=======================================
  Coverage   74.23%   74.23%           
=======================================
  Files         703      703           
  Lines       93100    93100           
=======================================
  Hits        69113    69113           
  Misses      22720    22720           
  Partials     1267     1267

Flag	Coverage Δ
go	`51.52% <0.00%> (ø)`
python	`83.61% <0.00%> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7dcc6d8...ee7bf73. Read the comment docs.

kennknowles · 2022-04-08T13:18:44Z

run java postcommit

kennknowles · 2022-04-08T13:18:50Z

run flink validatesrunner

kennknowles · 2022-04-08T13:18:57Z

run spark validatesrunner

kennknowles · 2022-04-08T13:19:07Z

run samza validatesrunner

kennknowles · 2022-04-08T13:19:14Z

run dataflow validatesrunner

cushon · 2022-04-14T20:00:30Z

R: @kennknowles

kennknowles · 2022-06-17T18:38:48Z

Thread at https://lists.apache.org/thread/sdp7tpvy5n1jlk3g8wqqp40dlyzp5llv did not encounter any major objections.

kennknowles · 2022-06-17T18:43:09Z

The tests are super flaky, but I think Jenkins runs against the HEAD commit of the PR so it will need a rebase to get any recent improvements in test flakiness. Let me investigate a little bit and I'll comment when I have news.

kennknowles · 2022-07-12T22:38:02Z

sdks/java/core/build.gradle

@@ -28,12 +28,17 @@ applyJavaNature(
 dependencies {
 include(dependency("org.apache.commons:.*"))
 include(dependency(library.java.antlr_runtime))
+ include(dependency(library.java.byte_buddy))


I don't think we should bundle bytebuddy in here, right? It should be a residual dependency that is resolved according to everyone's poms?

In other words, it should become "just like any other dependency"

Done, thanks

kennknowles

Perfect!

kennknowles · 2022-07-13T17:21:16Z

Now I just want to poke the tests until they are green, rather than ignoring red signals, even if they seem irrelevant.

cushon · 2022-07-13T21:41:29Z

Adding an explicit dep doesn't seem to be the way to go since it is technically a transitive dependency.

Thanks, any suggestions about what the right way to go would be?

kennknowles · 2022-07-13T23:12:14Z

sdks/java/harness/jmh/build.gradle

@@ -34,6 +34,7 @@ dependencies {
 implementation project(path: ":sdks:java:harness", configuration: "shadow")
 implementation project(":runners:java-fn-execution")
 runtimeOnly library.java.slf4j_jdk14
+ runtimeOnly library.java.byte_buddy


I'm not sure how the failure occurred, but based on grep and IWYU this should just not be here. I understand you added it in response to an issue. It sounds like there must be an error in some other module's IWYU compliance, and presumably is suppressed? (just a guess; I don't know)

cushon · 2022-07-14T00:14:16Z

I did some more debugging, and my understanding is that this configuration is effectively removing the implementation dep:

beam/sdks/java/core/build.gradle

Lines 27 to 35 in f2e41be

 shadowClosure: { 

 dependencies { 

 include(dependency("org.apache.commons:.*")) 

 include(dependency(library.java.antlr_runtime)) 

 } 

 relocate "com.google.thirdparty", getJavaRelocatedPath("com.google.thirdparty") 

 relocate "org.apache.commons", getJavaRelocatedPath("org.apache.commons") 

 relocate "org.antlr.v4", getJavaRelocatedPath("org.antlr.v4") 

 },

because when shadowClosure is specified BeamModulePlugin removes all of the implementation deps:

beam/buildSrc/src/main/groovy/org/apache/beam/gradle/BeamModulePlugin.groovy

Lines 808 to 810 in f2e41be

 // * implementation - Required during compilation or runtime of the main source set. 

 // This configuration represents all dependencies that much also be shaded away 

 // otherwise the generated Maven pom will be missing this dependency.

Hacking up BeamModulePlugin to add a hard-coded implementation dep on bytebuddy fixes the build errors I was seeing, which seems to be consistent with that theory:

diff --git a/buildSrc/src/main/groovy/org/apache/beam/gradle/BeamModulePlugin.groovy b/buildSrc/src/main/groovy/org/apache/beam/gradle/BeamModulePlugin.groovy
index 791277a7e1..30b94333e0 100644
--- a/buildSrc/src/main/groovy/org/apache/beam/gradle/BeamModulePlugin.groovy
+++ b/buildSrc/src/main/groovy/org/apache/beam/gradle/BeamModulePlugin.groovy
@@ -496,6 +496,7 @@ class BeamModulePlugin implements Plugin<Project> {
     def testcontainers_version = "1.16.3"
     def arrow_version = "5.0.0"
     def jmh_version = "1.34"
+    def byte_buddy_version = "1.12.9"

     // A map of maps containing common libraries used per language. To use:
     // dependencies {
@@ -541,7 +542,7 @@ class BeamModulePlugin implements Plugin<Project> {
         aws_java_sdk2_utils                         : "software.amazon.awssdk:utils:$aws_java_sdk2_version",
         bigdataoss_gcsio                            : "com.google.cloud.bigdataoss:gcsio:$google_cloud_bigdataoss_version",
         bigdataoss_util                             : "com.google.cloud.bigdataoss:util:$google_cloud_bigdataoss_version",
-        byte_buddy                                  : "net.bytebuddy:byte-buddy:1.12.9",
+        byte_buddy                                  : "net.bytebuddy:byte-buddy:$byte_buddy_version",
         cassandra_driver_core                       : "com.datastax.cassandra:cassandra-driver-core:$cassandra_driver_version",
         cassandra_driver_mapping                    : "com.datastax.cassandra:cassandra-driver-mapping:$cassandra_driver_version",
         cdap_api                                    : "io.cdap.cdap:cdap-api:$cdap_version",
@@ -1063,7 +1064,15 @@ class BeamModulePlugin implements Plugin<Project> {
         // This contains many improved annotations beyond javax.annotations for enhanced static checking
         // of the codebase. It is runtime so users can also take advantage of them. The annotations themselves
         // are MIT licensed (checkerframework is GPL and cannot be distributed)
-        implementation "org.checkerframework:checker-qual:$checkerframework_version"
+
+        def implementationDeps = [
+          "org.checkerframework:checker-qual:$checkerframework_version",
+          "net.bytebuddy:byte-buddy:$byte_buddy_version",
+        ]
+
+        implementationDeps.each { dep ->
+          implementation dep
+        }
       }

       // Defines Targets for sonarqube analysis reporting.

What do you recommend? It seems like there are no existing examples of implementation deps that aren't included in the shadow closure. Does it make sense to include it in the shadow closure after all? Is there a more principled change to BeamModulePlugin to allow it to be configured to preserve some implementation deps?

lukecwik · 2022-07-14T02:35:25Z

I did some more debugging, and my understanding is that this configuration is effectively removing the implementation dep:

beam/sdks/java/core/build.gradle

Lines 27 to 35 in f2e41be

shadowClosure: {

dependencies {

include(dependency("org.apache.commons:.*"))

include(dependency(library.java.antlr_runtime))

}

relocate "com.google.thirdparty", getJavaRelocatedPath("com.google.thirdparty")

relocate "org.apache.commons", getJavaRelocatedPath("org.apache.commons")

relocate "org.antlr.v4", getJavaRelocatedPath("org.antlr.v4")

},

because when shadowClosure is specified BeamModulePlugin removes all of the implementation deps:

beam/buildSrc/src/main/groovy/org/apache/beam/gradle/BeamModulePlugin.groovy

Lines 808 to 810 in f2e41be

// * implementation - Required during compilation or runtime of the main source set.

// This configuration represents all dependencies that much also be shaded away

// otherwise the generated Maven pom will be missing this dependency.

Hacking up BeamModulePlugin to add a hard-coded implementation dep on bytebuddy fixes the build errors I was seeing, which seems to be consistent with that theory:
diff --git a/buildSrc/src/main/groovy/org/apache/beam/gradle/BeamModulePlugin.groovy b/buildSrc/src/main/groovy/org/apache/beam/gradle/BeamModulePlugin.groovy
index 791277a7e1..30b94333e0 100644
--- a/buildSrc/src/main/groovy/org/apache/beam/gradle/BeamModulePlugin.groovy
+++ b/buildSrc/src/main/groovy/org/apache/beam/gradle/BeamModulePlugin.groovy
@@ -496,6 +496,7 @@ class BeamModulePlugin implements Plugin<Project> {
     def testcontainers_version = "1.16.3"
     def arrow_version = "5.0.0"
     def jmh_version = "1.34"
+    def byte_buddy_version = "1.12.9"

     // A map of maps containing common libraries used per language. To use:
     // dependencies {
@@ -541,7 +542,7 @@ class BeamModulePlugin implements Plugin<Project> {
         aws_java_sdk2_utils                         : "software.amazon.awssdk:utils:$aws_java_sdk2_version",
         bigdataoss_gcsio                            : "com.google.cloud.bigdataoss:gcsio:$google_cloud_bigdataoss_version",
         bigdataoss_util                             : "com.google.cloud.bigdataoss:util:$google_cloud_bigdataoss_version",
-        byte_buddy                                  : "net.bytebuddy:byte-buddy:1.12.9",
+        byte_buddy                                  : "net.bytebuddy:byte-buddy:$byte_buddy_version",
         cassandra_driver_core                       : "com.datastax.cassandra:cassandra-driver-core:$cassandra_driver_version",
         cassandra_driver_mapping                    : "com.datastax.cassandra:cassandra-driver-mapping:$cassandra_driver_version",
         cdap_api                                    : "io.cdap.cdap:cdap-api:$cdap_version",
@@ -1063,7 +1064,15 @@ class BeamModulePlugin implements Plugin<Project> {
         // This contains many improved annotations beyond javax.annotations for enhanced static checking
         // of the codebase. It is runtime so users can also take advantage of them. The annotations themselves
         // are MIT licensed (checkerframework is GPL and cannot be distributed)
-        implementation "org.checkerframework:checker-qual:$checkerframework_version"
+
+        def implementationDeps = [
+          "org.checkerframework:checker-qual:$checkerframework_version",
+          "net.bytebuddy:byte-buddy:$byte_buddy_version",
+        ]
+
+        implementationDeps.each { dep ->
+          implementation dep
+        }
       }

       // Defines Targets for sonarqube analysis reporting.
What do you recommend? It seems like there are no existing examples of implementation deps that aren't included in the shadow closure. Does it make sense to include it in the shadow closure after all? Is there a more principled change to BeamModulePlugin to allow it to be configured to preserve some implementation deps?

You should declare byte_buddy within the shadow configuration as per

beam/buildSrc/src/main/groovy/org/apache/beam/gradle/BeamModulePlugin.groovy

Line 811 in f2e41be

// * shadow - Required during compilation or runtime of the main source set.

This will make it a compile time dependency for the source set and it will be exposed in the pom.xml file and should become a transitive dependency.

cushon · 2022-07-14T02:50:37Z

You should declare byte_buddy within the shadow configuration

Thanks so much! Done.

This is only for testing.

cushon · 2022-07-16T19:16:06Z

I think all of the open issues have been resolved:

the byte_buddy dep is now in the shadow configuration
the CI is green (I had to retry a few times but this didn't require any additional fixes, I think some of the tests might be flaky)

Let me know if there's any additional feedback!

lukecwik · 2022-07-18T17:50:43Z

Fixes #21519

This reverts commit 4b5efc3.

[release-2.41.0] Revert "[BEAM-14117] Unvendor bytebuddy dependency (#17317)"

github-actions bot added build extensions java protobuf labels Apr 7, 2022

cushon force-pushed the unvendor branch from d831963 to b48ce9c Compare April 7, 2022 22:29

cushon force-pushed the unvendor branch 2 times, most recently from 078f5de to d5f3e5c Compare April 8, 2022 01:13

cushon force-pushed the unvendor branch from b271707 to 381cd1c Compare April 12, 2022 18:42

cushon changed the title ~~Unvendor bytebuddy~~ [BEAM-14117] Unvendor bytebuddy dependency Apr 13, 2022

cushon force-pushed the unvendor branch from 381cd1c to 9e8437b Compare April 13, 2022 20:53

cushon mentioned this pull request Apr 21, 2022

[BEAM-14065] Upgrade vendored bytebuddy to version 1.12.8 #17029

Closed

aaltay requested a review from kennknowles April 28, 2022 17:52

aaltay requested a review from kileys June 9, 2022 20:21

cushon force-pushed the unvendor branch from c4d5c3b to c46fbcd Compare June 17, 2022 19:12

kennknowles reviewed Jul 12, 2022

View reviewed changes

cushon force-pushed the unvendor branch 2 times, most recently from e2c007f to 35af3a4 Compare July 13, 2022 17:07

kennknowles approved these changes Jul 13, 2022

View reviewed changes

kennknowles reviewed Jul 13, 2022

View reviewed changes

cushon force-pushed the unvendor branch 4 times, most recently from 8ae8744 to ee7bf73 Compare July 15, 2022 18:19

cushon added 7 commits July 15, 2022 16:15

Unvendor bytebuddy

e37e4a8

This is only for testing.

Fix :sdks:java:core:validateShadedJarDoesntLeakNonProjectClasses

4451419

Fix another import

7efad51

Remove a reference to vendored_bytebuddy_1_11_0

c2dcf7a

Remove from sdks/java/core/build.gradle

742f7f1

Add an explicit bytebuddy dep to jmh benchmarks

5e8f027

Declare byte_buddy within the shadow configuration

c88e692

cushon force-pushed the unvendor branch from ee7bf73 to c88e692 Compare July 15, 2022 23:15

cushon requested a review from kennknowles July 16, 2022 19:14

lukecwik approved these changes Jul 18, 2022

View reviewed changes

lukecwik merged commit 4b5efc3 into apache:master Jul 18, 2022

lukecwik mentioned this pull request Jul 18, 2022

Unvendor bytebuddy dependency #21519

Closed

lukecwik mentioned this pull request Aug 4, 2022

[BEAM-14117, #21519] Delete vendored bytebuddy gradle build #22594

Merged

4 tasks

kileys mentioned this pull request Aug 16, 2022

[Bug]: Bytebuddy version update causes Invisible parameter type error #22749

Closed

kileys added a commit to kileys/beam that referenced this pull request Aug 17, 2022

Revert "[BEAM-14117] Unvendor bytebuddy dependency (apache#17317)"

577502e

This reverts commit 4b5efc3.

kileys added a commit to kileys/beam that referenced this pull request Aug 17, 2022

Revert "[BEAM-14117] Unvendor bytebuddy dependency (apache#17317)"

b677723

This reverts commit 4b5efc3.

kileys added a commit to kileys/beam that referenced this pull request Aug 17, 2022

Revert "[BEAM-14117] Unvendor bytebuddy dependency (apache#17317)"

0483ac8

This reverts commit 4b5efc3.

kileys added a commit to kileys/beam that referenced this pull request Aug 17, 2022

Revert "[BEAM-14117] Unvendor bytebuddy dependency (apache#17317)"

c855da6

This reverts commit 4b5efc3.

kileys added a commit that referenced this pull request Aug 17, 2022

Merge pull request #22759 from kileys/releasebranch

7ef51bc

[release-2.41.0] Revert "[BEAM-14117] Unvendor bytebuddy dependency (#17317)"

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BEAM-14117] Unvendor bytebuddy dependency #17317

[BEAM-14117] Unvendor bytebuddy dependency #17317

cushon commented Apr 7, 2022 •

edited

Loading

asf-ci commented Apr 7, 2022

asf-ci commented Apr 7, 2022

codecov bot commented Apr 7, 2022 •

edited

Loading

kennknowles commented Apr 8, 2022

kennknowles commented Apr 8, 2022

kennknowles commented Apr 8, 2022

kennknowles commented Apr 8, 2022

kennknowles commented Apr 8, 2022

cushon commented Apr 14, 2022

kennknowles commented Jun 17, 2022

kennknowles commented Jun 17, 2022

kennknowles Jul 12, 2022

kennknowles Jul 12, 2022

cushon Jul 13, 2022

kennknowles left a comment

kennknowles commented Jul 13, 2022

cushon commented Jul 13, 2022

kennknowles Jul 13, 2022

cushon commented Jul 14, 2022

lukecwik commented Jul 14, 2022

cushon commented Jul 14, 2022

cushon commented Jul 16, 2022

lukecwik commented Jul 18, 2022

[BEAM-14117] Unvendor bytebuddy dependency #17317

[BEAM-14117] Unvendor bytebuddy dependency #17317

Conversation

cushon commented Apr 7, 2022 • edited Loading

GitHub Actions Tests Status (on master branch)

asf-ci commented Apr 7, 2022

asf-ci commented Apr 7, 2022

codecov bot commented Apr 7, 2022 • edited Loading

Codecov Report

kennknowles commented Apr 8, 2022

kennknowles commented Apr 8, 2022

kennknowles commented Apr 8, 2022

kennknowles commented Apr 8, 2022

kennknowles commented Apr 8, 2022

cushon commented Apr 14, 2022

kennknowles commented Jun 17, 2022

kennknowles commented Jun 17, 2022

kennknowles Jul 12, 2022

Choose a reason for hiding this comment

kennknowles Jul 12, 2022

Choose a reason for hiding this comment

cushon Jul 13, 2022

Choose a reason for hiding this comment

kennknowles left a comment

Choose a reason for hiding this comment

kennknowles commented Jul 13, 2022

cushon commented Jul 13, 2022

kennknowles Jul 13, 2022

Choose a reason for hiding this comment

cushon commented Jul 14, 2022

lukecwik commented Jul 14, 2022

cushon commented Jul 14, 2022

cushon commented Jul 16, 2022

lukecwik commented Jul 18, 2022

cushon commented Apr 7, 2022 •

edited

Loading

codecov bot commented Apr 7, 2022 •

edited

Loading