-
Notifications
You must be signed in to change notification settings - Fork 4.8k
HIVE-29245: Normalize Java license headers #6115
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
4f54dfc
to
1d39753
Compare
1d39753
to
51d9953
Compare
|
This PR contains ~1400 file changes in 45 commits. Most of the changes are generated. Could you please clarify which files/commits worth reviewing? TBH, I'm afraid, in that format the change will be squashed onto one single commit and later, it will be extremely hard to figure out what happened. |
@okumin , it seems that the bottom part of license header is not indented:
vs https://www.apache.org/legal/src-headers.html
|
FYI,
|
@InvisibleProgrammer How was this patch tested?I put the following configuration on <?xml version="1.0"?>
<!DOCTYPE module PUBLIC
"-//Puppy Crawl//DTD Check Configuration 1.2//EN"
"http://www.puppycrawl.com/dtds/configuration_1_2.dtd">
<module name="Checker">
<module name="Header">
<property name="headerFile" value="${config_loc}/asf.header"/>
<property name="fileExtensions" value="java"/>
</module>
</module> And I manually checked changes with @Aggarwal-Raghav |
@okumin, IMO we should change the expected file to be as same on asf website as mentioned above. But flip side is number of files modified in this PR might substantially increase 😬 |
@InvisibleProgrammer @Aggarwal-Raghav
I personally think we can postpone the conclusion for three reasons. First, the current issue is the reviewers' cost in explaining why SonarQube complains about contributors' headers every time. Second, once we have completed normalizing all headers, a single script can rewrite all at once, and it is easy to write such a script, validate the script, or validate the result. Finally, ours is likely to be legal, hopefully. |
TBH, I see no difference. The second PR has more than 700 file changes. And it doesn't even build due to the pom.xml of storage-api. Honestly, I love the concept of fixing those header files and forcing having the proper header files. But at this PR I don't see how the change can properly reviewed. I give you one example about what I'm thinking about: Iceberg applied spotless back then. It ended up changing more than 3000 files. But it happened in only a few, individually understandable commits. And all the generated changes are in a single one commit. And this commit doesn't contain anything but a generated content. At this PR, it is easy to understand the end goal. But I don't see the how. |
@InvisibleProgrammer
And the diff is 500+ lines. Is this still huge? In that case, I will split it more.
|
@InvisibleProgrammer |
What changes were proposed in this pull request?
Normalize license headers of all Java files.
The following ones are exceptional.
iceberg/**
: most of them are ported fromapache/iceberg
checkstyle/suppressions.xml
hbase-handler/src/test/org/apache/hadoop/hive/hbase/avro/*.java
: Generated by Avro. This is newly stored in the denylist incheckstyle/suppressions.xml
kafka-handler/src/test/gen/org/apache/hadoop/hive/kafka/SimpleRecord.java
: DittoWhy are the changes needed?
Our CI strictly verifies license headers of new files. I suppose many people copy and paste headers from existing files for convenience, so, I sometimes observe CI failing because of the format of licence headers. I have hit the case once, and I have reviewed such a pull request once.
As additional bonuses, consistent headers will reduce legal risks and future maintenance costs(e.g., rewriting the header format will be done by just one command next time).
Does this PR introduce any user-facing change?
No
How was this patch tested?
I put the following configuration on
checkstyle/checkstyle.xml
,standalone-metastore/checkstyle/checkstyle.xml
, and ``storage-api/checkstyle/checkstyle.xml. After that, I ranmvn checkstyle:check
.And I manually checked changes with
git diff master...HIVE-29245-asf-header