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

android-interop-testing/examples: upgrade android plugin #5525

Merged
merged 8 commits into from
Apr 2, 2019

Conversation

dapengzhang0
Copy link
Member

@dapengzhang0 dapengzhang0 commented Apr 1, 2019

This resolves #5523

While bumping com.android.tools.build:gradle:3.1.2 to 3.3.0, some other plugins/artifacts/maven repo/buildscripts have to be updated:

  • gradle (wrapper) need to upgrade to 4.10.x
  • protobuf gradle plugin need to bump a version compatible with gradle version.
  • need add google() and jcenter() repos for android (otherwise com.android.tools.build:aapt2:3.3.0x and trove4j will not be found resp.)
  • need to accept license for Android "build-tools;28.0.3" in kokoro env.

Copy link
Member

@ejona86 ejona86 left a comment

Choose a reason for hiding this comment

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

It looks like my #5526 is a bit more complete. Sorry for the duplicated work...

@@ -36,6 +36,8 @@ android {
}

repositories {
google()
Copy link
Member

Choose a reason for hiding this comment

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

Why were these added?

Copy link
Member Author

Choose a reason for hiding this comment

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

I had a build error "Could not find com.android.tools.build:aapt2:3.3.0-5013011".

Copy link
Member

Choose a reason for hiding this comment

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

That explains google, but why jcenter?

Copy link
Member Author

Choose a reason for hiding this comment

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

After fixing "Could not find com.android.tools.build:aapt2:3.3.0-5013011" by adding google(), there is another artifact missing Could not find org.jetbrains.trove4j:trove4j.

Copy link
Member

Choose a reason for hiding this comment

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

I see. Then go ahead and remove mavenCentral(), as it wouldn't be needed if jcenter() is available (and earlier in the list).

Copy link
Member

Choose a reason for hiding this comment

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

Hmmm... I'm not as sure if we want to remove mavenCentral() in the examples though, since there's been problems with jcenter not having new grpc releases. Meh. I guess you can leave them both.

Copy link
Member Author

Choose a reason for hiding this comment

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

The problems with jcenter not having new grpc releases becomes a problem now: Android from now on need jcenter.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, I see, having both jcenter (for trove4j) and mavenCentral (for grpc new release).

@@ -15,6 +15,9 @@ export LDFLAGS=-L/tmp/protobuf/lib
export CXXFLAGS=-I/tmp/protobuf/include
export LD_LIBRARY_PATH=/tmp/protobuf/lib
export OS_NAME=$(uname)
export ANDROID_SDK_HOME=/opt/android-sdk/current
Copy link
Member

Choose a reason for hiding this comment

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

ANDROID_HOME is already defined with this value.

Copy link
Member Author

Choose a reason for hiding this comment

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

Is ANDROID_HOME already defined for both android and android-interop-test in kokoru env?

Copy link
Member

Choose a reason for hiding this comment

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

Kokoro defines ANDROID_HOME. I think the android gradle plugin itself requires ANDROID_HOME to be set.

@@ -15,6 +15,9 @@ export LDFLAGS=-L/tmp/protobuf/lib
export CXXFLAGS=-I/tmp/protobuf/include
export LD_LIBRARY_PATH=/tmp/protobuf/lib
export OS_NAME=$(uname)
export ANDROID_SDK_HOME=/opt/android-sdk/current

echo y | ${ANDROID_SDK_HOME}/tools/bin/sdkmanager "build-tools;28.0.3"
Copy link
Member

Choose a reason for hiding this comment

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

Hmm... this will actually install this particular version. I sort of question if it would be better to drop the right MD5s into licenses/android-sdk-license

Copy link
Member Author

Choose a reason for hiding this comment

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

linux-artifacts is also failing. The same issue. http://g/kokoro-users/_P4EAOWi-ko/PRFQvowPDgAJ

Copy link
Member Author

Choose a reason for hiding this comment

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

I sort of question if it would be better to drop the right MD5s into licenses/android-sdk-license

I don't understand how to do that/ how that works.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm... it seems the license handling is done differently these days. Previously after you accepted a licenses you could get any of the binaries with that license. But that doesn't seem to work any more. So this seems fine.

@dapengzhang0
Copy link
Member Author

Now Android Interop is passing in kokoro.

@@ -1,5 +1,5 @@
distributionBase=GRADLE_USER_HOME
distributionPath=wrapper/dists
distributionUrl=https\://services.gradle.org/distributions/gradle-4.9-bin.zip
distributionUrl=https\://services.gradle.org/distributions/gradle-4.10.1-all.zip
Copy link
Member

Choose a reason for hiding this comment

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

-bin (to match the others, and because it is smaller)

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

Copy link
Member

@ejona86 ejona86 left a comment

Choose a reason for hiding this comment

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

You'll need to update the subject/description of the commit.

@dapengzhang0 dapengzhang0 merged commit 755a227 into master Apr 2, 2019
@dapengzhang0 dapengzhang0 deleted the dapengzhang0-patch-1 branch April 2, 2019 22:19
@lock lock bot locked as resolved and limited conversation to collaborators Jul 2, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Android Interopt Test is breaking due to mockito upgrade
2 participants