Add support for Gradle

[uschindler]

The Elasticsearch people started with https://github.com/elastic/forbidden-apis-gradle-plugin.

I think this should be included directly into the forbidden-apis tool. Basically this is more or less the Maven tool, just implemented as Gradle's DefaultTask extension. This can be implemented in pure Java, this is what I would prefer.

As usual all would be packaged in the same JAR file because there are no conflicts between the tools.

[uschindler]

I added a new branch: https://github.com/policeman-tools/forbidden-apis/compare/issue68

[rjernst]

Looking good, thank you!

Could you possibly make the plugin work with all sourceSets? This would allow eg. adding a new testInteg which is also checked?

[uschindler]

Hi @rjernst,
The problem with all sourcesets is that Gradle has no "process-classes" like Maven. To ensure that it starts, you need some task where you can add it as dependency. For unknown sourcesets this does not work (no defined)

In that case you would need to configure the task on your own, which should work with plain gradle/groovy. The class is there, you just have to define 2 things: classesDir + classpath.

By the way, the current version that was committed to the branch works correct now. I have several Gradle projects from the examples shipped with gradle extended by this plugin, works.

Are there any plans to switch Elasticsearch builds to Gradle?

[uschindler]

This is an example (modify Gradle's samples/java/quickstart example):

apply plugin: 'java'

buildscript {
  repositories {
    mavenLocal()
  }
  dependencies {
    classpath 'de.thetaphi:forbiddenapis:1.9-SNAPSHOT'
  }
}

apply plugin: 'de.thetaphi.forbiddenapis'

sourceCompatibility = 1.5
version = '1.0'
jar {
    manifest {
        attributes 'Implementation-Title': 'Gradle Quickstart',
                   'Implementation-Version': version
    }
}

repositories {
    mavenCentral()
}

dependencies {
    compile group: 'commons-collections', name: 'commons-collections', version: '3.2'
    testCompile group: 'junit', name: 'junit', version: '4.+'
}

test {
    systemProperties 'property': 'value'
}

forbiddenApis {
  signatures = ['java.lang.String']
  bundledSignatures = ['jdk-unsafe','jdk-deprecated']
  failOnViolation = false
}

testForbiddenApis {
  signatures = ['org.gradle.Person']
  bundledSignatures = ['jdk-unsafe','jdk-deprecated']
  failOnViolation = false
}

uploadArchives {
    repositories {
       flatDir {
           dirs 'repos'
       }
    }
}

[uschindler]

I changed the task dependencies in the branch a bit (after sleeping about it):
the new forbidden tasks are now dependencies of the "classes" and "testClasses" generic tasks. This is similar to Maven. So every task that requires class files gets the checked ones. Our tasks only depend on java compilation tasks.

I am currently investigating why the forbidden-checks run all the time although there are no changes. I tried to add outpus == inputs, but this did not help.

[uschindler]

I refactored the task now, so inputs/outputs work correctly. The issue was that annotation don't work correctly on fields without getters. I also changed the task to be PatternFilterable, so it now behaves similar to other tasks taking input files,

[rjernst]

Why not just install the tasks and let he user decide what should depend on them? It is really simple for the user to eg add it to check:

check.dependsOn forbiddenApis, testForbiddenApis

In my particular case, I want to add a precommit task, and have forbidden apis as part of that, and precommit be part of check. This would allow eg forbidding System.out.println, but still be able to run the tests directly when debugging with print outs. But a full build or check call would still do the forbidden checks. I realize you could do this through setting a system property to the failOnViolation, but why restrict the user to a single way of using the api, instead of allowing them to easily customize how they want to use it?

[uschindler]

Hi,
I did it the way how it is at the moment mostly because the current Maven plugin's MOJO is also located in the phase called "process-classes" (and because @pickypg did it the same way in his plugin).
My proposal would be: As many people already complained about that (or changed the defaults in ther Maven config to use the "verify" phase), I would do the following:

• release the Gradle plugin as forbiddenapis 2.0 (instead 1.9)
• change the Maven plugin to use the "verify" phase by default
• In the Gradle plugin let "check" (is same as "verify" phase in Maven) depend on forbidden-apis. This brings more convention as Gradle proposes and its ran by default. Also other Gradle plugins like CheckStyle or PMD have the same defaults! To disable the automatic assignement, we could add a parameter to the plugin initialization, so you can disable this. But for new users we really should "automatically" configure dependency tree correctly (just later phase).
• Maybe add the usafe signatures as default for Maven and Gradle (not sure about this). This would help for new users, too (you can just enable the plugin and it will work asap).
• Use minimum Java 6 & merge the old jdk1.5 unsafe signatures into 1.6, this makes Gradle development easier (because you can use the new plugin infrastructure).

What do you think?

[uschindler]

FYI: PMD says in https://docs.gradle.org/current/userguide/pmd_plugin.html: "The plugin adds a number of tasks to the project that perform the quality checks. You can execute the checks by running gradle check."

[dweiss]

Hmm... Maven's validate is executed before classes are compiled, Uwe -- this means you'd be verifying last run's output. Does it make sense?

[uschindler]

Maybe I mixed up the name. I mean the phase after test. I think it's "verify". Both just start with "v".

[dweiss]

You mean 'verify'. I personally think the way it was bound (process-classes) makes a lot of sense -- an early warning signal rather than delayed check.

[uschindler]

I personally think the way it was bound (process-classes) makes a lot of sense

Most projects I have seen changed this to verify. Also Elasticsearch did it very late for long time.

[dweiss]

Well, if "majority is right" is the argument then sure... :) Note that 'verify' is executed after packaging to you can build the package with invalid method calls. To me this is a no-no.

But people can change the binding anyway if they like, so up to you.

[uschindler]

But "install" or "deploy" requires to run "verify"... So you cannot publish in Maven Central without that passing (yes you can copy the JAR files away from target/ folder...).

In any case, a later stage also helps Gradle. If you bind it as dependency for "check", it is easy for anybody to just add another dependency. Removing the current dependency on "classes"/"testClasses" is almost impossible in "clean & nice groovy code".

[rjernst]

@uschindler The gradle plan sounds good to me. Maven I know nothing about, it is black magic.

[dweiss]

Yes, but if you're using maven and wish to stay sane you wouldn't install to a local repo, even for deployment (you'd redirect the repo to a temp. dir).

Anyway, it's just a different opinion, I don't care either way.

[uschindler]

Could you possibly make the plugin work with all sourceSets?

Now I can maybe do this. When the plugin installs itsself it would register a task for every sourceSet, for "main" and "test" it would depend on the corresponding compile tasks, but not for others. PMD/CheckStyle does the same, very cool: https://docs.gradle.org/current/userguide/pmd_plugin.html; https://docs.gradle.org/current/userguide/checkstyle_plugin.html

[pickypg]

@uschindler In terms of @rjernst's request, you can depend on the SourceSet's SourceSetOutput from the tasks. A SourceSetOutput is a Buildable, which can be depended upon. I chose not to do it because I was focused on making the plugin work properly with inputs/outputs and the phases, but it was the planned next step once we had another source set to test against.

I tried to setup the dependencies so that it would short circuit a build if the Forbidden APIs were used (by making jar depend on forbiddenApis and test depend on testForbiddenApis to prevent it from getting far with forbidden usage). I don't think there's much hope for the dynamically discovered ones, but I like the idea of making them all depend upon check.

You can also have the task marked with @ParallelizableTask so that parallel execution can run them together if it deems it worthwhile.

This looks like great progress to me.

[uschindler]

You can also have the task marked with @ParallelizableTask so that parallel execution can run them together

Without upgrading to Java 1.6 this is impossible (because I have to compile against Gradle 1.x if I want to stay on 1.5 for the plugin). See my comment above. For 2.0 I would drop Java 5.

[uschindler]

In terms of @rjernst's request, you can depend on the SourceSet's SourceSetOutput from the tasks

Thats already in the process of changing - I noticed that, too :-)

[pickypg]

@uschindler Is there any reason to maintain support for even Java 6? I chose to support Java 7 as a minimum because I know a lot of people still use it, but Java 6 has not been supported for years. I'm certainly only referring to 2.0 of the plugin.

[uschindler]

No need for Java 7, so I stay with Java 6. You cannot use NIO.2 anyways because thats incompatible with any build tool outside.

[uschindler]

I started to refactor plugin initialization. Don't ask why I did it that way, but that simplifies things enormous...

[rjernst]

Another small suggestion: set the group on the created tasks to Verification. This will have it show up when running gradle tasks with the other check tasks like test.

[rjernst]

Or maybe it doesn't matter. I was concerned it would show up in the "Other" category when running gradle tasks. But I think since it is a dependency of check it won't show up at all. So nevermind! I think I like that behavior better: no need to see details unless you want to (with gradle tasks --all).

[uschindler]

I implemented now the task for all source sets. This now works using same logic like the other check tasks (PMD, CheckStyle).

// initializes the plugin and binds it to the lifecycle

import org.gradle.api.plugins.PluginInstantiationException;
import org.gradle.api.tasks.TaskContainer;

if (!project.plugins.findPlugin("java")) {
  throw new PluginInstantiationException("Forbiddenapis only works in projects using the 'java' plugin.");
}

def tasks = project.getTasks();
def checkTask = tasks.getByName("check");

// Define our tasks (one for each SourceSet):
def forbiddenTasks = project.sourceSets.collect { sourceSet ->
  tasks.create(sourceSet.getTaskName(FORBIDDEN_APIS_TASK_NAME_PREFIX, null), CheckForbiddenApis.class) { task ->
    task.classesDir = sourceSet.output.classesDir;
    task.classpath = sourceSet.compileClasspath;
    task.description = "Runs forbiddenApis checks on '" + sourceSet.name + "' classes.";
    // task.group = checkTask.group;
    task.dependsOn(sourceSet.output);
  }
}

// Add our tasks as dependencies to chain
checkTask.dependsOn(forbiddenTasks);

I commented out the task's group for now. Adding it would make it show up next to "check".

The output now is:

Verification tasks
------------------
check - Runs all checks. [forbiddenApisMain, forbiddenApisTest, test]
forbiddenApisMain - Runs forbiddenApis checks on 'main' classes. [classes]
forbiddenApisTest - Runs forbiddenApis checks on 'test' classes. [classes, testClasses]
test - Runs the unit tests. [classes, testClasses]

Please note, the task name changed a bit, the new one is automatically generated from the SourceSet method.

[uschindler]

I did some testing already:

With the current code it is perfectly possible to declare own sourceSets, but you have to do this before loading the Plugin:

apply plugin: 'java'

sourceSets {
    dummy {
        java {
            compileClasspath = main.compileClasspath
            runtimeClasspath = main.runtimeClasspath
        }
    }
}

buildscript {
  repositories {
    mavenLocal()
  }
  dependencies {
    classpath 'de.thetaphi:forbiddenapis:1.9-SNAPSHOT'
  }
}

apply plugin: 'de.thetaphi.forbiddenapis'

If you load the plugin before, it would not create the tasks, because the sourceSets are not there at time of plugin loading. I chcked this: It behaves exactly the same for Checkstyle and PMD and all other check plugins (they all chare the same superclass).

The only thing that is missing currently is a ForbiddenApisExtension that can be used to define defaults. This extension looks like a task ("forbiddenApis") in the DSL, but its settings are just copied over into the task implementations. So you can define the bundledSignatures.

Without the extension, the above build script would fail, because you also have to add the task settings for the new (automatically defined) "forbiddenApisDummy" task.

I will look into this, it works with a mechanism called "ConventionMappings".

With the Extension, it may be needed to remove the "convenience" task "forbiddenApis" because this would create naming conflict. Alternatively call the extension "forbiddenApisDefaults".

[uschindler]

I implemented extension:

forbiddenApis {
  signatures = ['java.lang.String']
  bundledSignatures = ['jdk-unsafe','jdk-deprecated','jdk-system-out']
  ignoreFailures = true
}

forbiddenApisTest {
  signatures += 'org.gradle.Person'
}

The forbiddenApis closure refers to the extension and defines all defaults. The tasks inherit those. In the test task, you see that it is possible to add further signatures using standard groovy syntax (adding stuff to lists).

[uschindler]

I also tested recently that the plugin also works if you have a groovy-only project. If you setup a Groovy project and one of your source code files (implemented in Groovy) uses forbidden apis (after compilation with Groovyc) it also fails :-)

:forbiddenApisMain
Forbidden class/interface use: java.io.File
  in Test (Test.groovy:4)

[uschindler]

I added a pull request as all main features are working now: #70