[BEAM-4584] Migrate build_rules.gradle to buildSrc plugin by kennknowles · Pull Request #5683 · apache/beam

kennknowles · 2018-06-19T03:37:31Z

This is a direct move of the code from build_rules.gradle to a binary plugin in the buildSrc directory. A proxy build_rules.gradle is left in place, but should be migrated away from on a module-by-module basis.

I deliberately did not do anything to refactor the logic. Making this a plugin might make it more natural to do refactors later. Personally, I think each "nature" is probably a natural choice for a separate plugin, and it should be very straightforward to do that.

NOTE: All I did was ensure that ./gradlew build ran. There are some failures (prior description underestimated them due to masking). I didn't yet run the publishing bits that are hidden behind conditionals. Those likely still need some project. added, etc. Just putting this up for early review.

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.

R: @lukecwik
CC: @yifanzou @alanmyrvold @pabloem

kennknowles · 2018-06-19T04:04:35Z

Yea, I'm pretty certain I've introduced an infinite loop w/ allocation here.

kennknowles · 2018-06-19T12:50:40Z

Hmm, well Jenkins passed without any odd behavior though I had some unhappiness locally with heap space. It could be that I have simply never run a full ./gradlew build locally before, but always/only build modules I am editing.

Java - 34 minutes
Python - 18 minutes
Go - 3 minutes

kennknowles · 2018-06-19T16:02:27Z

OK I did some more fixups until publishToMavenLocal ran. I had to provide -Ppublishing which seems redundant with just naming the task. If I set -PisRelease then I get gpg errors but I think that is just me.

lukecwik · 2018-06-19T17:14:07Z

The publishToMavenLocal task only exists if the publishing plugin is applied and it is disabled by default because enabling publishing/signing prevents us from running parallel builds.

The gpg errors could because you have not setup a gpg agent correctly. Likely that your signing key requires a password and you haven't setup something to intercept the password request and prompt you for it or don't have a signing key.

kennknowles · 2018-06-19T17:31:02Z

Can the task exist but not be part of :build? It still messes up parallel builds?

kennknowles · 2018-06-19T17:35:24Z

