Migration to Eclipse formatter hosted on Maven Central

[jbduncan]

On reading diffplug/goomph#47, I'm kind of curious to know if there are any plans to migrate away from DiffPlug's spotless-ext-eclipse-jdt version of the Eclipse formatter, to whatever the appropriate, official artifact on Maven Central is (assuming it exists now). :)

[nedtwigg]

The official artifacts do exist now, but it's a little complicated. The mapping from "I want version 4.7.2" to the collection of jars that make up 4.7.2 is fairly complex. Coincidentally, I merged support for this mapping into the goomph plugin today.

I'd be happy to merge a PR for this into Spotless, but it will be quite a lot of work, and I'm not sure what the benefit would be. The shim code between Spotless and Eclipse is fairly complicated, so it would be hard to write in the reflection style that we normally use.

[jbduncan]

Hmm, okay. I don't think I'll be able to raise a PR myself in the foreseeable future, as my time and energy is a bit limited nowadays due to job searching, and also because admittedly my knowledge of the guts of Eclipse is... very minimal, to say the least. :)

Considering this, would it be best if I closed this issue for now to keep the issue count down? Or would you say that it's important enough to keep open?

[nedtwigg]

Let's close it. If anybody wants to pick it up they can reopen it.

[fvgh]

Will be fixed as part of eclipse-base provision and update of current Eclipse based formatters.
This will also provide a fix for #191.

[fvgh]

I am afraid I am not quite sure whether I have the same idea/problem in mind when I assigned the issue to myself.

As you can see in #234 , I am using for the Eclipse core already the versions provided are Maven Central.
The Eclipse-JDT file I am using currently for testing #234 using the following dependencies:

	compile "com.diffplug.spotless:spotless-ext-eclipse-base:4.7.+"
	compile("org.eclipse.jdt:org.eclipse.jdt.core:[3.12.0,4.0.0[") {
		exclude group: 'org.eclipse.platform', module: 'org.eclipse.ant.core'
		exclude group: 'org.eclipse.platform', module: 'org.eclipse.core.expressions'
		exclude group: 'org.eclipse.platform', module: 'org.eclipse.core.filesystem'
	}

So my idea was basically that the _ext projects are responsible to exclude unused transitive dependencies and formulate a range of versions in which they should work. This would have the benefit that we do not need to publish new _ext project versions when we just want to new compatible versions of the underlying Eclipse formatter. Instead the lib-extra formatter steps shall restrict the version numbers further.

This leaves me with following problems:

1. The _ext project versions shall not have much to do with the Eclipse version. Is it really beneficial for the user to see the Eclipse version. Should we not better use only the mayor version of the formatters plugin, or no correspondence to the Eclipse version at all?

2. Following the version rules of Eclipse as defined in their POMs (everything is compatible as long as the Major version matches) is not an option. The _ext projects using internal Eclipse classes, instead of the plugin interfaces, hence we do not follow the compatibility rules of Eclipse. But restricting all transitive versions to a fixed once is just too much effort and it violates the Maven way. On the other hand I had already bad experiences with with build failures since Maven plugin transitives which should be compatible, were not. I think we should leave the possibility for the user to fixate the transitive versions. At least for gradle they can do this already by specifying exact versions numbers in the build script dependencies. This should be only the last resort in case they are unexpected errors, since this versioning can not be done for individual formatter steps. On the other hand the "important" Eclipse artifact versions (if not already specified by the user) shall be set to more restrictive versions by the lib-extra formatter steps. This should include all plugins (so in the previous example org.eclipse.core.resources and org.eclipse.jdt.core) as well as the org.eclipse.core.runtime. The latter one is restricted by the mentioned plugins only in the range [3.12.0,4.0.0), but I am using an incubation interface, which had not changed since more than 3 years, but might change before run-time 4.0.0 (which would probably correspond to Eclipse Version 10.X.Y... 😉 ).

Any thoughts? Otherwise I would propose the following way forward:

1. _ext projects version become independent from the Eclipse versions.
2. The configuration of _ext project versions in lib-extra formatter steps is replaced by a configuration of the corresponding formatter plugin.
3. Additional common code is provided for the Eclipse formatter steps which allows an easy configuration in the Java code of default Eclipse artifact versions.

[nedtwigg]

