Detect isAndroidProject based on plugin IDs #237

trevjonez · 2018-06-07T08:11:01Z

should fix #236

Gradle wrapper to latest as android gradle 3.1.2 needs at minimum 4.4 Test cases to show false positive no longer occurs when parent project has an android extension

trevjonez · 2018-06-07T17:54:14Z

Looks like code narc is real upset with a bunch of files I didn't touch as well as with some that I did. My attempt was to hand match the style based on what was in the Utils class already. Reformatting the classes just to appease the travis script seems like a commit that should be external to this bug fix PR. What is the right way to proceed with this?

zpencer · 2018-06-08T16:48:55Z

I downgraded the gradle wrapper from 4.8 back to 4.3 and most of the errors went away. I guess the new version of gradle comes with more strict rules. It did find a few lints in the one of the added files though. Let's downgrade for now.

trevjonez · 2018-06-08T20:43:40Z

Glad you thought of that, didn't even cross my mind as a variable. I'll try to get this all touched up tonight.

codenarc fixes for UtilsTest.groovy

trevjonez · 2018-06-10T16:38:15Z

So interesting thing. I can't even run the tests to completion locally unless I upgrade the gradle wrapper. I don't get any errors but it hangs after about 5 tests have ran. I am wondering if the current test suite shouldn't be reworked to minimize the time required? The majority of the time spent isn't even running anything related to this plugin. It should be possible to verify the critical plugin behaviors by analyzing the project and task graph after configuration phase without actually spending all the time required to run the project builds to completion?

zpencer · 2018-06-11T18:17:32Z

src/test/groovy/com/google/protobuf/gradle/UtilsTest.groovy

+    assert isAndroidProject
+  }
+
+  @Requires({ Version.ANDROID_GRADLE_PLUGIN_VERSION.startsWith("3.") })


Should this be for only 3.x or for 3.x and up?

project.apply plugin:'com.android.base' will only work from 3.x and up. they add that marker plugin for this exact use case. My thought is that when you can safely abandon pre 3.0 support that case would be all that is required. which is why I have it as the first thing the isAndroidProject checks for.

The problem though is this test will fail with a no such plugin error when the tests are built/ran with 2.+ plugin versions.

I'm a bit concerned about the string matching here for the "and up" part of "3.x and up". Should we adapt Utils.compareGradleVersion to be able to check any version string?

Yea if you think it is worth while I would be happy to make the change. Was just trying to get a guard in there fast so that if someone needed to debug against 2.3.0 they still could.

zpencer · 2018-06-11T19:47:00Z

build.gradle

