Android lint variant filter support #79

tasomaniac · 2018-02-09T08:46:01Z

Fixes #66
Fixes #77

Problem

The lint setup didn't have filters around Android variants. In a big project with 10+ variants, the evaluateViolations task take too long. (around 20 mins)

Solution

This PR make lintOptions variant aware.

Introduced a trait to encapsulate Android variant awareness in Configurators.
- A follow-up will be done to this to make it cleaner.
Instead of skipping non-android projects, use project.plugins.withId('com.android.application') to wait for potential plugin addition. This will fix the timing issue described in Timing issue with lint configuration #77
When lint runs on a single variant, the output names change. That's why introduced Nullable variant param to task creation and change the report file path accordingly.

Note: When filter is not provided, it will keep the old behavior. When filter is used, we may get duplicated results when the same problem is available in an intersection of the sourceSets of multiple variants. This problem is available in all tasks and fix will be introduced later.

Tests

Lots of tests added including the duplicate number behavior.
A test is also added to check if we really ignore problems when the variant is ignored.

…ter to know if we have a custom filter set. Simplify lint configurator by extracting methods

mr-archano

Left a couple of remarks. We can discuss this IRL if you prefer.

mr-archano · 2018-02-09T15:57:35Z

plugin/src/main/groovy/com/novoda/staticanalysis/internal/VariantAware.groovy

