ARROW-1827: [Java] Add checkstyle file and license template #1326
Conversation
|
cc @BryanCutler @siddharthteotia Same as siddharthteotia#2 |
java/checkstyle/checkstyle.xml
Outdated
|
|
||
| To completely disable a check, just comment it out or delete it from the file. | ||
|
|
||
| Authors: Max Vetrenko, Ruslan Diachenko, Roman Ivanov. |
There was a problem hiding this comment.
So this is pretty annoying. checkstyle is LGPL which means this file cannot simply be copy-and-pasted from the checkstyle project (https://www.apache.org/legal/resolved.html#category-x) . Can we find a checkstyle.xml that implements the Google Java style from somewhere else that is ASF-compatible for inclusion here?
There was a problem hiding this comment.
Ahhh, anonying.
How about https://github.com/GoogleCloudPlatform/google-cloud-intellij/blob/master/google_checks.xml ?
There was a problem hiding this comment.
Looks like that file was also copied from the checkstyle project. I am looking at git histories for these files to try to determine the IP origin.
Kudu has this file, but it looks like it also came directly out of checkstyle https://github.com/apache/kudu/blob/master/java/kudu_style.xml. cc @jdcryans
looks like we can use the one from Apache Beam, we might want to review that it's the style that we expect (e.g. import order, etc.)
There was a problem hiding this comment.
It seems most "google_checks.xml" on the internet are from checkstyle project. Although some ASF licensed projects are using it and/or re license it under ASF license?
To be on the safe side, we could use the checkstyle from beam and make changes if we have to. I can look into that.
There was a problem hiding this comment.
I believe we decided it was OK because the checkstyle xml doesn't end up linked into any resulting binaries, etc -- similar how it's OK to rely on GPL or LGPL build tools in a project. I do recall finding some precedent in other Apache projects or legal resolutions, but can't find it now looking back at my email.
There was a problem hiding this comment.
I see. It looks like we can use the Beam file and avoid the issue altogether, but it would be good to have the definitive judgment from ASF legal
There was a problem hiding this comment.
I am actually leaning towards using google_checks.xml if possible. The beam file seems to be somewhat different of the google checks, which seems to be more widely used and probably is easier to find an IDE codestyle config for.
Some other ASF project, such as Spark, also seems to be using google checks:
https://github.com/apache/spark/blob/master/dev/checkstyle.xml
@wesm Is it possible to consult with ASF legal?
There was a problem hiding this comment.
With both Spark and Kudu using the checkstyle as is, I think it's OK to include. If the issue comes up later we can revisit. Thanks for looking into this
There was a problem hiding this comment.
If we want to be pedantic on this issue, we should ensure that the file does not end up in the source tarballs. As we are using git archive to create the final tarballs, it should be sufficent to add an entry to .gitattrbutes:
/java/checkstyle/checkstyle.xml export-ignore
There was a problem hiding this comment.
Thank @xhochy! I added .gitattribtues
wesm
changed the title
icexelloss
force-pushed
the
ARROW-1827
branch
from
9c0ae9b
to
cee9721
Compare
icexelloss
force-pushed
the
ARROW-1827
branch
from
c4732e7
to
291dbf9
Compare
|
I re-added checkstyle.xml file from checkstyle project. I verified checkstyle works as expected and git archive doesn't include the checkstyle files. This should be good to go. |
There was a problem hiding this comment.
+1, thanks @icexelloss! Let's see if we can hustle to get the style cleaned up, but first we should probably merge @BryanCutler patch
No description provided.