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

[BEAM-5875] Fix scope of dependency ensuring that it is packaged within the jar. #6894

Merged
merged 1 commit into from Oct 31, 2018

Conversation

lukecwik
Copy link
Member


Follow this checklist to help us incorporate your contribution quickly and easily:

  • 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.
  • If this contribution is large, please file an Apache Individual Contributor License Agreement.

It will help us expedite review of your Pull Request if you tag someone (e.g. @username) to look at it.

Post-Commit Tests Status (on master branch)

Lang SDK Apex Dataflow Flink Gearpump Samza Spark
Go Build Status --- --- --- --- --- ---
Java Build Status Build Status Build Status Build Status Build Status Build Status Build Status Build Status
Python Build Status --- Build Status
Build Status
Build Status --- --- ---

@lukecwik
Copy link
Member Author

R: @lgajowy @kennknowles

@kennknowles
Copy link
Member

LGTM. Thanks for the fix.

Copy link
Contributor

@lgajowy lgajowy left a comment

Choose a reason for hiding this comment

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

Thank you. It looks good but I have some questions so that I'm more aware in the future.

@@ -23,7 +23,7 @@ description = "Apache Beam :: SDKs :: Java :: Test Utils"

dependencies {
shadow project(path: ":beam-sdks-java-core", configuration: "shadow")
shadow library.java.guava
compile library.java.guava
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you explain what is the difference between "compile" and "shadow"? I thought I understand this but as I read through "Gradle Primier" doc and "BeamModulePlugin.java" file, I start to have doubts. Some questions:

  • both shadow and compile configurations "shadow" guava, because it is listed in the DEFAULT_SHADOW_CLOSURE that is used there, right? So whichever configuration I use it will still get shadowed the same way. What if Guava wasn't listed in the DEFAULT_SHADOW_CLOSURE? Would It get shadowed if I use "compile"?
  • you said that using "compile" packages the depencency "within the jar". So what happens with the guava dependency when I use "shadow"?

Copy link
Member

Choose a reason for hiding this comment

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

There are two independent parts to what we call "shading": relocating (moving classes into different namespace) and bundling (putting classes into jars). You can do one and not the other. The difference between compile and shadow is bundling. If you have a compile dependency, then it will be bundled into the shadowJar. If you have a shadow dependency, then it will not be bundled into the shadowJar.

The DEFAULT_SHADOW_CLOSURE configures relocations and TBH not sure what include(dependency(...)) does. It is not the same as putting it in the scope to be bundled.

Copy link
Member Author

Choose a reason for hiding this comment

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

Another part which is important for shading between compile and shadow is whether the dependency is listed in the pom.xml that is uploaded to Maven central for the artifact.
compile: Not part of pom.xml
shadow: Part of pom.xml

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the clarification.

@lukecwik lukecwik merged commit 4e786b0 into apache:master Oct 31, 2018
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.

None yet

3 participants