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

Initial version of Spotless Maven plugin #188

Merged
merged 93 commits into from
Feb 12, 2018

Conversation

lutovich
Copy link
Contributor

Hi!

This PR adds a very minimal implementation of the Maven plugin. It's primary goal is to get feedback on the general approach and potential tips for further implementation.

Plugin is able to apply Eclipse formatter step to all java files in the project. It only takes a single parameter - 'eclipseFormatFile' which is a path to the formatter configuration.

Two main blocks:

  1. Build of the plugin
    This part is quite ugly. I was not able to find a good way to make Gradle natively create a JAR equivalent to one with <packaging>maven-plugin</packaging> in Maven. Such packaging includes some additional plugin configuration xml files.
    That is why Gradle build script invokes Maven via Exec plugin and makes it assemble the final plugin JAR. Two Spotless dependencies lib.jar and lib-extra.jar are installed in a directory-based Maven repo that is used during the build. They are also shaded into the final plugin JAR. Plugin sources and prepared pom.xml are copied to a temp directory. Then simple mvn clean install is executed there by Gradle Exec. Final JAR is copied to build/libs.

  2. Plugin code
    Lives in the class SpotlessMojo that has needed Maven components and 'eclipseFormatFile' config value injected by the Plexus container. It uses ArtifactResolver to create a MavenProvisioner that is able to download specified Maven dependencies.
    Mojo creates a single EclipseFormatterStep, then walks all *.java` files in the compile sources roots and formats them using this step.

Missing/unclear bits:

  • unclear if it is actually possible to not invoke Maven from Gradle
  • dependency information is duplicated in pom.xml and build.gradle. pom.xml can be generated with all dependencies from build.gradle
  • publishing of the Maven plugin to Maven Central
  • support for more configuration of the plugin, different formatter steps, line endings, etc.
  • use Formatter
  • use Maven Wrapper to not require pre-installed Maven when building
  • unclear how to version the plugin
  • unit tests

Existence of mvn command is assumed during the build. Plugin can be added to a Maven project like this:

<plugin>
  <groupId>com.diffplug.spotless</groupId>
  <artifactId>spotless-maven-plugin</artifactId>
  <version>1.0-SNAPSHOT</version>
  <configuration>
    <eclipseFormatFile>${basedir}/build/eclipse-format.xml</eclipseFormatFile>
  </configuration>
</plugin>

and invoked manually using this command:

mvn com.diffplug.spotless:spotless-maven-plugin:spotless

"As is" PR is not really ready for merge. Maybe it can be a start?

P.S. thanks a lot for the Gradle plugin! :)

Commit introduces a prototype of the Maven plugin which is able to
apply Eclipse formatter step to all java files in the project. It
only takes a single parameter - 'eclipseFormatFile' which is a path
to the formatter configuration.

Maven plugin is build and packaged using Maven which is executed from
the Gradle build script via Exec plugin. This seems to be the simplest
way to get a JAR with "maven-plugin" packaging. Gradle does not have
a native plugin to do this.

This change just adds a bare minimum to verify the approach.
@nedtwigg
Copy link
Member

Huzzah!! Fireworks!!! Thanks so much @lutovich, great to finally have this off the ground! Having a functional plugin and implementation of MavenProvisioner is a huge deal! I'm going to cheat and take the easy ones first :)

  • unclear how to version the plugin / publishing of the Maven plugin to Maven Central

As steps are added and improved, lib and lib-extra clunk along 1.n -> 1.n+1. For a build-specific plugin like plugin-gradle, it starts off at MAJOR.n, and each time a new lib comes out, a new plugin-gradle also comes out with a matching .n. The MAJOR is bumped to indicate that something about the integration code requires a breaking change - dropping an old version of gradle, changing the buildscript, etc. I would propose we do the same with the maven plugin. If the MAJOR ends up at 1, 2, 3, or even 15 before it stabilizes, I don't think it matters. Just as long as we're honest about when users will have to update their buildscripts.

Uploading this plugin to mavenCentral on each release will be trivial, I'll have to turn some cranks for the initial release but it'll be automatic from there.

  • dependency information is duplicated in pom.xml and build.gradle. pom.xml can be generated with all dependencies from build.gradle

Do maven plugins require that they have no dependencies? It seems that if we remove the shading, the build will get a lot simpler. The gradle project has the stableLib property, which is the version of the latest libs in mavenCentral, and versionLib, which is the latest published snapshot (published to https://oss.sonatype.org/content/repositories/snapshots/). It's rare that changes in lib and the plugin are coupled (I can't think of any cases).

  • use Maven Wrapper to not require pre-installed Maven when building

I trust your research on the tradeoffs of creating maven metadata manually vs shelling out to maven. I would love to switch to mvnw to better control what version of Maven gets used, if it's not a big job.

  • Formatter / other steps / etc.

Without being inside Formatter, some of the FormatterStep can misbehave. They rely on always getting \n line endings, and Formatter is the code that maintains that contract.

I'm 100% fine with v1 of the maven plugin supporting only a single formatter step - this was definitely the right place to start. Just to establish what we'd need to reach near-parity with the gradle plugin:

spotless {
  encoding 'UTF-8'
  lineEndings 'GIT_ATTRIBUTES'
  generic {
    encoding / lineEndings
    license
    replaceRegex
    endWithNewline
    indent
  }
  java {
    eclipse / importOrder / googleJavaFormat
  }
  groovy {
    greclipse
  }
  kotlin {
    ktlint
  }
  scala {
    scalaFmt
  }
}

The natural way for Spotless to work is:

  • define a set of files
  • for that set, define a list of steps
  • the generic steps are always available, and the other steps depend on what kind of files they are

I've got no idea if a structure anything like that is possible in the @Mojo annotation world. That seems like the next big question. Something like:

  <configuration>
    <java>
      <licenseHeader>...</licenseHeader>
      <eclipseFormatFile>${basedir}/build/eclipse-format.xml</eclipseFormatFile>
    </java>
    <kotlin>
      <licenseHeader>...</licenseHeader>
      <ktlint>...</ktlint>
    </kotlin>
  </configuration>

If we can get v1 to support at least all of <java>, that seems like a big win relative to supporting only a single step, but I'm fine with starting small.

@lutovich
Copy link
Contributor Author

Hi @nedtwigg, thanks for the response!

Do maven plugins require that they have no dependencies?

I think it's fine for a maven plugin to have dependencies. Shading was added because plugin needs lib and lib-extra when building. I thought maven plugin will be released together with lib and lib-extra and that's why can't depend on any specific version, because such version is released together with the plugin itself. Gradle plugin contains this in the build script:

if (!project.versionGradle.endsWith('-SNAPSHOT') && project.versionLib.endsWith('-SNAPSHOT')) {
  // gradle = release, lib = snapshot, therefore gradle should depend on the last stable lib
  compile "com.diffplug.spotless:spotless-lib:${project.stableLib}"
  compile "com.diffplug.spotless:spotless-lib-extra:${project.stableLib}"
} else {
  compile project(':lib')
  compile project(':lib-extra')
}

I think else branch is equivalent to what is achieved by shading - plugin depends on lib and lib-extra from the same version of the project/source. if branch however is missing and does not require shading - plugin can just depend on lib and lib-extra of version project.stableLib. Is my understanding correct? Do you think it is possible to completely skip shading and depend on project.stableLib or project.versionLib version of lib and lib-extra in the pom?

If we can get v1 to support at least all of <java>, that seems like a big win relative to supporting only a single step, but I'm fine with starting small.

Can't fully assess the complexity of other build steps right now. Feels like it should be easy to add them when configuration and Formatter infra is in place.

Feels like I have 3 main things to work on next:

  • improve build of the plugin, use mvnw, prettify dependencies in pom
  • use Formatter
  • figure out how to pass nested configuration from pom.xml to the plugin

Does these look fine to you?

Thanks!

@lutovich
Copy link
Contributor Author

Oh and one other small question. Do you prefer to treat this PR as a feature branch? I can keep adding commits to this branch and it can be merged when stable. Another option is to prettify it a bit then merge then create other small PRs for subsequent improvements.

@nedtwigg
Copy link
Member

Do you think it is possible to completely skip shading and depend on project.stableLib or project.versionLib version of lib and lib-extra in the pom?

Yup! The point of the "else" block that you point out is to ensure that a published gradle plugin never accidentally depends on a SNAPSHOT version of the lib. That happened once on accident, so we put that in to prevent a relapse.

Do you prefer to treat this PR as a feature branch? Another option is to prettify it a bit then merge then create other small PRs for subsequent improvements.

My preference is to treat this as a feature branch for now. If we hit a forking point (maybe we should do A or B) then I'd be happy to merge.

@nedtwigg
Copy link
Member

I took the liberty of Formatter-fying your code. Feel free to revert or force push if it causes merge conflicts for you. Of the three todo's you identified (mvnw, Formatter, and nested config), this is the only one that I can help much with :)

Exposed `encoding` and `lineEndings` in the plugin xml configuration.
Renamed `eclipseFormatFile` to `eclipseConfigFile` and moved it inside
the `<java>` tag. Maven plugins can handle nested configs using Plexus
dependency injection and POJO objects. Thus introduced a dedicated
class for all java related configuration called `Java`.

Plugin xml configuration can now look like:

```
<plugin>
  <groupId>com.diffplug.spotless</groupId>
  <artifactId>spotless-maven-plugin</artifactId>
  <version>${spotless.version}</version>
  <configuration>
    <encoding>UTF-8</encoding>
    <lineEndings>UNIX</lineEndings>
    <java>
      <eclipseConfigFile>${basedir}/eclipse-fmt.xml</eclipseConfigFile>
    </java>
  </configuration>
