8000 trd: license and copyright policy by ppannuto · Pull Request #3318 · tock/tock · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

trd: license and copyright policy #3318

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
Jan 20, 2023
Merged

trd: license and copyright policy #3318

merged 8 commits into from
Jan 20, 2023

Conversation

ppannuto
Copy link
Member
@ppannuto ppannuto commented Nov 5, 2022

Pull Request Overview

First draft of TRD for license and copyright rules for the Tock project.

Testing Strategy

N/A

TODO or Help Wanted

Review of language and policy decisions.

Documentation Updated

  • Updated the relevant files in /docs, or no updates are required.

Formatting

  • Ran make prepush.

First drat of TRD for license and copyright rules for the Tock project.
@ppannuto ppannuto requested review from jrvanwhy and a team November 5, 2022 17:04
Copy link
Contributor
@hudson-ayers hudson-ayers left a comment

Choose a reason for hiding this comment

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

Updating #3317 to match the process in this document, as best as I can tell, would require the following changes:

  1. Updating non-rust files that are still code files (e.g. the python/bash code in tools/ and our Makefiles) to include the license notice
  2. Remove the "Copyright Tock Contributors" line from each file, only have a Copyright line if someone chooses to add it.
  3. Add the line "Licensed under the Apache License, Version 2.0 or the MIT License." as the first line of each file, before the SPDX license identifier.

Overall this approach looks good to me. Curious to hear what @jrvanwhy in particular thinks about not requiring the "Copyright Tock Contributors" line on all files without a different copyright assignment

brghena
brghena previously approved these changes Nov 6, 2022
Copy link
Contributor
@brghena brghena left a comment

Choose a reason for hiding this comment

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

This all looks like a reasonable approach to me.

@alevy
Copy link
Member
alevy commented Nov 11, 2022

We're going to take another pass on this to condense it and remove some unnecessary requirements.

Unlike most other PRs, let's hold off on merging after approvals in order to get feedback from external contributors first.

But please approve once you're comfortable so we can be confident that what we get feedback on is similar to what we'll merge.

(draft coming later today)

@ppannuto
Copy link
Member Author

Quick comment re the formatting bits: the pedantic, low-level bits were written with an eye towards making Johnathan's life easier re: automatic enforcement. I do think top of file is important and clustering licenses and copyright; the rest I have less attachment to.

@jrvanwhy
Copy link
Contributor

Quick comment re the formatting bits: the pedantic, low-level bits were written with an eye towards making Johnathan's life easier re: automatic enforcement. I do think top of file is important and clustering licenses and copyright; the rest I have less attachment to.

In this respect, I think there are two distinct groups of license notices:

Tock project notices: Notices added by Tock contributors when they contribute to the Tock project.
3p notices: Notices in code that the Tock project copied from another project, such as the sip_hash.rs notice.

When I commented on the Format section, I assumed this policy would apply to both sets of license notices. I think that being picky about 3p notices makes it difficult to import code from other projects.

We could limit our formatting requirements and automated enforcement to Tock project notices. Then we could import code from other projects as long as that code is also (Apache 2.0 OR MIT)-licensed, while remaining picky about most of the license headers in the repository.

We'll probably end up with an exceptions list in the tool. For example, if someone copies a file from another project without making any changes to it, then it should be committed without a Tock project notice. In that case, we'll need to list the file as an exception in the tool. When we eventually make our own contributions to that file, we will then add a Tock project notice and remove the license exception.

@jrvanwhy
Copy link
Contributor

Rendered view

@jrvanwhy
Copy link
Contributor

We'll probably end up with an exceptions list in the tool. For example, if someone copies a file from another project without making any changes to it, then it should be committed without a Tock project notice. In that case, we'll need to list the file as an exception in the tool. When we eventually make our own contributions to that file, we will then add a Tock project notice and remove the license exception.

I just finished typing up the notes from today's core WG call. Having listened to the discussion again, I've changed my mind slightly on this.