As a user interface, I think it's important to be 4.7.2, not an obscure eclipse internal number. Igf we have to do some juggling, that's fine, but users shouldn't have to know about it. People are trying to match their CI behavior to their IDE behavior, and there are differences from one version to the next.

FWIW, the latest version of Goomph has this function. You pass it 4.7.2, and it passes back a map from bundleid -> version for that eclipse release.

[fvgh]

For the user interface you are right for JDT, since it is still more bound to the Eclipse project than any other language extension. I have my doubts that the approach would make sense for CDT users. There the versioning is completely decoupled from the Eclipse version.

Anyhow, the user interface and version of the Spotless Eclipse extension should be different issues.
The user interface should be handled in lib-extra, the Spotless Eclipse extension versions and their changing should be reasonable.

With the new Spotless JDT, we have nothing else than glue code, which will not change between 4.7.2 and 4.8.x. So it is somehow confusing to call the Spotless JDT version 4.7... if it actually also will support higher and lower versions. So there is no technical binding between code version and Eclipse version, and if hopefully the same code still works with Eclipse 4.9, we will not count up the Spotless JDT version.

With WTP you will find that the formatter relevant code has not changed since 5 years. Furthermore the WTP is a collection of several projects with independent numbers. So also for Spotless extensions using fat JARs there is no fix relation between Eclipse version and the Spotless extension version. Keep in mind that we have no pure fat JARs anymore, the Eclipse core is no always coming from Maven Central.

Hence, leaving the user interface aside, I propose that the Spotless JDT, WTP... version should be <x.y.z>, whereas

• X should be the major version of the most relevant Eclipse bundle.

  • Base: Eclipse resource core (3.Y.Z)
  • JDT: JDT core (3.Y.Z)
  • CDT: project version (9.Y.Z)
  • WTP: project version (3.Y.Z)
  • GrEclipse: project version (2.Y.Z)

• Y (currently 0) is incremented when there are changes on fat JAR source, or changes on minimum dependency versions are required (together with corresponding code changes).

• Z (currently 0) is incremented for internal bug fixes.

[nedtwigg]

The versioning you propose sounds good to me.

[fvgh]

Leaves the problem that we are reducing the version numbers of existing Maven artifacts (spotless-ext-greclipse, spotless-ext-eclipse-jdt), which I find not acceptable,
If you don't mind, I would also align the artifact names as follows:

• spotless-eclipse-base
• spotless-eclipse-cdt
• spotless-eclipse-groovy
• spotless-eclipse-jdt
• spotless-eclipse-wtp

[fvgh]

@nedtwigg
Before I continue with the integration of the new/refactored Spotless Eclipse formatters, I would like to assure that we have the same understanding of requirements.

Requirements

1. The transitive dependency versions must be fixed within a project

   • Bug-fixes in transitive dependencies which fixes errors in the formatting, must not lead to to errors in an exiting automated build which is using Spotless.
   • Changes in transitive dependencies must not lead to run-time errors like missing classes, or change of internal interfaces directly called by Spotless formatter implementation.

2. The user can select between previous versions of the Spotless formatters and latest one provided by Spotless maven/gradle plugin.

   • Changing the Sptoless maven/gradle plugin version, shall provide means to ensure backward compatibility of formatter outputs by selecting previous formatter versions.
   • Tested new versions of dependencies, including transitive dependencies, for Spotless formatters can still lead run-time errors due to missing tests for certain execution paths. This is e.g. caused by missing tests for certain valid syntax constructs or by missing tests for Eclipse formatter error handling. Hence the user must be able to switch back to previously used versions of individual Spotless formatters.

3. The user can try untested new versions of the Spotless formatter dependencies in case they are available via M2.

   • The release of new tested Spotless formatter dependency version configurations cannot be provided in high frequency since this would be an inefficiently time consuming task, since most dependency version updates have no effect on the formatter behaviour. Hence the user shall be able to integrate new dependency versions his/her self.
   • The Spotless developer shall use the same mechanisms as the user, for providing new tested versions of the Spotless formatter dependencies.

4. The Spotless formatter version configuration shall be common for all Eclispe based formatters, and intuitive for the user. Hence the official Eclipse version shall be used.

   • Not all Eclipse release versions must be supported, in case the formatter has not changed. But they can be supported by simply linking multiple Eclipse versions to the same dependency version configuration.
   • The Eclipse versions of the transitive dependencies must not necessarily match the Spotless dependency version configuration. It is only required that the Spotless formatter behaves like a certain Eclipse version with the corresponding plugin installed.