</plugin>
```

Extracted an abstract `AbstractSpotlessMojo` to hold all injected
dependencies. It can be potentially used in future to implement
the `check` goal.

Changed name of `SpotlessMojo` from "spotless" to "apply" so that
it can be invoked with `mvn spotless:apply`.
@lutovich
Copy link
Contributor Author

Thanks @nedtwigg.

Added another commit that improves config handling. It makes use of nested xml tags, like you suggested. Will next simplify build process to use project.stableLib.

lutovich and others added 11 commits January 17, 2018 22:39
Injected `MavenProject` is now replaced by injection of multiple
primitive values, like `project.basedir`.
Removed shading & local repository for `lib` and `lib-extra`. Instead
made plugin directly depend on stable version denoted
by `project.stableLib`.

Made all maven plugin API dependencies provided and their versions
injected by the Gradle build script.
These don't match the current implementation exactly, but
I think they're close.  I don't know much about maven, lets
get the docs right, and then build to the docs.
Gradle build script will now use Mustache template to create pom.xml
with needed versions. This should be more reliable and future-proof
than previously used `String#replace()`.
Restructured code to better support addition of new formatter steps and
formatters. Spotless mojo now accepts java configuration with list of
steps. It is also possible to specify global encoding, line endings
and override them for java.

New `pom.xml` config looks like:

