-
Notifications
You must be signed in to change notification settings - Fork 274
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
Update for Android Gradle plugin 2.5 #121
Update for Android Gradle plugin 2.5 #121
Conversation
61f66d3
to
84b420b
Compare
84b420b
to
16b199a
Compare
Thanks @rschiu, this let me build with Android Studio 3.0 Canary 1 / Gradle 4.0-milestone-1 / build tools 3.0.0-alpha1. Hopefully this will make it in a 0.8.2 released soon? :) (CC @zhangkun83) |
Why this PR still has not been merged almost a month later? My Android project is dying to try the new Android Studio! |
I am still waiting for @rschiu to post the latest changes to this PR. |
16b199a
to
753be10
Compare
looks like it's rebased now 🙇 |
def extractIncludeProtosTask = maybeAddExtractIncludeProtosTask(sourceSetName) | ||
// Extract proto file from the compile classpath configuration exposed in Android Gradle plugin 2.5. Revert to | ||
// original behavior if the configuration is not found. | ||
Configuration compileConfiguration = variant.hasProperty("compileConfiguration") ? variant.compileConfiguration : null |
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 compileConfiguration
is not used in the else-branch, why not:
if (variant.hasProperty("compileConfiguration")) {
...
variant.compileConfiguration.incoming.artifactView{ ...
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
maybeAddExtractIncludeProtosTask( | ||
variant.name, | ||
compileConfiguration.incoming.artifactView{ attributes{ it.attribute(artifactType, "jar") }}.files, | ||
testedCompileConfiguration == null ? |
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.
Same here:
variant.hasProperty("testedVariant") ? variant.testedVariant.compileConfiguration.incoming ... : null
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
@@ -3,4 +3,4 @@ distributionBase=GRADLE_USER_HOME | |||
distributionPath=wrapper/dists | |||
zipStoreBase=GRADLE_USER_HOME | |||
zipStorePath=wrapper/dists | |||
distributionUrl=https\://services.gradle.org/distributions/gradle-3.0-all.zip | |||
distributionUrl=https\://services.gradle.org/distributions/gradle-4.0-rc-3-all.zip |
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.
Please add 4.0-rc-3
to ProtobufJavaPluginTest.gradleVersions
and ProtobufAndroidPluginTest
.
// Extract proto file from the compile classpath configuration exposed in Android Gradle plugin 2.5. Revert to | ||
// original behavior if the configuration is not found. | ||
Configuration compileConfiguration = variant.hasProperty("compileConfiguration") ? variant.compileConfiguration : null | ||
if (compileConfiguration != null) { |
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.
protobuf-gradle-plugin/testProjectAndroid/build.gradle
is still using com.android.tools.build:gradle:2.2.0-beta1
. This new branch is not tested. You will need some work on the test code (ProtobufAndroidPluginTest
and its friends) to test both the old and the new Android plugins.
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
Use compileClasspath Configuration for each variant instead of the compile Configuration for the source sets for the extract include proto tasks.
753be10
to
c5b0bdb
Compare
@zhangkun83 Sorry, I didn't realized you did the review last week. I made the changes. PTAL. |
@rschiu I probably know less about Android SDK than you are. You can select from the SDK UI and install the packages, can't you? |
@rschiu You can now use SDK 26 and build tools |
@rschiu I tested your PR with Android tools installed. It seems
|
|
||
def extractIncludeProtosTask = maybeAddExtractIncludeProtosTask(sourceSetName) | ||
// Extract proto file from the compile classpath configuration exposed in Android Gradle plugin 2.5. Revert to | ||
// original behavior if the configuration is not found. |
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.
Just say the if-branch is for Android plugin 2.5+, and the else-branch is for below 2.5.
There is no "original" behavior outside the context of this PR.
@zhangkun83 How did you run the test to get that build failure? I have updated the tools to 26, but wasn't able to reproduce it. |
Did your run succeed instead?I just installed various SDK packages and ran |
Ya, I re-ran the tests and all runs succeeded. I will dive deeper into it. |
@rschiu Could you provide an example of |
@zhangkun83 I have updated to the latest Android Gradle plugin. Tests are passing on Travis, can you take another look? |
@LouisCAD I am not sure what you mean by a before/after gradle script for the workaround. The change described in the first message in implemented in this PR. |
@rschiu Have you tried testing locally? I keep getting the same failure as mentioned above when testing it locally. |
Yes, the test pass locally as well. |
Hmm, it succeeded on my workstation after I |
I have tried quite a few things, including running my own docker instance, but I am unable to reproduce the error locally. Is anyone else able to do this on their local machine? Any ideas? |
It runs locally for me, I suggest clearing the Travis CI cache and trying to run the build again. $HOME/.gradle may also be too aggressive of a cache anyways. |
Update plugin to alpha7
I assume you would need correct access to do that. Could someone on the protobuf team try clearing the cache? |
I cleared the cache and re-ran the test. It's still broken. |
<< is deprecated by Gradle
This reverts commit cb5ac79.
I was finally able to reproduce the problem with a docker instance limiting the memory. So this seems to be a memory issue. Is it possible to increase the memory size of the build server? |
Currently Travis is allocating 4GB of memory. How much memory did you allocate in your docker instance? I wonder if you need to really increase the memory, or just tune up the java heap size. Anyway, it seems the way to have memory in Travis is to change |
That's kind of odd. Is there anything else that affect the available memory? I reproduced the problem when I limit the memory from the default of 2g to 1.5g. |
Ok, I changed to sudo:required to get VM with larger memory and it's working now. The tests ran successfully twice in a row, the previous failure was due to codenarc. PTAL |
LGTM |
Use compileClasspath Configuration for each variant instead of the compile Configuration for the source sets for the extract include proto tasks.
Support for Android plugin 2.5+ was added in google#121. While it made extractIncludeProtosTask to copy the protos to a different destination with Android plugin 2.5+ than with older verisons, it didn't make generateProtoTask to "include" the new location. This causes Android tests to fail with Gradle 5.0 and Android plugin 3.1.0. The bug has remained undetected because test was never run with Android 2.5+.
Use compileClasspath Configuration for each variant instead
of the compile Configuration for the source sets for the
extract include proto tasks.