Skip to content
This repository has been archived by the owner on Feb 11, 2022. It is now read-only.

Make evaluateViolations task run for specific buildVariant #115

Closed
bitsydarel opened this issue Jul 15, 2018 · 15 comments
Closed

Make evaluateViolations task run for specific buildVariant #115

bitsydarel opened this issue Jul 15, 2018 · 15 comments

Comments

@bitsydarel
Copy link

When running evaluateViolations in a project with multiple buildFlavors. The task compile all the buildVariants even if we're running the build for a specific buildVariant. Because of this behavior it's increases the time it's take for all the checks to run even on small projects.

@tasomaniac
Copy link
Contributor

What about the variantFilter? Is that not solving this problem?

@bitsydarel
Copy link
Author

bitsydarel commented Jul 15, 2018

No, because variant filter does not allow to dynamically choose a variant to include.
Which means that i have only two choices, include all variant or only one... Which is not ideal for automation...

@bitsydarel
Copy link
Author

bitsydarel commented Jul 15, 2018

So if I have to run checks for each build I have to change the included variant everytime I want to run static analysis tool for a specific variant... Which is not ideal.

@tasomaniac
Copy link
Contributor

I think this is really nice to have. I'm really curious why the tasks were designed to have only single evaluateViolations that runs all variants. Maybe @mr-archano can enlighten us.

Here is a "hack/trick" or "gradle magic" that you can use to achieve what you want.

Have this following variant filter in your configurations. This was you will be able to configure it from command line. You can run ./gradlew evaluateViolations -PcheckVariant=fooBarDebug

staticAnalysis {
    lintOptions {
        includeVariants { 
            it.name == project.properties['checkVariant']
        }
    }
}

On top of that, you can also make use of GradleBuild task to create tasks for each flavor to do this for you.

applicationVariants.all { variant ->
    project.tasks.create("evaluateViolations${variant.name.capitalize()}", GradleBuild) {
            startParameter = gradle.startParameter.newBuild() // copy from the real build. Needs to come first

            tasks = [ 'evaluateViolations' ]

            startParameter.projectProperties['checkVariant'] = variant.name
        }
}

Note: GradleBuild is a special task which creates another build inside a task execution. Especially if you enable parallel builds, you should not run this with other tasks together.

@bitsydarel
Copy link
Author

bitsydarel commented Jul 20, 2018

@tasomaniac Sorry for the delay,

To get the current buildVariant i have this function:

def getCurrentFlavor() {
    File cxConfigFile = new File(project.projectDir.toString() + "/cx.iml")
    if (!cxConfigFile.exists()) return ""
    else {
        String cxconfig = cxConfigFile.text
        Pattern mather = Pattern.compile("(<option name=\"SELECTED_BUILD_VARIANT\" value.*)")
        Matcher matcher = mather.matcher(cxconfig)
        String flavor
        if(matcher.find()) {
            // we found <option name="SELECTED_BUILD_VARIANT" value="variant" />
            flavor = matcher.group(1)
            // transform it to "variant_name"
            flavor = flavor.substring(flavor.lastIndexOf("value"), flavor.length())
            // remove the " before the name and after the name "
            flavor = flavor.substring(flavor.indexOf("\"") + 1, flavor.lastIndexOf("\""))
        } else {
            flavor = ""
        }

        return flavor
    }
}


The issue is that it's only work when building from android studio but from jenkins nope, that's because the iml file is created by android studio but for now this is not an issue.

LintOptions completly ignore the includeVariants, that was my first try by the way.

So if i understand, GradleBuild will create an new build related to each buildvariant.
this line create a ext property that the includeVariant filter for lintOptions:
startParameter.projectProperties['checkVariant'] = variant.name

But i don't understand the purpose of this line :
tasks = [ 'evaluateViolations' ]
Because if we specify "evaluateViolations" as the only available task for each new build then why not just running it directly when building ?

so the only important configuration here is

lintOptions {
        includeVariants { 
            it.name == project.properties['checkVariant'] // the current selected buildVariant
        }
    }

@tasomaniac
Copy link
Contributor

Well, it is because you are able to parameterize and have different configurations for evaluateViolations with this trick.

You can create flavor and buildType dependant variations of the same task but dynamically configure that differently.

@bitsydarel
Copy link
Author

bitsydarel commented Jul 21, 2018

I see, thank you very much for the suggestions.
Any plan on adding this feature into the tool or we should just use the provided suggestions?

@tasomaniac
Copy link
Contributor

