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

Spotless Maven apply should be bound to a phase by default #1135

Closed
aaime opened this issue Feb 19, 2022 · 4 comments
Closed

Spotless Maven apply should be bound to a phase by default #1135

aaime opened this issue Feb 19, 2022 · 4 comments
Assignees
Labels

Comments

@aaime
Copy link

aaime commented Feb 19, 2022

I have a multi-module Maven project that contains a number of XML files, which I'd like to format with the WTP formatter, using a custom configuration for it. The configuration file resides in a top-level directory.
Here is a snipped of the Maven pom file:

      <!-- This one sets the top level directory reference -->
      <plugin>
        <groupId>org.commonjava.maven.plugins</groupId>
        <artifactId>directory-maven-plugin</artifactId>
        <version>0.3.1</version>
        <executions>
          <execution>
            <id>directories</id>
            <phase>initialize</phase>
            <goals>
              <goal>highest-basedir</goal>
            </goals>
            <configuration>
              <property>geoserverBaseDir</property>
            </configuration>
          </execution>
        </executions>
      </plugin>

      <plugin>
        <groupId>com.diffplug.spotless</groupId>
        <artifactId>spotless-maven-plugin</artifactId>
        <version>2.20.0</version>
        <executions>
          <execution>
            <phase>process-sources</phase>
            <goals>
              <goal>${spotless.action}</goal>
            </goals>
          </execution>
        </executions>
        <configuration>
          <java>
            <googleJavaFormat>
              <version>1.7</version>
              <style>AOSP</style>
            </googleJavaFormat>
          </java>
          <formats>
            <format>
              <includes>
                <include>src/main/java/application*Context.xml</include>
                <include>src/main/resources/application*Context.xml</include>
                <include>src/test/resources/**/application*Context.xml</include>
              </includes>
              <eclipseWtp>
                <type>XML</type>
                <files>
                  <!-- uses the top level dir to find the configuration file, one for all 100+ modules -->
                  <file>${geoserverBaseDir}/../build/qa/xml-format.properties</file>
                </files>
                <version>4.13.0</version>
              </eclipseWtp>
            </format>
          </formats>

          <upToDateChecking>
            <enabled>true</enabled>
            <indexFile>${project.basedir}/.spotless-index</indexFile>
          </upToDateChecking>
        </configuration>
      </plugin>

Trying to run mvn spotless:apply fails because the geoserverBaseDir is not initialized. I've been told in other reports this is likely happening because the SpotlessApplyMojo is not bound to a default phase, unlike for example the SpotlessCheckMojo.

The process-sources phase seems like it would be a good candidate for a default phase? Here is the reference to all phases.

Reference versions and OS:

  • Apache Maven 3.6.3
  • Spotless 2.20.0
  • Linux Mint 20.3
  • Java 1.8.0_312
@aaime aaime changed the title Spotless Maven apply should be bound to a phase Spotless Maven apply should be bound to a phase by default Feb 19, 2022
@lutovich
Copy link
Contributor

Hi @aaime,

I investigated this problem using your branch mentioned in the previous issue: https://github.com/aaime/geoserver/tree/spotless-wtp.

I think setting the default phase for SpotlessApplyMojo does not help. Both mvn spotless:apply and mvn spotless:check fail because they can't read ${geoserverBaseDir}/../build/qa/xml-format.properties. Checkstyle plugin also fails with the same error:

$ mvn -Pcheckstyle checkstyle:check

[INFO] BUILD FAILURE
[INFO] ------------------------------------------------------------------------
[INFO] Total time:  1.002 s
[INFO] Finished at: 2022-03-30T22:58:27+02:00
[INFO] ------------------------------------------------------------------------
[ERROR] Failed to execute goal org.apache.maven.plugins:maven-checkstyle-plugin:3.1.1:check (default-cli) on project geoserver: Failed during checkstyle execution: Unable to find configuration file at location: ${geoserverBaseDir}/../build/qa/checkstyle.xmll: Could not find resource '${geoserverBaseDir}/../build/qa/checkstyle.xmll'. -> [Help 1]

Note the -Pcheckstyle that activates the checkstyle profile and makes the plugin try to load the resource from the <configLocation> XML element. I think without -Pcheckstyle, mvn checkstyle:check simply runs with some default configuration and does not try to load the resource from <configLocation>.

Direct plugin invocations seem to be problematic because they rely on the geoserverBaseDir property that is set by a different plugin. This could work during a maven build that executes multiple phases, but direct plugin invocations execute only individual goals. To check this assumption, we can execute both directory-maven-plugin and spotless in a single command:

$ mvn org.commonjava.maven.plugins:directory-maven-plugin:highest-basedir@directories spotless:check
...

[INFO] ----------------------< org.geoserver:geoserver >-----------------------
[INFO] Building GeoServer 2.20-SNAPSHOT                                  [1/37]
[INFO] --------------------------------[ pom ]---------------------------------
[INFO]
[INFO] --- directory-maven-plugin:0.3.1:highest-basedir (directories) @ geoserver ---
[INFO] Highest basedir set to: /Users/lutovich/Projects/geoserver/src
[INFO]
[INFO] --- spotless-maven-plugin:2.22.0-SNAPSHOT:check (default-cli) @ geoserver ---
[INFO]
[INFO] ---------------------< org.geoserver:gs-platform >----------------------
[INFO] Building Core Platform Module 2.20-SNAPSHOT                       [2/37]
[INFO] --------------------------------[ jar ]---------------------------------
[INFO]
[INFO] --- directory-maven-plugin:0.3.1:highest-basedir (directories) @ gs-platform ---
[INFO] Highest basedir set to: /Users/lutovich/Projects/geoserver/src
[INFO]
[INFO] --- spotless-maven-plugin:2.22.0-SNAPSHOT:check (default-cli) @ gs-platform ---
...
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time:  4.476 s
[INFO] Finished at: 2022-03-30T23:07:06+02:00
[INFO] ------------------------------------------------------------------------

Note the @directories that tells Maven to run a specific directory-maven-plugin execution with ID directories that initializes geoserverBaseDir.

I think we can’t teach Spotless resolve geoserverBaseDir because this property comes from a different plugin.

We could bind SpotlessApplyMojo to process-sources phase but it wouldn't solve your problem. I think this will only allow users to omit <phase> in apply goal's configuration:

<plugin>
  <groupId>com.diffplug.spotless</groupId>
    <artifactId>spotless-maven-plugin</artifactId>
    <version>${spotless.version}</version>
    <executions>
      <execution>
        <id>apply-id</id>
        <!-- <phase>process-sources</phase> --> <!-- no need for this line -->
        <goals>
          <goal>apply</goal>
        </goals>
      </execution>
    </executions>
 </plugin>

Any thoughts or input much appreciated :)

@aaime
Copy link
Author

aaime commented Apr 22, 2022

Let me go back to the question that triggered this report then: is there a way to use the Spotless with WTP in a multi-module maven project, having multiple nesting levels, so pretty much needing an absolute reference to the configuration file?
Maybe including the config inline in the pom? But I did not see such a possibility.

The main difference between spotless and checkstyle is that we run spotless in every build, to reformat the files, while checkstyle is only used when specifically invoked (part of the QA profiles) because it would take too much time. In other words, we should not ask developers to enable a profile for something that should always be done.

@nedtwigg nedtwigg reopened this Apr 22, 2022
@lutovich
Copy link
Contributor

Hi @aaime,

It is not possible to inline the formatter config into the pom. Spotless maven plugin supports multi-module projects but the project structure requires a bit of additional configuration. You could follow this guide for PMD plugin multi-module configuration or take a look at #210 file MultiModuleProjectTest.java for a concrete example. Essentially, you add a new sub-module that contains all build-related resources like formatter XML config files. Then, pom configuration of the plugin itself should depend on the new sub-module. This way you remove the need to reference config files by an absolute path.

Please let us know if this suggestion works for you 🙏