+
+trait VariantAware {
+
+    private boolean hasFilter = false


why don't we keep out of the trait API and we just keep the getFiltered*()? this way we can choose in the configurator how to deal with that, for instance only Lint needs to know whether the filter is set or not, right?

trait allows me to have implementation and fields here. I don't want to lose that.

The other option is to make it nullable. Let me try and see how that goes actually. hasFilter is not necessary.

Oh, addressing here, I pushed to clean-up branch. That looks much nicer.

mr-archano · 2018-02-09T15:59:13Z

plugin/src/main/groovy/com/novoda/staticanalysis/internal/VariantAware.groovy

+trait VariantAware {
+
+    private boolean hasFilter = false
+    Closure<Boolean> includeVariantsFilter = { true }


this can be just Closure<Boolean> getIncludeVariantsFilter() that we'll leave to the configurators to implement.

setter is also used in subclasses. I can of course do this but the beauty of a trait is to encapsulate the variant behavior all here.
If I implement getIncludeVariantsFilter in the subclasses, then I would need a property there.

mr-archano · 2018-02-10T22:32:13Z

plugin/src/main/groovy/com/novoda/staticanalysis/internal/lint/LintConfigurator.groovy

+
+    private void configureWithVariants(Closure config, DomainObjectSet variants) {
+        configureLint(config)
+        if (hasFilter) {


where hasFilter is coming from now? not sure how this is working now - if it's working even

mr-archano · 2018-02-10T22:51:23Z

plugin/src/main/groovy/com/novoda/staticanalysis/internal/lint/LintConfigurator.groovy

        }
    }

-    private File getXmlOutputFile() {
-        project.android.lintOptions.xmlOutput ?: new File(defaultOutputFolder, 'lint-results.xml')
+    private File xmlOutputFileFor(reportSuffix) {


I would prefer to push the report name directly and take the decision up two levels from here, so to keep it close to where we call configureCollectViolationsTask()

I'm not sure what you mean by 2 level. Do mean the with clause above just after we create the task. The xmlReportFile property of the task is better to be configured while it is created.

what I mean is that instead of adding this suffix logic you can move the decision of specifying what task name and what report name to use up to L36 and L38 (in that case no need of if anymore, right?)

tasomaniac · 2018-02-12T16:15:03Z

retest this please

tobiasheine

lgtm, just some questions

tobiasheine · 2018-02-12T16:44:37Z

plugin/src/main/groovy/com/novoda/staticanalysis/internal/VariantAware.groovy

+import org.gradle.api.DomainObjectSet
+import org.gradle.api.NamedDomainObjectSet
+
+trait VariantAware {


Personally I would haven chosen composition over inheritance here since we mostly expose fields and methods to be shared.

tobiasheine · 2018-02-12T16:46:14Z

plugin/src/main/groovy/com/novoda/staticanalysis/internal/lint/LintConfigurator.groovy

+            project.plugins.withId('com.android.library') {
+                configureWithVariants(config, filteredLibraryVariants)
+            }
+


nit: empty line

tobiasheine · 2018-02-12T16:49:54Z

plugin/src/main/groovy/com/novoda/staticanalysis/internal/lint/LintConfigurator.groovy

        }
    }

    private void configureLint(Closure config) {
+        project.android.lintOptions.ext.includeVariants = { Closure<Boolean> filter ->


@mr-archano
Could u explain us what exactly happens in this closure?

I'm not sure what you're asking @tobiasheine (as I didn't write this), but I will try to explain what I can see going on here: L54 adds an extension method to lintOptions that takes a filter predicate, used by VariantAware.variantSelector to actually filter in only the variants for which filter is evaluated as true

Right, he was asking because this is a copy paste from the other ones.
I was also wondering how this works exactly. One question comes to mind, there is no mention of the input of the closure. We expect the input to be a variant but I guess there is no limit? Can it be anything?

I understand that we define a extension includeVariants with a Closure<Boolean> and what it is used for.
What is confusing to me is the assignment includeVariantsFilter = filter within the closure or when it is executed. Is it like a callback that is triggered when the user specifies the extension?

mr-archano · 2018-02-13T00:06:03Z

plugin/src/main/groovy/com/novoda/staticanalysis/internal/VariantAware.groovy

+
+    Closure<Boolean> includeVariantsFilter
+
+    final variantSelector = { variants ->


can't this just be a normal private method then? it's weird to see it defined it like this...

Method didn't work how I wanted it to be with this trait thingy. I guess I can do Tobi's suggestion and make this a real collaborator class.

mr-archano · 2018-02-13T00:35:57Z

plugin/src/main/groovy/com/novoda/staticanalysis/internal/lint/LintConfigurator.groovy

-    private File getXmlOutputFile() {
-        project.android.lintOptions.xmlOutput ?: new File(defaultOutputFolder, 'lint-results.xml')
+    private File xmlOutputFileFor(reportSuffix) {
+        project.android.lintOptions.xmlOutput ?: new File(defaultOutputFolder, "lint-results${reportSuffix}.xml")


does this mean that when the user sets xmlOutput then every variant task will output to the same report file?

No idea. ☺ Good question. Only way to find out is to try. I don't think the documentation mentions that.

It overrides the files. If you have xmlOuput defined, you get a single output no matter which lint task you run. So this seems to be the right way to do. But also we should do it for HTML report actually. Same happens for that too.

that's weird behaviour. I agree that this is in line with the default reporting, but I wonder if we should provide a better way to handle that when using variant-aware lint tasks (ie: log a warning/error, or ignore that, or try to generate variant-aware reports from xmlOutput).

I agree. We can talk to Android team about this.

I don't think it is possible to do variant-aware custom xmlOutput at the moment. It is because this configuration is done only once in the configuration phase. Whatever you do overrides the file name. You can only have 1 single name for the output. If it is null, they generate file names with variant name in it.

to have less logic in the task creation Also changed html output to be the same as the xml one

tasomaniac · 2018-02-13T10:45:21Z

retest this please

tasomaniac · 2018-02-13T10:45:39Z

@mr-archano please have another look.

mr-archano · 2018-02-13T14:55:35Z

plugin/src/main/groovy/com/novoda/staticanalysis/internal/VariantFilter.groovy

+import org.gradle.api.NamedDomainObjectSet
+import org.gradle.api.Project
+
+final class VariantFilter {


mr-archano · 2018-02-13T14:57:36Z

plugin/src/main/groovy/com/novoda/staticanalysis/internal/lint/LintConfigurator.groovy

+    private void configureWithVariants(DomainObjectSet variants) {
+        if (variantFilter.includeVariantsFilter != null) {
+            variants.all {
+                configureCollectViolationsTask(it.name, "lint-results-${it.name}")


me happy! 🐶

Said Tahsin Dane added 4 commits February 8, 2018 12:56

Extract trait for VariantAware implementation

06d45ae

Make lint configurator variant aware and add tests for that.

b7027f0

Make the tests more clear by extracting logic

d92177f

Still have default closure to keep backward compatible but have a set…

4a8e826

…ter to know if we have a custom filter set. Simplify lint configurator by extracting methods

tasomaniac requested review from tobiasheine and mr-archano February 9, 2018 08:46

tasomaniac and others added 2 commits February 9, 2018 09:46

Merge branch 'develop' into android-lint-improves

e4307ae

Add a test around ignoring inactive variant

3a5b7bb

tasomaniac force-pushed the android-lint-improves branch from a51339a to 3a5b7bb Compare February 9, 2018 09:11

tasomaniac mentioned this pull request Feb 9, 2018

Clean up variant aware code #80

Merged

mr-archano suggested changes Feb 9, 2018

View reviewed changes

Said Tahsin Dane added 2 commits February 9, 2018 17:45

Make it simpler by removing hasFilter and make the filter nullable

83fb0a3

Have a generic type closure instead of DomainObjectSet

ccff5d2

tasomaniac changed the title ~~Android lint improves~~ Android lint variant filter support Feb 9, 2018

mr-archano suggested changes Feb 10, 2018

View reviewed changes

null check instead of using old hasFilter method

a3e1b75

tobiasheine previously approved these changes Feb 12, 2018

View reviewed changes

inline method

69efef0

tasomaniac dismissed tobiasheine’s stale review via 69efef0 February 12, 2018 17:58

mr-archano reviewed Feb 13, 2018

View reviewed changes

Said Tahsin Dane added 2 commits February 13, 2018 09:32

Calculate the file and task name in the upper level

935adbb

to have less logic in the task creation Also changed html output to be the same as the xml one

Use real collaborator instead of a trait

98050dc

tobiasheine previously approved these changes Feb 13, 2018

View reviewed changes

field needs to be protected. Weird groovy

c85f80a

tasomaniac dismissed tobiasheine’s stale review via c85f80a February 13, 2018 10:24

Extract method again for clarity

43c8b0e

mr-archano approved these changes Feb 13, 2018

View reviewed changes

tasomaniac merged commit 5c4e39b into develop Feb 13, 2018

tasomaniac deleted the android-lint-improves branch February 13, 2018 15:17

This was referenced Feb 13, 2018

includeVariants support for Android Lint setup #66

Closed

Timing issue with lint configuration #77

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Android lint variant filter support #79

Android lint variant filter support #79

tasomaniac commented Feb 9, 2018 •

edited

mr-archano left a comment

mr-archano Feb 9, 2018

tasomaniac Feb 9, 2018

tasomaniac Feb 9, 2018

mr-archano Feb 9, 2018

tasomaniac Feb 9, 2018

mr-archano Feb 10, 2018

mr-archano Feb 10, 2018

tasomaniac Feb 12, 2018

mr-archano Feb 13, 2018 •

edited

tasomaniac commented Feb 12, 2018

tobiasheine left a comment

tobiasheine Feb 12, 2018

tobiasheine Feb 12, 2018

tobiasheine Feb 12, 2018

mr-archano Feb 12, 2018

tasomaniac Feb 13, 2018

tobiasheine Feb 13, 2018

mr-archano Feb 13, 2018 •

edited

tasomaniac Feb 13, 2018

mr-archano Feb 13, 2018

tasomaniac Feb 13, 2018

tasomaniac Feb 13, 2018

mr-archano Feb 13, 2018

tasomaniac Feb 13, 2018 •

edited

tasomaniac commented Feb 13, 2018

tasomaniac commented Feb 13, 2018

mr-archano Feb 13, 2018

mr-archano Feb 13, 2018


		Closure<Boolean> includeVariantsFilter

		final variantSelector = { variants ->

Android lint variant filter support #79

Android lint variant filter support #79

Conversation

tasomaniac commented Feb 9, 2018 • edited

Problem

Solution

Tests

mr-archano 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

mr-archano Feb 13, 2018 • edited

Choose a reason for hiding this comment

tasomaniac commented Feb 12, 2018

tobiasheine 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

mr-archano Feb 13, 2018 • edited

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

tasomaniac Feb 13, 2018 • edited

Choose a reason for hiding this comment

tasomaniac commented Feb 13, 2018

tasomaniac commented Feb 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tasomaniac commented Feb 9, 2018 •

edited

mr-archano Feb 13, 2018 •

edited

mr-archano Feb 13, 2018 •

edited

tasomaniac Feb 13, 2018 •

edited