```
<configuration>
  <encoding>UTF-8</encoding>
  <lineEndings>UNIX</lineEndings>

  <java>
    <encoding>US-ASCII</encoding>
    <lineEndings>WINDOWS</lineEndings>
    <steps>
      <eclipse>
        <file>${basedir}/build/eclipse-format.xml</file>
        <version>4.7.1</version>
      </eclipse>
    </steps>
  </java>
</configuration>
```
@lutovich
Copy link
Contributor Author

lutovich commented Jan 21, 2018

Hi @nedtwigg,

Thanks for adding docs! Last commit makes impl very close but not entirely follow them. Couple questions:

  1. Maven docs recommend naming plugins like <yourplugin>-maven-plugin. Do you think it's fine to keep spotless-maven-plugin name instead of spotless-plugin-maven?

  2. I'm not sure how to control step ordering without additional indirection in the pom.xml. Right now config looks like:

    <configuration>
      <encoding>UTF-8</encoding>
      <lineEndings>UNIX</lineEndings>
    
      <java>
        <encoding>US-ASCII</encoding>
        <lineEndings>WINDOWS</lineEndings>
        <steps>
          <eclipse>
            <file>${basedir}/build/eclipse-format.xml</file>
            <version>4.7.1</version>
          </eclipse>
        </steps>
      </java>
    </configuration>

    notice an additional <steps> tag. It makes configuration for all steps be injected into mojo as a list, so ordering is defined. Do you think it's ok to have this <steps> tag?

  3. Current conventional naming of the plugin and mojo allows shorter command line invocation
    mvn spotless:apply. Do you like it this way or think it should be same as in docs
    mvn com.diffplug.spotless:spotless-plugin-maven:spotless-apply?

Thanks in advance for your answers.

I'll proceed with adding more steps (as docs suggest), tests and implementation of the check goal.

