Skip to content

Add @license tags #1507

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

Merged
merged 8 commits into from
Feb 2, 2019
Merged

Add @license tags #1507

merged 8 commits into from
Feb 2, 2019

Conversation

hsubox76
Copy link
Contributor

@hsubox76 hsubox76 commented Jan 30, 2019

Adding @license tags to all existing license comments and configuring rollup to add license comments to generated code in the same format as Webpack was previously doing. This is to conform to google3 third_party rules about licenses in code.

const licenseOptions = {
banner: `@license Firebase v${pkg.version}
Build: rev-${gitRev.short()}
Terms: https://firebase.google.com/terms/`
Copy link
Contributor

Choose a reason for hiding this comment

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

So @license is supposed to designate the software license that applies to the code, right? So what does @license Firebase vX.Y.Z mean? The Terms URL doesn't really mention software licenses so I'm not sure what this header is communicating about our license.

Also, I believe in the distant past there was discussion about whether we needed to include license headers in our generated build products and the decision was that we did not, since these are closer to binaries than to sources (even though one could argue that any JavaScript code is "source"). So we omitted it since folks are pretty size-sensitive and the full header is 500+ bytes. That said I don't feel too strongly about this. So if we think the header is supposed to be here, fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The Third Party policy is pretty explicit about wanting it in the compiled code. It's possible we could make a second Rollup config just for generating code to put in third_party.

I'm not sure about the unusual format for the compiled code headers, I copied it from the Webpack config that previously added the same format of header to our generated files before we switched to Rollup. The Third Party policy doc seems to indicate we should be putting the entire license text in so I'm not sure if something changed or if the requirement has always been very loose.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, I see! I didn't realize that this is just restoring previous functionality. In that case I'm on board with this PR.

@mikelehen
Copy link
Contributor

Oh, I think you also need to re-run yarn and check in the updated yarn.lock file, since you changed some package.json dependencies.

Copy link
Member

@Feiyang1 Feiyang1 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 should have a separate build target for google3 to get the license header and leave the public ones intact. It doesn't make sense to push the header to developers if the its only purpose is to satisfy google3 requirement.

Alternatively we can just manually add the headers to them when importing to google3. Since there is only a handful of files, it won't be too bad.

@Feiyang1
Copy link
Member

I just realized we should update license.js as well, so the annotation is added for new files.

@hsubox76
Copy link
Contributor Author

hsubox76 commented Feb 1, 2019

Now the plan is to add license headers to the generated text only for internal deployment, so most of the code for that workflow will be in a script living in the third_party dir. The only part that remains in this repo is an alternate rollup config file, because it depends so much on the original.

@hsubox76 hsubox76 merged commit 6b53e00 into master Feb 2, 2019
@hsubox76 hsubox76 deleted the ch-license branch February 11, 2019 21:59
@firebase firebase locked and limited conversation to collaborators Oct 13, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants