You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "xijiu (via GitHub)" <gi...@apache.org> on 2024/12/22 14:21:27 UTC
[PR] KAFKA-17607: Add CI step to verify LICENSE-binary [kafka]
xijiu opened a new pull request, #18299:
URL: https://github.com/apache/kafka/pull/18299
As title.
### Committer Checklist (excluded from commit message)
- [ ] Verify design and implementation
- [ ] Verify test coverage and CI build status
- [ ] Verify documentation (including upgrade notes)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-17607: Add CI step to verify LICENSE-binary [kafka]
Posted by "mumrah (via GitHub)" <gi...@apache.org>.
mumrah commented on code in PR #18299:
URL: https://github.com/apache/kafka/pull/18299#discussion_r1981718530
##########
committer-tools/verify_license.py:
##########
@@ -74,12 +75,20 @@ def get_license_deps(license_text):
return set(LICENSE_DEP_PATTERN.findall(license_text))
def main():
+ # Argument parser
+ parser = argparse.ArgumentParser(description="Whether to skip executing ReleaseTarGz.")
+ parser.add_argument('--skipReleaseTarGz', type=str, default=False, help='Whether to skip ReleaseTarGz (true/false)')
Review Comment:
Yea that makes sense. I wasn't sure what default behavior we would want. In that case, I'd suggest `--skip-build` :) (i like kebab-case for CLI arguments)
##########
committer-tools/verify_license.py:
##########
@@ -74,12 +75,20 @@ def get_license_deps(license_text):
return set(LICENSE_DEP_PATTERN.findall(license_text))
def main():
+ # Argument parser
+ parser = argparse.ArgumentParser(description="Whether to skip executing ReleaseTarGz.")
+ parser.add_argument('--skipReleaseTarGz', type=str, default=False, help='Whether to skip ReleaseTarGz (true/false)')
Review Comment:
Yea that makes sense. I wasn't sure what default behavior we would want.
In that case, I'd suggest `--skip-build` :) (i like kebab-case for CLI arguments)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-17607: Add CI step to verify LICENSE-binary [kafka]
Posted by "xijiu (via GitHub)" <gi...@apache.org>.
xijiu commented on code in PR #18299:
URL: https://github.com/apache/kafka/pull/18299#discussion_r1899514202
##########
.github/workflows/build.yml:
##########
@@ -114,6 +114,7 @@ jobs:
# --no-scan: For public fork PRs, we won't attempt to publish the scan
run: |
./gradlew --build-cache --info $SCAN_ARG check siteDocTar -x test
+ ./gradlew releaseTarGz
Review Comment:
Nice idea, I will update it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-17607: Add CI step to verify LICENSE-binary [kafka]
Posted by "xijiu (via GitHub)" <gi...@apache.org>.
xijiu commented on PR #18299:
URL: https://github.com/apache/kafka/pull/18299#issuecomment-2576521368
@chia7712 I have added a `GradleException` to fail the task when there are missing lib licenses.
PTAL
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-17607: Add CI step to verify LICENSE-binary [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on code in PR #18299:
URL: https://github.com/apache/kafka/pull/18299#discussion_r1904604076
##########
build.gradle:
##########
@@ -1326,6 +1326,42 @@ project(':core') {
from(project(':tools:tools-api').jar) { into("libs/") }
from(project(':tools:tools-api').configurations.runtimeClasspath) { into("libs/") }
duplicatesStrategy 'exclude'
+
+ doLast {
+ def prefixPath = "$rootDir/core/build/distributions/"
+ def bathPath = prefixPath + "kafka_${versions.baseScala}-${project.properties['version']}"
+ def tgzPath = bathPath + '.tgz'
+ exec {
+ commandLine 'tar', 'xzf', tgzPath, '-C', prefixPath
+ }
+
+ def licenseFile = file(bathPath + '/LICENSE')
+ def libsDir = file(bathPath + '/libs')
+ def result = true
+ libsDir.eachFile { file ->
+ if (file.name.endsWith('.jar')
+ && !file.name.startsWith('kafka')
+ && !file.name.startsWith('connect')
+ && !file.name.startsWith('trogdor')) {
+
+ def isLicensePresent = false
+ licenseFile.eachLine { line ->
+ if (line.contains(file.name - '.jar')) {
+ isLicensePresent = true
+ }
+ }
+ if (!isLicensePresent) {
+ println "warning: ${file.name} is missing in the license file"
Review Comment:
@xijiu Could you please open a jira to trace this? I'd like to merge this PR first.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-17607: Add CI step to verify LICENSE-binary [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on code in PR #18299:
URL: https://github.com/apache/kafka/pull/18299#discussion_r1981708674
##########
committer-tools/verify_license.py:
##########
@@ -74,12 +75,20 @@ def get_license_deps(license_text):
return set(LICENSE_DEP_PATTERN.findall(license_text))
def main():
+ # Argument parser
+ parser = argparse.ArgumentParser(description="Whether to skip executing ReleaseTarGz.")
+ parser.add_argument('--skipReleaseTarGz', type=str, default=False, help='Whether to skip ReleaseTarGz (true/false)')
Review Comment:
> I would suggest a flag name like "rebuild" or similar.
The script should build the distribution if the flag is unset, as it shouldn't assume a distribution already exists. Perhaps the flag could be named skipBuild?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-17607: Add CI step to verify LICENSE-binary [kafka]
Posted by "xijiu (via GitHub)" <gi...@apache.org>.
xijiu commented on code in PR #18299:
URL: https://github.com/apache/kafka/pull/18299#discussion_r1984638564
##########
committer-tools/verify_license.py:
##########
@@ -77,14 +77,14 @@ def get_license_deps(license_text):
def main():
# Argument parser
parser = argparse.ArgumentParser(description="Whether to skip executing ReleaseTarGz.")
- parser.add_argument('--skipReleaseTarGz', type=str, default=False, help='Whether to skip ReleaseTarGz (true/false)')
+ parser.add_argument('--skip-build', action="store_true", help='Whether to skip ReleaseTarGz (true/false)')
Review Comment:
yeah, the details are very important, I have fixed it, PTAL
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-17607: Add CI step to verify LICENSE-binary [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on PR #18299:
URL: https://github.com/apache/kafka/pull/18299#issuecomment-2665810561
@xijiu #18931 offers a script for license verification. could you please consider integrating it into CI?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-17607: Add CI step to verify LICENSE-binary [kafka]
Posted by "xijiu (via GitHub)" <gi...@apache.org>.
xijiu commented on PR #18299:
URL: https://github.com/apache/kafka/pull/18299#issuecomment-2683795730
@chia7712 I have added it, PTAL.
For test, I added the invalid content to `LICENSE-binary`:
<img width="490" alt="image" src="https://github.com/user-attachments/assets/eb505eb8-5284-449c-b6c3-d00b19255891" />
Then triggered CI and failed:
<img width="966" alt="image" src="https://github.com/user-attachments/assets/9a21df97-c2fd-4664-9410-4b420988e262" />
<img width="1161" alt="image" src="https://github.com/user-attachments/assets/542ff715-a3e6-429f-bb77-5695bb0580a5" />
So the verify license file has taken effect.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-17607: Add CI step to verify LICENSE-binary [kafka]
Posted by "xijiu (via GitHub)" <gi...@apache.org>.
xijiu commented on code in PR #18299:
URL: https://github.com/apache/kafka/pull/18299#discussion_r1983299509
##########
committer-tools/verify_license.py:
##########
@@ -74,12 +75,20 @@ def get_license_deps(license_text):
return set(LICENSE_DEP_PATTERN.findall(license_text))
def main():
+ # Argument parser
+ parser = argparse.ArgumentParser(description="Whether to skip executing ReleaseTarGz.")
+ parser.add_argument('--skipReleaseTarGz', type=str, default=False, help='Whether to skip ReleaseTarGz (true/false)')
Review Comment:
@mumrah @chia7712 Thanks for the wonderful suggestions. I have fixed them, PTAL
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-17607: Add CI step to verify LICENSE-binary [kafka]
Posted by "xijiu (via GitHub)" <gi...@apache.org>.
xijiu commented on PR #18299:
URL: https://github.com/apache/kafka/pull/18299#issuecomment-2574203060
@chia7712
I ran the `./gradlew clean releaseTarGz` again after I merged the `upstream/trunk` branch. And the test was successful as below:
<img width="393" alt="image" src="https://github.com/user-attachments/assets/12edb2d2-ecc1-4570-9dc5-dbe04d38db50" />
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-17607: Add CI step to verify LICENSE-binary [kafka]
Posted by "xijiu (via GitHub)" <gi...@apache.org>.
xijiu commented on PR #18299:
URL: https://github.com/apache/kafka/pull/18299#issuecomment-2558475570
I added a new gradlew task `libLicenseCheck`, so when execute command `./gradlew clean libLicenseCheck`, it will print the missing libs in license file
@mumrah @chia7712 @chiacyu PTAL ☺
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-17607: Add CI step to verify LICENSE-binary [kafka]
Posted by "fonsdant (via GitHub)" <gi...@apache.org>.
fonsdant commented on PR #18299:
URL: https://github.com/apache/kafka/pull/18299#issuecomment-2585007385
Hi, @mumrah! I have noted this task is very similiar to another old one I have picked up. This is my PR: #17888. Are they the same?
https://issues.apache.org/jira/browse/KAFKA-12622
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-17607: Add CI step to verify LICENSE-binary [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on code in PR #18299:
URL: https://github.com/apache/kafka/pull/18299#discussion_r1981005687
##########
committer-tools/verify_license.py:
##########
@@ -74,12 +75,20 @@ def get_license_deps(license_text):
return set(LICENSE_DEP_PATTERN.findall(license_text))
def main():
+ # Argument parser
+ parser = argparse.ArgumentParser(description="Process some integers.")
Review Comment:
please replace `Process some integers.` by other description
##########
.github/workflows/build.yml:
##########
@@ -166,6 +166,8 @@ jobs:
find ./site-docs/generated -type f -exec grep -L "." {} \; >&2
exit 1
fi
+ - name: Verify license file
+ run: python committer-tools/verify_license.py --skipReleaseTarGz true
Review Comment:
`true` is redundant.
##########
committer-tools/verify_license.py:
##########
@@ -74,12 +75,20 @@ def get_license_deps(license_text):
return set(LICENSE_DEP_PATTERN.findall(license_text))
def main():
+ # Argument parser
+ parser = argparse.ArgumentParser(description="Process some integers.")
+ parser.add_argument('--skipReleaseTarGz', type=str, default='false', help='Whether to skip ReleaseTarGz (true/false)')
+ args = parser.parse_args()
+
# Assume the current working directory is the project root.
project_dir = os.getcwd()
print("Using project directory:", project_dir)
-
+
+ if args.skipReleaseTarGz.lower() == 'true':
+ print("Skip running './gradlew clean releaseTarGz'")
+ else:
+ run_gradlew(project_dir)
# Build the tarball.
Review Comment:
please move the comment to line#89
##########
committer-tools/verify_license.py:
##########
@@ -74,12 +75,20 @@ def get_license_deps(license_text):
return set(LICENSE_DEP_PATTERN.findall(license_text))
def main():
+ # Argument parser
+ parser = argparse.ArgumentParser(description="Process some integers.")
+ parser.add_argument('--skipReleaseTarGz', type=str, default='false', help='Whether to skip ReleaseTarGz (true/false)')
Review Comment:
`default='false'` -> `default=False`
##########
committer-tools/verify_license.py:
##########
@@ -74,12 +75,20 @@ def get_license_deps(license_text):
return set(LICENSE_DEP_PATTERN.findall(license_text))
def main():
+ # Argument parser
+ parser = argparse.ArgumentParser(description="Process some integers.")
+ parser.add_argument('--skipReleaseTarGz', type=str, default='false', help='Whether to skip ReleaseTarGz (true/false)')
+ args = parser.parse_args()
+
# Assume the current working directory is the project root.
project_dir = os.getcwd()
print("Using project directory:", project_dir)
-
+
+ if args.skipReleaseTarGz.lower() == 'true':
Review Comment:
`if args.skipReleaseTarGz:`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-17607: Add CI step to verify LICENSE-binary [kafka]
Posted by "xijiu (via GitHub)" <gi...@apache.org>.
xijiu commented on PR #18299:
URL: https://github.com/apache/kafka/pull/18299#issuecomment-2700736205
@chia7712 Thanks for CR and I have fixed them. There is a small point that when I executed the python script locally, it prompted that I needed to provide parameters. Please take a look.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-17607: Add CI step to verify LICENSE-binary [kafka]
Posted by "m1a2st (via GitHub)" <gi...@apache.org>.
m1a2st commented on code in PR #18299:
URL: https://github.com/apache/kafka/pull/18299#discussion_r1898843091
##########
.github/workflows/build.yml:
##########
@@ -114,6 +114,7 @@ jobs:
# --no-scan: For public fork PRs, we won't attempt to publish the scan
run: |
./gradlew --build-cache --info $SCAN_ARG check siteDocTar -x test
+ ./gradlew releaseTarGz
Review Comment:
how about `./gradlew --build-cache --info $SCAN_ARG check siteDocTar releaseTarGz -x test`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-17607: Add CI step to verify LICENSE-binary [kafka]
Posted by "xijiu (via GitHub)" <gi...@apache.org>.
xijiu commented on code in PR #18299:
URL: https://github.com/apache/kafka/pull/18299#discussion_r1972668711
##########
.github/workflows/build.yml:
##########
@@ -127,6 +127,9 @@ jobs:
gradle-cache-read-only: ${{ !inputs.is-trunk }}
gradle-cache-write-only: ${{ inputs.is-trunk }}
develocity-access-key: ${{ secrets.DEVELOCITY_ACCESS_KEY }}
+ - name: Verify license file
+ if: always()
Review Comment:
@m1a2st Thanks for the code review, and you are right, I have fixed it, PTAL
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-17607: Add CI step to verify LICENSE-binary [kafka]
Posted by "xijiu (via GitHub)" <gi...@apache.org>.
xijiu commented on PR #18299:
URL: https://github.com/apache/kafka/pull/18299#issuecomment-2658097428
@chia7712 @mumrah PTAL 🤝
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-17607: Add CI step to verify LICENSE-binary [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on PR #18299:
URL: https://github.com/apache/kafka/pull/18299#issuecomment-2565710373
@xijiu btw, I open https://issues.apache.org/jira/browse/KAFKA-18375 to fix the license file for 4.0 release.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-17607: Add CI step to verify LICENSE-binary [kafka]
Posted by "mumrah (via GitHub)" <gi...@apache.org>.
mumrah commented on code in PR #18299:
URL: https://github.com/apache/kafka/pull/18299#discussion_r1899638244
##########
.github/workflows/build.yml:
##########
@@ -113,7 +113,7 @@ jobs:
# --scan: Publish the build scan. This will only work on PRs from apache/kafka and trunk
# --no-scan: For public fork PRs, we won't attempt to publish the scan
run: |
- ./gradlew --build-cache --info $SCAN_ARG check siteDocTar -x test
+ ./gradlew --build-cache --info $SCAN_ARG check siteDocTar releaseTarGz -x test
Review Comment:
releaseTarGz includes siteDocTar, so I think we can exclude siteDocTar here
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-17607: Add CI step to verify LICENSE-binary [kafka]
Posted by "xijiu (via GitHub)" <gi...@apache.org>.
xijiu commented on code in PR #18299:
URL: https://github.com/apache/kafka/pull/18299#discussion_r1900013841
##########
.github/workflows/build.yml:
##########
@@ -113,7 +113,7 @@ jobs:
# --scan: Publish the build scan. This will only work on PRs from apache/kafka and trunk
# --no-scan: For public fork PRs, we won't attempt to publish the scan
run: |
- ./gradlew --build-cache --info $SCAN_ARG check siteDocTar -x test
+ ./gradlew --build-cache --info $SCAN_ARG check siteDocTar releaseTarGz -x test
Review Comment:
hi @mumrah , I have excluded `siteDocTar`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-17607: Add CI step to verify LICENSE-binary [kafka]
Posted by "xijiu (via GitHub)" <gi...@apache.org>.
xijiu commented on code in PR #18299:
URL: https://github.com/apache/kafka/pull/18299#discussion_r1904784672
##########
build.gradle:
##########
@@ -1326,6 +1326,42 @@ project(':core') {
from(project(':tools:tools-api').jar) { into("libs/") }
from(project(':tools:tools-api').configurations.runtimeClasspath) { into("libs/") }
duplicatesStrategy 'exclude'
+
+ doLast {
+ def prefixPath = "$rootDir/core/build/distributions/"
+ def bathPath = prefixPath + "kafka_${versions.baseScala}-${project.properties['version']}"
+ def tgzPath = bathPath + '.tgz'
+ exec {
+ commandLine 'tar', 'xzf', tgzPath, '-C', prefixPath
+ }
+
+ def licenseFile = file(bathPath + '/LICENSE')
+ def libsDir = file(bathPath + '/libs')
+ def result = true
+ libsDir.eachFile { file ->
+ if (file.name.endsWith('.jar')
+ && !file.name.startsWith('kafka')
+ && !file.name.startsWith('connect')
+ && !file.name.startsWith('trogdor')) {
+
+ def isLicensePresent = false
+ licenseFile.eachLine { line ->
+ if (line.contains(file.name - '.jar')) {
+ isLicensePresent = true
+ }
+ }
+ if (!isLicensePresent) {
+ println "warning: ${file.name} is missing in the license file"
Review Comment:
@chia7712
Sure, file: https://issues.apache.org/jira/browse/KAFKA-18420
And I will continue to work on the issue.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-17607: Add CI step to verify LICENSE-binary [kafka]
Posted by "xijiu (via GitHub)" <gi...@apache.org>.
xijiu commented on code in PR #18299:
URL: https://github.com/apache/kafka/pull/18299#discussion_r1900038679
##########
build.gradle:
##########
@@ -1326,6 +1326,42 @@ project(':core') {
from(project(':tools:tools-api').jar) { into("libs/") }
from(project(':tools:tools-api').configurations.runtimeClasspath) { into("libs/") }
duplicatesStrategy 'exclude'
+
+ doLast {
+ def prefixPath = "$rootDir/core/build/distributions/"
+ def bathPath = prefixPath + "kafka_${versions.baseScala}-${project.properties['version']}"
+ def tgzPath = bathPath + '.tgz'
+ exec {
+ commandLine 'tar', 'xzf', tgzPath, '-C', prefixPath
+ }
+
+ def licenseFile = file(bathPath + '/LICENSE')
+ def libsDir = file(bathPath + '/libs')
+ def result = true
+ libsDir.eachFile { file ->
+ if (file.name.endsWith('.jar')
+ && !file.name.startsWith('kafka')
+ && !file.name.startsWith('connect')
+ && !file.name.startsWith('trogdor')) {
+
+ def isLicensePresent = false
+ licenseFile.eachLine { line ->
+ if (line.contains(file.name - '.jar')) {
+ isLicensePresent = true
+ }
+ }
+ if (!isLicensePresent) {
+ println "warning: ${file.name} is missing in the license file"
Review Comment:
@chia7712
It seems quite necessary, but the formats of these jar licenses are not quite the same, such as:
```
jakarta.annotation-api-1.3.5
zstd-jni-1.5.6-6 see: licenses/zstd-jni-BSD-2-clause
argparse4j-0.7.0, see: licenses/argparse-MIT
```
We should extract the following strings
```
jakarta.annotation-api-1.3.5
zstd-jni-1.5.6-6
argparse4j-0.7.0
```
So if we want to extract those, the rules could be:
1. No spaces in each line
2. Contains spaces, but includes ' see:'
3. Contains spaces, but includes ', see:'
But as the subsequent modifications are made, this may be take mistakes.
Or we can establish a guideline to identify lib-licenses (such as add specified prefix or suffix), which should be followed when modifying `LICENSE-binary` in the future.
WDYT ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-17607: Add CI step to verify LICENSE-binary [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on code in PR #18299:
URL: https://github.com/apache/kafka/pull/18299#discussion_r1976317444
##########
.github/workflows/build.yml:
##########
@@ -127,6 +127,8 @@ jobs:
gradle-cache-read-only: ${{ !inputs.is-trunk }}
gradle-cache-write-only: ${{ inputs.is-trunk }}
develocity-access-key: ${{ secrets.DEVELOCITY_ACCESS_KEY }}
+ - name: Verify license file
+ run: python committer-tools/verify_license.py
Review Comment:
the script needs to run the `clean releaseTarGz`, so it will rebuild whole project and it seems to me it is a bit expensive as CI. Maybe we can do more works on this?
1. run this check after `Check generated documentation`
2. `Compile and validate` should build `releaseTarGz` too
3. `verify_license.py` should be able to skip `releaseTarGz`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-17607: Add CI step to verify LICENSE-binary [kafka]
Posted by "mumrah (via GitHub)" <gi...@apache.org>.
mumrah commented on code in PR #18299:
URL: https://github.com/apache/kafka/pull/18299#discussion_r1981634646
##########
committer-tools/verify_license.py:
##########
@@ -74,12 +75,20 @@ def get_license_deps(license_text):
return set(LICENSE_DEP_PATTERN.findall(license_text))
def main():
+ # Argument parser
+ parser = argparse.ArgumentParser(description="Whether to skip executing ReleaseTarGz.")
+ parser.add_argument('--skipReleaseTarGz', type=str, default=False, help='Whether to skip ReleaseTarGz (true/false)')
Review Comment:
I would suggest a flag name like "rebuild" or similar.
Also, this is not the correct way to configure a boolean argument. You should do something like:
```python
parser.add_argument("--my-flag", action="store_true", help="Help text")
```
https://docs.python.org/3/library/argparse.html#action
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-17607: Add CI step to verify LICENSE-binary [kafka]
Posted by "mumrah (via GitHub)" <gi...@apache.org>.
mumrah commented on code in PR #18299:
URL: https://github.com/apache/kafka/pull/18299#discussion_r1983640140
##########
committer-tools/verify_license.py:
##########
@@ -77,14 +77,14 @@ def get_license_deps(license_text):
def main():
# Argument parser
parser = argparse.ArgumentParser(description="Whether to skip executing ReleaseTarGz.")
- parser.add_argument('--skipReleaseTarGz', type=str, default=False, help='Whether to skip ReleaseTarGz (true/false)')
+ parser.add_argument('--skip-build', action="store_true", help='Whether to skip ReleaseTarGz (true/false)')
Review Comment:
nit: mixture of single and double quotes. I'd suggest using double quotes like the rest of the file.
Also the `(true/false)` part is not necessary. The help text should say what setting this flag does, e.g. "skip the build"
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-17607: Add CI step to verify LICENSE-binary [kafka]
Posted by "m1a2st (via GitHub)" <gi...@apache.org>.
m1a2st commented on code in PR #18299:
URL: https://github.com/apache/kafka/pull/18299#discussion_r1897336398
##########
.github/workflows/build.yml:
##########
@@ -143,6 +143,9 @@ jobs:
find ./site-docs/generated -type f -exec grep -L "." {} \; >&2
exit 1
fi
+ - name: Check missing lib license
+ run: |
+ ./gradlew releaseTarGz
Review Comment:
I think @mumrah means, this gradle command should move to `Compile and validate` step
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-17607: Add CI step to verify LICENSE-binary [kafka]
Posted by "mumrah (via GitHub)" <gi...@apache.org>.
mumrah commented on code in PR #18299:
URL: https://github.com/apache/kafka/pull/18299#discussion_r1895818795
##########
build.gradle:
##########
@@ -1328,6 +1328,43 @@ project(':core') {
duplicatesStrategy 'exclude'
}
+ task libLicenseCheck(dependsOn: 'releaseTarGz', type: Copy) {
Review Comment:
I think it would be better to do this as part of releaseTarGz so we can't mistakenly produce an artifact that is missing licenses. I glanced through build.gradle and I don't see where we're gathering the libraries into the LICENSE file. Do you know where that happens?
##########
.github/workflows/build.yml:
##########
@@ -143,6 +143,9 @@ jobs:
find ./site-docs/generated -type f -exec grep -L "." {} \; >&2
exit 1
fi
+ - name: Check missing lib license
+ run: |
+ ./gradlew libLicenseCheck
Review Comment:
Can you add this to the previous `gradlew` invocation?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-17607: Add CI step to verify LICENSE-binary [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on code in PR #18299:
URL: https://github.com/apache/kafka/pull/18299#discussion_r1899667957
##########
build.gradle:
##########
@@ -1326,6 +1326,42 @@ project(':core') {
from(project(':tools:tools-api').jar) { into("libs/") }
from(project(':tools:tools-api').configurations.runtimeClasspath) { into("libs/") }
duplicatesStrategy 'exclude'
+
+ doLast {
+ def prefixPath = "$rootDir/core/build/distributions/"
+ def bathPath = prefixPath + "kafka_${versions.baseScala}-${project.properties['version']}"
+ def tgzPath = bathPath + '.tgz'
+ exec {
+ commandLine 'tar', 'xzf', tgzPath, '-C', prefixPath
+ }
+
+ def licenseFile = file(bathPath + '/LICENSE')
+ def libsDir = file(bathPath + '/libs')
+ def result = true
+ libsDir.eachFile { file ->
+ if (file.name.endsWith('.jar')
+ && !file.name.startsWith('kafka')
+ && !file.name.startsWith('connect')
+ && !file.name.startsWith('trogdor')) {
+
+ def isLicensePresent = false
+ licenseFile.eachLine { line ->
+ if (line.contains(file.name - '.jar')) {
+ isLicensePresent = true
+ }
+ }
+ if (!isLicensePresent) {
+ println "warning: ${file.name} is missing in the license file"
Review Comment:
Can we find out the license which is in the license file but is not in distribution?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-17607: Add CI step to verify LICENSE-binary [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on code in PR #18299:
URL: https://github.com/apache/kafka/pull/18299#discussion_r1905584608
##########
build.gradle:
##########
@@ -1326,6 +1326,42 @@ project(':core') {
from(project(':tools:tools-api').jar) { into("libs/") }
from(project(':tools:tools-api').configurations.runtimeClasspath) { into("libs/") }
duplicatesStrategy 'exclude'
+
+ doLast {
+ def prefixPath = "$rootDir/core/build/distributions/"
+ def bathPath = prefixPath + "kafka_${versions.baseScala}-${project.properties['version']}"
+ def tgzPath = bathPath + '.tgz'
+ exec {
+ commandLine 'tar', 'xzf', tgzPath, '-C', prefixPath
+ }
+
+ def licenseFile = file(bathPath + '/LICENSE')
+ def libsDir = file(bathPath + '/libs')
+ def result = true
+ libsDir.eachFile { file ->
+ if (file.name.endsWith('.jar')
+ && !file.name.startsWith('kafka')
+ && !file.name.startsWith('connect')
+ && !file.name.startsWith('trogdor')) {
+
+ def isLicensePresent = false
+ licenseFile.eachLine { line ->
+ if (line.contains(file.name - '.jar')) {
+ isLicensePresent = true
+ }
+ }
+ if (!isLicensePresent) {
+ println "warning: ${file.name} is missing in the license file"
+ result = false
+ }
+ }
+ }
+
+ if (result)
+ println "LIBS LICENSE check succeed !"
+ else
+ println "LIBS LICENSE check failed !"
Review Comment:
should we mark the task failed to ensure developers have to fix it?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-17607: Add CI step to verify LICENSE-binary [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on PR #18299:
URL: https://github.com/apache/kafka/pull/18299#issuecomment-2573866397
@xijiu Could you please test this PR again? I assume there is no error as [KAFKA-18375](https://issues.apache.org/jira/browse/KAFKA-18375) had been fixed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-17607: Add CI step to verify LICENSE-binary [kafka]
Posted by "mumrah (via GitHub)" <gi...@apache.org>.
mumrah merged PR #18299:
URL: https://github.com/apache/kafka/pull/18299
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-17607: Add CI step to verify LICENSE-binary [kafka]
Posted by "xijiu (via GitHub)" <gi...@apache.org>.
xijiu commented on code in PR #18299:
URL: https://github.com/apache/kafka/pull/18299#discussion_r1897535052
##########
build.gradle:
##########
@@ -1328,6 +1328,43 @@ project(':core') {
duplicatesStrategy 'exclude'
}
+ task libLicenseCheck(dependsOn: 'releaseTarGz', type: Copy) {
Review Comment:
hi David, do you mean where we replace the content of LICENSE with the content of LICENSE-binary ?
If that's the case, I think it should be below:
`from "$rootDir/NOTICE-binary" rename {String filename -> filename.replace("-binary", "")}` in the releaseTarGz task
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-17607: Add CI step to verify LICENSE-binary [kafka]
Posted by "xijiu (via GitHub)" <gi...@apache.org>.
xijiu commented on PR #18299:
URL: https://github.com/apache/kafka/pull/18299#issuecomment-2562055945
@mumrah @m1a2st I have fixed it, PTAL
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-17607: Add CI step to verify LICENSE-binary [kafka]
Posted by "xijiu (via GitHub)" <gi...@apache.org>.
xijiu commented on PR #18299:
URL: https://github.com/apache/kafka/pull/18299#issuecomment-2696522554
@chia7712 I have added a ArgumentParser to `verify_license.py` and it cloud be able skip the `releaseTarGz` now:
Skip:
```
python verify_license.py --skipReleaseTarGz true
```
NOT Skip:
```
python verify_license.py
or
python verify_license.py --skipReleaseTarGz false
```
PTAL 🤝
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-17607: Add CI step to verify LICENSE-binary [kafka]
Posted by "m1a2st (via GitHub)" <gi...@apache.org>.
m1a2st commented on code in PR #18299:
URL: https://github.com/apache/kafka/pull/18299#discussion_r1971829502
##########
.github/workflows/build.yml:
##########
@@ -127,6 +127,9 @@ jobs:
gradle-cache-read-only: ${{ !inputs.is-trunk }}
gradle-cache-write-only: ${{ inputs.is-trunk }}
develocity-access-key: ${{ secrets.DEVELOCITY_ACCESS_KEY }}
+ - name: Verify license file
+ if: always()
Review Comment:
`always()` means this will run even if a previous step failed. I think we don't need this in this step, WDYT?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-17607: Add CI step to verify LICENSE-binary [kafka]
Posted by "xijiu (via GitHub)" <gi...@apache.org>.
xijiu commented on PR #18299:
URL: https://github.com/apache/kafka/pull/18299#issuecomment-2558827681
> Hello @xijiu, Thanks for this PR. Pardon me, I have a question that this `libLicenseCheck` gradle task will be exected on our CI, should we add verify step on the `build.yml`?
@m1a2st Thanks for the CR. I have added this to the CI, PTAL
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org