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

[536] Update LICENSE and NOTICE files #541

Merged
merged 1 commit into from
Sep 18, 2024
Merged

Conversation

vinishjail97
Copy link
Contributor

Important Read

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

What is the purpose of the pull request

After removing xtable-utilities from mvn deploy, include the dependencies where code is adapted. Remaining dependencies for parquet, delta, iceberg the library functions have been used directly provided for generating metadata but code hasn't been copied or reused.

The info regarding Xtable dependencies and their category (A, B, X) can be found here.
xtable-utilities is the only module which contains category B and X dependencies (because of bundling) and has been excluded from the release as part of this PR #540

Brief change log

(for example:)

  • Update LICENSE file for xtable

Verify this pull request

(Please pick either of the following options)

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

@vinishjail97
Copy link
Contributor Author

@pjfanning @jcamachor @zabetak Please review the LICENSE file and let me know if I need to include other dependencies as well.

This sheet contains information regarding XTable dependencies and their category (A, B, X).

@@ -199,3 +199,31 @@
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.

========================================================================
Copy link
Contributor

Choose a reason for hiding this comment

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

After reviewing this further, I'm wondering whether it's better to add this block to the NOTICE file and leave the LICENSE file unchanged. @zabetak, any thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

the XTable NOTICE should be updated to include the main details from the Avro and Hudi NOTICE files.

Copy link
Contributor

Choose a reason for hiding this comment

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

the changes in this PR seem unrelated to the xtable-utilities fat jar and seem more related to the XTable source code (and are needed - but the title of this PR seems wrong).

You also need a separate LICENSE and NOTICE for the xtable-utilities fat jar that list all the license and notice details of the classes that you include in that jar. There is a suspicion that some of the classes should not be in there because they have licenses that are not compatible with the Apache license. The OpenJDK JOL classes would seem to me that they should be removed because they are GPL licensed. https://github.com/openjdk/jol/blob/master/LICENSE

Copy link
Contributor Author

@vinishjail97 vinishjail97 Sep 18, 2024

Choose a reason for hiding this comment

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

@pjfanning Yes these changes are related to XTable source code and updated the PR title as well. There is an in-progress PR to exclude the category X dependencies from xtable-utilities bundle.
#538

For release 0.2.0, we will be excluding the utilities bundle from the release.

Copy link
Contributor

Choose a reason for hiding this comment

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

Although this PR is merged, I will comment here to keep the discussion in one place.

@jcamachor As a rule of thumb updating the LICENSE file is more frequent than updating the NOTICE file. In most cases we shouldn't touch the NOTICE file.

My understanding after reading https://infra.apache.org/licensing-howto.html#mod-notice is that in this case the NOTICE file does not need to be modified. In fact, we should strive to keep the NOTICE file very simple and "Do not add anything to NOTICE which is not legally required."

Other similar LEGAL tickets (e.g., LEGAL-234) imply that we don't need to duplicate the NOTICE file when we bundle code from other ASF projects.

@pjfanning Can you elaborate a bit on why you believe that the NOTICE file should be copied?

For the LICENSE, file we are in a similar situation. According to https://infra.apache.org/licensing-howto.html#alv2-dep it seems that we don't necessarily need to add anything in the file.

Finally, for the code adapted from Apache Hudi and Apache Spark I would argue that it does not classify as 3rd-party work since the files are developed under the same legal entity (The Apache Software Foundation). These files are already submitted to the ASF by the copyright owner. Moving them around (even across projects) and modifying them does not consist in re-submission.

Anyways, I am not a legal expert but I would say that for the two classes that we adapted from Spark and Hudi we don't need anything in the LICENSE and NOTICE file.

@vinishjail97 Let's converge on this discussion before creating the RC2.

Copy link
Contributor

Choose a reason for hiding this comment

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

Both too much and too little is problematic and adds burden to consumers. ASF projects copy the one another so we should not allow "bad" patterns to propagate.

Here we are talking about just two files so if there are doubts about what needs to be done let's open a LEGAL ticket.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not against removing the changes to the NOTICE file. The LICENSE changes were the most important part.

Copy link
Contributor

Choose a reason for hiding this comment

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

I also think that the LICENSE changes are redundant.

Copy link
Contributor

Choose a reason for hiding this comment

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

create a LEGAL issue then - most ASF projects include details of borrowed code from other ASF projects in their LICENSEs.

Copy link
Contributor

Choose a reason for hiding this comment

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

@jcamachor jcamachor self-requested a review September 17, 2024 22:33
@vinishjail97 vinishjail97 changed the title [536] Update LICENSE file with dependencies [536] Update LICENSE and NOTICE files Sep 18, 2024
NOTICE Outdated Show resolved Hide resolved
@vinishjail97
Copy link
Contributor Author

Squashed into single commit.

@vinishjail97 vinishjail97 merged commit 2bb8680 into main Sep 18, 2024
2 checks passed
@vinishjail97 vinishjail97 deleted the 536-License-Review branch September 18, 2024 17:15
@vinishjail97 vinishjail97 mentioned this pull request Oct 3, 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.

4 participants