Perhaps we need a new documentation section explaining multi-module setup with concrete examples 🤔

bwgjoseph added a commit to bwgjoseph/spring-boot-mvn-starter that referenced this issue Dec 10, 2022
after moved to parent project, running `./mvnw clean verify`, project will be built correctly
but if changed to use `./mvnw spotless:apply`, it will throw the following error

```
No plugin found for prefix 'spotless' in the current project and in the plugin groups [org.apache.maven.plugins, org.codehaus.mojo] available from the repositories [local (C:\Users\Joseph\.m2\repository), central (https://repo.maven.apache.org/maven2)]
```

Running `./mvnw -pl starter-mvc-core spotless:apply`would be fine though

To fix root cause, the fix, it seem, is to ensure submodules point to the parent pom.xml

See diffplug/spotless#1135
Reference https://kurular4.medium.com/why-you-should-use-parent-pom-for-your-multi-module-java-projects-b575017fab2e
bwgjoseph added a commit to bwgjoseph/spring-boot-mvn-starter that referenced this issue Dec 10, 2022
* initiate converting to maven multi-module structure

* move spotless plugin to parent project

after moved to parent project, running `./mvnw clean verify`, project will be built correctly
but if changed to use `./mvnw spotless:apply`, it will throw the following error

```
No plugin found for prefix 'spotless' in the current project and in the plugin groups [org.apache.maven.plugins, org.codehaus.mojo] available from the repositories [local (C:\Users\Joseph\.m2\repository), central (https://repo.maven.apache.org/maven2)]
```

Running `./mvnw -pl starter-mvc-core spotless:apply`would be fine though

To fix root cause, the fix, it seem, is to ensure submodules point to the parent pom.xml

See diffplug/spotless#1135
Reference https://kurular4.medium.com/why-you-should-use-parent-pom-for-your-multi-module-java-projects-b575017fab2e

* move maven-toolchains to parent pom

* move maven-enforcer to parent pom

fix an issue where requireJavaVersion is not enforced, which is caused by having multiple <rules>, it only requires a single <rules>

* move maven-site to parent pom

When running mvn site on a top level project, Maven will build all sites for every module defined in the modules section of the pom individually. Note that each site is generated in each individual project's target location. As a result, relative links between different modules will NOT work. They will however work when you deploy or stage the site. See https://maven.apache.org/plugins/maven-site-plugin/examples/multimodule.html

Note that https://maven.apache.org/plugins/maven-site-plugin/examples/multimodule.html#aggregate-reports is not configured yet

* move versions-maven to parent pom

when running command from parent pom, it will scan through parent and all sub-modules to update the pom.xml, if want to scan specify pom.xml then run with -pl <module> command

See https://stackoverflow.com/questions/4146638/maven-variable-for-reactor-root/48879554#48879554 for using ${session.executionRootDirectory} as the root directory of where .mvnw is ran
See https://cwiki.apache.org/confluence/display/MAVEN/Maven+Properties+Guide for a standard maven variables

* move git-commit-id-maven to parent pom

when run `./mvnw verify`, the sub-modules will run the git info and generate in its build directory

* move modernizer-maven to parent pom

Possible to suppress via annotation, see https://github.com/gaul/modernizer-maven-plugin#ignoring-elements

when run `./mvnw verify`, the sub-modules will run the modernizer

* move spotbugs to parent pom

adds spotbugs reporting to submit when running ./mvnw site
however, spotbugs report is not reporting on parent site project but is reporting at the sub-module

* move maven-pmd to parent pom

* move jacoco-maven to parent pom

thinking if jacoco should exist in its own sub-module but still leave it at parent pom first

* move java.version properties to parent pom

* move jacoco and git-commit-id-maven to pluginManagement section

Placing in pluginManagement kind of allowing to centalize the configuration but not taking effect on any modules until any of the module declare this in the plugin section on their pom.xml

See https://www.baeldung.com/maven-plugin-management

* polish pom.xml
@lutovich lutovich closed this as not planned Won't fix, can't repro, duplicate, stale Feb 4, 2023
@spotlesscoder
Copy link