@nedtwigg
Copy link
Member

I changed the docs and artifactId to be "spotless-maven-plugin". I lean towards leaving the project's folder in the Spotless git repo to be "plugin-maven" - a little confusing, but seems a worthwhile compromise. I also added the <steps> container and changed task names to "mvn spotless:check/apply".

I'd be happy to help adding steps, but I don't know how to run tests. I've got an idea for how we could reuse a lot of the plugin-gradle tests, I'll push a skeleton in the next hour or two.

@nedtwigg
Copy link
Member

I added a first cut at an integration test framework for the maven plugin. Feel free to revert and do something completely different if you prefer. <sheepish>I think maybe we need your publish to a local maven repo back</sheepish>. I'm not sure how to make MavenRunner work without it...

Process process = builder.start();
// slurp and return the stdout, stderr, and exitValue
Slurper output = new Slurper(process.getInputStream());
Slurper error = new Slurper(process.getErrorStream());
Copy link
Member

Choose a reason for hiding this comment

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

@nedtwigg If we expect MavenRunner to be instantiated many times, I think we should consider using a shared ExecutorService, like an Executors.newFixedThreadPool(Runtime.getRuntime().getAvailableProcessors() - 1). Otherwise, this whole commit looks pretty reasonable to me. :)

Copy link
Member

Choose a reason for hiding this comment

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

To clarify, I'm proposing we use an ExecutorService instead of using Threads directly through the Slurper class.

Copy link
Member

Choose a reason for hiding this comment

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

Roger. There will only be 10's of integration tests, so I don't think there's much of a performance hit for starting 2x threads per.

Copy link
Member

Choose a reason for hiding this comment

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

Okay, that sounds reasonable to me.

while ((numRead = input.read(buffer)) != -1) {
output.write(buffer, 0, numRead);
}
result = output.toString(Charset.defaultCharset().name());
Copy link
Member

Choose a reason for hiding this comment

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

@nedtwigg Minor nit: I seem to remember that we have access to Guava in some way through a re-packaged version that Diffplug distributes on Maven Central and JCenter.

If we do have access to Guava in this way, then I think we should consider using ByteStreams.copy to simplify the code inside this try statement. But I'd understand if you don't want to do this for dependency reasons or such. :)

Copy link
Member

Choose a reason for hiding this comment

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

Good call, implemented in next commit. For the rest of this review, I'd like to focus on bashing out the high-level design. We can fix performance nits and duplicate code after we have something working :)

Copy link
Member

Choose a reason for hiding this comment

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

Roger! :)

It is not needed because maven plugin and tests specify custom
local repository via `-Dmaven.repo.local` system property.
@lutovich
Copy link
Contributor Author

lutovich commented Feb 4, 2018

I think CI build is now fixed. Build and tests for maven plugin will now use custom local repository instead of .m2. It is specified via command line -Dmaven.repo.local=<DIR> system property. This solves dependency caching problems and removed need for <repositories> and <pluginRepositories> in POMs. I can only see one drawback right now - it will download all dependencies every time because custom repo is initially always an empty folder.

What do you think about this solution?

@nedtwigg
Copy link
Member

nedtwigg commented Feb 5, 2018

Huzzah!!! You fixed it!!! I think that solution is okay, but not ideal. I added a slight tweak - we now cache the plugin-maven/build/localMavenRepository folder, but we delete the entire com.diffplug.spotless group before before saving the cache, so none of our artifacts ever gets cached.

@lutovich
Copy link
Contributor Author

lutovich commented Feb 5, 2018

Nice! I'll continue with include/exclude configs next. Current logic of scanning everything from project.basedir is most likely not ideal, especially for multi-module maven projects.

This commit makes it possible to configure includes and excludes for
paths/files with Ant style pattern in configuration groups (currently
Java and Scala). It also changes the default source roots for scanning.

Default policy for scanning used to recursively start from the `basedir`
and include files with needed extensions, like "*.java" or "*.scala".
This worked fine for simple maven projects but resulted in a lot of
extra work for multi-module maven projects. Same files had to be
scanned more than once during parent and then child module scanning.
Now scanning will use most common source roots for supported languages.
They are "scr/main/java/**/*.java", "scr/test/java/**/*.java" for Java
and "scr/main/scala/**/*.scala", "scr/main/scala/**/*.sc",
"scr/test/scala/**/*.scala", "scr/test/scala/**/*.sc" for Scala. So
only these common source roots will be scanned by default.

