spotless
spotless copied to clipboard
The `greclipse` formatter changes the line separator unconditionally, breaking subsequent executions of the `java` formatter on Windows
- 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
https://github.com/diffplug/spotless/blob/a7f25eb51c6d4006591ea911157e62d0213e320b/_ext/eclipse-base/src/main/java/com/diffplug/spotless/extra/eclipse/base/SpotlessEclipseServiceConfig.java#L111-L121
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.
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
https://github.com/diffplug/spotless/blob/a7f25eb51c6d4006591ea911157e62d0213e320b/_ext/eclipse-base/src/main/java/com/diffplug/spotless/extra/eclipse/base/SpotlessEclipseServiceConfig.java#L119-L121
And modify this method:
https://github.com/diffplug/spotless/blob/a7f25eb51c6d4006591ea911157e62d0213e320b/_ext/eclipse-groovy/src/main/java/com/diffplug/spotless/extra/eclipse/groovy/GrEclipseFormatterStepImpl.java#L80-L82
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
https://github.com/diffplug/spotless/blob/bd2195ecf70b92b065611b6378e612c770af24a4/lib/src/main/java/com/diffplug/spotless/Formatter.java#L236-L237
Probably safer for the formatter to mutate the input to match its expectations, rather than mutate the system property.
Are you using the default
GIT_ATTRIBUTES
line endings policy, or are you setting it toPLATFORM_NATIVE
manually?
The default.
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.
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 newspotless-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 thelib-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.
No longer an issue as of the latest version.