Skip to content

[BEAM-1871] [BEAM-1019 ] Shade dependencies in sdks/core#2996

Closed
vikkyrk wants to merge 2 commits intoapache:release-2.0.0from
vikkyrk:shade_2.0.0
Closed

[BEAM-1871] [BEAM-1019 ] Shade dependencies in sdks/core#2996
vikkyrk wants to merge 2 commits intoapache:release-2.0.0from
vikkyrk:shade_2.0.0

Conversation

@vikkyrk
Copy link
Contributor

@vikkyrk vikkyrk commented May 9, 2017

Be sure to do all of the following to help us incorporate your contribution
quickly and easily:

  • Make sure the PR title is formatted like:
    [BEAM-<Jira issue #>] Description of pull request
  • Make sure tests pass via mvn clean verify.
  • Replace <Jira issue #> in the title with the actual Jira issue
    number, if there is one.
  • If this contribution is large, please file an Apache
    Individual Contributor License Agreement.

  • Shade com.google.guava:guava, com.google.protobuf:protobuf-java, net.bytebuddy:byte-buddy, org.apache.commons:*
  • Shading net.bytebuddy fails the GcpApiSurfaceTest with NoClassDefFoundError for org/apache/beam/sdk/repackaged/net/bytebuddy/jar/asm/tree/MethodNode. Looks like an issue with how net.bytebuddy does shading for org.objectweb.asm. Either way the ApiSurfaceTest should be fixed to be able to prune and load classes lazily. For release-2.0.0 we catch and ignore any class loading errors.

@vikkyrk
Copy link
Contributor Author

vikkyrk commented May 9, 2017

R: @davorbonaci @kennknowles
CC: @lukecwik

@kennknowles
Copy link
Member

This uncovered many issues with ApiSurface, recorded in https://issues.apache.org/jira/browse/BEAM-2230, https://issues.apache.org/jira/browse/BEAM-2231, https://issues.apache.org/jira/browse/BEAM-2232.

Skipping unloadable classes isn't great, because we might have botched something, but it works around these problems. Perhaps you could narrow the catch block to class not found errors and add a TODO with a link to BEAM-2231?

Copy link
Member

@davorbonaci davorbonaci left a comment

Choose a reason for hiding this comment

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

LGTM, modulo @kennknowles' comments

@vikkyrk
Copy link
Contributor Author

vikkyrk commented May 9, 2017

Done

asfgit pushed a commit that referenced this pull request May 9, 2017
@davorbonaci
Copy link
Member

Merged. Please close.

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