Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

issue #98: Add support for Groovy projects #99

Merged
merged 3 commits into from Aug 25, 2019

Conversation

@siordache
Copy link
Collaborator

commented Aug 9, 2019

Changes:

  • in CompileJavaTaskMutator: configure the java and groovy source sets
  • in CompileTask: configure compileJava to run after compileGroovy
  • in ModulePluginSmokeTest: add test-project-groovy to the list of projects to be tested.
GroovyCompile compileGroovy = (GroovyCompile)project.getTasks().findByName(COMPILE_GROOVY_TASK_NAME);
if (compileGroovy != null) {
JavaPluginConvention javaConvention = project.getConvention().getPlugin(JavaPluginConvention.class);
for(String sourceSetName: List.of(SourceSet.MAIN_SOURCE_SET_NAME, SourceSet.TEST_SOURCE_SET_NAME)) {

This comment has been minimized.

Copy link
@tlinkowski

tlinkowski Aug 10, 2019

Collaborator

FYI, you can use helper() method that I introduced to:

  • access source sets without such boilerplate (mainSourceSet(), testSourceSet())
  • find tasks findTask(name, type)
for(String sourceSetName: List.of(SourceSet.MAIN_SOURCE_SET_NAME, SourceSet.TEST_SOURCE_SET_NAME)) {
SourceSet sourceSet = javaConvention.getSourceSets().getByName(sourceSetName);
sourceSet.getJava().include("module-info.java");
javaCompile.setDestinationDir(compileGroovy.getDestinationDir());

This comment has been minimized.

Copy link
@tlinkowski

tlinkowski Aug 10, 2019

Collaborator

I'm wondering if always changing the destination dir is the right way here. I have a project where the Groovy plugin is applied but there are no main Groovy sources, and this would probably break my project. See #100

javaCompile.setDestinationDir(compileGroovy.getDestinationDir());

GroovySourceSet groovySourceSet = (GroovySourceSet)new DslObject(sourceSet).getConvention().getPlugins().get("groovy");
groovySourceSet.getGroovy().setSrcDirs(List.of("src/" + sourceSetName + "/java", "src/" + sourceSetName + "/groovy"));

This comment has been minimized.

Copy link
@tlinkowski

tlinkowski Aug 10, 2019

Collaborator

Would you mind explaining why you need to modify the source dirs of the main and test Groovy source sets?

I'm wondering because:

  • it was solved differently for Kotlin
  • I'm afraid it may break my project, where I always apply all three plugins:
    1. Java (for main code)
    2. Groovy (for Spock)
    3. Kotlin (for test helpers)

Right now, I'm testing on classpath only, because Groovy 2 doesn't work on modulepath.

I specifically configured compileTestGroovy to have the results of compileTestKotlin on its classpath. I wonder whether such approach would work on modulepath (but that's my problem to figure it out - here, I'd only like to make sure that this plugin doesn't make any incompatible assumptions).

@@ -29,6 +30,11 @@ private void configureCompileJava(JavaCompile compileJava) {
} else {
configureModularityForCompileJava(compileJava, moduleOptions);
}
helper().findTask(CompileJavaTaskMutator.COMPILE_GROOVY_TASK_NAME, GroovyCompile.class)
.ifPresent(compileGroovy -> {
compileGroovy.getDependsOn().remove(JavaPlugin.COMPILE_JAVA_TASK_NAME);

This comment has been minimized.

Copy link
@tlinkowski

tlinkowski Aug 10, 2019

Collaborator

Again, would you please explain why it's handled differently for Kotlin and differently for Groovy?

This comment has been minimized.

Copy link
@siordache

siordache Aug 10, 2019

Author Collaborator

The Kotlin plugin configures:
compileJava.dependsOn(compileKotlin)
while the Groovy plugin configures:
compileGroovy.dependsOn(compileJava)

Without changing this dependency the module-info.java would be compiled before compiling the Groovy classes and it will fail.

This comment has been minimized.

Copy link
@tlinkowski

tlinkowski Aug 11, 2019

Collaborator

I see, thanks!

@siordache siordache force-pushed the beryx:groovy branch from 45b73e6 to e47a535 Aug 10, 2019

@siordache

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 10, 2019

I force pushed some changes but I'm actually not happy with the current solution, because it doesn't work when mixing Java and Groovy code. Inverting the dependency between compileJava and compileGroovy was probably not a good idea. Maybe it's better to leave the dependsOn relation untouched and set the compileModuleInfoSeparately instead. What do you think?

@siordache

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 10, 2019

The current commit implements the idea of setting compileModuleInfoSeparately when using the Groovy plugin.

Also, instead of changing the destination dir of compileJava, I now change the destination dir of compileGroovy.

This code allows both calling Java from Groovy and Groovy from Java (and test-project-groovy now includes such calls). If we remove these lines, only calling Java from Groovy remains possible.

However, using the Groovy compiler exclusively is undesirable when Groovy is only used for tests. Maybe we should introduce a new flag to enable/disable this code.

I would love your feedback.

@tlinkowski

This comment has been minimized.

Copy link
Collaborator

commented Aug 11, 2019

Serban, thanks for your feedback and your awesome work!

Let me first summarize what we know already, because it seems to be a rather complex issue. Please correct me if I got anything wrong (note that throughout, I use src/main/java, etc. for brevity, but I really mean mainSourceSet().java.srcDirs, etc.).

  1. default relations between compile tasks:
    1. compileJava depends on compileKotlin
    2. compileGroovy depends on compileJava
    3. no relation between compileKotlin and compileGroovy
  2. default visibility between Java/Groovy/Kotlin sources
    1. if Java is in src/main/java, Groovy in src/main/groovy, and Kotlin in src/main/kotlin:
      1. you can call Kotlin from Java on classpath (source)
      2. you can call Java from Groovy on classpath (source)
    2. if you also want to call:
      1. Java from Kotlin: you place your Java files in src/main/kotlin
      2. Groovy from Java: you place your Java files in src/main/groovy
  3. whenever module-info.java is present in src/main/java:
    1. it must be compiled with/after other Java sources AND after both Kotlin and Groovy (to see all the packages)
    2. it must be compiled by the Java compiler (other compilers don't understand it yet)
    3. it must have access to the output produced by point 3.i (i.e. must have all the compiled classes on its modulepath)

To sum up, I'd aim at a solution that modifies as little as possible to achieve its goals. And the goals (as I see them) are:

  1. Compile Java sources on the module-path (hence, using Java compiler).
  2. Compile Kotlin sources using Kotlin compiler.
  3. Compile Groovy sources using Groovy compiler.
  4. Compile module-info.java using Java compiler properly (so that it "sees" all Java, Kotlin, and Groovy packages).

Now, your last solution (with compileModuleInfoSeparately) is really nice, and I hate to voice further concerns about your great work so far, but:

  • by using the Groovy compiler for Java sources, this solution doesn't meet goal 1 (if you agree with this goal)
  • by enabling 2.ii.b implicitly (i.e. calling Groovy from Java without the need to place Java sources in src/main/groovy), it seems to be somewhat overzealous and non-intuitive

After considering all this, and inspired by your idea to employ compileModuleInfoSeparately, I'd like to propose the following solution for both Groovy and Kotlin (hence, solving #100 ):

  1. whenever module-info.java is present in src/main/java (we already don't do anything it's absent, which is good)
  2. AND
    1. if compileGroovy is present AND there's anything in src/main/groovy:
      1. set compileModuleInfoSeparately = true
      2. append project.files(compileGroovy.destinationDir) to the classpath of compileModuleInfoJava (before it gets converted to modulepath) - might be done conditionally here
        compileModuleInfoJava.setClasspath(project.files()); // empty
    2. if compileKotlin is present:
      1. append project.files(compileKotlin.destinationDir) to the classpath of compileModuleInfoJava or compileJava (if the former doesn't exist; also before any gets converted to modulepath)

What do you think of all this, Serban?

PS. Note that I just added #101 which takes this "append destinationDir to classpath" approach back to compileModuleInfoJava.

@siordache

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 11, 2019

Thanks for this excellent analysis, Tomasz! I 100% agree with your proposed solution.

How should we go about it?

  1. Implementing all changes required by #98, #100, and #101 in this PR
    a. squashing all changes into a single commit
    b. using issue-related separate commits
  2. Submitting a separate PR for each issue
@tlinkowski

This comment has been minimized.

Copy link
Collaborator

commented Aug 11, 2019

Thanks, I'm glad you appreciate my effort, and thank you that you're willing to put your effort to actually implement this! 😃

As to how to proceed, if I were to decide, I'd probably go for option 1.b (including Fix #issue-number in commit messages to close appropriate issues) so that it's easier for @paulbakker to merge.

@paulbakker

This comment has been minimized.

Copy link
Collaborator

commented Aug 15, 2019

@siordache this is great, thanks for putting in all the work.
@tlinkowski Thanks for reviewing :-)

Essentially the only change is this, correct?

                    .ifPresent(compileGroovy -> {
                        moduleOptions.setCompileModuleInfoSeparately(true);
                        compileGroovy.setDestinationDir(compileJava.getDestinationDir());

                        // Use the Groovy compiler for both Java and Groovy code
                        var sourceSet = helper().mainSourceSet();
                        sourceSet.getJava().exclude("**/*");
                        GroovySourceSet groovySourceSet = (GroovySourceSet)new DslObject(sourceSet).getConvention().getPlugins().get("groovy");
                        groovySourceSet.getGroovy().setSrcDirs(List.of("src/" + sourceSet.getName() + "/java", "src/" + sourceSet.getName() + "/groovy"));
                        groovySourceSet.getGroovy().exclude("**/module-info.java");
                    });

I'm (probably more than needed) a little worried about adding more and more special cases that change how a project is compiled, because it will be harder to understand what's happening exactly. The same applies to the Kotlin support obviously.
The actual scope of the change is pretty limited though, so maybe this isn't really a problem.

As for how to proceed: I agree with @tlinkowski, separate commits for specific issues is most easy to review.

@tlinkowski

This comment has been minimized.

Copy link
Collaborator

commented Aug 15, 2019

Hi @paulbakker, thanks for your feedback 🙂

I think you're rightly worried about the compilation process becoming too complicated. However, note that after what we've arrived at with @siordache, this piece of code:

.ifPresent(compileGroovy -> {
  moduleOptions.setCompileModuleInfoSeparately(true);
  compileGroovy.setDestinationDir(compileJava.getDestinationDir());

  // Use the Groovy compiler for both Java and Groovy code
  var sourceSet = helper().mainSourceSet();
  sourceSet.getJava().exclude("**/*");
  GroovySourceSet groovySourceSet = (GroovySourceSet)new DslObject(sourceSet).getConvention().getPlugins().get("groovy");
  groovySourceSet.getGroovy().setSrcDirs(List.of("src/" + sourceSet.getName() + "/java", "src/" + sourceSet.getName() + "/groovy"));
  groovySourceSet.getGroovy().exclude("**/module-info.java");
});

will actually be superseded by sth much simpler like:

.ifPresent(compileGroovy -> {
  GroovySourceSet mainSourceSet = ...
  if (!mainSourceSet.getGroovy().isEmpty()) {
    moduleOptions.setCompileModuleInfoSeparately(true);
    var compileModuleInfoJava = helper().compileJavaTask(CompileModuleOptions.COMPILE_MODULE_INFO_TASK_NAME);
    compileModuleInfoJava.setClasspath(project.files(compileGroovy.getDestinationDir()));
  }
});

Generally, we've established that we want to modify as little as possible (and only when necessary) so that the chance that the plugin breaks something (as it currently does through #100) is minimized.

@siordache

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 18, 2019

@tlinkowski, @paulbakker
I just pushed the changes as 3 separate commits. I would love your feedback.

@tlinkowski
Copy link
Collaborator

left a comment

I raised some concerns (some may actually be unfounded), but overall, you did an amazing job! Thanks, Serban 🙂

@siordache

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 22, 2019

@tlinkowski Thanks for your thorough review, Tomasz! I will make the required changes later today.

@siordache siordache force-pushed the beryx:groovy branch from f6e213a to 05133f3 Aug 22, 2019

@siordache

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 22, 2019

@tlinkowski The last commit addresses the issues raised in your review. Please let me know if I missed something.

@tlinkowski

This comment has been minimized.

Copy link
Collaborator

commented Aug 23, 2019

Thanks for your changes, Serban!

I now noticed there's some more room for improvement - let me know what you think about it:

  1. CompileTask should actually configure only compileJava, but right now, it seems to also (indirectly) do what MergeClassesTask could do (with moduleOptions.addModuleClassesDir).
  2. We're storing moduleClassesDirs on moduleOptions, but I have an impression this set is redundant (later on that in a moment). In general, I'd gladly see moduleClassesDirs removed from moduleOptions.
  3. MergeClassesTask always creates mergeClasses task (even if there's no Groovy, Kotlin, nor separate module-info compilation).

Here's what I'd propose to do:

  1. In CompileTask, replace:
            helper().findTask(COMPILE_GROOVY_TASK_NAME, GroovyCompile.class)
                    .ifPresent(compileGroovy -> {
                        if(!helper().groovyMainSourceSet().getGroovy().isEmpty()) {
                            moduleOptions.setCompileModuleInfoSeparately(true);
                            moduleOptions.addModuleClassesDir(compileGroovy.getDestinationDir());
                        }
                    });

            helper().findTask(COMPILE_KOTLIN_TASK_NAME, AbstractCompile.class)
                    .map(AbstractCompile::getDestinationDir)
                    .ifPresent(moduleOptions::addModuleClassesDir);

with

            MergeClassesHelper.POST_JAVA_COMPILE_TASK_NAMES.stream()
                    .map(name -> helper().findTask(name, AbstractCompile.class))
                    .filter(task -> !task.getSource().isEmpty())
                    .findAny()
                    .ifPresent(task -> moduleOptions.setCompileModuleInfoSeparately(true));

and remove COMPILE_GROOVY_TASK_NAME and COMPILE_KOTLIN_TASK_NAME constants.

In MergeClassesHelper, have the following:

    public static List<String> PRE_JAVA_COMPILE_TASK_NAMES = List.of("compileKotlin");
    public static List<String> POST_JAVA_COMPILE_TASK_NAMES = List.of("compileGroovy");

    public Stream<AbstractCompile> otherCompileTaskStream() {
      return otherCompileTaskNameStream()
                 .map(name -> helper().findTask(name, AbstractCompile.class))
                 .flatMap(Optional::stream);
    }

    private Stream<String> otherCompileTaskNameStream() {
      return Stream.concat(
              Stream.concat(PRE_JAVA_COMPILE_TASK_NAMES.stream(), POST_JAVA_COMPILE_TASK_NAMES.stream()),
              Stream.of(CompileModuleOptions.COMPILE_MODULE_INFO_TASK_NAME)
      );
    }

    public JavaCompile javaCompileTask() {
      return helper().task(JavaPlugin.COMPILE_JAVA_TASK_NAME, JavaCompile.class);
    }

    public Stream<AbstractCompile> allCompileTaskStream() {
      return Stream.concat(Stream.of(javaCompileTask()), otherCompileTaskStream());
    }

    public boolean isMergeRequired() {
      return otherCompileTaskStream().filter(task -> !task.getSource().isEmpty()).findAny().isPresent();
    }

    public Sync createMergeClassesTask() {
        return project.getTasks().create(MERGE_CLASSES_TASK_NAME, Sync.class);
    }

    public FileCollection getMergeAdjustedClasspath(FileCollection classpath) {
        if (!isMergeRequired()) {
          return classpath;
        }

        Set<File> files = new HashSet<>(classpath.getFiles());
        allCompileTaskStream().map(AbstractCompile::getDestinationDir).forEach(files::remove);
        files.add(helper().getMergedDir());
        return project.files(files.toArray());
    }

    private JavaProjectHelper helper() {
      return new JavaProjectHelper(project);
    }

In MergeClassesTask, have the following:

    public void configureMergeClasses() {
        project.afterEvaluate(p -> configureMergeClassesAfterEvaluate());
    }

    public void configureMergeClassesAfterEvaluate() {
        if (!mergeClassesHelper().isMergeRequired()) {
          return;
        }
    
        var mergeClasses = mergeClassesHelper().createMergeClassesTask();

        mergeClassesHelper().allCompileTaskStream().forEach(task -> {
          mergeClasses.from(task.getDestinationDir());
          mergeClasses.dependsOn(task);
        });
        mergeClasses.into(helper().getMergedDir());

        Stream.of(ApplicationPlugin.TASK_RUN_NAME, JavaPlugin.TEST_TASK_NAME)
            .map(helper()::findTask)
            .flatMap(Optional::stream)
            .forEach(task -> task.dependsOn(mergeClasses));
    }

I might've oversimplified some bits - feel free to point them out (I'm especially wondering if removing mergeClasses.onlyIf and checking isMergeRequired() early is OK).

Also, there might be some syntax errors (I was sketching in Notepat++).

Hope it helps! 🙂

@siordache

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 23, 2019

@tlinkowski Thanks for this great improvement! My last commit contains your proposed changes. I am delighted with your idea of handling the kotlin and groovy tasks in a generic manner (as pre-java-compile and post-java-compile tasks). It makes the code much cleaner. And with these changes, adding support for other JVM languages will be pretty easy.

@paulbakker

This comment has been minimized.

Copy link
Collaborator

commented Aug 24, 2019

@siordache @tlinkowski Since both of you went through a lot of detail in getting this perfect, feel free to merge whenever ready.

@tlinkowski

This comment has been minimized.

Copy link
Collaborator

commented Aug 24, 2019

Thanks for this great improvement! My last commit contains your proposed changes. I am delighted with your idea of handling the kotlin and groovy tasks in a generic manner (as pre-java-compile and post-java-compile tasks). It makes the code much cleaner. And with these changes, adding support for other JVM languages will be pretty easy.
-- @siordache

Thanks for your praise and your work, Serban 🙂 I like what we collaboratively achieved very much! Your last commit is great. My last suggestion before merge would be to force-push and squash the last 3 commits (all concern #101, mostly) into one (named simply "fix #101: Improve separate compilation of module-info.java") so that we have cleaner history.


Since both of you went through a lot of detail in getting this perfect, feel free to merge whenever ready.
-- @paulbakker

Thanks, Paul. However, it seems I don't have write access to this repo. I can see a button for closing this PR, but instead of a "Merge" button, I see: "Only those with write access to this repository can merge pull requests."

PS. I also don't have the privileges to manage milestones (there are two old ones that need closing, and it'd be good to create a 1.6.0 milestone and assign issues/PRs to it).

@siordache siordache force-pushed the beryx:groovy branch from b31aaef to 44c18e5 Aug 24, 2019

@siordache

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 24, 2019

it seems I don't have write access to this repo.
-- @tlinkowski

@paulbakker Tomasz and I have the triage role, which allows us to:

  • apply labels
  • close, reopen, and assign all issues and pull requests

However, we don't have write access.

@paulbakker

This comment has been minimized.

Copy link
Collaborator

commented Aug 24, 2019

it seems I don't have write access to this repo.
-- @tlinkowski

@paulbakker Tomasz and I have the triage role, which allows us to:

  • apply labels
  • close, reopen, and assign all issues and pull requests

However, we don't have write access.

Right! I thought the triage role would give write access as well. Fixed. Happy merging.

@tlinkowski tlinkowski merged commit 29d5d8d into java9-modularity:master Aug 25, 2019

1 check passed

Travis CI - Pull Request Build Passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.