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-7042] remove antlr from shadow configuration #8268

Merged
merged 1 commit into from
Apr 10, 2019

Conversation

adude3141
Copy link
Contributor

Please add a meaningful description for your change here


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

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 Build Status
Python Build Status
Build Status
--- Build Status
Build Status
Build Status --- --- ---

Pre-Commit Tests Status (on master branch)

--- Java Python Go Website
Non-portable Build Status Build Status Build Status Build Status
Portable --- Build Status --- ---

See .test-infra/jenkins/README for trigger phrase, status and link of all Jenkins jobs.

@iemejia
Copy link
Member

iemejia commented Apr 10, 2019

The tests passed but the antlr dependency is still leaking, it seems antlr gradle plugin leaks it in the compile scope when colled at antlr library.java.antlr

@iemejia
Copy link
Member

iemejia commented Apr 10, 2019

If you move it into the generateFromSource task, it still leaks I triied this way but I have a really low level of gradle-fu so maybe there is a way to remove it afterwards.

diff --git a/sdks/java/core/build.gradle b/sdks/java/core/build.gradle
index a4a1264922..f271733284 100644
--- a/sdks/java/core/build.gradle
+++ b/sdks/java/core/build.gradle
@@ -34,6 +34,7 @@ applyAvroNature()
 applyAntlrNature()
 
 generateGrammarSource {
+  dependencies { antlr library.java.antlr }
   arguments += ["-visitor"]
 }
 
@@ -58,7 +59,6 @@ test {
 }
 
 dependencies {
-  antlr library.java.antlr
   // Required to load constants from the model, e.g. max timestamp for global window
   shadow project(path: ":beam-model-pipeline", configuration: "shadow")
   shadow project(path: ":beam-model-job-management", configuration: "shadow")
@@ -69,7 +69,6 @@ dependencies {
   compile library.java.commons_compress
   compile library.java.commons_lang3
   compile library.java.guava_testlib
-  shadow library.java.antlr
   shadow library.java.jackson_core
   shadow library.java.jackson_annotations
   shadow library.java.jackson_databind

@adude3141
Copy link
Contributor Author

Sorry, what does 'it is still leaking' mean. Obviously I did not understand the issue here

@iemejia
Copy link
Member

iemejia commented Apr 10, 2019

Leaking means that we do not want the main antlr4 dependency to be available in the compile classpath, only antlr4-runtime because it is the only one needed. However it seems the gradle antlr plugin is adding it to the compile class path (leaking it).

@adude3141
Copy link
Contributor Author

I used

./gradlew -p sdks/java/core/ generatePomFileForMavenJavaPublication -Ppublishing

to generate the pom file.

Diff shows

Michaels-MBP-2:beam michel$ diff sdks/java/core/build/publications/mavenJava/pom-default.xml sdks/java/core/build/publications/mavenJava/pom-default-bak.xml 
157,184d156
<       <groupId>org.antlr</groupId>
<       <artifactId>antlr4</artifactId>
<       <version>4.7</version>
<       <scope>compile</scope>
<       <exclusions>
<         <exclusion>
<           <groupId>com.google.guava</groupId>
<           <artifactId>guava-jdk5</artifactId>
<         </exclusion>
<         <exclusion>
<           <groupId>jdk.tools</groupId>
<           <artifactId>jdk.tools</artifactId>
<         </exclusion>
<         <exclusion>
<           <groupId>com.google.protobuf</groupId>
<           <artifactId>protobuf-lite</artifactId>
<         </exclusion>
<         <exclusion>
<           <groupId>org.hamcrest</groupId>
<           <artifactId>hamcrest-all</artifactId>
<         </exclusion>
<         <exclusion>
<           <groupId>org.mockito</groupId>
<           <artifactId>mockito-all</artifactId>
<         </exclusion>
<       </exclusions>
<     </dependency>
<     <dependency>

@iemejia
Copy link
Member

iemejia commented Apr 10, 2019

Oh I understood now, thanks @adude3141. Gradle has a different view of dependencies and generated pom. Seems so error-prone, but well then your fix is ok.

Copy link
Member

@iemejia iemejia left a comment

Choose a reason for hiding this comment

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

LGTM

@iemejia iemejia merged commit cc698bc into apache:master Apr 10, 2019
apilloud added a commit that referenced this pull request Apr 12, 2019
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.

2 participants