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

Update NOTICE.txt #2163

Merged
merged 2 commits into from Sep 26, 2018
Merged

Conversation

danmaas
Copy link

@danmaas danmaas commented Sep 24, 2018

Summary
Update NOTICE.txt with latest open-source license info

Additional Notes
This follows the recent NOTICE.txt update to mattermost-server, PR here: mattermost/mattermost#9433

The only difference is that this is an NPM-based project, so the dependency manifest is created by listing the "dependencies" section of all package.json files.

  • Add new dependencies (jsc-android, moment-timezone, react-native-calendars, react-native-drawer-layout, react-navigation, rn-placeholder)
  • Remove no-longer-used dependencies (react-native-drawer, react-native-fast-image)
  • Update dependency with new name (TinyColor->tinycolor2)
  • Update homepage and project owner info for all dependencies
  • Add SPDX open-source license IDs
  • Alphabetize dependency list so future update diffs will be smaller

Linebreak/whitespace changes in this PR are to match the original LICENSE files supplied with each dependency. This is a one-time change that will make future automated updates smaller.

- Add new dependencies (jsc-android, moment-timezone, react-native-calendars, react-native-drawer-layout, react-navigation, rn-placeholder)
- Remove no-longer-used dependencies (react-native-drawer, react-native-fast-image)
- Update dependency with new name (TinyColor->tinycolor2)
- Update homepage and project owner info for all dependencies
- Add SPDX open-source license IDs
- Alphabetize dependency list so future update diffs will be smaller
Copy link

@jasonblais jasonblais left a comment

Choose a reason for hiding this comment

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

I haven't done a line-by-line review, but have done overall reviews and spot checks. Based on the changes + previous discussions in server and desktop repositories, this looks good to me.

@jasonblais jasonblais added the 4: Reviews Complete All reviewers have approved the pull request label Sep 25, 2018
NOTICE.txt Outdated

## commonmark

This product contains 'commonmark' by John MacFarlane.
Copy link
Contributor

Choose a reason for hiding this comment

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

a modified version of 'commonmark'

Copy link
Author

Choose a reason for hiding this comment

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

I'm happy to update the automation to distinguish "a modified version of..." from the unmodified upstream, where possible.

Would it be sufficient to look at the package.json version constraint, and if we find a version like "github:xxxx", where xxxx is not the official upstream repo, then we say it's "modified"?

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess that should work

Copy link
Author

Choose a reason for hiding this comment

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

done, the PR is now updated with "a modified version of..." as discussed above.


---

lib/normalize-reference.js is a slightly modified version of
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't seem to find this one

Copy link
Author

Choose a reason for hiding this comment

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

This is part of the commonmark.js LICENSE file which we include verbatim:
https://github.com/commonmark/commonmark.js/blob/master/LICENSE

It's a little awkward that upstream LICENSEs can inject confusing --- markers. Not sure what to do about this.

Copy link
Contributor

Choose a reason for hiding this comment

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

not sure either, @hmhealey any thoughts?

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 what the typical process for including upstream licenses would be. It's a bit awkward in cases, like this, but I don't think it should cause any problems outside of being a bit confusing.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks Harrison (good to hear from you btw! :) ). From a technical point of view, it would be most efficient just to include the SPDX license identifier (for known open-source licenses), rather than pasting in the full text. But, this NOTICE is connected to legal compliance requirements, so it probably makes sense to err on the side of caution. I'm still in the process of researching expert opinions on this.


---

lib/from-code-point.js is derived from a polyfill
Copy link
Contributor

Choose a reason for hiding this comment

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

can't find this one either

Copy link
Author

Choose a reason for hiding this comment

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

same as above, this is just part of the commonmark.js LICENSE file


bench/samples/*.md:

With the exception of `bench/samples/README.md`, the samples in
Copy link
Contributor

Choose a reason for hiding this comment

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

are these dependencies of the dependencies?

Copy link
Author

Choose a reason for hiding this comment

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

same as above, this is just part of the commonmark.js LICENSE file

NOTICE.txt Outdated

## commonmark-react-renderer

This product contains 'commonmark-react-renderer' by Espen Hovlandsdal.
Copy link
Contributor

Choose a reason for hiding this comment

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

a modified version of 'commonmark-react-renderer'

NOTICE.txt Outdated

This product contains 'tinycolor', a small, fast library for color manipulation and conversion in JavaScript. It allows many forms of input, while providing color conversions and other color utility functions
This product contains 'react-native-image-gallery' by Archriss.
Copy link
Contributor

Choose a reason for hiding this comment

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

a modified version of 'react-native-image-gallery'

NOTICE.txt Outdated
## react-native-fast-image
## react-native-notifications

This product contains 'react-native-notifications' by Lidan Hifi.
Copy link
Contributor

Choose a reason for hiding this comment

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

a modified version of 'react-native-notifications'

NOTICE.txt Outdated
## react-native-safe-area
## react-native-youtube

This product contains 'react-native-youtube' by Param Aggarwal.
Copy link
Contributor

Choose a reason for hiding this comment

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

a modified version of 'react-native-youtube'

NOTICE.txt Outdated

## react-native-permissions
This product contains 'rn-fetch-blob' by Joltup.
Copy link
Contributor

Choose a reason for hiding this comment

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

a modified version of 'rn-fetch-blob'

NOTICE.txt Outdated
## react-native-tableview
## rn-placeholder

This product contains 'rn-placeholder' by Marvin FRACHET.
Copy link
Contributor

Choose a reason for hiding this comment

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

a modified version of 'rn-placeholder'

@enahum enahum added Awaiting Submitter Action null and removed 4: Reviews Complete All reviewers have approved the pull request labels Sep 25, 2018
… modified by Mattermost

This is determined by looking at the requested version string in package.json. If the requested version is of the form "github:.../...(#...)", where the GitHub repo is something other than the official upstream repo (as given in the NPM registry entry), then we flag the dependency as being "a modified version".
@jasonblais jasonblais added 2: Dev Review Requires review by a core commiter and removed Awaiting Submitter Action null labels Sep 26, 2018
Copy link
Member

@hmhealey hmhealey left a comment

Choose a reason for hiding this comment

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

Looks good to me. Always a fan of more automation for tedious things like this

@enahum enahum merged commit bb1bbd5 into mattermost:master Sep 26, 2018
@danmaas danmaas deleted the notice-update-20180921 branch December 2, 2018 04:29
@lindy65 lindy65 added Tests/Not Needed Does not require tests and removed Tests/Not Needed Does not require tests labels Apr 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2: Dev Review Requires review by a core commiter Tests/Not Needed Does not require tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants