buildscripts: Add Kokoro-based CI for Android APK stats #3984

ericgribkoff · 2018-01-18T08:35:44Z

~~Internal change is cl/182328214.~~

~~The first commit just pulls in some prerequisite build updates from #3975 to allow the gradle dexcount and apksize plugins to work.~~

Edit: now incorporating the APK stat diffs into the existing cronet kokoro job, see comment below.

matt-kwong

Just a couple comments for future consideration. The build scripts LGTM

matt-kwong · 2018-01-18T19:20:08Z

buildscripts/kokoro/android-apk-stats.sh

+mkdir -p reports
+echo '<html><head></head><body>' > reports/kokoro_index.html
+echo '<h1>'${KOKORO_JOB_NAME}', build '#${KOKORO_BUILD_NUMBER}'</h1>' >> reports/kokoro_index.html
+echo '<h2><a href="https://kokoro2.corp.google.com/job/'${KOKORO_JOB_PATH}'/'${KOKORO_BUILD_NUMBER}'/">Kokoro build dashboard (internal only)</a></h2>' >> reports/kokoro_index.html


Just a heads up: we might be moved from kokoro2 in the future. Just saying so that if this link stops working, you'll know the reason this link broke the last time for us last time.

matt-kwong · 2018-01-18T19:24:50Z

buildscripts/kokoro/android-apk-stats.sh

+echo '<html><head></head><body>' > reports/kokoro_index.html
+echo '<h1>'${KOKORO_JOB_NAME}', build '#${KOKORO_BUILD_NUMBER}'</h1>' >> reports/kokoro_index.html
+echo '<h2><a href="https://kokoro2.corp.google.com/job/'${KOKORO_JOB_PATH}'/'${KOKORO_BUILD_NUMBER}'/">Kokoro build dashboard (internal only)</a></h2>' >> reports/kokoro_index.html
+echo '<h2><a href="https://sponge.corp.google.com/invocation?id='${KOKORO_BUILD_ID}'&searchFor=">Test result dashboard (internal only)</a></h2>' >> reports/kokoro_index.html


This will likely also be switched to a result storage site that is publicly accessible.

ericgribkoff · 2018-02-22T07:29:27Z

