-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Enable building Apache Beam using Gradle #4096
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome. I looked most closely at the global bits and then skimmed about 30 of the individual bits. Lots of questions, and mostly I think it deserves a ton of commenting so everyone can see exactly what is going on.
Even though I have lots of suggestions here, our maven configs are 10x the size and super crufty also, so as far as the choice between them, this is already way more clear and maintainable.
At sdks/java/core/src/test/avro/user.avsc:2:
"namespace": "org.apache.beam.sdk.io",
I see how src/test/avro is better for this than src/test/java but should keep the namespace directories IMO.
ext.applyGrpcNature = { | ||
println "applyGrpcNature with " + (it ? "$it" : "default configuration") + " for project $project.name" | ||
apply plugin: "com.google.protobuf" | ||
protobuf { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do these methods do, exactly?
} | ||
|
||
// Create a new configuration 'shadowTest' like 'shadow' for the test scope | ||
configurations { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This configuration stuff does make me think it would be ideal to have a single shaded dep in its own module, so there's none of this spooky configuration at a distance of all projects in really nontrivial ways.
build_rules.gradle
Outdated
} | ||
} | ||
|
||
if (configuration.enableFindbugs) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why if
? Can it be always on and be a separate target? It would be preferable to manage all aspects of what runs by specifying targets. I know in Gradle these are "tasks" but aren't those about the same?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Applying a plugin typically adds configuration sets, enables tasks to execute. I have an if
here to prevent the plugin from applying for a few modules (google-cloud-dataflow-java, google-cloud-platform, kafka, model/*) mirroring the Maven configuration.
Gradle doesn't follow the Maven plugin style where there is a skip
flag so either you enable the plugin or you don't. Some plugins have the ability to not fail the build if they fail.
build_rules.gradle
Outdated
maxErrors = 0 | ||
} | ||
|
||
apply plugin: 'com.diffplug.gradle.spotless' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here, too, comments about what plugin is being loaded, why, how it is configured.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
build_rules.gradle
Outdated
useJUnit { } | ||
} | ||
|
||
apply plugin: "net.ltgt.apt" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These two lines are close to each other - are they linked by groovy syntax or is it just coincidence?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not certain what your asking, please clarify
// Only one SparkContext may be running in a JVM (SPARK-2243) | ||
forkEvery 1 | ||
useJUnit { | ||
excludeCategories "org.apache.beam.runners.spark.UsesCheckpointRecovery" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where's the bit where it scans tests from sdks/java/core?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are no validates runner tests configured and wanted to make all validates runner tests their own test sets independent of the normal unit tests. This will allow us to have different dependency configurations for the different test sets.
jcenter() | ||
|
||
// Release staging repository | ||
maven { url "https://oss.sonatype.org/content/repositories/staging/" } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should these have names? Or how does the build distinguish which ones to use for which purpose? Etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I believe they are harmless and will only become useful once we start staging artifacts.
build_rules.gradle
Outdated
repositories { | ||
mavenLocal() | ||
mavenCentral() | ||
jcenter() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is jcenter?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bintray
build_rules.gradle
Outdated
|
||
// Define the default set of properties | ||
repositories { | ||
mavenLocal() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, so here is where the actual dep repos are configured, and where I commented in the other place was just where to find Gradle plugin deps?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct
build_rules.gradle
Outdated
* limitations under the License. | ||
*/ | ||
|
||
println "Applying build_rules.gradle to $project.name" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this runs in multiple projects and by this line you can tell when it hits each of them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is to be able to follow during evaluation/configuration what is being applied to what project and when.
This specifically says when a project "imports" build_rules.gradle".
There are others in the applyYYYNature() methods which state when a large configuration is being applied with what settings.
shadow library.java.google_auth_library_oauth2_http | ||
shadow library.java.bigdataoss_util | ||
shadow library.java.avro | ||
shadow library.java.joda_time |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this shading way more than we do today? Or is this not shaded and shadow means something else?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its the opposite of what you would expect since the shadow plugin conveniently by default decides to bundle everything instead of bundling nothing. Marking something with shadow
or the equivalent test one means you want to keep it. Anything in the runtime/testRuntime configurations is meant to be shaded and removed from the final artifact.
shadow library.java.google_http_client | ||
shadow library.java.bigdataoss_util | ||
shadow library.java.google_auth_library_oauth2_http | ||
shadow library.java.google_auth_library_credentials |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the surface, this shadow configuration seems way way better than maven-shade-plugin, where we have to configure things twice.
* limitations under the License. | ||
*/ | ||
|
||
apply from: project(":").file("build_rules.gradle") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like we are not required to blanket apply the root build_rules.gradle
. It might be cleaner to break it up into things that really are across all modules and separately things that are not. For example, checkstyle, findbugs, etc should not even by loaded for building Python, Go, etc. So those configurations would go in a root level java_base.gradle
or some such.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is what build_rules.gradle does already.
It contains configuration that applies to everything and then has a few closures which apply additional configuration like applyJavaNature/applyGrpcNature which projects can optionally call.
|
||
apply from: project(":").file("build_rules.gradle") | ||
applyJavaNature(enableFindbugs: false) | ||
applyGrpcNature() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where are the source files specified? Are they implied? How do we configure the output? Currently, our protos are actually put in the wrong place in the jar, really, and we should namespace them. How can we do that? And can we pull in protos from one module to another without having to pull in a jar? We should be able to build three dependent proto modules all to Python without touching any Java technology.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All great questions, I'm not familiar enough with the proto plugin to know how it does everything but the default configuration + gRPC did what we wanted it to do.
build.gradle
Outdated
classpath "com.diffplug.spotless:spotless-plugin-gradle:3.6.0" | ||
classpath "gradle.plugin.com.github.blindpirate:gogradle:0.7.0" | ||
classpath "gradle.plugin.com.palantir.gradle.docker:gradle-docker:0.13.0" | ||
classpath "cz.malohlava:visteg:1.0.3" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think a few of them are not self-documenting, like this one. It wouldn't hurt to comment each one separately, even obvious ones like protobuf-gradle-plugin.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
maven { url "https://plugins.gradle.org/m2/" } | ||
maven { url "http://repo.spring.io/plugins-release" } | ||
} | ||
dependencies { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are just dependencies for running the build, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes.
build.gradle
Outdated
], | ||
] | ||
|
||
buildscript { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since many people will not have used Gradle, and even those that have will edit builds less often than their main code, can you signpost the significance of sections and commands? Especially commenting on any that affect whole-project things, like this maven config looks like it does.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I commented build_rules.gradle heavily for parts that I had a lot of knowledge in and gave some thought to what made sense.
Also some parts of build.gradle.
build.gradle
Outdated
|
||
apply from: project(":").file("build_rules.gradle") | ||
|
||
def google_cloud_bigdataoss_version = "1.4.5" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could use some documentation about why some are pulled out and some are inlined.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
"**/build/**", | ||
"**/vendor/**", | ||
|
||
// .gitignore: Ignore files generated by the Maven build process |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Incidentally, the plugin might already check .gitignore. It was my mistake thinking that the Maven plugin didn't.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe improving this minor point can happen in the future.
build.gradle
Outdated
classpath "com.google.protobuf:protobuf-gradle-plugin:0.8.1" | ||
classpath "io.spring.gradle:propdeps-plugin:0.0.9.RELEASE" | ||
classpath "gradle.plugin.org.nosphere.apache:creadur-rat-gradle:0.3.1" | ||
classpath "com.commercehub.gradle.plugin:gradle-avro-plugin:0.11.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we isolate plugin dependencies to just the modules that use them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would lead to an inconsistent experience because the dependencies declared with a project’s buildscript() method are available to the build scripts of all its sub-projects and we have a couple of places where go/python/docker intermix (beam-sdks-python-container is a subproject of beam-sdks-python). So I feel that we should load all the plugins everytime so that every project has access to the same set irrespective of where they are in the three.
project(':beam-runners-parent:beam-runners-reference-parent:beam-runners-reference-job-orchestrator').projectDir = "$rootDir/runners/reference/job-server" as File | ||
project(':beam-runners-parent:beam-runners-reference-parent').projectDir = "$rootDir/runners/reference" as File | ||
project(':beam-runners-parent:beam-runners-direct-java').projectDir = "$rootDir/runners/direct-java" as File | ||
project(':beam-runners-parent:beam-runners-flink_2.10').projectDir = "$rootDir/runners/flink" as File |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fact that we have to set this looks pretty suspicious. Probably it means we are just reaching into other project directories when we should depend on declared outputs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To my knowledge setting the projectDir is how you tell gradle where subprojects are located.
@@ -16,7 +16,7 @@ | |||
# | |||
# SDK source version | |||
|
|||
version=${pom.version} | |||
version=@pom.version@ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto here that we might not make this change unless maven and gradle both support the same syntax. I'm sure we could find a way. But maybe this just works for both?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most of our other filter plugin changes that we have done have migrated to using @
as the separator instead of $
, see the archetypes.
legacy.environment.major.version=${dataflow.legacy_environment_major_version} | ||
fnapi.environment.major.version=${dataflow.fnapi_environment_major_version} | ||
container.version=${dataflow.container_version} | ||
[email protected]_environment_major_version@ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change will make it so mvn and gradle cannot coexist?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maven supports $
and @
syntax. Most of our project actually uses @
, see the archetypes.
This brings these parts inline and enables them to be used without additional configuration of the gradle filter plugin.
|
||
############################################################################## | ||
## | ||
## Gradle start up script for UN*X |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we have this or need it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its the recommended way for users to use the build without needing to install the version of gradle that this build expects (4.2.1). Its also the recommended approach when using Jenkins. Gradle will automatically download its own version and execute itself via the gradlew script. Ditto for the .bat
file for windows users.
For developers who only develop on this project, they can install Gradle themselves but having this script removes the need for them to do so.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At sdks/java/core/src/test/avro/user.avsc:2:
"namespace": "org.apache.beam.sdk.io",
I see how src/test/avro is better for this than src/test/java but should keep the namespace directories IMO.
// Only one SparkContext may be running in a JVM (SPARK-2243) | ||
forkEvery 1 | ||
useJUnit { | ||
excludeCategories "org.apache.beam.runners.spark.UsesCheckpointRecovery" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where's the bit where it scans tests from sdks/java/core?
build_rules.gradle
Outdated
|
||
apply plugin: "net.ltgt.apt" | ||
dependencies { | ||
def auto_value = "com.google.auto.value:auto-value:1.5.1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These don't go in the other toplevel file?
build_rules.gradle
Outdated
useJUnit { } | ||
} | ||
|
||
apply plugin: "net.ltgt.apt" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These two lines are close to each other - are they linked by groovy syntax or is it just coincidence?
build_rules.gradle
Outdated
maxErrors = 0 | ||
} | ||
|
||
apply plugin: 'com.diffplug.gradle.spotless' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here, too, comments about what plugin is being loaded, why, how it is configured.
build_rules.gradle
Outdated
} | ||
} | ||
|
||
if (configuration.enableFindbugs) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why if
? Can it be always on and be a separate target? It would be preferable to manage all aspects of what runs by specifying targets. I know in Gradle these are "tasks" but aren't those about the same?
build_rules.gradle
Outdated
* limitations under the License. | ||
*/ | ||
|
||
println "Applying build_rules.gradle to $project.name" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this runs in multiple projects and by this line you can tell when it hits each of them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes
jcenter() | ||
|
||
// Release staging repository | ||
maven { url "https://oss.sonatype.org/content/repositories/staging/" } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should these have names? Or how does the build distinguish which ones to use for which purpose? Etc.
build_rules.gradle
Outdated
repositories { | ||
mavenLocal() | ||
mavenCentral() | ||
jcenter() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is jcenter?
build_rules.gradle
Outdated
apply plugin: "java" | ||
|
||
sourceCompatibility = configuration.javaVersion | ||
targetCompatibility = configuration.javaVersion |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know that there exist third party tools to compile lambdas to Java 7 bytecode. Do they perhaps have easy gradle plugins so you can just say sourceCompatibility=1.8 targetCompatibility=1.7? Might be a sneaky way to get started.
build_rules.gradle
Outdated
/*************************************************************************************************/ | ||
|
||
/** Used as configuration within the applyJavaNature closure. */ | ||
class JavaNatureConfiguration { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be clear, this is our own POJO and the fields are just for us to go ahead and use? Or are these magic fields that some plugin looks at?
shadow library.java.google_auth_library_oauth2_http | ||
shadow library.java.bigdataoss_util | ||
shadow library.java.avro | ||
shadow library.java.joda_time |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this shading way more than we do today? Or is this not shaded and shadow means something else?
shadow library.java.google_http_client | ||
shadow library.java.bigdataoss_util | ||
shadow library.java.google_auth_library_oauth2_http | ||
shadow library.java.google_auth_library_credentials |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the surface, this shadow configuration seems way way better than maven-shade-plugin, where we have to configure things twice.
* limitations under the License. | ||
*/ | ||
|
||
apply from: project(":").file("build_rules.gradle") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like we are not required to blanket apply the root build_rules.gradle
. It might be cleaner to break it up into things that really are across all modules and separately things that are not. For example, checkstyle, findbugs, etc should not even by loaded for building Python, Go, etc. So those configurations would go in a root level java_base.gradle
or some such.
|
||
apply from: project(":").file("build_rules.gradle") | ||
applyJavaNature(enableFindbugs: false) | ||
applyGrpcNature() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where are the source files specified? Are they implied? How do we configure the output? Currently, our protos are actually put in the wrong place in the jar, really, and we should namespace them. How can we do that? And can we pull in protos from one module to another without having to pull in a jar? We should be able to build three dependent proto modules all to Python without touching any Java technology.
build.gradle
Outdated
|
||
apply from: project(":").file("build_rules.gradle") | ||
|
||
def google_cloud_bigdataoss_version = "1.4.5" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could use some documentation about why some are pulled out and some are inlined.
build.gradle
Outdated
], | ||
] | ||
|
||
buildscript { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since many people will not have used Gradle, and even those that have will edit builds less often than their main code, can you signpost the significance of sections and commands? Especially commenting on any that affect whole-project things, like this maven config looks like it does.
build.gradle
Outdated
classpath "com.diffplug.spotless:spotless-plugin-gradle:3.6.0" | ||
classpath "gradle.plugin.com.github.blindpirate:gogradle:0.7.0" | ||
classpath "gradle.plugin.com.palantir.gradle.docker:gradle-docker:0.13.0" | ||
classpath "cz.malohlava:visteg:1.0.3" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think a few of them are not self-documenting, like this one. It wouldn't hurt to comment each one separately, even obvious ones like protobuf-gradle-plugin.
maven { url "https://plugins.gradle.org/m2/" } | ||
maven { url "http://repo.spring.io/plugins-release" } | ||
} | ||
dependencies { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are just dependencies for running the build, right?
"**/build/**", | ||
"**/vendor/**", | ||
|
||
// .gitignore: Ignore files generated by the Maven build process |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Incidentally, the plugin might already check .gitignore. It was my mistake thinking that the Maven plugin didn't.
build.gradle
Outdated
classpath "com.google.protobuf:protobuf-gradle-plugin:0.8.1" | ||
classpath "io.spring.gradle:propdeps-plugin:0.0.9.RELEASE" | ||
classpath "gradle.plugin.org.nosphere.apache:creadur-rat-gradle:0.3.1" | ||
classpath "com.commercehub.gradle.plugin:gradle-avro-plugin:0.11.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we isolate plugin dependencies to just the modules that use them?
project(':beam-runners-parent:beam-runners-reference-parent:beam-runners-reference-job-orchestrator').projectDir = "$rootDir/runners/reference/job-server" as File | ||
project(':beam-runners-parent:beam-runners-reference-parent').projectDir = "$rootDir/runners/reference" as File | ||
project(':beam-runners-parent:beam-runners-direct-java').projectDir = "$rootDir/runners/direct-java" as File | ||
project(':beam-runners-parent:beam-runners-flink_2.10').projectDir = "$rootDir/runners/flink" as File |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fact that we have to set this looks pretty suspicious. Probably it means we are just reaching into other project directories when we should depend on declared outputs?
@@ -16,7 +16,7 @@ | |||
# | |||
# SDK source version | |||
|
|||
version=${pom.version} | |||
version=@pom.version@ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto here that we might not make this change unless maven and gradle both support the same syntax. I'm sure we could find a way. But maybe this just works for both?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are there tests of what this is expected to be?
legacy.environment.major.version=${dataflow.legacy_environment_major_version} | ||
fnapi.environment.major.version=${dataflow.fnapi_environment_major_version} | ||
container.version=${dataflow.container_version} | ||
[email protected]_environment_major_version@ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change will make it so mvn and gradle cannot coexist?
|
||
############################################################################## | ||
## | ||
## Gradle start up script for UN*X |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we have this or need it?
Do we have an equivalent to the enforcer plugin ? By the way, I'm testing the build right now. |
FYI, I have a build failure with Gradle 4.3:
due to:
|
Gradle 4.2.x fails on rat and python:
On the other hand, I will take a look for the Gradle 4.3 support. |
build.gradle
Outdated
maven { url "http://repo.spring.io/plugins-release" } | ||
} | ||
dependencies { | ||
classpath "net.ltgt.gradle:gradle-apt-plugin:0.12" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one need to be version 0.13 or it breaks with the latest version of gradle (4.3)
@iemejia @jbonofre @jbonofre |
All builds passed: Following changes are only going to be for Gradle build files so we know that the current set of changes don't break the Maven based build. |
OK. The only changes I could imagine messing with maven would be the move from |
run dataflow validatesrunner |
We have some basic unit tests for the property rules in: https://github.com/apache/beam/blob/master/sdks/java/core/src/test/java/org/apache/beam/sdk/util/ReleaseInfoTest.java |
Dataflow validates runner tests also passed: |
PTAL |
R: @kennknowles |
Testing the latest changes. Thanks ! |
LGTM but I think there have been a couple things that changed since the tests last ran. |
Resolve a spurious error that gogradle was not adding a task dependency between resolving build dependencies and building the dependency
…ependencies that changed
… that Dockerfile expects
…ts to use it. Add shadowTest configuration for sdks/java/core updating all other projects to use it.
Update repackaged locations to always use org.apache.beam.${project.name}.repackaged as prefix
Fix whitespace formatting.
…th any other go project Fix up evaluation message.
Fix Spark runners tests by excluding slf4j-jdk14 from test runtime classpath
Conflicts: build_rules.gradle
This was working in the past because http://creadur.apache.org/rat/apache-rat-plugin/check-mojo.html#useIdeaDefaultExcludes is true.
…aster branch up to f10399d
I got this merged and updated the build changes so the gradle build is upto sync with all pom changes up to current master. It currently passes on my desktop. |
SUCCESS --none-- |
Follow this checklist to help us incorporate your contribution quickly and easily:
[BEAM-XXX] Fixes bug in ApproximateQuantiles
, where you replaceBEAM-XXX
with the appropriate JIRA issue.mvn clean verify
to make sure basic checks pass. A more thorough check will be performed on your pull request automatically.Things that work:
Partially working:
Things that aren't done:
@NeedsRunner
/@ValidatesRunner
/integration tests