If we copy a file from another (Apache 2.0 OR MIT) project into Tock, I think it is totally fine to add something like:

// Licensed under the Apache License, Version 2.0 or the MIT License.
// SPDX-License-Identifier: Apache-2.0 OR MIT

at the top of the file. It's the copyright line that we shouldn't add if we didn't contribute to the file.

However, I do think we should allow some variation in the format if we use a file from another project. For example, if we copy in a file from another project with the following header:

// Copyright ExampleCo
// Licensed under the Apache License, Version 2.0 or the MIT license, at your option.
// SPDX-License-Identifier: Apache-2.0 OR MIT

I think we should leave it intact, rather than requiring us to change it to:

// Licensed under the Apache License, Version 2.0 or the MIT License.
// SPDX-License-Identifier: Apache-2.0 OR MIT
// Copyright ExampleCo
// Licensed under the Apache License, Version 2.0 or the MIT license, at your option.
// SPDX-License-Identifier: Apache-2.0 OR MIT

and the enforcement tool should be flexible enough to make that easy.

brghena
brghena previously approved these changes Nov 13, 2022
alevy
alevy previously approved these changes Nov 13, 2022
Copy link
Contributor
@jrvanwhy jrvanwhy left a comment

Choose a reason for hiding this comment

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

Based on my reading of the document, I think the enforcement tool should:

  1. Verify the license information is present near the top of each file and formatted according to this policy.
  2. NOT look for license information elsewhere in the file, and ignore any that is present.

With this implementation, we could copy code from other projects with license/copyright information in the middle of the file without adding an exception in the tool. The drawback is that it would not catch errant license headers in the middle of new contributions.

What do you think?

lschuermann
lschuermann previously approved these changes Nov 17, 2022
Copy link
Member
@lschuermann lschuermann left a comment

Choose a reason for hiding this comment

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

This looks pretty good to me.

@alevy
Copy link
Member
alevy commented Nov 18, 2022

Additional conclusions:

  • The expectations for formatting are about the Tock notices, other notices don't have to follow these requirements.
  • If notices different enough to modify then instead create a new one above it, but should be flexible to accept headers that are close enough.

@bradjc bradjc dismissed stale reviews from lschuermann, alevy, and brghena via 21609c7 November 21, 2022 16:48
@bradjc
Copy link
Contributor
bradjc commented Nov 21, 2022

I fixed the header formatting.

@jrvanwhy
Copy link
Contributor

Overall this approach looks good to me. Curious to hear what @jrvanwhy in particular thinks about not requiring the "Copyright Tock Contributors" line on all files without a different copyright assignment

I just noticed I never answered this question. Personally, I'm not a fan of making the copyright notice optional. Making the copyright notice optional will tend to overrepresent the contributions of some contributors relative to others.

For example, suppose contributors A, B, C, D, and E work together on a file (all contributing significantly), but the only contributor who wants to add a copyright notice is A. In that case, the license header will become:

# Licensed under the Apache License, Version 2.0 or the MIT License.
# SPDX-License-Identifier: Apache-2.0 OR MIT
# Copyright A 2022.

Someone who is not from the Tock project who stumbles across this file will see it as primarily A's work, rather than a community project.

Requiring the "Copyright Tock Contributors" line would change that to:

# Licensed under the Apache License, Version 2.0 or the MIT License.
# SPDX-License-Identifier: Apache-2.0 OR MIT
# Copyright Tock Contributors 2022.
# Copyright A 2022.

which is a more accurate representation of the code's authorship.

@lschuermann
Copy link
Member

I share the spirit of @jrvanwhy's comment. Yet, I'm unsure whether requiring such a line might not introduce some issues: obviously we'd not add such a line on a compatibly-licensed, vendored file. But even when a file is added e.g. by its original author to Tock, I think it might be weird to force this developer to attribute the wider "Tock contributors" despite the file not having received any contributions other than the original author's (yet). I think having a "Copyright Tock Contributors" line in each file as a convention is good, and I would adapt that, but I'm wary about enforcing it. I presume every contributor would be able to decide on their own whether to attribute just themselves, the "Tock contributors", or both.

