You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@drill.apache.org by ilooner <gi...@git.apache.org> on 2018/04/11 21:16:50 UTC
[GitHub] drill pull request #1207: DRILL-6320: Fixed License Headers
GitHub user ilooner opened a pull request:
https://github.com/apache/drill/pull/1207
DRILL-6320: Fixed License Headers
There were several issues with license headers before this change:
1. We used the apache-rat-plugin to check licenses which was not thread safe. So when doing concurrent builds of submodules with `mvn -T 2C clean install` it could fail.
2. The apache-rat-plugin did not check all files for licenses and did not provide a warning for the files it skipped.
3. It allows inconsistent formatting of license headers.
4. It allowed license headers to be in java doc comments. This is problematic since we don't want the license to be included in every documentation page when we decide to publish java docs. Further more rat provided no way to configure the comment style allowed, so we were forced to live with this.
All of these issues were fixed by moving to the license-maven-plugin. I have reformatted all the license headers and manually verified the headers on each file to make sure they are correct. Now every file in the project is checked for a license except for some shell scripts that do not have file extensions in their names. Also none of the license headers are in java doc comments now!!!!
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/ilooner/drill DRILL-6320
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/drill/pull/1207.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #1207
----
commit 066fc4b58a3de4237fab4a597c67c948882fdcd2
Author: Timothy Farkas <ti...@...>
Date: 2018-04-11T00:27:45Z
DRILL-6320: Switched from apache-rate-plugin to license-maven-plugin and disallowed javadoc comments for license headers.
commit 5fcea57acd47e6d1acc956517c1d3499f84729b1
Author: Timothy Farkas <ti...@...>
Date: 2018-04-11T00:45:49Z
- More fixes
commit d8704eea73294d5beab3b470e51f576e3c7f977f
Author: Timothy Farkas <ti...@...>
Date: 2018-04-11T00:49:25Z
- Even more fixes
commit 6c4ba191ed9fc84d32ea72ecd6fbf3972ba1028d
Author: Timothy Farkas <ti...@...>
Date: 2018-04-11T19:32:57Z
- fixed more licenses
commit 7274190b7314265a0d5ed293374069ca43b5611a
Author: Timothy Farkas <ti...@...>
Date: 2018-04-11T19:36:00Z
- fix new line
commit 6d7227e3b7df66120ad1ca698986c0e49eb77b96
Author: Timothy Farkas <ti...@...>
Date: 2018-04-11T19:45:15Z
- Put proper license headers in ftl files.
----
---
[GitHub] drill pull request #1207: DRILL-6320: Fixed License Headers
Posted by ilooner <gi...@git.apache.org>.
Github user ilooner commented on a diff in the pull request:
https://github.com/apache/drill/pull/1207#discussion_r181248949
--- Diff: pom.xml ---
@@ -375,12 +449,12 @@
</gitDescribe>
</configuration>
</plugin>
-
<plugin>
<groupId>com.mycila</groupId>
<artifactId>license-maven-plugin</artifactId>
<version>3.0</version>
<configuration>
+ <skip>${license.skip}</skip> <!-- DRILL-6320 License checks are intentionally skipped by default for development -->
--- End diff --
see comment above.
---
[GitHub] drill pull request #1207: DRILL-6320: Fixed License Headers
Posted by ilooner <gi...@git.apache.org>.
Github user ilooner commented on a diff in the pull request:
https://github.com/apache/drill/pull/1207#discussion_r181468772
--- Diff: pom.xml ---
@@ -198,6 +200,78 @@
<build>
<plugins>
+ <plugin>
+ <groupId>org.apache.rat</groupId>
+ <artifactId>apache-rat-plugin</artifactId>
+ <version>0.12</version>
+ <executions>
+ <execution>
--- End diff --
You are right, the docs say it is bound to the validate phase by default http://creadur.apache.org/rat/apache-rat-plugin/check-mojo.html . I also removed `<phase>validate</phase>` and tested that it work correctly.
---
[GitHub] drill pull request #1207: DRILL-6320: Fixed License Headers
Posted by vrozov <gi...@git.apache.org>.
Github user vrozov commented on a diff in the pull request:
https://github.com/apache/drill/pull/1207#discussion_r181434241
--- Diff: pom.xml ---
@@ -198,6 +200,78 @@
<build>
<plugins>
+ <plugin>
+ <groupId>org.apache.rat</groupId>
+ <artifactId>apache-rat-plugin</artifactId>
+ <version>0.12</version>
+ <executions>
+ <execution>
--- End diff --
I refer to `<phase>validate</phase>`.
---
[GitHub] drill pull request #1207: DRILL-6320: Fixed License Headers
Posted by vrozov <gi...@git.apache.org>.
Github user vrozov commented on a diff in the pull request:
https://github.com/apache/drill/pull/1207#discussion_r181197234
--- Diff: pom.xml ---
@@ -198,6 +200,78 @@
<build>
<plugins>
+ <plugin>
+ <groupId>org.apache.rat</groupId>
+ <artifactId>apache-rat-plugin</artifactId>
+ <version>0.12</version>
+ <executions>
+ <execution>
--- End diff --
Is this necessary? Should not `apache-rat:check` be already bound to the `validate` phase?
---
[GitHub] drill issue #1207: DRILL-6320: Fixed License Headers
Posted by priteshm <gi...@git.apache.org>.
Github user priteshm commented on the issue:
https://github.com/apache/drill/pull/1207
@arina-ielchiieva or @Ben-Zvi would it make sense to merge this in earlier before the batch commits later in the week? the number of files changes are large so don't want @ilooner to have to rebase each time.
---
[GitHub] drill pull request #1207: DRILL-6320: Fixed License Headers
Posted by vrozov <gi...@git.apache.org>.
Github user vrozov commented on a diff in the pull request:
https://github.com/apache/drill/pull/1207#discussion_r181198668
--- Diff: pom.xml ---
@@ -375,12 +449,12 @@
</gitDescribe>
</configuration>
</plugin>
-
<plugin>
<groupId>com.mycila</groupId>
<artifactId>license-maven-plugin</artifactId>
<version>3.0</version>
<configuration>
+ <skip>${license.skip}</skip> <!-- DRILL-6320 License checks are intentionally skipped by default for development -->
--- End diff --
Is the `skip` necessary?
---
[GitHub] drill pull request #1207: DRILL-6320: Fixed License Headers
Posted by ilooner <gi...@git.apache.org>.
Github user ilooner commented on a diff in the pull request:
https://github.com/apache/drill/pull/1207#discussion_r181248921
--- Diff: pom.xml ---
@@ -198,6 +200,78 @@
<build>
<plugins>
+ <plugin>
+ <groupId>org.apache.rat</groupId>
+ <artifactId>apache-rat-plugin</artifactId>
+ <version>0.12</version>
+ <executions>
+ <execution>
+ <id>rat-checks</id>
+ <phase>validate</phase>
+ <goals>
+ <goal>check</goal>
+ </goals>
+ </execution>
+ </executions>
+ <configuration>
+ <skip>${rat.skip}</skip> <!-- DRILL-6320 License checks are intentionally skipped by default for development -->
--- End diff --
I originally tried `<skip>true</skip>` . However, this does not allow the skip to be overridden when the `-Drat.skip=false` argument is provided. It seems the only way to have an overridable default is to explicitly define a property with the default value.
---
[GitHub] drill issue #1207: DRILL-6320: Fixed License Headers
Posted by priteshm <gi...@git.apache.org>.
Github user priteshm commented on the issue:
https://github.com/apache/drill/pull/1207
There is a large number of files affected - it might be good to get this in early so that we don't have to keep rebasing. @vrozov/ @Ben-Zvi can you glance through the changes?
@Ben-Zvi, since you are batch committer this week, might be good to get this in before the batch commits on Fri
---
[GitHub] drill issue #1207: DRILL-6320: Fixed License Headers
Posted by arina-ielchiieva <gi...@git.apache.org>.
Github user arina-ielchiieva commented on the issue:
https://github.com/apache/drill/pull/1207
@ilooner since mix required maven version has changed, please update `install.md` as well.
---
[GitHub] drill issue #1207: DRILL-6320: Fixed License Headers
Posted by vrozov <gi...@git.apache.org>.
Github user vrozov commented on the issue:
https://github.com/apache/drill/pull/1207
@arina-ielchiieva In Apex we use special git user Apex Dev `dev@apex.apache.org` to make format only changes. I'd recommend doing the same for Drill.
---
[GitHub] drill issue #1207: DRILL-6320: Fixed License Headers
Posted by arina-ielchiieva <gi...@git.apache.org>.
Github user arina-ielchiieva commented on the issue:
https://github.com/apache/drill/pull/1207
@ilooner please also update this part in install.md:
```
# mvn --version
Apache Maven 3.0.3 (r1075438; 2011-02-28 09:31:09-0800)
```
---
[GitHub] drill issue #1207: DRILL-6320: Fixed License Headers
Posted by ilooner <gi...@git.apache.org>.
Github user ilooner commented on the issue:
https://github.com/apache/drill/pull/1207
@vrozov @arina-ielchiieva Addressed comments.
---
[GitHub] drill issue #1207: DRILL-6320: Fixed License Headers
Posted by ilooner <gi...@git.apache.org>.
Github user ilooner commented on the issue:
https://github.com/apache/drill/pull/1207
@arina-ielchiieva @vrozov I'll split the change into two commits. One with a change to the pom files and install.md . Another attributed to `dev@drill.apache.org` with all the formatting changes.
---
[GitHub] drill issue #1207: DRILL-6320: Fixed License Headers
Posted by ilooner <gi...@git.apache.org>.
Github user ilooner commented on the issue:
https://github.com/apache/drill/pull/1207
@arina-ielchiieva fixed
---
[GitHub] drill pull request #1207: DRILL-6320: Fixed License Headers
Posted by ilooner <gi...@git.apache.org>.
Github user ilooner commented on a diff in the pull request:
https://github.com/apache/drill/pull/1207#discussion_r181249189
--- Diff: pom.xml ---
@@ -198,6 +200,78 @@
<build>
<plugins>
+ <plugin>
+ <groupId>org.apache.rat</groupId>
+ <artifactId>apache-rat-plugin</artifactId>
+ <version>0.12</version>
+ <executions>
+ <execution>
--- End diff --
This was here originally. I tried removing it. When it is removed the rat plugin is not executed when doing `mvn clean install`. So it seems to be required.
---
[GitHub] drill pull request #1207: DRILL-6320: Fixed License Headers
Posted by ilooner <gi...@git.apache.org>.
Github user ilooner commented on a diff in the pull request:
https://github.com/apache/drill/pull/1207#discussion_r181249226
--- Diff: pom.xml ---
@@ -198,6 +200,78 @@
<build>
<plugins>
+ <plugin>
+ <groupId>org.apache.rat</groupId>
+ <artifactId>apache-rat-plugin</artifactId>
+ <version>0.12</version>
--- End diff --
fixed
---
[GitHub] drill pull request #1207: DRILL-6320: Fixed License Headers
Posted by vrozov <gi...@git.apache.org>.
Github user vrozov commented on a diff in the pull request:
https://github.com/apache/drill/pull/1207#discussion_r181197514
--- Diff: pom.xml ---
@@ -198,6 +200,78 @@
<build>
<plugins>
+ <plugin>
+ <groupId>org.apache.rat</groupId>
+ <artifactId>apache-rat-plugin</artifactId>
+ <version>0.12</version>
+ <executions>
+ <execution>
+ <id>rat-checks</id>
+ <phase>validate</phase>
+ <goals>
+ <goal>check</goal>
+ </goals>
+ </execution>
+ </executions>
+ <configuration>
+ <skip>${rat.skip}</skip> <!-- DRILL-6320 License checks are intentionally skipped by default for development -->
--- End diff --
Is this necessary? Should not `<skip>` use `rat.skip` property by default?
---
[GitHub] drill issue #1207: DRILL-6320: Fixed License Headers
Posted by ilooner <gi...@git.apache.org>.
Github user ilooner commented on the issue:
https://github.com/apache/drill/pull/1207
Abhishek has already updated jenkins to the latest maven. So we should be clear to merge after review is done.
---
[GitHub] drill issue #1207: DRILL-6320: Fixed License Headers
Posted by ilooner <gi...@git.apache.org>.
Github user ilooner commented on the issue:
https://github.com/apache/drill/pull/1207
@arina-ielchiieva Updated INSTALL.md
@vrozov Added the rat plugin back. Disabled license checking for both rat and the license-maven-plugin by default and enabled it for travis
---
[GitHub] drill pull request #1207: DRILL-6320: Fixed License Headers
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/drill/pull/1207
---
[GitHub] drill issue #1207: DRILL-6320: Fixed License Headers
Posted by ilooner <gi...@git.apache.org>.
Github user ilooner commented on the issue:
https://github.com/apache/drill/pull/1207
@arina-ielchiieva Please let me know if you have any comments or if things look good.
---
[GitHub] drill pull request #1207: DRILL-6320: Fixed License Headers
Posted by vrozov <gi...@git.apache.org>.
Github user vrozov commented on a diff in the pull request:
https://github.com/apache/drill/pull/1207#discussion_r181196340
--- Diff: pom.xml ---
@@ -198,6 +200,78 @@
<build>
<plugins>
+ <plugin>
+ <groupId>org.apache.rat</groupId>
+ <artifactId>apache-rat-plugin</artifactId>
+ <version>0.12</version>
--- End diff --
Do not specify the version explicitly, the one from the parent Apache pom should be used.
---
[GitHub] drill issue #1207: DRILL-6320: Fixed License Headers
Posted by arina-ielchiieva <gi...@git.apache.org>.
Github user arina-ielchiieva commented on the issue:
https://github.com/apache/drill/pull/1207
+1, thanks for making the changes!
---
[GitHub] drill pull request #1207: DRILL-6320: Fixed License Headers
Posted by vrozov <gi...@git.apache.org>.
Github user vrozov commented on a diff in the pull request:
https://github.com/apache/drill/pull/1207#discussion_r181267547
--- Diff: pom.xml ---
@@ -198,6 +200,78 @@
<build>
<plugins>
+ <plugin>
+ <groupId>org.apache.rat</groupId>
+ <artifactId>apache-rat-plugin</artifactId>
+ <version>0.12</version>
+ <executions>
+ <execution>
+ <id>rat-checks</id>
+ <phase>validate</phase>
+ <goals>
+ <goal>check</goal>
+ </goals>
+ </execution>
+ </executions>
+ <configuration>
+ <skip>${rat.skip}</skip> <!-- DRILL-6320 License checks are intentionally skipped by default for development -->
--- End diff --
try with `<skip>${rat.skip}<skip>` removed. Keep `<rat.skip>true</rat.skip>` in properties.
---
[GitHub] drill issue #1207: DRILL-6320: Fixed License Headers
Posted by ilooner <gi...@git.apache.org>.
Github user ilooner commented on the issue:
https://github.com/apache/drill/pull/1207
@vrozov followed your instructions. Building the source distribution with license checks enabled works.
---
[GitHub] drill pull request #1207: DRILL-6320: Fixed License Headers
Posted by ilooner <gi...@git.apache.org>.
Github user ilooner commented on a diff in the pull request:
https://github.com/apache/drill/pull/1207#discussion_r181474959
--- Diff: pom.xml ---
@@ -198,6 +200,78 @@
<build>
<plugins>
+ <plugin>
+ <groupId>org.apache.rat</groupId>
+ <artifactId>apache-rat-plugin</artifactId>
+ <version>0.12</version>
+ <executions>
+ <execution>
+ <id>rat-checks</id>
+ <phase>validate</phase>
+ <goals>
+ <goal>check</goal>
+ </goals>
+ </execution>
+ </executions>
+ <configuration>
+ <skip>${rat.skip}</skip> <!-- DRILL-6320 License checks are intentionally skipped by default for development -->
--- End diff --
Your suggestion works. Made the change.
---
[GitHub] drill pull request #1207: DRILL-6320: Fixed License Headers
Posted by ilooner <gi...@git.apache.org>.
Github user ilooner commented on a diff in the pull request:
https://github.com/apache/drill/pull/1207#discussion_r181475030
--- Diff: pom.xml ---
@@ -375,12 +449,12 @@
</gitDescribe>
</configuration>
</plugin>
-
<plugin>
<groupId>com.mycila</groupId>
<artifactId>license-maven-plugin</artifactId>
<version>3.0</version>
<configuration>
+ <skip>${license.skip}</skip> <!-- DRILL-6320 License checks are intentionally skipped by default for development -->
--- End diff --
updated with suggestion above
---
[GitHub] drill issue #1207: DRILL-6320: Fixed License Headers
Posted by vrozov <gi...@git.apache.org>.
Github user vrozov commented on the issue:
https://github.com/apache/drill/pull/1207
-1 on removing apache-rat maven plugin. It is an Apache plugin to invoke "Release Audit Tool". More plugins can be added to check license headers if they do a better job in addition to apache-rat.
Both apache-rat and any other license verification plugins can be disabled by default (this will allow to run `mvn -T 2C clean install` if necessary) and only enabled in Travis CI.
---
[GitHub] drill issue #1207: DRILL-6320: Fixed License Headers
Posted by vrozov <gi...@git.apache.org>.
Github user vrozov commented on the issue:
https://github.com/apache/drill/pull/1207
LGTM, @ilooner please double check that Apache source distribution can be built with '-Drat.skip=false -Dlicense.skip=false':
- build with `-P apache.release -Dgpg.skip=true`
- extract created source .tar.gz or .zip to a temp directory
- build with apache-rat and license check enabled (you may need to add more files to the license check exclusion).
---