Skip to content

Separate compilation of module-info.java#73

Merged
paulbakker merged 4 commits intojava9-modularity:masterfrom
tlinkowski:compile-module-info-separately
Apr 4, 2019
Merged

Separate compilation of module-info.java#73
paulbakker merged 4 commits intojava9-modularity:masterfrom
tlinkowski:compile-module-info-separately

Conversation

@tlinkowski
Copy link
Copy Markdown
Collaborator

@tlinkowski tlinkowski commented Mar 21, 2019 •
edited
Loading

A proof of concept for #72 (actually, it's a fully working solution, but since #72 hasn't been discussed yet, I call it a proof of concept for now).

This PR consists of four clearly separated commits (every commit passes the build):

  1. initial refactorings/improvements
    • no new features introduced - necessary to make further commits much clearer
    • bugfix: renamed TestModuleOptions.isRunOnClasspath() to getRunOnClasspath() (otherwise it doesn't work with Kotlin DSL)
    • commit details
  2. support for moduleOptions.compileModuleInfoSeparately (low-level API)
    • when moduleOptions.compileModuleInfoSeparately = true, introduces a special compileModuleInfoJava task → boils down to (Groovy DSL): 
      compileJava {
  exclude 'module-info.java'
}

tasks.register('compileModuleInfoJava', JavaCompile) {
  classpath = files()
  source = 'src/main/java/module-info.java'
  destinationDir = compileJava.destinationDir
}

compileModuleInfoJava.dependsOn compileJava
classes.dependsOn compileModuleInfoJava
      + modular behavior is configured on compileModuleInfoJava instead of compileJava (but always using compileJava's class-path as module-path)
    • module-info.class location: root output directory
    • NOTE: introduces CompileModuleOptions extends ModuleOptions, which is a potentially breaking change for compileJava in Kotlin DSL (see modified build.gradle.kts)
    • commit details
  3. support for modularity project extension (high-level API)
  4. added test-project-mixed
    • tests modularity.mixedJavaRelease and modularity.standardJavaRelease
    • the test verifies generated class file versions
    • project is "mixed" in two senses:
      1. classes target various JDKs
      2. build files are in Groovy DSL & Kotlin DSL
    • I managed to get rid of local_maven_build.gradle and smoke_test_build.gradle by using pluginManagement in *_settings.gradle files (can be ported to other test projects)
    • commit details

PS. If you decide to merge this PR, I'd appreciate if you didn't squash the commits (they are separated on purpose, to provide a clear change history).

@tlinkowski tlinkowski force-pushed the compile-module-info-separately branch from 9b94f1d to 365192c Compare March 21, 2019 13:16
This was referenced Mar 21, 2019
Closed
Closed
@tlinkowski tlinkowski mentioned this pull request Mar 29, 2019
Closed
4 tasks
paulbakker
paulbakker reviewed Apr 1, 2019
View reviewed changes
Copy link
Copy Markdown
Collaborator

@paulbakker paulbakker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Finally had time to review this. Really excellent work!
I have a few very minor suggestions and questions, but other than that I'm happy to merge.

Comment thread src/main/java/org/javamodularity/moduleplugin/extensions/ModularityExtensionImpl.java Outdated
Comment thread src/main/java/org/javamodularity/moduleplugin/extensions/ModularityExtensionImpl.java
Comment thread README.md Outdated
Comment thread README.md Outdated
Compilation to a specific Java release
----

For Java releases 6-8, see [Separate compilation of `module-info.java`](#separate-compilation-of-module-infojava).
Copy link
Copy Markdown
Collaborator

@paulbakker paulbakker Apr 1, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add some more description why this is useful and when to use which option. You explained this really well in the issue, maybe move some of that to the docs.

Copy link
Copy Markdown
Collaborator Author

@tlinkowski tlinkowski Apr 1, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, will do.

Copy link
Copy Markdown
Collaborator Author

@tlinkowski tlinkowski Apr 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added more description, although I didn't copy it from the issue, because it seemed to me I already copied all the necessary stuff to the "Separate compilation of module-info.java" section.

Please, let me know if the updated version reads well. I'll gladly reword/restructure it further if you provide me with any remarks you might have.

@tlinkowski
Copy link
Copy Markdown
Collaborator Author

tlinkowski commented Apr 1, 2019

Thanks for your feedback, Paul! I'll provide the requested changes within a few days.

Please, let me know if you prefer:

  • a force push with modified commits (total of 4, as now),
  • or just one more commit (total of 5).

As I mentioned, I'd much prefer that the commits aren't squashed for clear change history.

@paulbakker
Copy link
Copy Markdown
Collaborator

paulbakker commented Apr 1, 2019

I'm fine with either, whatever you think shows intentions best.

@tlinkowski
java9-modularity#72: initial refactorings/improvements
86e65c4 
necessary for further commits

MAIN:
1) renamed TestModuleOptions.isRunOnClasspath() to getRunOnClasspath() (otherwise won't work with Kotlin DSL) + added a Kotlin DSL example for "runOnClasspath = true" to README.md
2) introduced JavaProjectHelper and applied it to CompileTask
3) added comments for all anonymous classes that should not be removed
4) minor improvements in ModuleSystemPlugin
5) minor fixes in test-project-kotlin/README.md

TEST:
1) bumped smoke-test Gradle version to 5.0 (for improved Kotlin DSL)
2) introduced a no-op "moduleOptions" access in greeter.api (for testing DSL)
3) introduced SmokeTestHelper for ModulePluginSmokeTest
4) disabled stackTraceFilters in all tests
5) updated Kotlin to 1.3.20
@tlinkowski
java9-modularity#72: support for "moduleOptions.compileModuleInfoSepa…
00c78ce 
…rately"

added CompileModuleOptions and CompileModuleInfoTask

NOTE: potentially breaking change for "compileJava" in Kotlin DSL (see modified "build.gradle.kts")
@tlinkowski
java9-modularity#72: support for "modularity" project extension
c5b98dc 
via ModularityExtension interface
@tlinkowski
java9-modularity#72: added test-project-mixed
bf40098 
tests the "modularity" extension (incl. verifying generated class file formats)
@tlinkowski tlinkowski force-pushed the compile-module-info-separately branch from 3df9122 to 86e65c4 Compare April 3, 2019 20:07
@tlinkowski
Copy link
Copy Markdown
Collaborator Author

tlinkowski commented Apr 3, 2019

I force-pushed the new version. Please, review.

PS. I also rebased it to the current master so that you don't have to resolve a syntactic conflict (visibility of ModuleOptions.mutateArgs) that occurred due to @aalmiray's last merged PR (#74).

@paulbakker paulbakker merged commit 284b3c3 into java9-modularity:master Apr 4, 2019
@tlinkowski tlinkowski deleted the compile-module-info-separately branch April 4, 2019 18:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@paulbakker paulbakker paulbakker left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@tlinkowski @paulbakker