(anyhow yea I probably don't have gpg set up right)

lukecwik · 2018-06-19T17:36:21Z

To my knowledge, it did still break it for some reason. Not enough investigation was done though and disabling parallel was the easiest. With gradle 4.8, they added support for signing so upgrading may allow us to get rid of the broken plugins.

kennknowles · 2018-06-20T03:54:03Z

run java postcommit

kennknowles · 2018-06-20T03:54:11Z

run dataflow postrelease

kennknowles · 2018-06-20T03:55:36Z

So, we have a job we can trigger "Run Gradle Publish" but that seems like something that should not exist. I guess if it is just a snapshot that will be wiped out it could be useful for PRs just like this one. But anyhow PTAL as I think this is pretty much alright and it is better to get it moving along as soon as possible after 2.5.0

lukecwik

The tabs are really annoying

lukecwik · 2018-06-20T17:38:53Z

buildSrc/build.gradle

+
+// Plugins for configuring _this build_ of the module
+plugins {
+	id 'groovy'


nit: please use spaces instead of tabs

Agreed. Gross. Done. Noting that this is all autoformat.

lukecwik · 2018-06-20T17:40:06Z

buildSrc/src/main/groovy/org/apache/beam/gradle/BeamModulePlugin.groovy

+
+/** This plugin adds methods to configure a module with Beam's defaults, called "natures".
+ *
+ * The natures available:


nit: add <p> tag

lukecwik · 2018-06-20T17:41:47Z

buildSrc/src/main/groovy/org/apache/beam/gradle/BeamModulePlugin.groovy

+	// A class defining the set of configurable properties accepted by applyJavaNature
+	class JavaNatureConfiguration {
+		double javaVersion = 1.8
+		// Controls the JDK source language and target compatibility


move comments to the line before instead of after

lukecwik · 2018-06-20T17:44:02Z

buildSrc/src/main/groovy/org/apache/beam/gradle/BeamModulePlugin.groovy

+
+	// A class defining the set of configurable properties accepted by containerImageName.
+	class ContainerImageNameConfiguration {
+		String root = null // Sets the docker repository root (optional).


nit: null is the default already so we don't need to explicitly set it

I like it explicit, but also this is a non-diff from build_rules.gradle.

Its just noise since groovy/java developers know what the default values for unassigned variables are.

lukecwik · 2018-06-20T17:48:25Z

buildSrc/src/main/groovy/org/apache/beam/gradle/BeamModulePlugin.groovy

+		/** ***********************************************************************************************/
+		// Apply common properties/repositories and tasks to all projects.
+
+		project.group = 'org.apache.beam'


In a follow-up PR consider using

project.with { group = 'org.apache.beam' ... }

Acknowledged

lukecwik · 2018-06-20T17:54:38Z

Only thing that is worth investigating is the

Could not determine the dependencies of task ':release:runJavaExamplesValidationTask'.
Task with path ':beam-runners-google-cloud-dataflow-java:runQuickstartJavaDataflow' not found in project ':release'.

in the Jenkins: ./gradlew :release:runJavaExamplesValidationTask

kennknowles · 2018-06-20T18:35:55Z

It looks like runQuickstartJavaDataflow is a generated name. It also looks like it would be just as easy to pass it in to the place it is generated. It is indeed generated (log lines show) but not available as a task. So there's something wonky about that particular method.

kennknowles · 2018-06-20T19:04:37Z

Good catch. The issue was that I had changed

task foo(type: X, ...)

into

project.task(name: 'foo', type: X, ...)

The whole first arg was a map that was being toStringed (or in some cases a map having a closure wrapping it being toStringed). The correct syntax is:

project.task('foo', type: X, ...)

And that is why we like types. I wonder if we can make this Kotlin without too much pain.

kennknowles · 2018-06-21T15:26:00Z

Checkstyle failure in Flink runner. I'll update and merge the new build_rules changes in.

kennknowles · 2018-06-21T19:39:32Z

Precommit failure due to Dataflow quota. I have tested publishToMavenLocal and a full build locally. I'll see about the DF issue.

kennknowles · 2018-06-21T19:46:47Z

run java precommit

kennknowles · 2018-06-21T22:00:11Z

We've established that Dataflow is having an outage for the apache-beam-testing project. We will sickbay those tests and move to fix.

alanmyrvold · 2018-06-22T16:37:53Z

I now see errors like:
A problem occurred evaluating project ':beam-sdks-go-container'.

No such property: rootProject for class: org.apache.beam.gradle.BeamModulePlugin

A problem occurred evaluating project ':beam-sdks-python-container'.

No such property: rootProject for class: org.apache.beam.gradle.BeamModulePlugin

This is causing failures in https://builds.apache.org/job/beam_PostCommit_Go_GradleBuild and https://builds.apache.org/job/beam_PostCommit_Py_ValCont

@kennknowles Can you take a look?

https://issues.apache.org/jira/browse/BEAM-4623

lukecwik · 2018-07-23T17:28:40Z

This broke the updateOfflineRepository task with Caused by: org.gradle.internal.typeconversion.UnsupportedNotationException: Cannot convert the provided notation to a File or URI: []. More details on https://issues.apache.org/jira/browse/BEAM-4846

…e to buildSrc plugin

kennknowles requested a review from lukecwik June 19, 2018 03:37

kennknowles force-pushed the buildSrc branch from 36286d4 to fa874c9 Compare June 19, 2018 16:15

lukecwik approved these changes Jun 20, 2018

View reviewed changes

kennknowles force-pushed the buildSrc branch from fa874c9 to 9e44c64 Compare June 20, 2018 18:23

kennknowles force-pushed the buildSrc branch 3 times, most recently from bef6791 to 59ef085 Compare June 20, 2018 19:33

kennknowles force-pushed the buildSrc branch from 59ef085 to f53edc4 Compare June 21, 2018 16:21

Migrate build_rules.gradle to buildSrc plugin

220b180

kennknowles force-pushed the buildSrc branch from f53edc4 to 220b180 Compare June 21, 2018 17:59

kennknowles merged commit 16748b1 into apache:master Jun 21, 2018

kennknowles deleted the buildSrc branch July 3, 2018 21:56

charlesccychen pushed a commit to charlesccychen/beam that referenced this pull request Jul 24, 2018

Merge pull request apache#5683: [BEAM-4584] Migrate build_rules.gradl…

2e2f2f2

…e to buildSrc plugin

Conversation

kennknowles commented Jun 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kennknowles commented Jun 19, 2018

Uh oh!

kennknowles commented Jun 19, 2018

Uh oh!

kennknowles commented Jun 19, 2018

Uh oh!

lukecwik commented Jun 19, 2018

Uh oh!

kennknowles commented Jun 19, 2018

Uh oh!

kennknowles commented Jun 19, 2018

Uh oh!

lukecwik commented Jun 19, 2018

Uh oh!

kennknowles commented Jun 20, 2018

Uh oh!

kennknowles commented Jun 20, 2018

Uh oh!

kennknowles commented Jun 20, 2018

Uh oh!

lukecwik left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lukecwik commented Jun 20, 2018

Uh oh!

kennknowles commented Jun 20, 2018

Uh oh!

kennknowles commented Jun 20, 2018

Uh oh!

kennknowles commented Jun 21, 2018

Uh oh!

kennknowles commented Jun 21, 2018

Uh oh!

kennknowles commented Jun 21, 2018

Uh oh!

kennknowles commented Jun 21, 2018

Uh oh!

alanmyrvold commented Jun 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lukecwik commented Jul 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

kennknowles commented Jun 19, 2018 •

edited

Loading

alanmyrvold commented Jun 22, 2018 •

edited

Loading

lukecwik commented Jul 23, 2018 •

edited

Loading