-
Notifications
You must be signed in to change notification settings - Fork 943
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
Add @license tags #1507
Conversation
…ment to generated files
packages/firebase/rollup.config.js
Outdated
const licenseOptions = { | ||
banner: `@license Firebase v${pkg.version} | ||
Build: rev-${gitRev.short()} | ||
Terms: https://firebase.google.com/terms/` |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Oh, I think you also need to re-run |
There was a problem hiding this 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.
I just realized we should update license.js as well, so the annotation is added for new files. |
…ense comment to generated files" This reverts commit adb17a2.
…into ch-license
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. |
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.