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

Restore copyright attribution #10155

Closed
wants to merge 1 commit into from
Closed

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Dec 6, 2016

Checklist
  • make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

everything.

Description of change

Refs: nodejs/TSC#174

Back in the io.js days, 3e1b1dd removed copyright headers from the source files. This commit generally restores the original copyright with one edit (s/Node/Node.js) and includes a reference to the LICENSE file for the current licensing details.

I have no doubt that there are some files in here that do not require the addition of the attribution (such as new files added after the project merge, etc). It is a ton of work to identify those files so I went with the bulk approach to start and will go back and remove the attribution comment as those files are identified. I would appreciate help identifying those files and ask that such files be listed in the comments in this PR. Anyone wishing to go the extra mile would open a PR against my dev branch to remove those attributions

Note: This change has been requested by the Node.js Foundation Legal Committee. The specific details of this change are still being discussed and determined.

Moderation note (jasnell): Please direct any discussion or questions about this change to nodejs/TSC#174. This PR should not be used for discussion of the reasons or merits of this edit. Any comment posted here that does not have to do with technical specifics of the individual edits included in this PR will be removed.

@jasnell jasnell added ctc-agenda wip Issues and PRs that are still a work in progress. meta Issues and PRs related to the general management of the project. labels Dec 6, 2016
LICENSE Outdated
@@ -1,7 +1,7 @@
Node.js is licensed for use as follows:

"""
Copyright Node.js contributors. All rights reserved.
Copyright Joyent, Inc. and other Node.js contributors. All rights reserved.
Copy link
Member

@richardlau richardlau Dec 6, 2016

Choose a reason for hiding this comment

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

Does this block need to be updated? Immediately following it is

This license applies to parts of Node.js originating from the
https://github.com/joyent/node repository:
"""
Copyright Joyent, Inc. and other Node contributors. All rights reserved.

which is almost identical and already includes the Joyent attribution.

Copy link
Member Author

Choose a reason for hiding this comment

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

The first block is the current copyright and license block, the second is the original. While almost identical they need to be separate.

Copy link
Member

Choose a reason for hiding this comment

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

Fair enough. My interpretation would be that the first block applies to files that don't originate from https://github.com/joyent/node (as that is what the second block is for). But IANAL.

Copy link
Member Author

Choose a reason for hiding this comment

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

Consider this change a WIP and I'm waiting to hear back from the lawyers to let me know if it's adequate :-)

@mikeal
Copy link
Contributor

mikeal commented Dec 6, 2016

Can we just do a list of files that are in 3e1b1dd and only add it to those?

@mikeal
Copy link
Contributor

mikeal commented Dec 6, 2016

ya, you can use this to get a list of files we need to modify:

git diff-tree --no-commit-id --name-only -r 3e1b1dd

@mikeal
Copy link
Contributor

mikeal commented Dec 6, 2016

Looks like there are 1105 files that need to be altered, well under the 1,878 in the current diff.

@jasnell
Copy link
Member Author

jasnell commented Dec 6, 2016 via email

@mikeal
Copy link
Contributor

mikeal commented Dec 6, 2016

I'm not sure what the rules are for derivative works. In this particular case we are being asked to restore attribution that was explicitly removed. I think that if we keep to that the interested parties will be satisfied.

We have a longer term issue that we need to resolve about what form of attribution and copyright notice we want in all files by default which is almost resolved. I'd prefer not to complicate that with a lot of unnecessary notices already in the files.

@jasnell
Copy link
Member Author

jasnell commented Dec 6, 2016

I generally agree in spirit but the issue is this text from the original attribution: "in all copies or substantial portions of the Software", specifically the "or substantial portions" bit. That would cover files created after 3e1b1dd that contain wholesale copies or significant blocks of code from the original files. These would likely include the various /lib/internal/*.js files and many of the tests that have been broken out (test/parallel/buffer.js was recently split out into multiple files, for instance).

@jasnell
Copy link
Member Author

jasnell commented Dec 6, 2016

@mikeal ... perhaps you can can clarification from the lawyers on this bit?

@jasnell
Copy link
Member Author

jasnell commented Dec 7, 2016

@mikeal: updated per suggestions.

@trevnorris
Copy link
Contributor

@mikeal I'd like that the legal team at least let us know that they have begun review of the new header (as done in #10599). I, and I'm assuming anyone else who works on core code, doesn't want to be stuck with this massive header for the next 6 months or more while we wait for the legal team to give a crap about changing it.

@jasnell
Copy link
Member Author

jasnell commented Feb 24, 2017

@nodejs/ctc ... The Foundation Board approved this change. I will need to rebase it to resolve the current conflicts then it will need to be landed shortly

@jasnell jasnell removed the wip Issues and PRs that are still a work in progress. label Mar 9, 2017
@jasnell jasnell changed the title [WIP] Restore copyright attribution Restore copyright attribution Mar 9, 2017
A prior io.js era commit inappropriately removed the
original copyright statements from the source. This
restores those in any files still remaining from that
edit.

Ref: nodejs/TSC#174
Ref: nodejs#10599
@jasnell
Copy link
Member Author

jasnell commented Mar 9, 2017

@nodejs/ctc ... This is ready. Please take a final look. I'll land tomorrow if everything looks ok

CI: https://ci.nodejs.org/job/node-test-pull-request/6755/

Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

Find it a bit odd that this touched the readable-stream files in deps/npm, but I guess we’ll have to live with that

@jasnell
Copy link
Member Author

jasnell commented Mar 9, 2017

Yep. It touches everything modified by the original pull request that is still remaining in the tree.

jasnell added a commit that referenced this pull request Mar 10, 2017
A prior io.js era commit inappropriately removed the
original copyright statements from the source. This
restores those in any files still remaining from that
edit.

Ref: nodejs/TSC#174
Ref: #10599
PR-URL: #10155

Note: This PR was required, reviewed-by and approved
by the Node.js Foundation Legal Committee and the TSC.
There is no `Approved-By:` meta data.
@jasnell
Copy link
Member Author

jasnell commented Mar 10, 2017

Landed in 98e54b0

@jasnell jasnell closed this Mar 10, 2017
@italoacasas
Copy link
Contributor

@jasnell is this needed in the release?

If that is the case, we need to backport to v7.x

@jasnell
Copy link
Member Author

jasnell commented Mar 13, 2017

For now, no. It might need to be back ported later but don't worry about it just yet

@italoacasas
Copy link
Contributor

Perfect.

jungx098 pushed a commit to jungx098/node that referenced this pull request Mar 21, 2017
A prior io.js era commit inappropriately removed the
original copyright statements from the source. This
restores those in any files still remaining from that
edit.

Ref: nodejs/TSC#174
Ref: nodejs#10599
PR-URL: nodejs#10155

Note: This PR was required, reviewed-by and approved
by the Node.js Foundation Legal Committee and the TSC.
There is no `Approved-By:` meta data.
@jasnell jasnell mentioned this pull request Apr 4, 2017
@gibfahn gibfahn mentioned this pull request Jun 15, 2017
3 tasks
@gibfahn
Copy link
Member

gibfahn commented Jun 17, 2017

@jasnell assuming the same applies to v6.x, feel free to change to a lts-watch-v6.x if that's not the case.

@jasnell
Copy link
Member Author

jasnell commented Jun 18, 2017

Please do not backport this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
meta Issues and PRs related to the general management of the project.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants