PT-391: [NNP] Support java only modules #256

tobiasheine · 2017-01-16T14:00:54Z

Description
This PR adds support for java-only modules to the gradle-nonnull-plugin.
Problem: The generated folders are not picked up by IDEA as source folders in java-only modules, see this ticket.

Details
We're applying the idea plugin programatically to add the generated folder as source folder.

Tests
We added a test that verifies the generated folder in our java-only module is added to the generatedSourceDirectories.

Paired with @tasomaniac

…ly when needed

mr-archano

LGTM. It would be good for @devisnik to scan this too.

mr-archano · 2017-01-17T15:07:54Z

...-nonnull-plugin/plugin/src/main/groovy/com.novoda.gradle.nonnull/AndroidNonNullPlugin.groovy

+            classesTask.mustRunAfter task
+
+            JavaCompile compileTask =
+                    (JavaCompile) project.tasks.getByName("compile${taskName.capitalize()}Java")


why this is on a separate line? it doesn't look like too long

Because copy pasta 😄

mr-archano · 2017-01-17T15:10:41Z

gradle-nonnull-plugin/sample/core/src/main/java/com/novoda/gradle/nonnull/core/SomeClass.java

+        setValue(null);
+    }
+
+    // this should result in a warning in the IDEA


is this the right place for this comment? shouldn't be on line 6 instead?

devisnik

Some suggestions.

Apart from that, LGTM.

devisnik · 2017-01-17T15:53:33Z

gradle-nonnull-plugin/README.md

@@ -14,3 +14,4 @@ TODO

 The plugin is **under early development** and to be considered in pre-alpha stage.

+The implementation of the java-only module support is inspired by https://github.com/tbroyer/gradle-apt-plugin


devisnik · 2017-01-17T15:54:24Z

