Change formatter plugin to @diffplug's Spotless plugin (Attempt 2) #214

jbduncan · 2018-07-25T20:31:29Z

This is a follow-on to #198 to attempt to get get things working again on Travis CI.

Advantages of Spotless over current solution: - Error messages are (in my opinion) easier to understand. - We can easily adopt other formatters in the future, such as a formatter which checks that files have license headers that follow a certain template.

jbduncan · 2018-07-25T20:41:39Z

Hmm. To say that this is slightly maddening would IMO be an understatement.

For some reason, the Spotless plugin for Maven continues to report that the checked-out code has \r\n line endings (which then causes build failure because Spotless expects the line endings to be \n). However, it shouldn't be having this problem since Travis CI runs on Linux IIUC.

@jrtom @nedtwigg @lutovich I wonder if y'all have any ideas as to what may be causing this, and/or how we may work around it?

jbduncan · 2018-07-25T20:43:30Z

Note to self: try checking out the code for this PR and running mvn clean verify on a Linux VM.

jrtom · 2018-07-25T21:07:08Z

Two responses, off the top of my head:

(1) Is there any way to configure Spotless so that it doesn't care about line endings?
(2) It seems that Spotless is imposing different formatting rules regarding line length (looks like comment line length?) that the existing plugin was. Is there any way to configure that to match the existing rules? I don't mind changing infrastructure, but the comment whitespace changes seem like needless churn and may be obscuring the real issuess.
(Unless the previous plugin was supposed to be imposing this limit and simply wasn't doing so effectively, in which case this is fine.)

jbduncan · 2018-07-25T22:12:17Z

(2) It seems that Spotless is imposing different formatting rules regarding line length (looks like comment line length?) that the existing plugin was. Is there any way to configure that to match the existing rules? I don't mind changing infrastructure, but the comment whitespace changes seem like needless churn and may be obscuring the real issuess.
(Unless the previous plugin was supposed to be imposing this limit and simply wasn't doing so effectively, in which case this is fine.)

I think it's happening due to google-java-format, which Spotless calls. The latest google-java-format version, 1.6, enforces that all comments be at most n characters long (I think 100) and that there is a space between the start of a comment and the first word in that comment.

I could downgrade to 1.4 or 1.5 (whichever one is the last version that doesn't have this new rule) if you wish. :)

(1) Is there any way to configure Spotless so that it doesn't care about line endings?

I'll investigate!

jrtom · 2018-07-25T23:15:25Z

Line wraps: It looks like the comment-line-length enforcement has been there since 1.4, and we're currently using 1.3, so that would explain it: https://github.com/google/google-java-format/releases

(That said, if there's any convenient way of separating the migration from 1.3 to 1.6 from the migration to use spotless, I'd advise doing that; it will make the reviews simpler.)

jbduncan · 2018-07-25T23:58:37Z

(That said, if there's any convenient way of separating the migration from 1.3 to 1.6 from the migration to use spotless, I'd advise doing that; it will make the reviews simpler.)

Yep, I think there is. I'll focus on Spotless in this PR, then, and if I remember to do so, I'll upgrade google-java-format in a separate PR.

jrtom · 2018-07-26T00:08:12Z

Thanks. Filed #215 for the google-java-format upgrade.

jbduncan · 2018-07-26T19:58:45Z

The google-java-format upgrade has been reverted. I'll investigate whether I can ignore line endings with Spotless next.

…h newlines properly on Travis CI See the following link for more information: https://github.com/diffplug/spotless/tree/master/plugin-maven#line-endings-and-encodings-invisible-stuff

…mit didn't convince Travis to behave

jrtom · 2018-07-26T20:28:05Z

So maybe I'm missing something, but looking at the errors on Travis, it looks like there's a relatively small number of files that are being flagged as having /r/n instead of just /n. It looks like they were among the most recently added files. (And I would expect to see others among the visualization code.) So I'd guess that it's just something about an editor that Tom was using.

What if you just let Spotless fix them? It would be nice if we didn't have an inconsistency in the line ending characters in the repo, and it's arguably better than hacking Spotless to not care about the distinction.

nedtwigg · 2018-07-26T20:42:20Z

It may be that one of your committers has git config --global core.autocrlf false, in which case they are putting windows line endings into the repository, and they will stay \r\n when they get checked out by other users, unless those users have git config --global core.autocrlf input, which is rare.

https://git-scm.com/book/en/v2/Customizing-Git-Git-Configuration

It looks to me like spotless is probably behaving correctly here.

jbduncan · 2018-07-26T20:48:26Z

So maybe I'm missing something, but looking at the errors on Travis, it looks like there's a relatively small number of files that are being flagged as having /r/n instead of just /n. It looks like they were among the most recently added files. (And I would expect to see others among the visualization code.) So I'd guess that it's just something about an editor that Tom was using.

@jrtom I remember trying that some time ago with no success, but I can't really remember what the difficulty was. I'll try manually changing the files on my machine and see if committing them afterwards fixes the problem.

What if you just let Spotless fix them? It would be nice if we didn't have an inconsistency in the line ending characters in the repo, and it's arguably better than hacking Spotless to not care about the distinction.

On both my Windows and Linux machines, Spotless runs perfectly fine - it doesn't report any errors - so I think I'll need to manually adjust line endings.

jbduncan · 2018-07-26T20:53:07Z

@nedtwigg Thanks for the suggestion! I've just confirmed on my Windows machine that my installation of Git is git config --global core.autocrlf=true. I've not been doing any software development on my Linux VM, so I don't think I need to check there.

jrtom · 2018-07-26T20:56:03Z

@jbduncan if you just apply Spotless locally (as the error is asking) does that do anything?

Run 'mvn spotless:apply' to fix these violations.

(If it does, that's kind of interesting that it would ignore them on your machine but not on Travis.)

jbduncan · 2018-07-26T21:08:33Z

Oh, I figured out why my Linux VM wasn't reported any errors - it was because I forgot to git checkout origin/spotless-2. 😜

On my Windows machine, running mvn spotless:apply && git status doesn't cause any of the files to change. But if I run mvn spotless:apply; git status on my Ubuntu VM, it shows a number of files that indeed were changed due to the run of Spotless. Huzzah!

Might the end be in sight? :)

I'll report back when I know more.

…y` on my Ubuntu VM

jbduncan · 2018-07-26T21:14:37Z

Do my eyes deceive me? I do believe Travis is passing, now. 🎉

jbduncan · 2018-07-26T21:15:02Z

Many thanks @jrtom and @nedtwigg for prompting me to tackle this problem from an angle I hadn't considered before. :)

jbduncan · 2018-07-26T21:16:00Z

In other words, @jrtom, I'm ready to continue reviewing and merge whenever you're ready.

jrtom · 2018-07-26T21:18:21Z

If at first you don't succeed, try a bigger hammer.
If that doesn't work either, re-examine the problem to see whether you needed knitting needles instead. ;)

My one remaining question is: why did you have to check out spotless-2, i.e., isn't this something that Maven is supposed to be handling?

jbduncan · 2018-07-26T21:21:36Z

My one remaining question is: why did you have to check out spotless-2, i.e., isn't this something that Maven is supposed to be handling?

It's because I just ran git clone https://github.com/jbduncan/jung when I was checking things on my Ubuntu VM; I'd forgotten to switch to the actual branch where I was doing development for this PR, which is branch spotless-2.

I... can't remember what it was I was doing that meant that Maven didn't complain about it sooner. Sorry!

jrtom · 2018-07-26T21:20:19Z

.gitattributes

+*.txt text
+
+# Known binary files
+*.jar binary


missing newline at EOF

Nice catch! Done.

jrtom · 2018-07-26T21:26:03Z

Oh, never mind, I just got confused, you said checkout and I read it as clone because Git and I are still not entirely comfortable with one another. Makes sense that you wouldn't have seen this problem if you weren't on the right branch. :)

Thanks for getting this sorted out.

At some point we might want to see whether it is possible for spotless to automatically change the line-ending characters rather than asking the user to do it explicitly, but I'm not 100% convinced that that's the right answer.

jbduncan · 2018-07-26T21:27:54Z

At some point we might want to see whether it is possible for spotless to automatically change the line-ending characters rather than asking the user to do it explicitly, but I'm not 100% convinced that that's the right answer.

I'm not 100% convinced either. Would that mean that each time Travis CI sees a new commit, it would have to create a new automatic commit with the result of mvn spotless:apply? If so, wouldn't that clutter up the git log a bit?

jrtom · 2018-07-26T21:35:47Z

I don't mind the git log getting cluttered--I've moved to squash-and-merge anyway for PRs--but you're right that we don't want Travis making these changes. What I'd prefer is that there be something integrated into git push on the client side that would reject a push if the tests or formatting checks failed. But I'm used to coding in an environment in which that's a fact of life. shrug

Anyway, this is a separate discussion and should not block merging your PR, so without further ado... :)

jbduncan · 2018-07-26T21:41:38Z

pre-commit might be very useful for this! But yeah, I'll open that as a new issue to discuss. :)

Cheers! 🎉

jrtom · 2018-08-19T07:43:16Z

I'm running into a problem building with Maven since this change; any idea what's going on?

This is the result of running mvn install on a clean pull from master.

[INFO] Reactor Summary:
[INFO] 
[INFO] JUNG (parent metadata project) .................... FAILURE [0.199s]
[INFO] JUNG - API ........................................ SKIPPED
[INFO] JUNG - Graph Implementations ...................... SKIPPED
[INFO] JUNG - Algorithms ................................. SKIPPED
[INFO] JUNG - I/O Support ................................ SKIPPED
[INFO] JUNG - Visualization Support ...................... SKIPPED
[INFO] JUNG - Samples .................................... SKIPPED
[INFO] ------------------------------------------------------------------------
[INFO] BUILD FAILURE
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 0.307s
[INFO] Finished at: Sun Aug 19 00:40:28 PDT 2018
[INFO] Final Memory: 9M/309M
[INFO] ------------------------------------------------------------------------
[ERROR] Failed to execute goal com.diffplug.spotless:spotless-maven-plugin:1.14.0:check (default) on project jung-parent: Execution default of goal com.diffplug.spotless:spotless-maven-plugin:1.14.0:check failed: A required class was missing while executing com.diffplug.spotless:spotless-maven-plugin:1.14.0:check: Lorg/eclipse/aether/RepositorySystem;
[ERROR] -----------------------------------------------------
[ERROR] realm =    plugin>com.diffplug.spotless:spotless-maven-plugin:1.14.0
[ERROR] strategy = org.codehaus.plexus.classworlds.strategy.SelfFirstStrategy
[ERROR] urls[0] = file:/Users/joshua/.m2/repository/com/diffplug/spotless/spotless-maven-plugin/1.14.0/spotless-maven-plugin-1.14.0.jar
[ERROR] urls[1] = file:/Users/joshua/.m2/repository/com/diffplug/spotless/spotless-lib/1.14.0/spotless-lib-1.14.0.jar
[ERROR] urls[2] = file:/Users/joshua/.m2/repository/com/diffplug/spotless/spotless-lib-extra/1.14.0/spotless-lib-extra-1.14.0.jar
[ERROR] urls[3] = file:/Users/joshua/.m2/repository/com/diffplug/durian/durian-core/1.2.0/durian-core-1.2.0.jar
[ERROR] urls[4] = file:/Users/joshua/.m2/repository/com/diffplug/durian/durian-collect/1.2.0/durian-collect-1.2.0.jar
[ERROR] urls[5] = file:/Users/joshua/.m2/repository/org/eclipse/jgit/org.eclipse.jgit/4.9.0.201710071750-r/org.eclipse.jgit-4.9.0.201710071750-r.jar
[ERROR] urls[6] = file:/Users/joshua/.m2/repository/com/jcraft/jsch/0.1.54/jsch-0.1.54.jar
[ERROR] urls[7] = file:/Users/joshua/.m2/repository/com/googlecode/javaewah/JavaEWAH/1.1.6/JavaEWAH-1.1.6.jar
[ERROR] urls[8] = file:/Users/joshua/.m2/repository/org/apache/httpcomponents/httpclient/4.3.6/httpclient-4.3.6.jar
[ERROR] urls[9] = file:/Users/joshua/.m2/repository/org/apache/httpcomponents/httpcore/4.3.3/httpcore-4.3.3.jar
[ERROR] urls[10] = file:/Users/joshua/.m2/repository/commons-logging/commons-logging/1.1.3/commons-logging-1.1.3.jar
[ERROR] urls[11] = file:/Users/joshua/.m2/repository/commons-codec/commons-codec/1.6/commons-codec-1.6.jar
[ERROR] urls[12] = file:/Users/joshua/.m2/repository/org/slf4j/slf4j-api/1.7.2/slf4j-api-1.7.2.jar
[ERROR] urls[13] = file:/Users/joshua/.m2/repository/com/googlecode/concurrent-trees/concurrent-trees/2.6.1/concurrent-trees-2.6.1.jar
[ERROR] urls[14] = file:/Users/joshua/.m2/repository/org/codehaus/groovy/groovy-xml/2.4.7/groovy-xml-2.4.7.jar
[ERROR] urls[15] = file:/Users/joshua/.m2/repository/org/codehaus/groovy/groovy/2.4.7/groovy-2.4.7.jar
[ERROR] urls[16] = file:/Users/joshua/.m2/repository/org/codehaus/plexus/plexus-resources/1.0.1/plexus-resources-1.0.1.jar
[ERROR] urls[17] = file:/Users/joshua/.m2/repository/org/codehaus/plexus/plexus-utils/3.0.8/plexus-utils-3.0.8.jar
[ERROR] urls[18] = file:/Users/joshua/.m2/repository/junit/junit/3.8.1/junit-3.8.1.jar
[ERROR] Number of foreign imports: 1
[ERROR] import: Entry[import  from realm ClassRealm[maven.api, parent: null]]
[ERROR] 
[ERROR] -----------------------------------------------------: org.eclipse.aether.RepositorySystem

Stephan202 · 2018-08-19T08:12:50Z

@jrtom just a semi-educated guess from a casual observer: perhaps your version of Maven is incompatible. See this wiki for some context.

I just checked out master and built it successfully with Maven 3.5.3. (Latest release is 3.5.4.)

Stephan202 · 2018-08-19T08:14:49Z

Just realized I still have old versions of Maven installed. Build still passes when I downgrade to Maven 3.1.1, but fails with Maven 3.0.5. So that's it indeed :)

jbduncan · 2018-08-19T10:29:27Z

Yes, I think @Stephan202 is right.

It's not obvious, but according to the Spotless Maven plugin README, at least Maven 3.1.0 is required. :)

But if you're using a compatible version of Maven, and if it doesn't work still even if you upgrade to the latest version, then it's not immediately obvious to me what the problem would be...

nedtwigg · 2018-08-20T02:22:43Z

Since 1.14.0, Spotless warns proactively about this.

jrtom · 2018-08-26T01:46:54Z

@Stephan202 thanks. Installing Maven 3.5.4 seems to have done the trick. Problem solved.

@nedtwigg Oddly enough, that error didn't appear with 3.0.4 (which is what I had) + Spotless 1.14.
So I'm now a little bit concerned that we may get users trying to use the latest thing that don't recognize that they need to update Maven.

nedtwigg · 2018-08-26T03:46:32Z

Thanks for letting us know that the proactive warn isn't working. Making the error proactive is tricky - it's a race between us and maven, who throws their error first, and looks like Maven is winning. Maven 3.1.0 was released on 2013-07-15, so it's not ancient, but it's not that recent either. Unlikely that this will make it to the top of anyone's todo list, but I'll let @lutovich know just in case.

lutovich · 2018-08-29T22:47:04Z

Hi @nedtwigg, @jrtom!

Spotless Maven plugin has a bug and Maven version prerequisite is not included in the correct POM. I've only tested this prerequisite manually by editing the plugin's POM and forgot that final POM for Maven Central is generated. Sorry about this, hope you did not lose too much time on it :)

This PR should be the fix diffplug/spotless#289 and make error messages nicer.

jbduncan · 2018-08-29T22:49:39Z

Cheers @lutovich!

Can't speak for @jrtom, but I personally didn't lose much time over it, so no worries from me!

I'm looking forward to seeing this bug fix being merged in. 👍

jbduncan force-pushed the spotless-2 branch from 4e750f7 to 0d71d33 Compare July 25, 2018 20:33

jbduncan added 2 commits July 26, 2018 20:54

Revert back to google-java-format 1.3 to reduce churn

07078ec

Add back unintentionally-removed TODO

c92d292

jbduncan added 3 commits July 26, 2018 21:05

See if adding a .gitattributes file coerces Spotless into dealing wit…

db70508

…h newlines properly on Travis CI See the following link for more information: https://github.com/diffplug/spotless/tree/master/plugin-maven#line-endings-and-encodings-invisible-stuff

What if we convince Git that all line endings are Unix LFs?

8819426

Revert back to line-ending-auto-normalization, since the previous com…

3887fb5

…mit didn't convince Travis to behave

Change (and hopefully fix) line endings by running `mvn spotless:appl…

7f2176e

…y` on my Ubuntu VM

jrtom reviewed Jul 26, 2018

View reviewed changes

Add missing newline at EOF

2022cce

jrtom merged commit b3a2461 into jrtom:master Jul 26, 2018

jbduncan mentioned this pull request Jul 26, 2018

Consider using pre-commit or equivalent to prevent unformatted (and untested?) code from being checked in #216

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change formatter plugin to @diffplug's Spotless plugin (Attempt 2) #214

Change formatter plugin to @diffplug's Spotless plugin (Attempt 2) #214

jbduncan commented Jul 25, 2018

jbduncan commented Jul 25, 2018

jbduncan commented Jul 25, 2018

jrtom commented Jul 25, 2018

jbduncan commented Jul 25, 2018

jrtom commented Jul 25, 2018

jbduncan commented Jul 25, 2018

jrtom commented Jul 26, 2018

jbduncan commented Jul 26, 2018

jrtom commented Jul 26, 2018

nedtwigg commented Jul 26, 2018

jbduncan commented Jul 26, 2018

jbduncan commented Jul 26, 2018

jrtom commented Jul 26, 2018

jbduncan commented Jul 26, 2018

jbduncan commented Jul 26, 2018

jbduncan commented Jul 26, 2018

jbduncan commented Jul 26, 2018

jrtom commented Jul 26, 2018

jbduncan commented Jul 26, 2018 •

edited

Loading

jrtom Jul 26, 2018

jbduncan Jul 26, 2018

jrtom commented Jul 26, 2018

jbduncan commented Jul 26, 2018

jrtom commented Jul 26, 2018

jbduncan commented Jul 26, 2018 •

edited

Loading

jrtom commented Aug 19, 2018

Stephan202 commented Aug 19, 2018

Stephan202 commented Aug 19, 2018

jbduncan commented Aug 19, 2018

nedtwigg commented Aug 20, 2018

jrtom commented Aug 26, 2018

nedtwigg commented Aug 26, 2018

lutovich commented Aug 29, 2018

jbduncan commented Aug 29, 2018

Change formatter plugin to @diffplug's Spotless plugin (Attempt 2) #214

Change formatter plugin to @diffplug's Spotless plugin (Attempt 2) #214

Conversation

jbduncan commented Jul 25, 2018

jbduncan commented Jul 25, 2018

jbduncan commented Jul 25, 2018

jrtom commented Jul 25, 2018

jbduncan commented Jul 25, 2018

jrtom commented Jul 25, 2018

jbduncan commented Jul 25, 2018

jrtom commented Jul 26, 2018

jbduncan commented Jul 26, 2018

jrtom commented Jul 26, 2018

nedtwigg commented Jul 26, 2018

jbduncan commented Jul 26, 2018

jbduncan commented Jul 26, 2018

jrtom commented Jul 26, 2018

jbduncan commented Jul 26, 2018

jbduncan commented Jul 26, 2018

jbduncan commented Jul 26, 2018

jbduncan commented Jul 26, 2018

jrtom commented Jul 26, 2018

jbduncan commented Jul 26, 2018 • edited Loading

jrtom Jul 26, 2018

Choose a reason for hiding this comment

jbduncan Jul 26, 2018

Choose a reason for hiding this comment

jrtom commented Jul 26, 2018

jbduncan commented Jul 26, 2018

jrtom commented Jul 26, 2018

jbduncan commented Jul 26, 2018 • edited Loading

jrtom commented Aug 19, 2018

Stephan202 commented Aug 19, 2018

Stephan202 commented Aug 19, 2018

jbduncan commented Aug 19, 2018

nedtwigg commented Aug 20, 2018

jrtom commented Aug 26, 2018

nedtwigg commented Aug 26, 2018

lutovich commented Aug 29, 2018

jbduncan commented Aug 29, 2018

jbduncan commented Jul 26, 2018 •

edited

Loading

jbduncan commented Jul 26, 2018 •

edited

Loading