@@ -82,7 +83,7 @@ dependencies {
    // Add android plugin to the test classpath, so that we can jump into class definitions,
    // read their sources, set break points, etc while debugging android unit tests.
    // Change the version if necessary to match the specific unit test.
-    testRuntime 'com.android.tools.build:gradle:2.3.0'
+    testCompile 'com.android.tools.build:gradle:3.1.2'


I think if we follow the pattern in the existing android tests we can construct a project with an android version on a per test project basis. Then we can validate that the new code does not cause problems for android plugin 2.x.

So if I follow correctly you are saying we take and create external projects at different android gradle plugin version to validate the utils class implementation?(similar to the way the other test files work)

It will be a decent bit more code and add some runtime overhead of the external gradle processes but should be the most robust way to validate backwards compat.

Yes, validating backwards compat is a requirement for all versions we support. Maybe we some point we would drop support for 2.x but for now I think we should validate all new code for 2.x versions.

I think the tests would probably end up looking something like BuildLogicFunctionalTest.java

I've done tests that way in some other projects I was just trying to be too lazy here. I'll go revamp these tests to use the external run api's which should get rid of the need for the version check Require on the 3.+ test as well.

…cess.

trevjonez · 2018-06-13T06:21:42Z

ok hopefully that will get through travis without hanging. I want to say I really dislike how hacky the current test project classpath solution is. it doesn't allow gradle to resolve dependencies the way it does in a production environment.

I don't know if it will work cleanly with travis but usually what I do is have the test task depend on the maven install task so that my test projects can reference the mavenLocal() in order to resolve the plugin under test as well as getting the normal pom file list of dependencies.

so fwiw I don't like what I had to do in ProtobufPluginTestHelper to substitute guava but can't change the project to use guava 22 because of the backwards compat concerns.

I would love to help refactor the testing setup classpath config to be more straight forward but I don't think it is appropriate for the scope of this PR.

here is an example of what that looks like:
https://github.com/trevjonez/Kontrast/blob/master/plugin/build.gradle

trevjonez · 2018-06-13T07:22:09Z

any hints on how to get the tests to not hang? If I run locally with the gradle wrapper it will deadlock, if I run with local install of 4.8 they run and pass in under 8 minutes

For unknown reasons, the unit tests hang for Gradle 4.3 on this PR on Travis: google#237 I have confirmed that the code works fine on 4.8.

zpencer · 2018-06-13T22:36:31Z

I spent some time increasing the verbosity of the tests, but I couldn't figure out why it hangs in travis. FWIW the tests work fine for me locally even with gradle 4.3. I'll bump gradle to 4.8:
#239

For unknown reasons, the unit tests hang for Gradle 4.3 on this PR on Travis: google#237 I have confirmed that the code works fine on 4.8.

For unknown reasons, the unit tests hang for Gradle 4.3 on this PR on Travis: #237 I have confirmed that the code works fine on 4.8.

# Conflicts: # gradle/wrapper/gradle-wrapper.properties

zpencer · 2018-06-15T01:55:16Z

Could the build failure be caused by a mismatch in build tools versions in build_base.gradle?

trevjonez · 2018-06-15T02:56:10Z

yea it is license. with newer versions of gradle plugin they remove the need to specify build tools version. Seeing if I can do both version in travis file. do you want me to squash all these once it is passing or is that the merge policy?

trevjonez · 2018-06-21T04:44:55Z

@zpencer Anything else I need to do for this to get merged? Wanting to do some workflow demo's on Friday morning which requires these changes. hoping I can avoid doing a local install to facilitate a smoke and mirrors demo.

zpencer · 2018-06-25T15:28:00Z

@trevjonez sorry I let this update slip past me! I've merged the PR now.

For unknown reasons, the unit tests hang for Gradle 4.3 on this PR on Travis: google#237 I have confirmed that the code works fine on 4.8.

Check for android project by plugin ID

trevjonez added 2 commits June 7, 2018 02:09

Check for android project by plugin ID

6e3cfff

Gradle wrapper to latest as android gradle 3.1.2 needs at minimum 4.4 Test cases to show false positive no longer occurs when parent project has an android extension

add UtilsTest to travis configuration

d694eff

zpencer self-requested a review June 8, 2018 16:40

trevjonez added 2 commits June 8, 2018 21:40

revert gradle wrapper to 4.3

d9b526b

codenarc fixes for UtilsTest.groovy

add google repository for android gradle plugin resolution.

1e36a42

zpencer reviewed Jun 11, 2018

View reviewed changes

rework android detection test cases to operate in external gradle pro…

f0d143d

…cess.

zpencer added a commit to zpencer/protobuf-gradle-plugin that referenced this pull request Jun 13, 2018

Bump to Gradle 4.8 and fix codenarc lints

19929bb

For unknown reasons, the unit tests hang for Gradle 4.3 on this PR on Travis: google#237 I have confirmed that the code works fine on 4.8.

zpencer added a commit to zpencer/protobuf-gradle-plugin that referenced this pull request Jun 13, 2018

Bump to Gradle 4.8 and fix codenarc lints

7866a00

For unknown reasons, the unit tests hang for Gradle 4.3 on this PR on Travis: google#237 I have confirmed that the code works fine on 4.8.

zpencer mentioned this pull request Jun 13, 2018

Bump to Gradle 4.8 and fix codenarc lints #239

Merged

zpencer added a commit to zpencer/protobuf-gradle-plugin that referenced this pull request Jun 13, 2018

Bump to Gradle 4.8 and fix codenarc lints

fdde06a

For unknown reasons, the unit tests hang for Gradle 4.3 on this PR on Travis: google#237 I have confirmed that the code works fine on 4.8.

zpencer added a commit that referenced this pull request Jun 14, 2018

Bump to Gradle 4.8 and fix codenarc lints (#239)

5bcb634

For unknown reasons, the unit tests hang for Gradle 4.3 on this PR on Travis: #237 I have confirmed that the code works fine on 4.8.

zpencer and others added 4 commits June 14, 2018 15:43

Merge branch 'master' into bug/236

7fab029

Merge remote-tracking branch 'upstream/master' into bug/236

ea01914

# Conflicts: # gradle/wrapper/gradle-wrapper.properties

Merge remote-tracking branch 'origin/bug/236' into bug/236

0c1fb21

up the build tools version for travis

0b51608

can I do both?

93b9501

zpencer merged commit 8f0a7a6 into google:master Jun 25, 2018

zhangkun83 pushed a commit to zhangkun83/protobuf-gradle-plugin-1 that referenced this pull request Nov 7, 2018

Detect isAndroidProject based on plugin IDs (google#237)

2fa8611

Check for android project by plugin ID

trevjonez deleted the bug/236 branch April 24, 2019 22:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Detect isAndroidProject based on plugin IDs #237

Detect isAndroidProject based on plugin IDs #237

trevjonez commented Jun 7, 2018

trevjonez commented Jun 7, 2018

zpencer commented Jun 8, 2018

trevjonez commented Jun 8, 2018

trevjonez commented Jun 10, 2018

zpencer Jun 11, 2018

trevjonez Jun 11, 2018

zpencer Jun 11, 2018

trevjonez Jun 11, 2018

zpencer Jun 11, 2018

trevjonez Jun 11, 2018

zpencer Jun 11, 2018

trevjonez Jun 11, 2018

trevjonez commented Jun 13, 2018

trevjonez commented Jun 13, 2018

zpencer commented Jun 13, 2018

zpencer commented Jun 15, 2018

trevjonez commented Jun 15, 2018

trevjonez commented Jun 21, 2018

zpencer commented Jun 25, 2018

Detect isAndroidProject based on plugin IDs #237

Detect isAndroidProject based on plugin IDs #237

Conversation

trevjonez commented Jun 7, 2018

trevjonez commented Jun 7, 2018

zpencer commented Jun 8, 2018

trevjonez commented Jun 8, 2018

trevjonez commented Jun 10, 2018

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

trevjonez commented Jun 13, 2018

trevjonez commented Jun 13, 2018

zpencer commented Jun 13, 2018

zpencer commented Jun 15, 2018

trevjonez commented Jun 15, 2018

trevjonez commented Jun 21, 2018

zpencer commented Jun 25, 2018