@jrvanwhy
Copy link
Contributor

We could require the "Copyright Tock Contributors" line only in cases where non-trivial contributions are made by contributors who do not have their own copyright lines. In practice, I think this would result in most files carrying only the "Copyright Tock Contributors" line (because most of y'all seem to prefer that over per-person copyrights), and a handful of files having 1 or 2 specific contributors named.

The downside is that "non-trivial contributions" is a bit vague and therefore this cannot be automatically enforced, but I don't expect that will be an issue in practice.

@hudson-ayers
Copy link
Contributor

We could require the "Copyright Tock Contributors" line only in cases where non-trivial contributions are made by contributors who do not have their own copyright lines. In practice, I think this would result in most files carrying only the "Copyright Tock Contributors" line (because most of y'all seem to prefer that over per-person copyrights), and a handful of files having 1 or 2 specific contributors named.

The downside is that "non-trivial contributions" is a bit vague and therefore this cannot be automatically enforced, but I don't expect that will be an issue in practice.

I disagree that this will not be an issue in practice. What will happen without automatic enforcement is there will be tons of files with "Copyright FIRST_AUTHOR" at the top that have in fact been modified by dozens of other people. For example, Phil used to often add an Author: Philip Levis line to the top of files he created. If you grep for that in the Tock repo, you will find dozens of files with just "Author: Philip Levis" or maybe "Author: Philip Levis, Leon Schuermann" where looking at the git history will reveal many other authors with significant contributions.

I think it is much easier in practice to just require "Copyright Tock Contributors". If an individual author wishes to add an additional copyright for just their own name they are free to do so.

@alevy
Copy link
Member
alevy commented Jan 5, 2023

I'm going to convert this from a Draft PR to a real PR. I believe the remaining issue is just whether to require a copyright line or make it optional. Please comment if there is more missing still.

@alevy alevy marked this pull request as ready for review January 5, 2023 21:30
alevy
alevy previously approved these changes Jan 5, 2023
Johnathan Van Why added 2 commits January 13, 2023 09:59
…ate examples, include "Copyright Tock Contributors" in the examples.
License policy 2023-01-23 core call updates
…ividual contributor copyright from the main example.
@lschuermann lschuermann requested a review from a team January 15, 2023 12:54
@alevy
Copy link
Member
alevy commented Jan 20, 2023

bors r+

@bors
Copy link
Contributor
bors bot commented Jan 20, 2023

@bors bors bot merged commit eb4aa9d into master Jan 20, 2023
bors bot added a commit that referenced this pull request Jan 20, 2023
3345: Add a license checking tool. r=alevy a=jrvanwhy

### Pull Request Overview

This pull request adds a tool that checks for license headers as specified in #3318.

### Testing Strategy

I adapted `ci-job-tools` to run tests in the `tools/` workspace, and ran the tool by hand to verify it reports errors + success correctly.

### TODO or Help Wanted

#3318 is undergoing revision.

I currently have the license checker disabled on all files *except new files added in this PR*, because otherwise it does its job and fails CI.

Also, after license headers are added, I need to evaluate the speed of the tool. It currently runs in 100 milliseconds, which seems fast enough, but it may slow down when license headers are added (it may also speed up). If it slows down too much (e.g. takes over a second), I'll revert to a simpler -- but less accurate -- parsing implementation.

### Documentation Updated

- [X] Updated the relevant files in `/docs`, or no updates are required.

### Formatting

- [X] Ran `make prepush`.


Co-authored-by: Johnathan Van Why <jrvanwhy@google.com>
@bors bors bot deleted the doc-copy-license branch January 20, 2023 13:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants
0