Skip to content
This repository has been archived by the owner. It is now read-only.

RFC: Add CONTRIBUTING.md and require DCO #171

Merged
merged 1 commit into from Jul 9, 2021
Merged

Conversation

Semisol
Copy link
Contributor

@Semisol Semisol commented Jul 8, 2021

To people here: We want your feedback, so please approve, or request changes/comment on what you think needs to be improved.

A CONTRIBUTING.md document has been added, and along with
it a Developer Certificate of Origin (DCO) which has been
partially discussed on IRC.

A Developer Certificate of Origin (DCO) is used in projects
big and small to ensure that liability falls on the person
who wrote the commits if they violate any licenses. You need to
"sign off" your commits by adding Signed-off-by: user <email@email.email>
to the last lines (where Co-authored-by, etc reside).
We have decided to go this way due to a sign off being permanent
and verifiable in the commit history, unlike GH issues/srht mailing
list archives.

A summed up version of it is:
You're either submitting your own work, or affirming that you have permission to submit another person's work under this license, while keeping the original contributor's copyright over the code.

Currently unmerged patches/PRs will be requested to sign-off their commits.

  • The code in this pull request is licensed under "GPLv2 or any later version"*
  • (N/A) I made sure the code compiles on my machine
  • I made sure there are no unnecessary changes in the code*
  • I made sure the title of the PR reflects the core meaning of the issue you are solving*
  • I made sure the commit message(s) contain a description and answer the question "Why do those changes fix that particular issue?" or "Why are those changes really necessary as improvements?"*

* indicates required

CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
solfisher
solfisher previously approved these changes Jul 8, 2021
Copy link
Contributor

@solfisher solfisher left a comment

The concept of "confirmed issues" is worth talking about at some point, but I think this is enough for now.

@imachug
Copy link
Contributor

imachug commented Jul 8, 2021

Tricky question: does it make sense to ask those who contribute via GitHub for a DCO? GitHub already includes the name of the PR author in the description of the merge commit, so what's the point? Just a checkbox in the PR description looks enough.

@Semisol
Copy link
Contributor Author

Semisol commented Jul 8, 2021

Tricky question: does it make sense to ask those who contribute via GitHub for a DCO? GitHub already includes the name of the PR author in the description of the merge commit, so what's the point? Just a checkbox in the PR description looks enough.

The problem with a checkbox though is that it does not persist. It only is on GH and nowhere to be found on the actual git clone, but DCOs are bound to the whole commit history.

@imachug
Copy link
Contributor

imachug commented Jul 8, 2021

The problem with a checkbox though is that it does not persist. It only is on GH and nowhere to be found on the actual git clone, but DCOs are bound to the whole commit history.

Right, but my point is that merge commits by GitHub are also bound. You can have CI check that the checkbox is set and not accept the PR otherwise or something. It wouldn't be very troublesome for you but would be much easier for contributors.

@Semisol
Copy link
Contributor Author

Semisol commented Jul 8, 2021

The problem with a checkbox though is that it does not persist. It only is on GH and nowhere to be found on the actual git clone, but DCOs are bound to the whole commit history.

Right, but my point is that merge commits by GitHub are also bound. You can have CI check that the checkbox is set and not accept the PR otherwise or something. It wouldn't be very troublesome for you but would be much easier for contributors.

Also it is easy to sign-off commits in Git: -s in the commit command.
And also, I still don't feel comfortable using that approach.

@solfisher
Copy link
Contributor

solfisher commented Jul 8, 2021

The problem with a checkbox though is that it does not persist. It only is on GH and nowhere to be found on the actual git clone, but DCOs are bound to the whole commit history.

Right, but my point is that merge commits by GitHub are also bound. You can have CI check that the checkbox is set and not accept the PR otherwise or something. It wouldn't be very troublesome for you but would be much easier for contributors.

You can't prove with 100% certainty that a contributor checked that box, just from the git log. All we're doing is passing the responsibility to the contributor.

@Be-ing
Copy link
Contributor

Be-ing commented Jul 8, 2021

I don't understand the difference between "signing off" and the author of the commit. Can you elaborate on this?

@solfisher
Copy link
Contributor

solfisher commented Jul 9, 2021

I don't understand the difference between "signing off" and the author of the commit. Can you elaborate on this?

By "Signing-off" you agree to the DCO, and affirm that to your knowledge you have permission to contribute this code under the GPLv2.