My suggestion is a workaround/hack. I personally think this would be good for the plugin.

@mr-archano
Copy link
Contributor

I think I somehow missed this issue. It looks like an interesting request: we should be able to create specific evaluate${variantName}Violations tasks, but the issue I see is that I don't know how the thresholds will work in that case. Is that desired to use (for instance) the same maxWarnings/maxErrors across variants?

@tasomaniac
Copy link
Contributor

To be honest, Android approach with flavors is powerful but makes everything complicated. I don't think any of the tools we support handles this situation. Your suggestion makes sense to me, although I think we need to make it super clear.

@bitsydarel
Copy link
Author

The thresholds are not configuration dependent? So why not just generate different configuration for each flavor?

@mr-archano
Copy link
Contributor

mr-archano commented Sep 4, 2018

@bitsydarel, to be honest I am pretty confused by your messages, so I will try to clarify few points. Apologies in advance if I am stating obvious things, I just want to be sure we're on the same page.

Gradle builds in Android (and specifically the Android Gradle Plugin) support build variants but there is no such a thing as "selected variant"; that instead is a feature of the IDE that decides to just enable one build variant per module to allow you to work comfortably. This is basically the same as disabling variants using variantFilter yourself. Validating this is as simple as running evaluationViolations clicking on the task listed in the Gradle tool window: you will notice that each static analysis tool will run only on the sourceset(s) included in the active build variant of the IDE.

When you say "building from Android Studio" I am not sure what you mean, as the default run configuration to build the app definitely doesn't run check nor evaluateViolations, so maybe you mean when you build the app from the terminal integrated in Android Studio? (more on this below)

Building from command line (like Jenkins will do, or like you do using the terminal integrated in your IDE) means that you are just using Gradle, no IDE gimmick, hence you will experience the standard behaviour of the build script, where all the build variants are active for an Android module unless explicitly disabled (ie: via variantFilter as mentioned above). To avoid this and to allow people to actually target a single variant (or conversely - to skip the analysis on some build variants) we have provided the includeVariant as documented in advanced-usage.

What @tasomaniac suggested is a clever use of a nested build to keep using evaluateViolations in its current form by creating a child Gradle build execution where you will explicitly enable only one build variant. That will be still using the common thresholds as defined in the staticAnalysis {} extension. This trick can be avoided if we manufacture a meta-task for each build variant that can be used to trigger all the configured tools on the sourcesets/classes relevant to that variant.

The thing I'm not onboard with at all is to provide thresholds and tool configurations per-variant: that will add IMO a level of complexity and a whole new level of edge cases that I am not really sure will be worth the effort. Consider again that as @tasomaniac mentioned none of the static analysis tools supported by this plugin is build variant-aware, not even Android lint, where the configuration is using the lintOptions {} extension.

In conclusion: this plugin already offers includeVariants as a way to address exactly the needs like yours and it is based on a more idiomatic and simpler way of defining build scripts. It should be quite simple to inject the variant to be analysed as build parameter in your Jenkins configuration and then use includeVariants to solve your problem. What do you think?

@bitsydarel
Copy link
Author

Thanks for the suggestions, that's actually what I did at first and it's currently configured that way in our Jenkins. But I wanted to also make it dynamically in android studio so my colleagues can run checks before doing a pull request. The issue was that even after applying the incudeVariants it's still compile other variants and since we have many variants it's take time to build ...

@rock3r
Copy link
Contributor

rock3r commented Sep 5, 2018

Unfortunately static analysis takes a long time, there's not much to do about it. Running Detekt and KtLint is fast because they don't compile the app, but Android Lint takes forever. You can optimise your setup a bit, but it'll still be very slow. Checkstyle, Findbugs and PMD are all kinds of slow, and there's very little you can do to make them faster unfortunately.

If you have a mixed Java + Kotlin project then it'll take even longer since compilation will be slower, especially if you have apt + kapt in action, since the Kotlin compiler needs to be invoked more than once (before and after javac) if there's any Java code in the same module as Kotlin code.

All that is hardly fixable in a meta-plugin such as this; we can't even do things such as using PMD incremental support as Gradle's PMD plugin doesn't support it yet either. Having almost all the supported tools being non-Android specific makes things even harder; trying to selectively compile things is near impossible to obtain without going crazy either... and it's still not going to solve all the perf issues outlined above, unfortunately.

@maxbach
Copy link

maxbach commented Nov 25, 2019

Guys, I think, tasks for specific buildVariant is useful. Using includeVariants is not so flexible. What are the arguments against the feature?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

6 participants