@ejona86 This change is now redone as an addition to our existing Cronet Kokoro build (which I'll rename to simply android.{cfg,sh} in this PR and internally once this is approved, as it now includes our Android examples as well). The APK size and dex count deltas are reported as Github statuses as part of the Android build process.

Edit: The second commit can be ignored and will be dropped before merging. It introduces an artificial change to core that increases the APK size and dex count to demo the new status messages.

ejona86

Didn't finish, but sending what I have.

ejona86 · 2018-03-01T18:46:36Z

buildscripts/kokoro/cronet.sh

+../../../buildscripts/set_github_status.py \
+  --sha1 $KOKORO_GITHUB_PULL_REQUEST_COMMIT \
+  --state success \
+  --description "New APK size in bytes: $new_apk_size (delta: $apk_size_delta)" \


Could we make these easier to read? Probably $(printf "%'d" "$new_apk_size") to add thousands separator would be enough.

ejona86 · 2018-03-01T18:46:56Z

core/src/main/java/io/grpc/internal/ManagedChannelImpl.java

@@ -105,6 +106,8 @@
  static final Status SUBCHANNEL_SHUTDOWN_STATUS =
      Status.UNAVAILABLE.withDescription("Subchannel shutdown invoked");

+  private final Gson gson = new Gson();


Yes - this is a demo commit so the status messages on this PR show non-zero increases.

ejona86 · 2018-03-01T18:49:55Z

buildscripts/kokoro/cronet.sh

+
+# Set up APK size and dex count statuses
+
+gsutil cp gs://grpc-testing-secrets/github_credentials/oauth_token.txt ~/


It seems the code needs to handle KOKORO_GITHUB_PULL_REQUEST_COMMIT being unset. This script can be run on master/release branches.

ejona86 · 2018-03-01T18:53:07Z

buildscripts/kokoro/cronet.sh

@@ -3,6 +3,119 @@
 set -exu -o pipefail
 cat /VERSION

-cd ./github/grpc-java/cronet
+BASE_DIR=$(pwd)


Quote all variables when used. Quote commands (like "$(pwd)").

ejona86 · 2018-03-01T19:01:16Z

buildscripts/kokoro/cronet.sh

+  cd $BASE_DIR/github/grpc-java
+
+  # TODO(ericgribkoff) Remove once merged
+  git checkout $KOKORO_GITHUB_PULL_REQUEST_COMMIT


This seems unfortunate. I don't think it's something I'd want us to remove (rather, I want us to always use the most recent version of the script.) Maybe there's an alternative, like copying the script some place else...

ejona86 · 2018-03-01T21:15:44Z

buildscripts/kokoro/cronet.sh

+
+
+# Install gRPC and codegen for the Android examples
+# (a composite gradle build can't find protoc-gen-grpc-java)


Could this have been caused by d4b11e5 (i.e., it is no longer a problem)? I know I saw the problem earlier, but I don't see it any more.

No, it's still a problem. The Gradle composite build documentation's section on current limitations says "Native builds are not supported. (Binary dependencies are not yet supported for native builds)." Although I'm not entirely sure this is the reason for the failure to find protoc-gen-grpc-java, as I'm not sure it applies to the direction of the dependency on a native build here.

ejona86 · 2018-03-02T01:01:30Z

buildscripts/kokoro/cronet.sh

+# Get the APK size and dex count stats using the target branch
+
+sudo apt-get install -y jq
+target_branch=$(curl -s https://api.github.com/repos/grpc/grpc-java/pulls/$KOKORO_GITHUB_PULL_REQUEST_NUMBER | jq -r .base.ref)


Parsing JSON... that's unfortunate. I wonder if we could look at the merge commit parents. Also, it should probably be .base.sha since the ref may be different from when the clone was done.

Don't we want to compare to the latest master (or whatever branching the PR is targeting), regardless of when they created their branch?

We want to compare against the base this commit was built from. That is fixed at the start of the build. However, master can change over time, even during our build. If there is a merge commit, we could do git rev-parse HEAD^ (or more unlikely HEAD^2, depending on how the merge was done). If there's no merge commit... maybe we could use git merge-base... But I sort of expect to see merge commits.

Done. I was assuming that the base branch (e.g., master) was already in the local repo at the start of the build. That is not always the case, so if the checkout ends up pulling from the remote repository, using the base branch name could indeed end up pulling in changes that didn't exist when the build began.

It turns out that Github already creates the merge commit for you, and that can be used to build and test the PR: it's not quite documentation, but it appears to match reality: https://stackoverflow.com/questions/42626068/how-do-i-list-all-remote-refs

As far I can tell there will never be more than two parents of the merge commit, so we don't need to worry about HEAD^2.

cc @matt-kwong The curl -s https://api.github.com/repos/grpc/grpc-java/pulls/$KOKORO_GITHUB_PULL_REQUEST_NUMBER | jq -r .base.ref approach was copied from what grpc/grpc was doing. It looks like there might be a simpler and more correct alternative in git rev-parse (Also, it is possible to change the target branch for a PR mid-build via Github's web UI, so the value being parsed from the JSON here could be different than what was used for the merge commit)

This reverts commit 927bc75.

ejona86 · 2018-03-07T23:20:28Z

buildscripts/kokoro/cronet.sh

+trap set_status_to_fail_on_error ERR
+
+
+"$SET_GITHUB_STATUS" \


Setting these statuses is pretty late. You've already done a full build, you just have a small incremental build left. These should either be earlier or removed.

Removed. If this fails the main build status will be reported as failure anyway, so the extra statuses for apk size and dex count are unnecessary.

ericgribkoff requested a review from matt-kwong January 18, 2018 08:35

matt-kwong approved these changes Jan 18, 2018

View reviewed changes

ericgribkoff force-pushed the android_apk_kokoro branch 6 times, most recently from 404210a to 5719fde Compare February 22, 2018 00:04

ericgribkoff added the kokoro:force-run Add this label to a PR to tell Kokoro to re-run all tests. Not generally necessary label Feb 22, 2018

kokoro-team removed the kokoro:force-run Add this label to a PR to tell Kokoro to re-run all tests. Not generally necessary label Feb 22, 2018

ericgribkoff force-pushed the android_apk_kokoro branch 20 times, most recently from a62a4f1 to 5a30417 Compare February 22, 2018 07:19

ericgribkoff force-pushed the android_apk_kokoro branch from 5a30417 to 4a8f791 Compare February 22, 2018 07:28

ericgribkoff assigned ejona86 Feb 22, 2018

ericgribkoff force-pushed the android_apk_kokoro branch from 4a8f791 to babed13 Compare February 22, 2018 08:03

ericgribkoff added 2 commits February 22, 2018 14:54

buildscripts: Add Kokoro-based CI for Android APK stats

6e7edff

demo for an increase in method count

927bc75

ericgribkoff force-pushed the android_apk_kokoro branch from babed13 to 927bc75 Compare February 22, 2018 22:55

ericgribkoff requested a review from zpencer March 1, 2018 16:06

ejona86 reviewed Mar 1, 2018

View reviewed changes

ejona86 reviewed Mar 2, 2018

View reviewed changes

ericgribkoff force-pushed the android_apk_kokoro branch 5 times, most recently from 9afb599 to 9849d88 Compare March 7, 2018 00:04

address comments

bbda65b

ericgribkoff force-pushed the android_apk_kokoro branch from 9849d88 to bbda65b Compare March 7, 2018 00:15

ericgribkoff added 2 commits March 6, 2018 16:16

Revert "demo for an increase in method count"

27e9efc

This reverts commit 927bc75.

use git rev-parse

e7f8b1a

ericgribkoff force-pushed the android_apk_kokoro branch from 1c0504b to e7f8b1a Compare March 7, 2018 21:09

use HEAD^ directly

e8d8c96

ericgribkoff force-pushed the android_apk_kokoro branch from 4605090 to e8d8c96 Compare March 7, 2018 21:41

ejona86 approved these changes Mar 7, 2018

View reviewed changes

ericgribkoff added 2 commits March 7, 2018 15:47

remove temporary status messages

4b28497

rename cronet.{cfg,sh} to android.{cfg,sh}

9beab53

ericgribkoff mentioned this pull request Mar 8, 2018

buildscripts: rename cronet buildscripts to android #4193

Merged

ericgribkoff merged commit 4369e8c into grpc:master Mar 9, 2018

ericgribkoff deleted the android_apk_kokoro branch March 9, 2018 02:41

lock bot locked as resolved and limited conversation to collaborators Jan 18, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

buildscripts: Add Kokoro-based CI for Android APK stats #3984

buildscripts: Add Kokoro-based CI for Android APK stats #3984

ericgribkoff commented Jan 18, 2018 •

edited

Loading

matt-kwong left a comment

matt-kwong Jan 18, 2018

matt-kwong Jan 18, 2018

ericgribkoff commented Feb 22, 2018 •

edited

Loading

ejona86 left a comment

ejona86 Mar 1, 2018

ericgribkoff Mar 6, 2018

ejona86 Mar 1, 2018

ericgribkoff Mar 6, 2018

ejona86 Mar 1, 2018

ericgribkoff Mar 6, 2018

ejona86 Mar 1, 2018

ericgribkoff Mar 6, 2018

ejona86 Mar 1, 2018

ericgribkoff Mar 6, 2018

ejona86 Mar 1, 2018

ericgribkoff Mar 6, 2018

ejona86 Mar 2, 2018

ericgribkoff Mar 6, 2018

ejona86 Mar 7, 2018

ericgribkoff Mar 7, 2018

ejona86 Mar 7, 2018

ericgribkoff Mar 7, 2018


		# Set up APK size and dex count statuses

		gsutil cp gs://grpc-testing-secrets/github_credentials/oauth_token.txt ~/



		# Install gRPC and codegen for the Android examples
		# (a composite gradle build can't find protoc-gen-grpc-java)

		trap set_status_to_fail_on_error ERR


		"$SET_GITHUB_STATUS" \

buildscripts: Add Kokoro-based CI for Android APK stats #3984

buildscripts: Add Kokoro-based CI for Android APK stats #3984

Conversation

ericgribkoff commented Jan 18, 2018 • edited Loading

matt-kwong left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericgribkoff commented Feb 22, 2018 • edited Loading

ejona86 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericgribkoff commented Jan 18, 2018 •

edited

Loading

ericgribkoff commented Feb 22, 2018 •

edited

Loading