Hi @aaime,

I investigated this problem using your branch mentioned in the previous issue: https://github.com/aaime/geoserver/tree/spotless-wtp.

I think setting the default phase for SpotlessApplyMojo does not help. Both mvn spotless:apply and mvn spotless:check fail because they can't read ${geoserverBaseDir}/../build/qa/xml-format.properties. Checkstyle plugin also fails with the same error:

$ mvn -Pcheckstyle checkstyle:check

[INFO] BUILD FAILURE
[INFO] ------------------------------------------------------------------------
[INFO] Total time:  1.002 s
[INFO] Finished at: 2022-03-30T22:58:27+02:00
[INFO] ------------------------------------------------------------------------
[ERROR] Failed to execute goal org.apache.maven.plugins:maven-checkstyle-plugin:3.1.1:check (default-cli) on project geoserver: Failed during checkstyle execution: Unable to find configuration file at location: ${geoserverBaseDir}/../build/qa/checkstyle.xmll: Could not find resource '${geoserverBaseDir}/../build/qa/checkstyle.xmll'. -> [Help 1]

Note the -Pcheckstyle that activates the checkstyle profile and makes the plugin try to load the resource from the <configLocation> XML element. I think without -Pcheckstyle, mvn checkstyle:check simply runs with some default configuration and does not try to load the resource from <configLocation>.

Direct plugin invocations seem to be problematic because they rely on the geoserverBaseDir property that is set by a different plugin. This could work during a maven build that executes multiple phases, but direct plugin invocations execute only individual goals. To check this assumption, we can execute both directory-maven-plugin and spotless in a single command:

$ mvn org.commonjava.maven.plugins:directory-maven-plugin:highest-basedir@directories spotless:check
...

[INFO] ----------------------< org.geoserver:geoserver >-----------------------
[INFO] Building GeoServer 2.20-SNAPSHOT                                  [1/37]
[INFO] --------------------------------[ pom ]---------------------------------
[INFO]
[INFO] --- directory-maven-plugin:0.3.1:highest-basedir (directories) @ geoserver ---
[INFO] Highest basedir set to: /Users/lutovich/Projects/geoserver/src
[INFO]
[INFO] --- spotless-maven-plugin:2.22.0-SNAPSHOT:check (default-cli) @ geoserver ---
[INFO]
[INFO] ---------------------< org.geoserver:gs-platform >----------------------
[INFO] Building Core Platform Module 2.20-SNAPSHOT                       [2/37]
[INFO] --------------------------------[ jar ]---------------------------------
[INFO]
[INFO] --- directory-maven-plugin:0.3.1:highest-basedir (directories) @ gs-platform ---
[INFO] Highest basedir set to: /Users/lutovich/Projects/geoserver/src
[INFO]
[INFO] --- spotless-maven-plugin:2.22.0-SNAPSHOT:check (default-cli) @ gs-platform ---
...
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time:  4.476 s
[INFO] Finished at: 2022-03-30T23:07:06+02:00
[INFO] ------------------------------------------------------------------------

Note the @directories that tells Maven to run a specific directory-maven-plugin execution with ID directories that initializes geoserverBaseDir.

I think we can’t teach Spotless resolve geoserverBaseDir because this property comes from a different plugin.

We could bind SpotlessApplyMojo to process-sources phase but it wouldn't solve your problem. I think this will only allow users to omit <phase> in apply goal's configuration:

<plugin>
  <groupId>com.diffplug.spotless</groupId>
    <artifactId>spotless-maven-plugin</artifactId>
    <version>${spotless.version}</version>
    <executions>
      <execution>
        <id>apply-id</id>
        <!-- <phase>process-sources</phase> --> <!-- no need for this line -->
        <goals>
          <goal>apply</goal>
        </goals>
      </execution>
    </executions>
 </plugin>

Any thoughts or input much appreciated :)

I think binding to the process-sources phase by default is a good choice

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

No branches or pull requests

4 participants