Skip to content

Comments

License report & automatic LICENSE content check#54

Merged
RussellSpitzer merged 7 commits intoapache:mainfrom
snazy:license-report-notice-check
Sep 4, 2024
Merged

License report & automatic LICENSE content check#54
RussellSpitzer merged 7 commits intoapache:mainfrom
snazy:license-report-notice-check

Conversation

@snazy
Copy link
Member

@snazy snazy commented Aug 1, 2024

Adds a Gradle plugin to generate license reports, which is then used for two purposes:

  1. Generate a license report
  2. Verify that dependencies that have to be mentioned in LICENSE are mentioned there

The Gradle check task depends on the "dependencies to mention in LICENSE" - so CI will fail if a dependency isn't mentioned there. Those are dependencies that are not licensed as Apache and are either MIT, BSD, Go, ISC or "Universal Permissive".

The licence report is generated as HTML and packed as a ZIP file for each Gradle project that is considered a distribution (currently only :polaris-service).

A new project aggregated-license-report is there to collect the NOTICE + LICENSE files and all individual projects' license reports and add those to a single ZIP file.

This PR is a port of the same functionality that Nessie used for quite a while now.

Depends on #10 and #53

@snazy
Copy link
Member Author

snazy commented Aug 1, 2024

Only the last two commits actually belong to this PR:

  • one that adds the build code
  • one that updates the LICENSE file

@snazy snazy force-pushed the license-report-notice-check branch from 3cfc906 to e70f1a5 Compare August 1, 2024 14:58
@snazy snazy changed the title License report & automatic NOTICE content check License report & automatic LICENSE content check Aug 1, 2024
@snazy snazy force-pushed the license-report-notice-check branch 3 times, most recently from 16605b2 to b622b0b Compare August 7, 2024 13:13
@snazy snazy marked this pull request as ready for review August 7, 2024 13:14
@snazy snazy requested a review from a team as a code owner August 7, 2024 13:14
@snazy
Copy link
Member Author

snazy commented Aug 7, 2024

/cc @jbonofre

@snazy snazy force-pushed the license-report-notice-check branch from b622b0b to befed2c Compare August 8, 2024 22:56
@snazy snazy force-pushed the license-report-notice-check branch from befed2c to 346f06e Compare August 22, 2024 12:02
@snazy snazy force-pushed the license-report-notice-check branch 4 times, most recently from ed95168 to 2214ad4 Compare August 25, 2024 06:38
Copy link
Member

Choose a reason for hiding this comment

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

Could do Set: NeedsMentionLicenses.contains(license) instead of || chain

Copy link
Member

Choose a reason for hiding this comment

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

Comment from another project I think, shouldn't apply here

Copy link
Member Author

Choose a reason for hiding this comment

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

yup - copy-paste left-over

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure these excludes are relevant to Polaris (yet)

Copy link
Member Author

Choose a reason for hiding this comment

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

true

Copy link
Member

Choose a reason for hiding this comment

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

Not relevant to this project as well

Copy link
Member Author

Choose a reason for hiding this comment

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

also true

Copy link
Member

@RussellSpitzer RussellSpitzer left a comment

Choose a reason for hiding this comment

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

I have a few questions on this related to why we aren't including some elements in our license file.

There are also a few comments/ exclusions that I think are particular to Nessie where this was I think copied from that need to be cleaned up.

@snazy snazy force-pushed the license-report-notice-check branch from 2214ad4 to bc0dd26 Compare August 26, 2024 10:26
@snazy snazy requested a review from RussellSpitzer August 26, 2024 16:18
Copy link
Member

Choose a reason for hiding this comment

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

"Used to generate reference license reports"

Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be the Apache license here?

Copy link
Member

Choose a reason for hiding this comment

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

Yes correct

Copy link
Member

Choose a reason for hiding this comment

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

Comment is no longer accurate

Copy link
Member

Choose a reason for hiding this comment

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

Couldn't we just automatically check whether or not our license file matches our dependencies ? We could always do this in a future PR

Copy link
Member

Choose a reason for hiding this comment

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

This is probably a feature for the plugin and not our build, but it would be great if this json could include the rules for license reproduction rather than us coding it ourselves. (IE. Apache needs x type license, MIT needs full repro, etc ...)

RussellSpitzer
RussellSpitzer previously approved these changes Aug 28, 2024
Copy link
Member

@RussellSpitzer RussellSpitzer left a comment

Choose a reason for hiding this comment

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

I think we just need to fix the Copyright notice on LicenseFileValidation.kt and the comment in the same file and we are good to merge here.

Copy link
Member

Choose a reason for hiding this comment

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

We can update the header here.

Copy link
Member

Choose a reason for hiding this comment

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

Yes correct

@RussellSpitzer
Copy link
Member

@snazy There are still some Snowflake headers in there, could you please tidy them up?

@snazy snazy force-pushed the license-report-notice-check branch from 2cd772e to 267030d Compare August 29, 2024 11:46
@snazy snazy force-pushed the license-report-notice-check branch from 267030d to 6d709a8 Compare August 30, 2024 12:03
@snazy
Copy link
Member Author

snazy commented Sep 4, 2024

Ping @RussellSpitzer

@RussellSpitzer RussellSpitzer merged commit fe4d16c into apache:main Sep 4, 2024
@RussellSpitzer
Copy link
Member

Thanks @snazy!

@snazy snazy deleted the license-report-notice-check branch September 4, 2024 15:42
@singhpk234 singhpk234 mentioned this pull request Oct 8, 2025
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