This way, if it turns out someone's intellectual property was infringed upon, the responsibility isn't on the maintainers' hands, but on the original contributor's.

I suggest reading the entire DCO if you haven't already — it's short and simple to understand.

@adrianheine
Copy link

adrianheine commented Jul 9, 2021

I feel like this DCO tries to solve a rather academical legal problem while making contributing a bit more complicated.

@solfisher
Copy link
Contributor

solfisher commented Jul 9, 2021

I think it's a worthwhile tradeoff. Many projects already use a DCO, including Docker, Samba, Git, Open vSwitch, OpenDaylight, Ceph, OpenSearch and qemu.

If you use the Git CLI, it's a one-time configuration:

$ git config format.signoff true

That makes git commit automatically include a signoff.

CONTRIBUTING.md Show resolved Hide resolved
@imachug
Copy link
Contributor

imachug commented Jul 9, 2021

Also it is easy to sign-off commits in Git: -s in the commit command.

Not if the changes have already been committed: you'll either have to redo them or perform a signoff rebase. Also there's no checkbox for signoff in GitHub commit UI. I'm not saying all these problems are impossible to solve, but every nuanced requirement reduces the potential number of contributors and increases the rate of invalid PRs.

You can't prove with 100% certainty that a contributor checked that box, just from the git log. All we're doing is passing the responsibility to the contributor.

See, the point of a DCO is that the maintainers know for sure whom they got the commits from. From this point of view, using a bot under your control or simply checking if the checkbox is checked (pun not intended) with your eyes is as evidentiary (if not more so, because replacing [ ] with [x] manually is a less common action than slightly modifying a CLI command) as a footer in the commit description.

On the other hand, normal users cannot completely trust either of the ways if they don't trust you. You say that a merge committed via GitHub UI is not a proof that the checkbox was set. My response is that if someone doesn't trust the maintainers, Signed-Off-By is not a proof either, because absolutely everyone can fake commit authorship and commit something as if it was signed off by Linus Torvalds, including you. Secondly, maybe you don't know that but when you create a PR from me/repo to upstream/repo, GitHub immediately allows upstream/repo maintainers to push to me/repo, which would allow you to add the Signed-Off-By header.

If you were going to use Signed-Off-By as a proof in court, that won't work. Then why not make it easier for contributors while retaining the same level of security?

@solfisher
Copy link
Contributor

solfisher commented Jul 9, 2021

you'll either have to redo them or perform a signoff rebase.

That's negligible. git rebase --signoff HEAD~5 is a very simple command, and maintainers ask contributors to rebase their PRs all of the time.

Signed-Off-By is not a proof either, because absolutely everyone can fake commit authorship and commit something as if it was signed off by Linus Torvalds, including you.

That is only correct if you don't sign your commits with PGP, which I urge everyone to do.
Generally, though, yes. It's not any more "secure" than checking a box.

If you were going to use Signed-Off-By as a proof in court, that won't work. Then why not make it easier for contributors while retaining the same level of security?

Keep in mind GitHub is not the only way to contribute: people sending patches through sourcehut will still be required to sign a DCO. Only requiring DCO from one channel is unnecessary complexity which will inevitably lead to confusion.

When someone makes a PR and doesn't sign the DCO, we'll kindly ask them to sign it, and will provide the appropriate commands to do so. It's not very complicated. If it was, no one would be using it.

@imachug
Copy link
Contributor

imachug commented Jul 9, 2021

That's negligible. git rebase --signoff HEAD~5 is a very simple command, and maintainers ask contributors to rebase their PRs all of the time.

That is easy for you and for me, but may not be so for those who use graphical git clients or GitHub UI (neither is good but still).

That is only correct if you don't sign your commits with PGP, which I urge everyone to do.

An evil maintainer could just strip a PGP signature they don't like so that's not an argument.

Generally, though, yes. It's not any more "secure" than checking a box.

So why not allow both?

Keep in mind GitHub is not the only way to contribute: people sending patches through sourcehut will still be required to sign a DCO. Only requiring DCO from one channel is unnecessary complexity which will inevitably lead to confusion.

How is that complex? Requiring two actions from contributors (-s and a checkbox) is complex. Optimization makes things easier, not harder. You will have non-signed-off commits in the codebase either way, so what is the problem? I really don't see it, is that just maintainers' laziness or am I missing something?

When someone makes a PR and doesn't sign the DCO, we'll kindly ask them to sign it, and will provide the appropriate commands to do so. It's not very complicated. If it was, no one would be using it.

Tenacity was founded because people didn't want Audacity to do what everyone else does (read: privacy policy, CLA, etc.).

The list you provided contains:

  • Docker, OpenSearch and Ceph, that are maintained by for-profit companies,
  • Samba, that inherently had problems with code licensing because it's an implementation of proprietary SMB and you don't want to accidentally copy leaked Microsoft code,
  • I'd also add Linux here, that is maintained by Linus Torvalds, the inventor of DCO that most people use, who also could have some problems with UNIX proprietary code back then,
  • Git, Open vSwitch, OpenDaylight, that are maintained by Linux Foundation too and probably adopted DCO only because of Linus,
  • qemu, which looks like an exception to me.

Im my very humble opinion, Tenacity had not and will not face the same problems as Amazon, Red Hat and other companies do, it does not contain anything that potentionally can be a trade secret, and it is not maintained by Linus our saviour Torvalds.

You are not a corporation, and people switched from Audacity to Tenacity exactly because of this. You do not have to repeat what others do, and you have to keep in mind that not everything that others do is best practice. Seeing how you use 'but mum they did it too' argument just a few days after shaming Audacity team for that is astonishing at best.

@falkTX
Copy link
Contributor

falkTX commented Jul 9, 2021

There could very likely be other examples of projects using DCO, we should not look at the list as an indication of anything but that other projects are successfully using it.

DCO is to protect the maintainers and not the users, it seems quite good to me.
The push from DCO is not because some others are doing it, but because we want to.

It is complicated to manage Github and sourcehut in 2 different ways, if one requires signed-off DCO and the other doesn't.

The contributions from upstream audacity will obviously not have DCO on them, but IMO contributions made to this repository should.

@solfisher
Copy link
Contributor

solfisher commented Jul 9, 2021

Keep in mind I'm not part of the team: I'm talking for myself, my opinions and takes are my own.

If Semisol or any other dev provides a different response, this isn't them "taking anything back" like Muse did. Both of our standpoints are clear, let's see what others have to say.

@imachug
Copy link
Contributor

imachug commented Jul 9, 2021

I'm no longer sure if this discussion is still beneficial so if it's not feel free to hide this comment and I'll shut up.

It is complicated to manage Github and sourcehut in 2 different ways, if one requires signed-off DCO and the other doesn't.

  • If got a patch via email
    • Make sure the patch has Signed-Off-By field
  • If got a PR via GitHub
    • It's already OK

What's 'complicated' here? I really don't understand what exactly is the problem, you're developers, not, say, managers who have little experience in coding, or users or contributors who shouldn't even care about the differences (I believe SourceHut users know what signoff is already and mainly GitHub users won't even try to use SourceHut once so it's not a problem for them).

@Semisol
Copy link
Contributor Author

Semisol commented Jul 9, 2021

Also let me explain why I wanted to add this: The GitHub checkbox isn't that permanent enough in the Git log.
For the "faking" of commits, if needed we can enforce PGP signatures but that is another whole thing in itself up for another debate/RFC.

falkTX
falkTX previously requested changes Jul 9, 2021
Copy link
Contributor

@falkTX falkTX left a comment

please mention version 2 or any later version instead of version 2 only.

GPLv2+ is compatible with GPLv2, we gain nothing by restricting to GPLv2 only.

falkTX
falkTX previously approved these changes Jul 9, 2021
@Semisol Semisol requested a review from solfisher Jul 9, 2021
solfisher
solfisher previously approved these changes Jul 9, 2021
Co-authored-by: Filipe Coelho <falktx@falktx.com>
Co-authored-by: Sol Fisher Romanoff <sol@solfisher.com>
@Semisol Semisol dismissed stale reviews from solfisher and falkTX via 0c7d853 Jul 9, 2021
@Semisol
Copy link
Contributor Author

Semisol commented Jul 9, 2021

@Semisol Semisol requested review from solfisher and falkTX Jul 9, 2021
Copy link
Contributor

@solfisher solfisher left a comment

If no one has any more suggestions, this can be merged.

falkTX
falkTX approved these changes Jul 9, 2021
@Semisol Semisol merged commit 61e614f into master Jul 9, 2021
4 checks passed
@Semisol Semisol deleted the add-contributing-dco branch Jul 9, 2021
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.

None yet

8 participants