Added support for Maven POM sorting/formatting #946

tisoft · 2021-09-24T05:03:40Z

Adds the sorting mechanism of the sortpom-maven-plugin.

While the sortpom-maven-plugin is a maven plugin in itself, I like all my code formatting in one plugin, so this PR integrates the pom sorting from that plugin. Configuration options are the same as in the upstream project.

tisoft · 2021-09-24T05:04:46Z

Documentation is still missing, would like to know if this is something you want inside spotless, before I do the tedious documentation work. 😄

nedtwigg

It would be great to add this, but the third-party dep needs to be resolved dynamically via JarState. If every formatter in lib and lib-extra just added itself as a dependency, we would have an absolute nightmare of transitive conflicts, and we would have to publish a new plugin every few days as one or another plugin releases a new version.

nedtwigg · 2021-09-24T22:41:41Z

lib-extra/build.gradle

@@ -16,6 +16,8 @@ dependencies {
 	implementation "com.googlecode.concurrent-trees:concurrent-trees:2.6.1"
 	// used for xml parsing in EclipseFormatter
 	implementation "org.codehaus.groovy:groovy-xml:3.0.9"
+	// used for pom sorting
+	implementation 'com.github.ekryd.sortpom:sortpom-sorter:3.0.0'


This dep needs to be resolved like this:

spotless/lib/src/main/java/com/diffplug/spotless/java/GoogleJavaFormatStep.java

Line 122 in 85344bd

this.jarState = JarState.from(MAVEN_COORDINATE + version, provisioner);

nedtwigg · 2021-09-24T22:47:33Z

lib-extra/src/main/java/com/diffplug/spotless/extra/pom/SortPomStep.java

+			return input -> {
+				// SortPom expects a file to sort, so we write the inpout into a temporary file
+				File pom = File.createTempFile("pom", ".xml");
+				pom.deleteOnExit();
+				try (FileWriter fw = new FileWriter(pom)) {
+					fw.write(input);
+				}
+				SortPomImpl sortPom = new SortPomImpl();
+				sortPom.setup(new SortPomLogger() {
+					@Override
+					public void warn(String content) {
+						logger.warning(content);
+					}
+
+					@Override
+					public void info(String content) {
+						logger.info(content);
+					}
+
+					@Override
+					public void error(String content) {
+						logger.severe(content);
+					}
+				}, PluginParameters.builder()
+						.setPomFile(pom)
+						.setFileOutput(false, null, null, false)
+						.setEncoding(encoding)
+						.setFormatting(lineSeparator, expandEmptyElements, spaceBeforeCloseEmptyElement, keepBlankLines)
+						.setIndent(nrOfIndentSpace, indentBlankLines, indentSchemaLocation)
+						.setSortOrder(sortOrderFile, predefinedSortOrder)
+						.setSortEntities(sortDependencies, sortDependencyExclusions, sortPlugins, sortProperties, sortModules, sortExecutions)
+						.setTriggers(false)
+						.build());
+				sortPom.sortPom();
+				return IOUtils.toString(new FileReader(pom));


And the jar should be called using reflection. You can do it raw as we do for google-java-format,

or it might be worth incorporating some pieces of jOOR to make this easier, or if you're really fluent with Gradle you could do it with #524

tisoft · 2021-09-26T06:46:55Z

@nedtwigg I played around with #524. Classloading is a bit awkward... Do you have any better idea?

nedtwigg

I think the way that you added a sortPom sourceSet is great. I don't understand why JarState::getClassLoader wasn't enough, I don't think you need the serialization tricks nor to make a new kind of classloader. I might be wrong though.

The classloader you already get from JarState is built like so:

spotless/lib/src/main/java/com/diffplug/spotless/SpotlessCache.java

Line 71 in 85344bd

    
           .computeIfAbsent(serializedKey, k -> new FeatureClassLoader(state.jarUrls(), this.getClass().getClassLoader()));

https://github.com/diffplug/spotless/blob/main/lib/src/main/java/com/diffplug/spotless/FeatureClassLoader.java

I might be wrong, but I think that you should be able to pass objects from SortPomStep to SortPomFormatterFunc without serializing them. This stuff is tricky, and I'm very willing to believe that I'm wrong.

If the serialization stuff really is necessary, then we should either:

add them as API's onto JarState so that the code inside SortPomStep is easy to read, and the serialization boilerplate happens inside JarState
this would be a bummer, but maybe FeatureClassLoader needs to be public API, and the serialization boilerplate can go there.

But ideally it isn't necessary in the first place!

lib/build.gradle

lib/src/sortPom/java/com/diffplug/spotless/pom/DelegatingClassLoader.java

lib/src/sortPom/java/com/diffplug/spotless/pom/SortPomStep.java

tisoft · 2021-09-27T05:48:43Z

I'll have another go at the class loading stuff after the other two PRs are in. Can't handle all the parallelity in my brain. 🤣

tisoft · 2021-09-28T07:18:38Z

I could get rid of the serializing deserializing, but I couldn't get rid of the DelegatingClassLoader.

The problem is the following:

The SortPomFormatterFunc directly references classes from the sortpom dependency
- so we can't load it from the current ClassLoader, since that doesn't know about the sortpom dependencies
- we can't load it from the JarState ClassLoader, since that doesn't know about SortPomFormatterFunc, SortPomState and FormatterFunc
- So we need a ClassLoader, that knows about both
My first idea was to change the FeatureClassLoader to allow loading SortPomFormatterFunc from the buildToolClassLoader
- This would attachSortPomFormatterFunc to the buildToolClassLoader
- As soon as the constructor is retrieved via reflection, the dependencies of the class will be tried to load with the buildToolClassLoader
- This fails, since the buildToolClassLoader doesn't know about the sortpom package, since it is effectively a parent from the `FeatureClassLoader, and classes from it's children are invisible
So the SortPomFormatterFunc needs to be attached to a ClassLoader, that knows about the sortpom package: the DelegatingClassLoader
- It uses the current ClassLoader and the FeatureClassLoader as sources
- But it doesn't load the classes directly from them (since they would be attached to the source classloaders, bringing us back to the previous problem)
- Instead it loads the classes as resources, and then calls define class, to attach them to the DelegatingClassLoader
- Now SortPomFormatterFunc and all classes from the 'JarState' are loaded by the same Classloader and can see each other and all is fine...
... except that now SortPomState and FormatterFunc are also attached to DelegatingClassLoader and the funny De-/Serializing of the state and the call to 'apply' via reflection is needed
That's why the DelegatingClassLoader now excludes those 2 classes from the custom classloading process and just returns the Class objects already loaded
Which only works, if the SortPomState and its member fields are public

I hope this helps to understand, why I did it in this way. If there is an easier or more elegant solution, I'm all for it. 😄

- SortPomCfg needs no dependencies, just Strings and booleans - SortPomCfg now has the default values, which simplifies plugin-maven/.../SortPom.java - SortPomStep does the JarState and classloader stuff

…ineClass`, and `com.diffplug.spotless` with `buildToolClassLoader`. This unifies FeatureClassLoader with DelegatingClassLoader.

…`SortPomFormatterFunc` needs `src/sortPom/java`.

nedtwigg · 2021-09-29T23:59:30Z

Well I broke eclipse, but I fixed PomStep. Does this approach look good to you @tisoft? You got all the hard parts working, I just massaged things around with an eye towards other parts of Spotless adopting this same "glue" approach.

nedtwigg · 2021-09-30T01:14:28Z

lib/src/main/java/com/diffplug/spotless/FeatureClassLoader.java

+ *
+ * For `com.diffplug.spotless.glue.`, classes are redefined from within the lib jar
+ * but linked against the `Url[]`. This allows us to ship classfiles which function as glue
+ * code but delay linking/definition to runtime after the user has specified which version
+ * of the formatter they want.
+ *
+ *  For `"org.slf4j.` and (`com.diffplug.spotless.` but not `com.diffplug.spotless.extra.`)
+ * 	the classes are loaded from the buildToolClassLoader.


@fvgh, just FYI, we finally have a way to ship glue code without requiring all deps of that glue code. It works like so:

com.diffplug.spotless.glue. -> we define classes from the jars, which requires linking against com.diffplug.spotless from buildToolClassLoader

we make an exception for com.diffplug.spotless.extra, which we always load from the jars as (we have in the past)

we also load org.slf4j. from buildToolClassLoader (as we have in the past)

This will allow us to replace a lot of manual reflection code, ala #524.

nedtwigg · 2021-09-30T01:22:02Z

@tisoft I think all we need now are changelog updates and documentation. Another fantastic PR, thanks very much! You were the savior to come and finally figure out how we can do #524, which will make the project much easier to read and contribute to.

tisoft · 2021-09-30T04:16:28Z

@nedtwigg Added documentation and changelog entries.

nedtwigg · 2021-09-30T17:53:57Z

Released in plugin-maven 2.15.0

nedtwigg · 2021-09-30T17:59:45Z

I advise the following:

test in a production setting to make sure that the published artifact has the glue code
once it is confirmed working, submit a PR to https://github.com/Ekryd/sortpom showing how to use it with spotless

I also anticipate that you will want to add a version property to SortPomCfg, so that you can use the latest version of sortpom without needing to publish a whole new set of Spotless artifacts each time, but that can wait until it's needed.

nedtwigg requested changes Sep 24, 2021

View reviewed changes

tisoft force-pushed the sortpom branch 2 times, most recently from 14ee011 to b7958a9 Compare September 26, 2021 06:32

tisoft requested a review from nedtwigg September 26, 2021 07:48

nedtwigg reviewed Sep 26, 2021

View reviewed changes

lib/build.gradle Outdated Show resolved Hide resolved

lib/src/sortPom/java/com/diffplug/spotless/pom/DelegatingClassLoader.java Outdated Show resolved Hide resolved

lib/src/sortPom/java/com/diffplug/spotless/pom/SortPomStep.java Outdated Show resolved Hide resolved

tisoft force-pushed the sortpom branch from b7958a9 to 2e0a3b3 Compare September 27, 2021 05:15

tisoft added 2 commits September 27, 2021 22:08

Added support for Maven POM sorting/formatting

22ddb38

load SortPom from JarState

503f4c0

tisoft force-pushed the sortpom branch from 2e0a3b3 to 503f4c0 Compare September 27, 2021 20:08

use FormatterFunc and SortPomState from current ClassLoader

8b7c3eb

Fixed SpotBugs errors

c0d1b68

tisoft requested a review from nedtwigg September 28, 2021 07:59

nedtwigg added 6 commits September 29, 2021 11:42

Rename SortPomTest to SortPomMavenTest.

680d95c

Create a no-maven SortPomTest which can run quickly.

4a80576

Rename SortPomState to SortPomCfg, and:

f2c0e4a

- SortPomCfg needs no dependencies, just Strings and booleans - SortPomCfg now has the default values, which simplifies plugin-maven/.../SortPom.java - SortPomStep does the JarState and classloader stuff

FeatureClassLoader now loads com.diffplug.spotless.glue.* via `def…

bcd3735

…ineClass`, and `com.diffplug.spotless` with `buildToolClassLoader`. This unifies FeatureClassLoader with DelegatingClassLoader.

SortPomStep can now live within the normal src/main/java, and only …

4b780c0

…`SortPomFormatterFunc` needs `src/sortPom/java`.

Fix typo.

6d6ab66

nedtwigg added 2 commits September 29, 2021 18:06

Fix FeatureClassLoader for eclipse entries.

24bc908

plugin-maven doesn't need any special dependency on the lib jar.

2b85d84

nedtwigg reviewed Sep 30, 2021

View reviewed changes

Generalize the sortPom stuff to facilitate diffplug#524 (glue code).

b95e93b

Added documentation for sortPom

617abf5

nedtwigg merged commit 5951f3d into diffplug:main Sep 30, 2021

tisoft deleted the sortpom branch October 1, 2021 12:00

nedtwigg mentioned this pull request Dec 7, 2021

Use custom source sets and compile-only dependencies instead of reflection #524

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added support for Maven POM sorting/formatting #946

Added support for Maven POM sorting/formatting #946

tisoft commented Sep 24, 2021

tisoft commented Sep 24, 2021

nedtwigg left a comment

nedtwigg Sep 24, 2021

nedtwigg Sep 24, 2021

tisoft commented Sep 26, 2021

nedtwigg left a comment

tisoft commented Sep 27, 2021

tisoft commented Sep 28, 2021

nedtwigg commented Sep 29, 2021

nedtwigg Sep 30, 2021 •

edited

Loading

nedtwigg commented Sep 30, 2021

tisoft commented Sep 30, 2021

nedtwigg commented Sep 30, 2021

nedtwigg commented Sep 30, 2021

Added support for Maven POM sorting/formatting #946

Added support for Maven POM sorting/formatting #946

Conversation

tisoft commented Sep 24, 2021

tisoft commented Sep 24, 2021

nedtwigg left a comment

Choose a reason for hiding this comment

nedtwigg Sep 24, 2021

Choose a reason for hiding this comment

nedtwigg Sep 24, 2021

Choose a reason for hiding this comment

tisoft commented Sep 26, 2021

nedtwigg left a comment

Choose a reason for hiding this comment

tisoft commented Sep 27, 2021

tisoft commented Sep 28, 2021

nedtwigg commented Sep 29, 2021

nedtwigg Sep 30, 2021 • edited Loading

Choose a reason for hiding this comment

nedtwigg commented Sep 30, 2021

tisoft commented Sep 30, 2021

nedtwigg commented Sep 30, 2021

nedtwigg commented Sep 30, 2021

nedtwigg Sep 30, 2021 •

edited

Loading