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

Separate compilation of #73



Copy link

@tlinkowski tlinkowski commented Mar 21, 2019

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 ''
      tasks.register('compileModuleInfoJava', JavaCompile) {
        classpath = files()
        source = 'src/main/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).

Copy link

@paulbakker paulbakker left a comment

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. Outdated Show resolved Hide resolved Outdated
Compilation to a specific Java release

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

@paulbakker paulbakker Apr 1, 2019

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
Collaborator Author

@tlinkowski tlinkowski Apr 1, 2019

OK, will do.

Copy link
Collaborator Author

@tlinkowski tlinkowski Apr 3, 2019

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" 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.

Copy link
Collaborator Author

@tlinkowski 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.

Copy link

@paulbakker paulbakker commented Apr 1, 2019

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

tlinkowski added 4 commits Apr 2, 2019
necessary for further commits

1) renamed TestModuleOptions.isRunOnClasspath() to getRunOnClasspath() (otherwise won't work with Kotlin DSL) + added a Kotlin DSL example for "runOnClasspath = true" to
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/

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

added CompileModuleOptions and CompileModuleInfoTask

NOTE: potentially breaking change for "compileJava" in Kotlin DSL (see modified "build.gradle.kts")
tests the "modularity" extension (incl. verifying generated class file formats)
@tlinkowski tlinkowski force-pushed the compile-module-info-separately branch from 3df9122 to 86e65c4 Apr 3, 2019
Copy link
Collaborator Author

@tlinkowski 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
1 check passed
@tlinkowski tlinkowski deleted the compile-module-info-separately branch Apr 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
None yet
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants