Skip to content
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

[551] Create separate LICENSE and NOTICE files for xtable-hudi-support-extensions #572

Merged
merged 1 commit into from
Oct 31, 2024

Conversation

vinishjail97
Copy link
Contributor

@vinishjail97 vinishjail97 commented Oct 29, 2024

Important Read

  • Please ensure the GitHub issue is mentioned at the beginning of the PR

What is the purpose of the pull request

The bundled jar in xtable-hudi-support-extensions needs a different version of LICENSE and NOTICE file as it bundles MIT and BSD-3 license dependencies. Excluded hudi-common's transitive dependencies (hbase, hadoop etc.) from xtable-core by making it as provided to avoid the following non ASF compliant dependencies in the class path. Refer xtable-hudi-support-extensions tab in the sheet.

  1. org.openjdk.jol:jol-core:jar:0.16
  2. javax.activation:javax.activation-api:jar:1.2.0
  3. javax.ws.rs:javax.ws.rs-api:jar:2.1.1
  4. org.glassfish.web:javax.servlet.jsp:jar:2.3.2
  5. org.glassfish:javax.el:jar:3.0.0
  6. javax.xml.bind:jaxb-api:jar:2.2.11

Brief change log

(for example:)

  • Create separate LICENSE and NOTICE files for xtable-hudi-support-extensions
  • Excluded hudi-common from xtable-core to avoid non ASF compliant dependencies

Verify this pull request

(Please pick either of the following options)

This pull request is a trivial rework / code cleanup without any test coverage.

Verified changes locally by building jar.

mvn clean install -DskipTests
# Verified the contents of LICENSE and NOTICE.
jar xf ./xtable-hudi-support/xtable-hudi-support-extensions/target/xtable-hudi-support-extensions_2.12-0.2.0-SNAPSHOT-bundled.jar META-INF/ 
# Verified there's no presence of non-ASF compliant dependency in the class path. 
jar tf xtable-hudi-support/xtable-hudi-support-extensions/target/xtable-hudi-support-extensions_2.12-0.2.0-SNAPSHOT-bundled.jar | sort > jarOutput.txt 

Copy link
Contributor

@zabetak zabetak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall the changes look reasonable. I added a few suggestions for improvements. Apart from that I didn't verify the content of the jar file to see if the LICENSE info is complete. I will try to do that once all the other comments are addressed.

@vinishjail97
Copy link
Contributor Author

vinishjail97 commented Oct 29, 2024

Overall the changes look reasonable. I added a few suggestions for improvements. Apart from that I didn't verify the content of the jar file to see if the LICENSE info is complete. I will try to do that once all the other comments are addressed.

Thanks for the feedback @zabetak, addressed comments. You can refer the google sheet as well for checking licenses for transitive dependencies in the bundled jar in this section, but yeah you can do your independent check as well for the presence of non ASF compliant classes.
Bundled Depdencies through maven-shade-plugin (After making hudi-common as provided)

Copy link
Contributor

@zabetak zabetak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked the contents of the xtable-hudi-support-extensions_2.12-0.2.0-SNAPSHOT-bundled.jar and there are some small issues:

  • There are two LICENSE and multiple NOTICE files inside the jar
jar tf xtable-hudi-support-extensions_2.12-0.2.0-SNAPSHOT-bundled.jar | grep -i NOTICE
META-INF/NOTICE
NOTICE
NOTICE.txt

There should be only one and it should be under META-INF inside the jar; the extraneous NOTICE files may need to be merged or removed.

  • Found some javax classes inside the jar that seem to be under CDDL + GPLv2 with classpath exception; they should either be removed or mentioned in the LICENSE file.

@vinishjail97
Copy link
Contributor Author

Appreciate the patience on this @zabetak, have addressed the issues you pointed out.

jar tf xtable-hudi-support/xtable-hudi-support-extensions/target/xtable-hudi-support-extensions_2.12-0.2.0-SNAPSHOT-bundled.jar | grep NOTICE
META-INF/NOTICE

Copy link
Contributor

@zabetak zabetak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are still two LICENSE files inside the jar:

LICENSE
META-INF/LICENSE

We should keep only the one under the META-INF directory.

@vinishjail97
Copy link
Contributor Author

vinishjail97 commented Oct 31, 2024

@zabetak Updated, my bad didn't notice LICENSE file when re-ordering the transformers for maven shade.

jar tf xtable-hudi-support/xtable-hudi-support-extensions/target/xtable-hudi-support-extensions_2.12-0.2.0-SNAPSHOT-bundled.jar | grep NOTICE
META-INF/NOTICE

jar tf xtable-hudi-support/xtable-hudi-support-extensions/target/xtable-hudi-support-extensions_2.12-0.2.0-SNAPSHOT-bundled.jar | grep LICENSE
META-INF/licenses/LICENSE-antlr4-runtime
META-INF/licenses/LICENSE-checker-qual
META-INF/licenses/LICENSE-javax-annotation-api
META-INF/licenses/LICENSE-paranamer
META-INF/licenses/LICENSE-slf4j-api
META-INF/LICENSE

Copy link
Contributor

@zabetak zabetak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Many thanks for your efforts Vinish and apologies for the multiple review rounds.

@vinishjail97
Copy link
Contributor Author

LGTM! Many thanks for your efforts Vinish and apologies for the multiple review rounds.

Appreciate your patience and reviews as well to get LICENSE and NOTICE related work in good shape.

@vinishjail97
Copy link
Contributor Author

Squash into a single commit.

@vinishjail97 vinishjail97 merged commit fb08e2d into main Oct 31, 2024
2 checks passed
@vinishjail97 vinishjail97 deleted the 551-NOTICE-MIT-BundledJar branch October 31, 2024 19:00
@vinishjail97 vinishjail97 mentioned this pull request Oct 31, 2024
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants