[BEAM-4392] Fix :beam-runners-java-fn-execution failure#5457
Merged
lukecwik merged 1 commit intoapache:masterfrom May 24, 2018
Merged
[BEAM-4392] Fix :beam-runners-java-fn-execution failure#5457lukecwik merged 1 commit intoapache:masterfrom
lukecwik merged 1 commit intoapache:masterfrom
Conversation
Member
Contributor
Author
Member
|
This passes with my tests in :beam-runners-java-fn-execution:test with c17332a There is a default rule to include all I have no opinion on which way is right, I'm learning gradle as I go. |
Member
|
Boyuan and I looked through the source code for the shadow plugin and the matching rule was specifically if its explicitly included and not explicitly excluded (with no include rules meaning to include all): So as long as there is one include rule, it disables the default include all rule. |
lukecwik
requested changes
May 23, 2018
| @@ -321,7 +321,6 @@ ext.getJavaRelocatedPath = { String suffix -> | |||
|
|
|||
| ext.DEFAULT_SHADOW_CLOSURE = { | |||
Member
There was a problem hiding this comment.
Add a link to the https://github.com/johnrengelman/shadow/blob/98191096a94674245c7b3e63975df9e14f67074e/src/main/groovy/com/github/jengelman/gradle/plugins/shadow/internal/DefaultDependencyFilter.groovy#L123 with an explanation of how include/exclude matching works.
sdks/java/harness/build.gradle
Outdated
| { | ||
| dependencies { | ||
| include('**/*.class') | ||
| //Directly include all depended projects |
sdks/java/harness/build.gradle
Outdated
| include(project(":beam-sdks-java-extensions-google-cloud-platform-core")) | ||
| include(project(":beam-runners-core-java")) | ||
| include(project(":beam-runners-core-construction-java")) | ||
| //Include all dependencies and transitive dependencies |
sdks/java/harness/build.gradle
Outdated
| dependencies { | ||
| include('**/*.class') | ||
| //Directly include all depended projects | ||
| include(project(":beam-model-pipeline")) |
Member
There was a problem hiding this comment.
We want to use the shadow configuration so we'll need to specify these as path: projectName, configuration: "shadow"
We should also use a list containing all the projects and loop over it here and loop over it down below where we add the compile dependencies. This will allow for a person to update the list in one location and have it apply to both the shadow plugin and also the dependency set.
Member
|
It seems like is broken as well with the wrong shadowClosure and should instead exclude all. It is unclear what.* is actually applying to (if anything) and the exclude("io/netty") seems redundant.
|
Contributor
Author
Member
|
Thanks for getting to the bottom of |
Contributor
Author
|
Hey Luck @lukecwik , commit a0945b5 addressed exclude all problems. Followings are the shadowJar content: |
Member
|
I suggested some comment fix-ups in boyuanzz#1, feel free to merge into your branch and then this LGTM. |
Contributor
Author
|
Merged Luke's comments. @lukecwik |
Member
|
With this PR a bunch of dependencies are being included in jars without being relocated. For example, protobuf in sdks-java-core, or protobuf, publicsuffix, and runners-local-java in runners-direct-java. This seems to account for a good number of the remaining problems in my test. |
Contributor
Author
|
Could you please attach more details about how you notice that |
Member
|
I ran |
Member
|
LGTM! Lets merge this! Looks like the issues I "found" are not caused by this and are present on master. The root cause of my failures was renaming my configuration to |
2 tasks
lukecwik
approved these changes
May 24, 2018
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Root cause:
We have wrong shadowClosure configurations in beam/sdks/java/harness/build.gradle: https://github.com/apache/beam/blob/master/sdks/java/harness/build.gradle#L27, which cause produced jar doesn't include expected dependencies.
Solution:
Change bulid rules of sdks/java/harness to make harness as a uber-jar.
r: @lukecwik @aaltay