The greclipse formatter changes the line separator unconditionally, breaking subsequent executions of the java formatter on Windows

[basil]

• Apache Maven 3.8.4 (9b656c72d54e5bacbed989b64718c159fe39b537)
• Java version: 1.8.0_312, vendor: Private Build, runtime: /usr/lib/jvm/java-8-openjdk-amd64/jre
• Spotless 2.17.7

Observed in jenkinsci/jenkins#6104 when trying to add greclipse formatting to a Maven multi-module project with the java formatter already enabled. The checks started failing on Windows but kept passing on Linux.

I looked into how this was working before adding greclipse. The java formatter was working fine on both Linux and Windows; in Formatter#computeLineEndings, lineEndingsPolicy.getEndingFor(file) was returning \n on Linux and \r\n on Windows as expected.

After adding greclipse to the Maven multi-module project, the behavior continued to be as expected until the greclipse formatter ran. During its execution, it got to

spotless/_ext/eclipse-base/src/main/java/com/diffplug/spotless/extra/eclipse/base/SpotlessEclipseServiceConfig.java

Lines 111 to 121 in a7f25eb

	/**
	* In case the string which will be formatted does not contain any
	* line delimiter (single line), Eclipse falls back to use the
	* system property.
	* Change the system default to the UNIX line separator as required
	* by Spotless.
	* @throws ServiceException in case service has already been configured
	*/
	default public void changeSystemLineSeparator() throws ServiceException {
	System.setProperty("line.separator", LINE_DELIMITER);
	}

upon which it changed the line separator to \n unconditionally for both Linux and Windows. That is as expected for greclipse's own execution, but then it never set the value back to the original value. So in a subsequent execution of the java formatter on a different Maven module, lineEndingsPolicy.getEndingFor(file) started returning \n on Windows (not expected) and the Windows build failed with hundreds of errors like

Failed to execute goal com.diffplug.spotless:spotless-maven-plugin:2.17.7:check (default) on project jenkins-core: The following files had format violations:
    src\main\java\hudson\ClassicPluginStrategy.java
        @@ -1,720 +1,720 @@
        -/*\r\n
        - * The MIT License\r\n
        - * \r\n

The greclipse formatter should set the line ending back to the original value when it is done.

[nedtwigg]

Very interesting! Thanks for the detailed investigation. Are you using the default GIT_ATTRIBUTES line endings policy, or are you setting it to PLATFORM_NATIVE manually?

@fvgh, I think one way to resolve this would be to remove this method

spotless/_ext/eclipse-base/src/main/java/com/diffplug/spotless/extra/eclipse/base/SpotlessEclipseServiceConfig.java

Lines 119 to 121 in a7f25eb

	default public void changeSystemLineSeparator() throws ServiceException {
	System.setProperty("line.separator", LINE_DELIMITER);
	}

And modify this method:

spotless/_ext/eclipse-groovy/src/main/java/com/diffplug/spotless/extra/eclipse/groovy/GrEclipseFormatterStepImpl.java

Lines 80 to 82 in a7f25eb

	/** Formatting Groovy string */
	public String format(String raw) throws Exception {
	IDocument doc = new Document(raw);

by adding:

static final LINE_ENDINGS_NATIVE = System.getProperty("line.separator");

public String format(String rawUnix) throws Exception { 
  String raw;
  if (LINE_ENDINGS_NATIVE.equals("\n")) {
    raw = rawUnix;
  } else {
    raw = rawUnix.replace("\n", LINE_ENDINGS_NATIVE);
  }
  IDocument doc = new Document(raw); 

Spotless always passes unix line endings to each step, but it doesn't care which line endings it gets back

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

Lines 236 to 237 in bd2195e

	// Should already be unix-only, but some steps might misbehave.
	unix = LineEnding.toUnix(formatted);

Probably safer for the formatter to mutate the input to match its expectations, rather than mutate the system property.

[basil]

Are you using the default GIT_ATTRIBUTES line endings policy, or are you setting it to PLATFORM_NATIVE manually?

The default.

[basil]

Spotless always passes unix line endings to each step, but it doesn't care which line endings it gets back

An excellent example of Postel's law:

Be conservative in what you send, be liberal in what you accept.

[fvgh]

As stated in the code comment,

In case the string which will be formatted does not contain any line delimiter (single line)
this code has only be included for cases where Eclipse does not find a line separator in the input in the first place. Hence the line separator conversion before formatting proposed by @nedtwigg will not work.

E.g. Eclipse formatters use the first line ending they find as the default one. If there is none, they fall back to the system one (at least that was the behavior some years ago). Some formatters like JDT and CDT also accept the desired line separator as argument.

Removing the the changeSystemLineSeparator as proposed by @basil would lead to errors in the cases where the input has no line separators and the formatters introduce them.

To keep the code clean I would propose to:

• remove the changeSystemLineSeparator and release new spotless-eclipse-base
• add tests to check the Eclipse formatter functionality in case no line endings are found
• add documentation to the formatter functions stating whether the formatted output ensures Unix line endings
• add LineEnding.toUnix to the formatter steps in the lib-extra where required (is be backward compatible, so no version binding required)
• Apply new spotless-eclipse-base version in case compatible

Since the scenario described by @basil is relatively rare, I don't think a back port to older formatters is required.

[basil]

No longer an issue as of the latest version.