...-nonnull-plugin/plugin/src/main/groovy/com.novoda.gradle.nonnull/AndroidNonNullPlugin.groovy

            applyAndroid(project, project.android.applicationVariants)
        } else if (project.plugins.hasPlugin("com.android.library")) {
            applyAndroid(project, project.android.libraryVariants)
+        } else if (project.plugins.hasPlugin('java')) {
+            applyJava(project)
        } else {
            throw new StopExecutionException("The 'android' plugin is required.")


This message doesn't seem up to date anymore.

devisnik · 2017-01-17T15:54:46Z

...-nonnull-plugin/plugin/src/main/groovy/com.novoda.gradle.nonnull/AndroidNonNullPlugin.groovy


 public class AndroidNonNullPlugin implements Plugin<Project> {

    void apply(Project project) {

-        if (project.plugins.hasPlugin("com.android.application")) {
+        if (project.plugins.hasPlugin('com.android.application')) {
            applyAndroid(project, project.android.applicationVariants)
        } else if (project.plugins.hasPlugin("com.android.library")) {


Can we have single quotes here as well?

devisnik · 2017-01-17T15:55:57Z

...-nonnull-plugin/plugin/src/main/groovy/com.novoda.gradle.nonnull/AndroidNonNullPlugin.groovy

-            task.outputDir = project.file("${project.buildDir}/generated/source/nonNull/${variant.dirName}")
-            task.variant = variant
+            def outputPath = "${project.buildDir}/generated/source/nonNull/${variant.dirName}"
+            def sourceSets = variant.sourceSets


Can this be inlined?

devisnik · 2017-01-17T15:59:31Z

...-nonnull-plugin/plugin/src/main/groovy/com.novoda.gradle.nonnull/AndroidNonNullPlugin.groovy

+        def task = project.task("generate${taskName.capitalize()}NonNullAnnotations", type: GeneratePackageAnnotationsTask)
+        task.outputDir = project.file(outputPath)
+        task.sourceSets = sourceSets
+        task


I think this here might work as well:

project.task(...) { outputDir = ... sourceSets = ... }

It returns the created task, so no need to define any local variable.

devisnik · 2017-01-17T16:32:04Z

...-nonnull-plugin/plugin/src/main/groovy/com.novoda.gradle.nonnull/AndroidNonNullPlugin.groovy

+            String taskName = "main".equals(sourceSetName) ? '' : sourceSetName
+
+            def generatedSourcesDir = "${project.buildDir}/generated/source/nonNull/${sourceSet.name}"
+            GeneratePackageAnnotationsTask task = createTask(project, taskName, generatedSourcesDir, [sourceSet])


Do we need this specific type, or could we just have a Taskhere?

devisnik · 2017-01-17T16:35:37Z

...-nonnull-plugin/plugin/src/main/groovy/com.novoda.gradle.nonnull/AndroidNonNullPlugin.groovy

+
+    // inspired by https://github.com/tbroyer/gradle-apt-plugin/blob/master/src/main/groovy/net/ltgt/gradle/apt/AptPlugin.groovy#L171-L213
+    private static void configureIdeaModule(Project project) {
+        // so the user does not need to apply it when using the plugin


Hm, I'm not sure about that. If the user doesn't care about idea, I'd rather not force it. There's other IDEs out there. ;)

Downloading Eclipse now.
Something I haven't been done for a long long time. 😈

I've tried it:

The compilation works and it generates the files successfully in Eclipse.

They are not picked up by Eclipse as source folders. But that is something we should do manually. (Just like the gradle-apt-plugin)

I think it's better to let the user apply the plugin (as gradle-apt-plugin does).

I'm embarrassed. The screenshot was like that because I opened the project that already has the files.

We tried with Eclipse with a brand new clone of the project. With or without idea plugin, we couldn't make it work.

As side note, we also couldn't make the NonNull annotation work.

devisnik · 2017-01-17T16:42:50Z

...-nonnull-plugin/plugin/src/main/groovy/com.novoda.gradle.nonnull/AndroidNonNullPlugin.groovy

+            def generatedSourcesDir = new File("${project.buildDir}/generated/source/nonNull/${sourceSet.name}")
+
+            project.afterEvaluate {
+                project.idea.module {


This is inspired by https://github.com/tbroyer/gradle-apt-plugin , right?

Exactly.
It was only checking if the idea plugin is applied. We just removed that.

devisnik · 2017-01-17T16:46:37Z

gradle-nonnull-plugin/sample/core/build.gradle

+apply plugin: 'java'
+apply plugin: com.novoda.gradle.nonnull.AndroidNonNullPlugin
+
+sourceCompatibility = "1.7"


Single quotes?

devisnik · 2017-01-17T16:47:10Z

...plugin/plugin/src/test/groovy/com/novoda/gradle/nonnull/AndroidProjectIntegrationTest.groovy

 import org.junit.ClassRule
 import org.junit.Test
 import org.junit.rules.TestRule
 import org.junit.runner.Description
 import org.junit.runners.model.Statement

+


…ess the parameter via owner. owner does not work to access method parameters above the closure

…here. This requires the `idea` plugin to be applied manually, if the project using the plugin is developed under IntelliJ Idea.

And use constant in SourceSet instead of hardcoded value

tobiasheine added 11 commits January 13, 2017 16:04

set the sourceSet instead of the variant in the task

2daef9d

generate package annotation for java module

5ab2157

mark generated folder manually as source folder

0ec4b6f

mark generated folders as source/generated source programatically

056bb7e

remove idea plugin from the sample and apply it instead programatical…

5f30668

…ly when needed

improve readability by extracting methods

9e318f0

fix typo in test - it should exclude the core build folder

49141ba

Clean-up tests bz extracting methods

7e6634c

Add attribution to the apt plugin

2d642fe

Rename variable to coreModule for better readability

47e742a

Remove not needed loop

4e121c0

tasomaniac changed the base branch from PT-380/gradle_nonnull_plugin to master January 17, 2017 13:19

Merge branch 'master' into PT-391/support_java_only_moduls

25c0d3c

mr-archano reviewed Jan 17, 2017

View reviewed changes

devisnik approved these changes Jan 17, 2017

View reviewed changes

Said Tahsin Dane added 7 commits January 18, 2017 08:29

Join 2 lines because the assignment is not too long

7cecf06

Comment moved to the right place

6a6f8d3

Style changes

f9ba19b

Use task variable as an input in the closure instead of trying to acc…

f99adb8

…ess the parameter via owner. owner does not work to access method parameters above the closure

Do not apply idea plugin automatically but check if the plugin is t…

adfc879

…here. This requires the `idea` plugin to be applied manually, if the project using the plugin is developed under IntelliJ Idea.

Rename subdirs to subDirs

cb4a606

Inline sourceSetName

c983196

And use constant in SourceSet instead of hardcoded value

tasomaniac added the x-do-not-merge label Jan 18, 2017

stefanhoth closed this Jan 18, 2017

tobiasheine reopened this Jan 18, 2017

tobiasheine removed the x-do-not-merge label Jan 18, 2017

mr-archano merged commit ab02c21 into master Jan 18, 2017

mr-archano deleted the PT-391/support_java_only_moduls branch January 18, 2017 17:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PT-391: [NNP] Support java only modules #256

PT-391: [NNP] Support java only modules #256

tobiasheine commented Jan 16, 2017 •

edited by tasomaniac

mr-archano left a comment

mr-archano Jan 17, 2017

tasomaniac Jan 17, 2017

mr-archano Jan 17, 2017

tasomaniac Jan 17, 2017

devisnik left a comment

devisnik Jan 17, 2017

devisnik Jan 17, 2017 •

edited

devisnik Jan 17, 2017

devisnik Jan 17, 2017

devisnik Jan 17, 2017

devisnik Jan 17, 2017

devisnik Jan 17, 2017

tasomaniac Jan 18, 2017

tasomaniac Jan 18, 2017

devisnik Jan 18, 2017

tasomaniac Jan 18, 2017

tobiasheine Jan 18, 2017

devisnik Jan 17, 2017

tasomaniac Jan 17, 2017

devisnik Jan 17, 2017

devisnik Jan 17, 2017

		@@ -14,3 +14,4 @@ TODO

		The plugin is under early development and to be considered in pre-alpha stage.

		The implementation of the java-only module support is inspired by https://github.com/tbroyer/gradle-apt-plugin

PT-391: [NNP] Support java only modules #256

PT-391: [NNP] Support java only modules #256

Conversation

tobiasheine commented Jan 16, 2017 • edited by tasomaniac

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

devisnik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devisnik Jan 17, 2017 • 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

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

tobiasheine commented Jan 16, 2017 •

edited by tasomaniac

devisnik Jan 17, 2017 •

edited