Implementation (in case the requirements are agreed)
To my understanding, the Goomph approach seems not to be suitable for the following reasons:

• It does not guarantee fixed versions (see §1). For me this first requirement has the highest priority, but let me know if you have a different opinion.
• It introduces a linkage between M2 and P2 on "http://download.eclipse.org/eclipse/updates/".
  • It is not clear what the time gaps between publishing on M2 and P2 are. This can introduce unexpected behaviour.
  • Some formatter plugins may only be available via Eclipse Marketplace. Here we could add new P2 repositories and make a linkage to M2 goups, but again the question arises how well the provider synchronizes publications on M2 and P2.

I was thinking of an approach similar to Ruby Gemfile.lock. The Spotless lib-extra will provide for certain Eclipse versions for each Spotless Eclipse formatter dependency version configurations. These can be files (part for the JARs resources). The Maven/Gradle user can also specify its own dependency lock file per formatter. In case the specified lock file does not exist, the Spotless plugin will just resolve the dependencies via the configured M2 repositories and write the lock file. Since the Spotless Eclipse formatter dependency version range is configured to take the latest compatible version, it is likely that the resulting config will work, but as pointed out in §3, this is not ensured.
This approach has the drawback, that a strict alignment of Eclipse and Spotless formatter dependency versions is time consuming. As stated in §4, I see no need to be strict here. As long a the direct formatter plugin versions do match and all automated tests are passing, I think we are fine.

[nedtwigg]

This sounds good to me, except I'm hesitant about the "lockfile" part. Yet-another-file-format-for-specifying dependencies. When a formatter already has a file-format (e.g. .properties and .xml for eclipse), I think it makes sense to support it, but in every other scenario I'd rather put it in the plain-old build script. How about something like this?

eclipse('4.7.0') // officially supported eclipse version
eclipseCustom({  // custom unsupported verison
    dep('org.eclipse.jdt:jdt:7.8.0')
})

[fvgh]

For overwriting particular dependencies, I completely agree to provide build script extensions, though I would prefer to make it part of eclipse it self.
For example:

eclipse('4.7.0') {
  configFile 'spotless.eclipseformat.xml'
  dep 'org.eclipse.jdt:jdt:7.8.0'
}

'4.7.0' refers to a certain certain of a spotless-eclipse-jdt artifact, but also to a file in the Jar resources, containing the 14 transitive dependencies. These transitive dependencies can be overwritten.
But I don't want to store the transitive dependencies in plain Java code, since I think that this becomes too messy. For the spotless-eclipse-wtp you have for example 35 transitive dependencies (if it ever gets to Maven Central).
So I would propose that we anyway have a file format (I was thinking of Properties file), we can read.
But then we can provide this functionality also to the user.
For development we also need a possibility to write these files. But I think it would allow the user to quickly try out latest releases, same as we developers do.
So we end up with a complexity like:

eclipse('4.7.0') {
  configFile 'spotless.eclipseformat.xml' 'spotless.eclipseformat2.xml'
  dep 'org.eclipse.jdt:jdt:7.8.0' 'org.eclipse.platform:org.eclipse-core.runtime:3.13.0'
  depFile 'spotless.eclipsedeps1.properties' 'spotless.eclipsedeps2.properties'
  depNew 'spotless.eclipsedeps.properties'
}

depNew will write the resulting dependencies, but also prevent the loading of the transitive dependencies as they are linked to '4.7.0'.

[nedtwigg]

For the spotless-eclipse-wtp you have for example 35 transitive dependencies

But the user won't have to specify all 35. If they specify eclipse-wtp:1.2.3, that will specify all (or at least most) of the others, correct? For jdt, I believe that specifying a few of the top-level deps will suck in all the rest of the required deps. That's what makes them "transitive".

have a file format

I'm fairly skeptical of this. Here's our current approach:

spotless/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/JavaExtension.java

Lines 147 to 176 in e61bc71

	public EclipseConfig eclipse() {
	return eclipse(EclipseFormatterStep.defaultVersion());
	}
	public EclipseConfig eclipse(String version) {
	return new EclipseConfig(version);
	}
	public class EclipseConfig {
	final String version;
	Object[] configFiles;
	EclipseConfig(String version) {
	configFiles = new Object[0];
	this.version = Objects.requireNonNull(version);
	addStep(createStep());
	}
	public void configFile(Object... configFiles) {
	this.configFiles = requireElementsNonNull(configFiles);
	replaceStep(createStep());
	}
	private FormatterStep createStep() {
	Project project = getProject();
	return EclipseFormatterStep.create(version,
	project.files(configFiles).getFiles(),
	GradleProvisioner.fromProject(project));
	}
	}

The great thing about this is:

• discoverability - code-completion tells you what configuration options are available
• simplicity - the only construct the user needs to know are method calls - no file formats or closure-delegation-policies

More options is not better, it's more for us to document, more for us to test, more for the user to understand. eclipse('4.7.0') is the >95% use-case. If somebody wants to specify 35 different versions manually, I'm okay with making them go .dep('name:version').dep('name:version') in their buildscript 35 times. It's not any more LOC than a .properties file, and if they want to put it in a properties file, and parse it into a bunch of .dep() calls they could.

I'm fine with supporting the 5% use-case for cutting-edge users, but the API we provide should just be a .dep() escape hatch - not a complete suite for every possible use-case.

[fvgh]

The user does not have to, but we have to. When you look at the dependencies of org.eclipse.jdt:org.eclipse.jdt.core, you will find that many of them only enforce the major version.
I kept this relaxed constraints in spotless-eclipse-jdt. So the Eclipse version (lets say 4.7.3), will tell spotless-extra two things:

1. Which exact version of spotless-eclipse-jdt to use.
2. Which exact version of spotless-eclipse-jdt transitives dependencies to use.

dep and defFile can be used to overwrite these hash-set of transitives, or add new dependencies which are downloaded (the code will not check that).
depNew will just assure that we do not load the default fixed transitives mentioned in point 2.

[fvgh]

I must admit I am not sure what gradle/groovy editors support.
I thought that it's quite common for them to deal with closures. But I see your point.
What I originally foresaw was the following:

eclipse('4.7.0') {
  configFile 'spotless.eclipseformat.xml'
}

You can now additionally specify an URL instead of '4.7.0'.
The URL must point to a property file which does not only contain the transitives, but also the spotless-eclipse-... artifact.
In that case it does not use the built-in dependencies, but the new once. This would also provide us with the possibility to provide easily workarounds. The interested user/developer might then of course also take our property files from github and try new things (like deleting everything but the spotless-eclipse-... artifact.

[nedtwigg]

How about this for an API:

public EclipseConfig eclipse() { 
  return eclipse(EclipseFormatterStep.defaultVersion()); 
}
  
 public EclipseConfig eclipse(String version) { 
    return new EclipseConfig().depFile('classpath:org.eclipse.jdt/version.properties') 
 } 

public class EclipseConfig {
    private Map<String, String> artifactGroupToVersion;
    public EclipseConfig configFile(Object... configFiles) { }
    public EclipseConfig dep(String groupArtifactVersion) { ... set artifactGroupToVersion ... }
    public EclipseConfig depFile(Object depFile) { ... set artifactGroupToVersion ... }
}

[fvgh]

Sorry, I just don't like these train wracks, they just don't look like gradle. What about this compromise;

public EclipseConfig eclipse() { 
  return eclipse(EclipseFormatterStep.defaultVersion()); 
}
  
 public EclipseConfig eclipse(String version) { 
    /*
       Code that checks whether version is a an URL (file:/classpath:/https:/...).
       Conversion and error handling. To specify an URL here is an undocumented feature.
       In case the user specifies unsupported version, the supported once are listed in the error message.
    */
 } 

private EclipseConfig eclipse(URL defaultDepenencies) { 
....
}

public class EclipseConfig {
    public EclipseConfig configFile(Object... configFiles) { }
    public EclipseConfig dependency(String.... groupArtifactVersions)
}

So for the nominal (rare) scenario, where the interested user just specifies one, or at maximum three dependency deviations, the gradle configuration is still readable.
What I am afraid of, is the lesson I learned from #191. The code path was not executed by many people for many years. And I cannot guarantee that my integration tests don't miss a code path which reveals version incompatibilities. I just want the possibility to provide the user, in case of errors, with a quick patch by just referring to a file on a bug-fix branch.

[nedtwigg]

I think it's a great solution! 👍

[nedtwigg]

Published in plugin-gradle 3.14.0 and plugin-maven 1.14.0.