Mentioned patters form default includes. Default excludes are empty,
except output directory (usually "target"), temporary files and VCS
files are always skipped. It is possible to override includes and add
excludes. This can be done like this:

```
<java>
  <includes>
    <!-- include all java files in "java" folders under "src" -->
    <include>src/**/java/**/*.java</include>

    <!-- include all java files in "java" folders under "other" -->
    <include>other/java/**/*.java</include>
  </includes>

  <excludes>
    <!-- exclude examples from formatting -->
    <exclude>src/test/java/**/*Example.java</exclude>
  </excludes>
</java>
```

Similar configuration is possible for Scala. Custom includes completely
override default includes. Default excludes of output directory,
temporary and VCS files can't be overridden.
@nedtwigg
Copy link
Member

Hurrah! Extremely well-done. I'm ready to publish 0.x to mavenCentral. Get a round of feedback from users, get some example configurations to link to, and then we can publish 1.x and do some marketing. Any objections?

@lutovich
Copy link
Contributor Author

Could you please give me a day to play with the plugin and try it on couple projects? :)
I'm mostly interested in trying it out on multi-module maven projects.

@nedtwigg
Copy link
Member

Absolutely! Take as much time as you need.

@lutovich
Copy link
Contributor Author

I'm a bit uncertain about what to do with multi-module project support. It would definitely be nice for mvn spotless:check and mvn spotless:apply to work when executed in the root module and in sub-modules. Recommended way to achieve this is to create a separate module that only contains configuration (eclipse config, license header text, etc. in case of spotless) and depend on this module in the plugin declaration. Then all configs will be in the classpath and accessible to the plugin. Maven checkstyle and PMD recommend this way.

However, there exists a different, hackish way :) It is used by the Checkstyle plugin but is undocumented: source code. Here plugin iterates through the module hierarchy, which kind of violates the maven philosophy that module should only care about itself. This functionality makes it very convenient to specify files only once in the root module. Relevant checkstyle plugin issue and PMD issue contain a bit more details. Note that PMD authors closed the issue with "Won't fix" because this approach is considered to be a hack, though a very convenient one.

Despite it's convenience, later solution is a hack. I thought about implementing it but it's probably not a good idea, especially for the first iteration. Maybe it never needs to be implemented because it's not the "true maven way".

What do you think about this? I guess it's fine to proceed with 0.x release if we decide to not implement this thing.

Generic formatting steps, like LicenseHeader can be defined both
globally and inside language configs. Example:

```
<configuration>
<!-- global license header config -->
<licenseHeader>...</licenseHeader>

<java>
  <!-- overridden license header for java -->
  <licenseHeader>...</licenseHeader>
</java>
</configuration>
```

Previously global config had no effect. This commit fixes the problem
by passing globally configured generic steps down to `FormatterFactory`
which creates steps and uses globally configured ones, unless
overridden.
@nedtwigg
Copy link
Member

It seems that even the authors of the Checkstyle plugin aren't very happy with their hack.

// MCHECKSTYLE-131 ( olamy ) I don't like this hack.
// (dkulp) Me either.   It really pollutes the location stuff
// by allowing searches of stuff outside the current module.

I'm an incredibly novice maven user, but the "official" multi-module solution described in your linked checkstyle and PMD docs doesn't seem terrible. A little verbose, but it seems like everything in maven is pretty verbose ;-) Ready to release when you are!

License header is a generic step that is applicable to both `Java` and
`Scala`. This commit moves adder for it to `FormatterFactory`, which
is super class of both `Java` and `Scala`. Code duplication is thus a
bit reduced.
@lutovich
Copy link
Contributor Author

Yeah, I definitely agree that separate module just for configs is seriously verbose, even for Maven :)
I'm done with small fixes and ready for the release! 🎉

@nedtwigg nedtwigg merged commit 7ea2d66 into diffplug:master Feb 12, 2018
@nedtwigg
Copy link
Member

It should be available now on mavencentral as 1.0.0.BETA1. Unfortunately, there was already a 1.0.0.ALPHA which had been published to jcenter way over a year ago when we scaffolded this project out. Wanna do a sanity check, then we'll announce that #102 has been implemented and is ready for users!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants