THRIFT-5568: use spotless plugin and google-java-format to enforce a consistent code format

[jimexist]

use spotless plugin and google-java-format to enforce a consistent code format

• https://github.com/diffplug/spotless
• https://github.com/google/google-java-format

This needs to be rebased onto:

1. THRIFT-5553: java library to use newer gradle API #2561
2. THRIFT-5545: use gradle convention in organizing java project #2546

before actually applying the formatting, in order to avoid large diff and merge conflicts

The choice of google-java-format:

Note: There is no configurability as to the formatter's algorithm for formatting. This is a deliberate design decision to unify our code formatting on a single format.

• Did you create an Apache Jira ticket? (not required for trivial changes)
• If a ticket exists: Does your pull request title follow the pattern "THRIFT-NNNN: describe my issue"?
• Did you squash your changes to a single commit? (not required, but preferred)
• Did you do your best to avoid breaking changes? If one was needed, did you label the Jira ticket with "Breaking-Change"?
• If your change does not involve any code, include [skip ci] anywhere in the commit message to free up build resources.

[ctubbsii]

Are the formatting changes already checked in, or is that expected to be a follow-on?

[jimexist]

Are the formatting changes already checked in, or is that expected to be a follow-on?

thanks @ctubbsii the actual commit is f840a08 and it's done using gradle spotlessApply

[jimexist]

had to rebase to pickup fix from #2585

[jimexist]

After the rebase (and reformat) this seems fixed now. @ctubbsii you can verify by git checkout master -- lib/java/src/main and then run again gradle spotlessApply to see that all code changes are indeed auto-format.

[jimexist]

After the rebase (and reformat) this seems fixed now. @ctubbsii you can verify by git checkout master -- lib/java/src/main and then run again gradle spotlessApply to see that all code changes are indeed auto-format.

well actually that was a lie - for only one of the file i indeed need to remove the wildcard import as to remove ambiguity of String generated by thrift and java.lang.String

[ctubbsii]

After the rebase (and reformat) this seems fixed now. @ctubbsii you can verify by git checkout master -- lib/java/src/main and then run again gradle spotlessApply to see that all code changes are indeed auto-format.

well actually that was a lie - for only one of the file i indeed need to remove the wildcard import as to remove ambiguity of String generated by thrift and java.lang.String

If the check is fast enough, could add a task to format during CI, and fail the CI check if it results in any changes to tracked files.

[jimexist]

Yes the format is fast enough and it is included by default into gradle build so I think it's already enabled now

[jimexist]

The specific command is gradle spotlessCheck for checking which compliments gradle spotlessApply which applies the format, the former is by default included into gradle build and will do nothing is format is correct otherwise will fail gradle build

[ctubbsii]

The specific command is gradle spotlessCheck for checking which compliments gradle spotlessApply which applies the format, the former is by default included into gradle build and will do nothing is format is correct otherwise will fail gradle build

What I'm suggesting is that the check should verify that the code that is checked into the repo is formatted, not the code that is the result of the build is formatted. That implies that spotlessCheck should run as an independent check, without running spotlessApply.

[jimexist]

The specific command is gradle spotlessCheck for checking which compliments gradle spotlessApply which applies the format, the former is by default included into gradle build and will do nothing is format is correct otherwise will fail gradle build

What I'm suggesting is that the check should verify that the code that is checked into the repo is formatted, not the code that is the result of the build is formatted. That implies that spotlessCheck should run as an independent check, without running spotlessApply.

Yes sorry I wasn't clear in the beginning. The spotless check is a standalone step already included into gradle build - but you can also run it on its own. It does the check on Java lib code that's checked into git.

[ctubbsii]

Yes sorry I wasn't clear in the beginning. The spotless check is a standalone step already included into gradle build - but you can also run it on its own. It does the check on Java lib code that's checked into git.

I understand that spotlessCheck is included into the gradle build. However, I'm suggesting that for it to be useful as a CI check, it needs to be run on its own, outside the normal gradle build.

In development, we want: gradle build to execute spotlessApply so it formats all the code. After that, it can run spotlessCheck to make sure everything was formatted, but it's kind of redundant, because everything should have been formatted by the previous spotlessApply step. We don't expect the build to fail based on formatting here. But, the developer should check in any changed files, to include in their pull request.

In the pull request CI checks, we want something different. It is not sufficient to simply run gradle build, which will run spotlessApply to format all the source code. We actually want to run spotlessCheck without running spotlessApply, because we're not trying to format the source code during CI... we're trying to detect unformatted source code.

So, we need to run spotlessCheck on its own, without running gradle build, because we want to avoid running spotlessApply before running spotlessCheck. If we do gradle build, the check will always pass, and we will fail to detect that some files in the PR weren't formatted.

Imagine that there is unformatted code in a PR. We want a CI check to ensure code in PRs are formatted already, so we don't merge unformatted code.

As I understand it, gradle build will, among other things, run spotlessApply, then spotlessCheck.

[ctubbsii]

As an example, Accumulo does this to verify the format during CI. Note the -DskipFormat -DverifyFormat (feel free to consult the pom.xml to see how that works, but I think these two properties are enough to illustrate my point).

[jimexist]

As an example, Accumulo does this to verify the format during CI. Note the -DskipFormat -DverifyFormat (feel free to consult the pom.xml to see how that works, but I think these two properties are enough to illustrate my point).

@ctubbsii thanks for the suggestion, i added one step for checking within the GitHub workflow action

[jimexist]

the step in effective is https://github.com/apache/thrift/runs/6299416812?check_suite_focus=true

[jimexist]

As I understand it, gradle build will, among other things, run spotlessApply, then spotlessCheck.

thanks for the detailed explanation - i believe we arrived at an mutual understanding of what we want to achieve here. The fact is, gradle build runs gradle check which in turns only runs spotlessCheck, not spotlessApply, so there's no worry that a sweep of gradle build will format and then check. If the code is checked in unformatted, the build will fail.

[ctubbsii]

I resolved the merge conflicts. Just waiting on CI before merging this.

[ctubbsii]

@jimexist I've verified everything is working for me locally now... but I'd like to see it pass in CI. I had to modify the spotless plugin configuration to only target the src/ directory, because gradle build was failing for me locally on the spotlessCheck when it was looking at the generated Java files under the build/ directory. I don't think I saw this fail in the GitHub Actions, though, and I'm not sure why. Running the check before any of the files were generated always worked, though.

[ctubbsii]

Passing in GH Actions is sufficient for me, since I've verified gradle build locally, and Travis and Appveyor seem really flaky and unreliable.

[jimexist]

I don't think I saw this fail in the GitHub Actions, though, and I'm not sure why.

because generateJava is the step that actually generates the (unit test) java files from thrift, but it's run during make check not make.

i shall later add make check into the GitHub actions, but it's not related to the formatting changes in this pull request