[Twisted-Python] overview: new review queue venue
![](https://secure.gravatar.com/avatar/e1554622707bedd9202884900430b838.jpg?s=120&d=mm&r=g)
Hooray! We're on github now. Next: there's the question of how to deal with pull requests? It occurs to me that what we really need from our code review system is mainly one thing: the review queue: a single place for reviewers to look to find things that need to be reviewed. This is important because proposed changes need to be responded to in a timely manner, so that the code in them doesn't get stale, and so that contributors don't get frustrated. We have limited resources for doing so, of course, so sometimes we fall short of this objective, but the point is we need to apply our limited resources to it. The operations on the queue are: Proposing a change should put it into the queue. Accepting a change should remove it from the queue. Reviewing a change should remove it from the queue. Responding to review feedback should re-add it to the queue. A reviewer should be able to examine just the things in the queue so they can quickly grab the next one, without seeing noise. Our current workflow maps this into Trac via the following: Proposing: add the "review" keyword. Accepting: remove the "review" keyword, merge. Reviewing: removing the "review" keyword, reassign Responding: add the "review" keyword again Viewing: https://twistedmatrix.com/trac/report/25 It is therefore tempting to map it into GitHub via labels and webhooks and bot workflows. However, I think a better mapping would be this: Proposing: Just open a pull request. Any open pull request should be treated as part of the queue. Accepting: A committer pushes the big green button; this Reviewing: This is the potentially slightly odd part. I believe a review that doesn't result in acceptance should close the PR. We need to be careful to always include some text that explains that closing a PR does not mean that the change is rejected, just that the submitter must re-submit. Initially this would just mean opening a new PR, but Mark Williams is working on a bot to re-open pull requests when a submitter posts a "please review" comment: https://github.com/markrwilliams/txghbot Responding: A submitter can open a new PR, or, once we start running txghbot, reopen their closed PR. Viewing: https://github.com/twisted/twisted/pulls?utf8=✓&q=is%3Apr+is%3Aopen+-status%3Afailed The one thing that this workflow is missing from trac is a convenient way for committers, having eyeballed a patch for any obvious malware, to send it to the buildbots. We could also potentially just replace our buildbot build farm with a combination of appveyor and travis-ci; this would remove FreeBSD from our list of supported platforms, as well as eliminating a diversity of kernel integrations. However, for the stuff that doesn't work in containers (mostly inotify) we could run one builder on non-container-based infrastructure, and for everything else (integrating with different system toolchains) we can test using Docker: https://docs.travis-ci.com/user/docker/. I am very much on the fence about this, since I don't want to move backwards in terms of our test coverage, but this would accelerate the contribution process so much that it's probably worth discussing. 10 years ago or so, we would routinely discover kernel bugs in our integration test harness and they would be different on different platforms. But today's platform realities are considerably less harsh, since there are a lot more entities in the ecosystem that have taken responsibility for testing that layer of the stack; I couldn't find anything since 2008 or so where we really saw a difference between Fedora and Ubuntu at the kernel level, for example. Thoughts? -glyph
![](https://secure.gravatar.com/avatar/c8f4d4d1a8fc53c13ed05c202e0257fe.jpg?s=120&d=mm&r=g)
Hi Glyph On Sun, May 22, 2016 at 12:12 AM, Glyph <glyph@twistedmatrix.com> wrote:
- Reviewing: This is the potentially slightly odd part. I believe a review that doesn't result in acceptance should *close* the PR.
This feels wrong to me. I find github pull requests very useful, in ways that it sounds like your suggestion would cut off - if I understand you right. Do you mean that the one person who decides to do the formal review would set themself as the "assignee" and, following their review, close the ticket if the change isn't up to standard? And in the meantime others (who are not the assignee) would be free to comment at will, with the pr staying open? If so, that's good, but I still don't like to think that all discussion around a pr then gets shut down on the say so of the single reviewer. One thing that pull requests encourage is discussion - sometimes it's a way to ask for input on how to proceed, sometimes it's just people chipping in a little bit with a code optimization, sometimes people saying "if you do this then this other thing will happen" etc. They help people learn, to easily make small contributions (that can lead to larger ones), to see what's going on, to judge the health of a project, etc. I like how dynamic and lightweight that process can be in a Github pull request. It feels to me that closing pull requests is a step in the other direction - back towards something that feels more monolithic and more old fashioned. As usual, I'm sure that there's absolutely nothing I can say on any tech subject that you don't know already :-) But those are my thoughts... Terry
![](https://secure.gravatar.com/avatar/e1554622707bedd9202884900430b838.jpg?s=120&d=mm&r=g)
The "assignee" field isn't all that useful; really, it should be set to the submitter, but github has weird rules about who can be assigned (last I checked, only contributors).
And in the meantime others (who are not the assignee) would be free to comment at will, with the pr staying open?
Drive-by comments on a PR are sometimes helpful, but should be used sparingly. Mostly, discussion should happen on issues, not PRs. A PR is a suggested resolution to a problem, and we might reject one solution, but an issue should describe the problem itself.
If so, that's good, but I still don't like to think that all discussion around a pr then gets shut down on the say so of the single reviewer.
The PR gets closed, not deleted. People can still comment if they like.
One thing that pull requests encourage is discussion - sometimes it's a way to ask for input on how to proceed, sometimes it's just people chipping in a little bit with a code optimization, sometimes people saying "if you do this then this other thing will happen" etc. They help people learn, to easily make small contributions (that can lead to larger ones), to see what's going on, to judge the health of a project, etc. I like how dynamic and lightweight that process can be in a Github pull request. It feels to me that closing pull requests is a step in the other direction - back towards something that feels more monolithic and more old fashioned.
You see "dynamic and lightweight", I see "unfocused and noisy" :). One of the things I still don't like about github is the tendency for projects to build up huge piles of open PRs, which nobody wants to reject because they're maybe possibly useful, but they won't accept because they don't meet quality standards. Contributors don't get clear feedback about whether they're expected to do more or whether the project will take it over, and people frequently get mad about their stuff not being merged. I think by closing PRs that we aren't going to merge as-is we can send a much clearer signal about what the project is taking on versus what it expects external contributors to do more work on.
As usual, I'm sure that there's absolutely nothing I can say on any tech subject that you don't know already :-) But those are my thoughts...
While I disagree, I also think that this is a very common perception, and another one of the functions of the github bot could perform would be a form comment after the PR is closed, to always submit a form comment to explain what closing the PR means and how to reopen it. -glyph
![](https://secure.gravatar.com/avatar/61b1aad7d8e6c28832ecdffe8036a5f0.jpg?s=120&d=mm&r=g)
To qualify my comments, I've yet to contribute to Twisted because I don't have a good enough grasp of its internals, but I have contributed to a variety of Git-based OSS projects. I definitely get uneasy with the general idea that we're trying to "replicate workflow A from Trac in tangentially related Git PR feature". We're in Git. We're hoping to solicit PRs from Git users. Git users will be used to the way PRs are used in other OSS Git projects. Glyph has some valid criticisms of the situation in some projects, but it should still be the starting point. For example, closing a PR strikes me as a bad idea -- for lack of a better word, it feels "hostile" to me and certainly unwelcoming. In several of the projects I've seen, Git tags fill these roles. Piwik has a "needs review" tag -- the short list for reviewers. Looks like it's a manual add, but maybe it could be automated. Once reviewed, Piwik has tags like "Tests & QA". ZendFramework has a generic "awaiting author update". To address Glyph's concerns about lingering PRs, perhaps the combination of: - A policy like "a reviewer must accept, close, or tag with one of the next step tags" - A short list of common next steps like "code quality", "needs tests", "second opinion", "not review ready"... plus a generic "other author action" - Auto-close tickets except those with "needs review" or "second opinion" (say) 30 days after the last action. Drive-by comments on a PR are sometimes helpful, but should be used
While an Issue is a good place for discussion about a problem, it lacks the reference code often included in a PR. You can't ask "how about this approach" without showing the approach. As an added bonus, most systems run travis on PRs so you get a sense of "this approach is thorough" or "this idea still breaks something". Clayton Daley On Sat, May 21, 2016 at 8:29 PM, Glyph <glyph@twistedmatrix.com> wrote:
![](https://secure.gravatar.com/avatar/e1554622707bedd9202884900430b838.jpg?s=120&d=mm&r=g)
On May 21, 2016, at 7:25 PM, Clayton Daley <clayton.daley@gmail.com> wrote:
To qualify my comments, I've yet to contribute to Twisted because I don't have a good enough grasp of its internals, but I have contributed to a variety of Git-based OSS projects. I definitely get uneasy with the general idea that we're trying to "replicate workflow A from Trac in tangentially related Git PR feature".
The workflow is not "from Trac". The instantiation in Trac is not optimal either, which is why I described the abstract desired workflow separately from our existing instantiation.
We're in Git. We're hoping to solicit PRs from Git users. Git users will be used to the way PRs are used in other OSS Git projects.
I think you mean "GitHub". Git PRs don't work at all like GitHub PRs. :).
Glyph has some valid criticisms of the situation in some projects, but it should still be the starting point. For example, closing a PR strikes me as a bad idea -- for lack of a better word, it feels "hostile" to me and certainly unwelcoming.
The thing is, if you perceive it as "hostile" that a project closes a PR - i.e. "says that they're not going to do more work on it" - that is a cultural problem; it suggests a certain implicit level of passive aggression in opening a PR which I don't want to assume. It's sort of like having a culture where you can just send anybody an email asking them to do whatever and it would be "hostile" for them to refuse. In such a culture people don't say "yes", but they do start to ignore messages Closing the PR is a more accurate reflection of reality - the project (twisted) is not going to do anything about the PR in its current state, so it shouldn't be left open. It also clearly demarcates the completion of a review. People feel very differently about workflow, of course, but I've definitely heard from other OSS maintainers that the average workflow of volunteer projects often devolves into a huge backlog of un-reviewed stuff, which obscures the new stuff, and if you want something to actually get reviewed and move along you need to know the maintainers of the project and ask them personally. I'd much rather our new contributors get a little confused about the culturally unusual step of closing a PR than to have their work be accidentally but systematically discriminated against in favor of people who know how to bug the right people in IRC or email.
In several of the projects I've seen, Git tags fill these roles. Piwik has a "needs review" tag -- the short list for reviewers. Looks like it's a manual add, but maybe it could be automated. Once reviewed, Piwik has tags like "Tests & QA". ZendFramework has a generic "awaiting author update".
This was my original idea. The problem with GitHub labels ("Git tags" are something completely different) is that they can't be applied by external contributors. You need write access to the repository to be able to manipulate them. It's very important to our workflow that external contributors be able to re-submit their PRs. We could have a bot for that (again, this was the original plan). But it seems like using the open / closed state to reflect the we will do some work on this / we won't do any more work on this is actually closer to the "native" state of github.
Drive-by comments on a PR are sometimes helpful, but should be used sparingly. Mostly, discussion should happen on issues, not PRs. A PR is a suggested resolution to a problem, and we might reject one solution, but an issue should describe the problem itself.
While an Issue is a good place for discussion about a problem, it lacks the reference code often included in a PR. You can't ask "how about this approach" without showing the approach. As an added bonus, most systems run travis on PRs so you get a sense of "this approach is thorough" or "this idea still breaks something".
This is exactly why issues and PRs should be separated. If you only have one artifact - the PR - to represent both the issue and the potential solution, then you can't get rid of the potential solution (reject the PR) without also getting rid of the description of the problem. Github already automatically shows anywhere that your PR or issue is mentioned, so all you need to do to say "how about this approach" is to put the words "fixes #NNN" in your PR description. As a bonus, that will make it automatically close the issue when the PR is merged. -glyph
![](https://secure.gravatar.com/avatar/61b1aad7d8e6c28832ecdffe8036a5f0.jpg?s=120&d=mm&r=g)
... I'd much rather our new contributors get a little confused about the
It's your party, but I think this vastly undervalues first impressions in OSS engagement. From all the projects I've contributed to on Github -- yes technically Github not Git though Bitbucket has equivalent features (and unlimited private repos for free!) -- closing means "this is off the table". It's a nice clear signal that there's nothing the contributor can do to fix it -- e.g. I had an LSP-valid change that passed all tests run afoul of some obscure PHP method signature limitation when mixed with other packages. I'll wager only the active contributors will remember the proposed subtlety a month from now. As a big break from Github norms, It's going to hit everyone else at the worst time... a first PR submission. That's the moment when a contributor is trying to get a sense of the culture of the team that manages the project. They're at their most vulnerable and violating *their* norms significantly increases the odds that they'll leave the fix in a private fork and disengage. Twisted operates at a different level so this may not be a bad thing. You may benefit from actively discouraging dabblers -- especially given resource constraints. But there aren't going to be a lot of "first PR" folks on this list to point out the effect of this break from norms. Clayton Daley On Sun, May 22, 2016 at 12:12 AM, Glyph <glyph@twistedmatrix.com> wrote:
![](https://secure.gravatar.com/avatar/e1554622707bedd9202884900430b838.jpg?s=120&d=mm&r=g)
This is roughly the same story I've been getting for the last decade and a half: "You can't: require test coverage, require documentation, require coding standard compliance, require people to file a ticket before sending a patch to the mailing list, that's a terrible burden to put on new contributors!" Somehow we've survived much longer than most projects, and while some would say "in spite of" these restrictions, I think it's "because of". So, we are not trying to "discourage dabblers"; we would like new contributors who want to contribute only a little bit. So while I don't want to throw up arbitrary barriers, if you aren't willing to invest the effort to even read a single comment on your PR explaining why it was closed and how to reopen it, I cannot imagine that chances are good that you'll read the rest of the comments explaining what changes need to be made and make them effectively. Further, people who contribute trivial changes that can be immediately merged, like documentation typos, won't need to deal with this, because they won't see the intermediary "needs feedback" closed state; they'll just get their PRs accepted immediately. -glyph
![](https://secure.gravatar.com/avatar/61b1aad7d8e6c28832ecdffe8036a5f0.jpg?s=120&d=mm&r=g)
this caliber and the 4th doesn't make sense in a Github/Bitbucket/Gitlab world (and I fortunately avoided its predecessors). In fact, I'd be a little worried if the first there weren't required in a project. ResourceSpace is the only one I can think of that doesn't require them and I wouldn't touch it with a 10 foot pole if there were acceptable (free) alternatives. So I don't see the link between your success enforcing three norms of good OSS projects and your desire to break a 4th. === The idea that PRs are a substantial burden has caused me pause. Normally, a PR is a chance to give back to a project rather than freeload. In most projects, new features are part of a virtuous cycle: a new feature tends to benefit a large fraction of the user base and expand the user base... which draws more contributors. Especially given the recent deprecations (old, unmaintained protocols), it seems that Twisted doesn't work like this. If another user adds another protocol, it doesn't make the system better for most users. In fact, it makes sense that it actually increases the maintenance burden. I tried to look for myself, but was reminded that I don't know the internals well enough... are patches on non-central protocols a big part of the backlog? Or is the backlog mostly core features (like reactors or IO infrastructure) that most projects depend upon? Clayton Daley On Sun, May 22, 2016 at 4:22 PM, Glyph <glyph@twistedmatrix.com> wrote:
![](https://secure.gravatar.com/avatar/ebf132362b622423ed5baca2988911b8.jpg?s=120&d=mm&r=g)
Twisted has been enforcing these rules since before they were considered part of the norm and I believe that Glyph was referencing is that back then people said that Twisted was going to fail or w/e because of requiring those things. Sent from my iPhone
On May 22, 2016, at 9:12 PM, Clayton Daley <clayton.daley@gmail.com> wrote:
The first three of these are *already* norms in all of the OSS projects of this caliber
![](https://secure.gravatar.com/avatar/e1554622707bedd9202884900430b838.jpg?s=120&d=mm&r=g)
To make a long story short: we pretty much invented these norms, and were doing them a long time before other projects got on the bandwagon, over many, very loud objections. That said, I don't want to overstate the case. Test coverage was a huge deal. Doc coverage was a huge deal. This idea with PRs is a very small thing, maybe not the best idea, and not something we're unanimous about or dead set on. But the "all your friends are jumping off a bridge" argument against doing it just isn't very compelling.
The idea that PRs are a substantial burden has caused me pause. Normally, a PR is a chance to give back to a project rather than freeload. In most projects, new features are part of a virtuous cycle: a new feature tends to benefit a large fraction of the user base and expand the user base... which draws more contributors.
The issue is not that PRs are, inherently, a "burden". But, before I try to rephrase the issue with too many simultaneously open PRs, I should probably describe the tremendous asymmetry between core maintainers and external contributors. This asymmetry is general to all open source projects, not particularly specific to Twisted at all. If you listen to Nadia Eghbal's various comments about funding open source (motivated by <https://medium.com/@nayafia/how-i-stumbled-upon-the-internet-s-biggest-blind...>) you hear this complaint repeated by lots of project maintainers. External contributors typically have an initial interest in landing one "contribution", which is something that benefits them (whether personally or their company) specifically, rather than something that benefits the community at large. Transforming that initial interest into a long-term commitment to the project is very challenging for maintainers. Even if an improvement is specific to the contributor who is making it, of course, there will be others like them who want similar improvements, so it's never a purely selfish or purely altruistic motivation for making the contribution; it's a mix. And general benefits are good for the core maintainers, of course, since it makes their long-term maintenance job easier, and it does attract more people to the project. But that benefit has to be balanced with a cost. The cost is that it takes time and energy to code-review contributions. This is the source of a lot of friction between core maintainers and external contributors: contributors feel that they are generously giving their time and energy to the project, and it's rude of the project to make them jump through any hoops to get it accepted, whether that's test coverage, documentation, pre-commit code review, coding standard compliance, or, in this case, learning a slightly weird workflow. External contributors can affect this balance a lot, by making sure that their contributions are already as close as possible to acceptable. But even if they do, the ratio of external contributors to core maintainers is almost always far greater than 1; the only way that external contributors can _really_ affect this balance is to become core maintainers :). And this asymmetry brings us to why it's important to keep the 'Review Queue' short. To rephrase what I said earlier in this thread, if there are so many open PRs that reviewers don't know which ones to look at first, then the sorting algorithm will be "whichever ones my friends want me to look at first", which means we need to maintain a clear division between things-we-are-looking-at and things-we-are-not-looking-at, to maximize the effectiveness of the limiting factor of code reviewer time for impact on the latency of time it takes to get feedback on a potential change. Sorry that this is a bit long-winded but I really just object to the premise that the point is that I don't care about contributors. I care about contributors a great deal; this is why I want a carefully designed process that doesn't shut them out or waste their time.
Especially given the recent deprecations (old, unmaintained protocols), it seems that Twisted doesn't work like this. If another user adds another protocol, it doesn't make the system better for most users. In fact, it makes sense that it actually increases the maintenance burden.
This is true of all features for all projects, though. More code == more maintenance.
I tried to look for myself, but was reminded that I don't know the internals well enough... are patches on non-central protocols a big part of the backlog? Or is the backlog mostly core features (like reactors or IO infrastructure) that most projects depend upon?
I don't think we have any good metrics on the backlog, unfortunately. But certainly most of the maintenance work has been maintenance, like porting existing code to python 3, improving test coverage, and implementing improvements to TLS. -glyph
![](https://secure.gravatar.com/avatar/e93e18f71a5821a54c233690506bdbf7.jpg?s=120&d=mm&r=g)
FWIW I thought of another "open source" community which uses a similar idea to closing pull requests if they won't be accepted in their current form: Stack Exchange. I mostly frequent Physics Stack Exchange <http://physics.stackexchange.com/>. At any time some number of the questions on the front page are either "closed" or "on hold" for not living up to site standards in some way. We've asked almost exactly the same question as is being asked in this thread: does closing unfit questions discourage new users so much as to outweigh the benefits to site quality? I won't pretend to have an authoritative answer to this question, but I will mention the steps we've taken to help make sure new users aren't put off. 1. We have a help center (like all other Stack Exchange sites) explaining the rules and system. It's not great though, and its shortcomings show up all the time as new users do get confused about the rules. I think if anything this is just an indication that contributing guidelines need to be really clear about what the various signals from the maintainers mean. 2. It's really helpful to leave a comment on unfit questions explaining what the problem is and *how the poster can improve it*. Stack Exchange has canned close reasons, but they're rarely sufficient in my opinion. I leave comments like "Welcome to Physics Stack Exchange! I think there's an interesting question here but we have some rules...Please see the [help center](link)...". Starting the comment with something positive and directing the user to official guidelines seems rather helpful, although I can't provide metrics. 3. Our chat room is extremely easy to access and people there are friendly. IRC is great but I just clicked around the Twisted website for a minute and couldn't find any indication that the twisted IRC channel exists.
![](https://secure.gravatar.com/avatar/61b1aad7d8e6c28832ecdffe8036a5f0.jpg?s=120&d=mm&r=g)
... +1 for some guidelines from a similar policy Note that we're actually discussing one step more extreme. Closing poor questions is common across SE so the issue only affects new OPs. We're talking about a break from community norms... more like Physics deciding to close questions after the first answer is added. We should also consider any unintended side-effects due to Github's default searches behavior ("open"): [image: Inline image 1] Closing PRs will make them less likely to be found by searchers. If every PR has an issue (common for bug fixes, less common for new features), this is less of a problem -- is this something the bot would need to verify/fix? Clayton Daley On Mon, May 23, 2016 at 1:08 PM, Daniel Sank <sank.daniel@gmail.com> wrote:
![](https://secure.gravatar.com/avatar/e1554622707bedd9202884900430b838.jpg?s=120&d=mm&r=g)
On May 23, 2016, at 11:56 AM, Clayton Daley <clayton.daley@gmail.com> wrote:
Closing PRs will make them less likely to be found by searchers. If every PR has an issue (common for bug fixes, less common for new features), this is less of a problem -- is this something the bot would need to verify/fix?
The bot is just for the contributors to be able to re-open PRs to respond to review feedback. When a PR is closed, the reviewers themselves will ask the authors to open an issue separately if that is appropriate. -glyph
![](https://secure.gravatar.com/avatar/a93db92c60e9d5435d204407264457f2.jpg?s=120&d=mm&r=g)
Personally, I find closing PRs that aren't going to be merged "soon" mostly-beneficial. Even if it *might* be perceived as "hostile" by some contributers, a simple explanation should suffice. (And, if simply closing a PR with a nice note explaining, "please re-open when X is fixed/changed" scares away a potential contributer I have my doubts as to whether they would fix X if you *didn't* close it...) There's nothing worse than trolling through open PRs only to find that the last comment is "fix up X, Y, and Z and we'll merge" because then you have to (re-)figure out if X, Y and Z have been done etc. On the flip side, it's nice to know if your PR has problems or not. The other plus of closing is that it's way more obvious when the PR is once again considered ready for merging (even without 'completely baked' workflows like Twisted's) and keeps the "open PRs" list (hopefully!) shorter. -- meejah
![](https://secure.gravatar.com/avatar/469df05f5dfd5b75fb3cb3a0868d36bf.jpg?s=120&d=mm&r=g)
I submitted this PR, which is now closed: https://github.com/twisted/twisted/pull/62 I don't want to re-open that PR, but I am using that as an example As an example, if I wanted to re-open that, how would I go about doing it? I am not an administrator of the Twisted GitHub project, so on that web link, there is no option for me to re-open the PR. Are you suggesting that I would need to -> create a new branch in my repo with new commits -> create a new pull request ? -- Craig On Sun, May 22, 2016 at 9:39 PM, meejah <meejah@meejah.ca> wrote:
![](https://secure.gravatar.com/avatar/3d37232726396a1d3c7412dd915095ea.jpg?s=120&d=mm&r=g)
Mark has been working on a bot which would reopen it with a comment: https://github.com/markrwilliams/txghbot - Amber
![](https://secure.gravatar.com/avatar/e1554622707bedd9202884900430b838.jpg?s=120&d=mm&r=g)
On May 22, 2016, at 10:51 PM, Tristan Seligmann <mithrandi@mithrandi.net> wrote:
Note that even without the bot, I believe you can just create a new PR for the same branch, so it's not *too* bad, but definitely a little clunky.
Many of my comments have had to do why we want this kind of process generally, rather than why specifically closing PRs is the way I'd prefer to go; this is one benefit, which is that even if our bot infrastructure breaks down, there's a clear workaround available to contributors. If resubmitting requires a label change on an existing *open* PR there's not really any way to do that if the bot is temporarily offline. -glyph
![](https://secure.gravatar.com/avatar/e1554622707bedd9202884900430b838.jpg?s=120&d=mm&r=g)
While FreeBSD support is important, there are no CI-as-a-service platforms that support it, that I'm aware of. Right now, we need to manually vet each change before sending it to buildbots, because they are shared mutable environments that we can't afford to have running untrusted code automatically. If we could switch to Travis and Appveyor, then we could let them worry about malicious code, which would allow contributors to get instant feedback, rather than waiting for reviewers to manually run the builders. Travis does support OS X, which means that some level of BSD coverage would still be maintained. And of course if someone could find an equivalent service that supports FreeBSD we could add it to the list of pull request statuses :). -glyph
![](https://secure.gravatar.com/avatar/c194a4d2f2f8269aa052942e87985198.jpg?s=120&d=mm&r=g)
On 22 May 2016 at 02:04, Glyph <glyph@twistedmatrix.com> wrote:
I am not sure that Travis supports Python on OSX.... and it might take some time until there is support for Python on Windows and I am not sure if Travis-ci.org will support this free of charge. I think that we can have both... and in the first instance automatically trigger Travis-CI builds and manually trigger buildbot builds. ---------- GitLab is an option, as I think that they allow you to bring your own build slaves.... but I think that the current buildbots are fine. -- Adi Roiban
![](https://secure.gravatar.com/avatar/e1554622707bedd9202884900430b838.jpg?s=120&d=mm&r=g)
On May 21, 2016, at 11:04 PM, Adi Roiban <adi@roiban.ro> wrote:
I am not sure that Travis supports Python on OSX....
It does. We test on OS X quite extensively on <https://github.com/rackerlabs/mimic>, including a py2app app bundle.
and it might take some time until there is support for Python on Windows and I am not sure if Travis-ci.org <http://travis-ci.org/> will support this free of charge.
Travis-CI doesn't need to support it; you can use both Travis (for OS X / Linux) and Appveyor (for Windows) separately. You can check it out here: http://www.appveyor.com I know some projects are using this quite successfully.
I think that we can have both... and in the first instance automatically trigger Travis-CI builds and manually trigger buildbot builds.
A lot of projects do follow this workflow, and maybe it will be fine for us. The real question is; is FreeBSD support really worth it for the cost to contributors, since that's the only platform we currently support but can't test?
GitLab is an option, as I think that they allow you to bring your own build slaves.... but I think that the current buildbots are fine.
I don't understand.
![](https://secure.gravatar.com/avatar/3d37232726396a1d3c7412dd915095ea.jpg?s=120&d=mm&r=g)
On 22 May 2016, at 14:23, Glyph <glyph@twistedmatrix.com> wrote:
A lot of projects do follow this workflow, and maybe it will be fine for us. The real question is; is FreeBSD support really worth it for the cost to contributors, since that's the only platform we currently support but can't test?
I'm guessing that we have more FreeBSD users than Windows users ;) - Amber
![](https://secure.gravatar.com/avatar/e1554622707bedd9202884900430b838.jpg?s=120&d=mm&r=g)
I realize it can feel like that sometimes, but Google Analytics suggests the large majority of our visitors (45%) are on Windows. By contrast, 0.05% are on FreeBSD. Granted, that's a very high percentage of FreeBSD clients for the Internet at large, but nevertheless, I think your perspective may be slightly statistically skewed. -glyph
![](https://secure.gravatar.com/avatar/e93e18f71a5821a54c233690506bdbf7.jpg?s=120&d=mm&r=g)
I can speak for ~20 scientific research groups who use Twisted via LabRAD's python API <https://github.com/labrad/pylabrad>. A lot of us use or deploy to Windows at least some times. So that's around 200 people you've never heard of who use Twisted on Windows :)
![](https://secure.gravatar.com/avatar/e1554622707bedd9202884900430b838.jpg?s=120&d=mm&r=g)
On May 21, 2016, at 11:42 PM, Daniel Sank <sank.daniel@gmail.com> wrote:
I can speak for ~20 scientific research groups who use Twisted via LabRAD's python API <https://github.com/labrad/pylabrad>. A lot of us use or deploy to Windows at least some times. So that's around 200 people you've never heard of who use Twisted on Windows :)
Wow, you are just knocking it out of the park with useful feedback in this thread :-). Any chance I could cajole you into submitting a success story to success@twistedmatrix.com? -glyph
![](https://secure.gravatar.com/avatar/469df05f5dfd5b75fb3cb3a0868d36bf.jpg?s=120&d=mm&r=g)
On Sat, May 21, 2016 at 11:37 PM, Glyph <glyph@twistedmatrix.com> wrote:
Isn't Google Analytics just telling you what type of OS + web browser is being used to access the twistedmatrix.com web site? That isn't the same as telling you who actually uses Twisted in a project or a piece of code. Unfortunately, FreeBSD isn't well represented in the third party CI systems out there such as Travis. It would be nice if it was, but it isn't. That's a judgment call that the Twisted project needs to make whether to support its own buildbots or not, in order to support configurations not supported by third party CI systems. -- Craig
![](https://secure.gravatar.com/avatar/e1554622707bedd9202884900430b838.jpg?s=120&d=mm&r=g)
Sure, it's not perfect. But even assuming only 1% of Windows desktop users are actually running any of their Twisted code on Windows, and 100% of FreeBSD users are, that's still roughly 9x as many Windows users as FreeBSD. I think that's probably a pretty conservative estimate.
One of the other interesting questions here, beyond "do we want to support it or not" (I think we do want to continue supporting it, in the sense that we want to fix bugs that affect it) is how often FreeBSD breaks; if 99.99% of the time, a change that works on Windows, OS X, and Linux works on FreeBSD, maybe we can still have some FreeBSD CI, but not make it a gating part of the review process. We can run it periodically, before each release, and in the exceedingly unlikely case that FreeBSD broke, we can roll back the change after the fact. -glyph
![](https://secure.gravatar.com/avatar/e1554622707bedd9202884900430b838.jpg?s=120&d=mm&r=g)
The existence of Netflix & WhatsApp doesn't indicate no correlation, just not a perfect correlation :). Although I suppose I'm being unfair there in that some of the Windows users represent actual FreeBSD users as well. Nevertheless, I think Windows consumers are less frequently acculturated to speaking up in open-source land, so if anything we are underestimating their prevalence. Unrelatedly, the existence of such high-profile FreeBSD users is encouraging :). Perhaps this is more important to keep in the critical path than I was assuming. -glyph
![](https://secure.gravatar.com/avatar/174e7b0ff60963f821d0b9a4f1a3ef52.jpg?s=120&d=mm&r=g)
Ah finally a fine bike shedding thread that gets everyone involved. ;)
Right now, we need to manually vet each change before sending it to buildbots, because they are shared mutable environments that we can't afford to have running untrusted code automatically. If we could switch to Travis and Appveyor, then we could let them worry about malicious code, which would allow contributors to get instant feedback, rather than waiting for reviewers to manually run the builders.
I have two points to add: 1. Appveyor is terribly slow and sometimes a bit flaky. I use it for argon2_cffi’s wheels and it drives me bonkers. It should never become an essential part of anything. As a first line of defense it’s fine of course. 2. PyCA has a workflow for Jenkins & GitHub by telling a bot to vet changes. You can see it here in action: https://github.com/pyca/cryptography/pull/2914#issuecomment-220592167 AFAIK that’s been mostly Paul’s work. Aren’t you kind of his boss or something *hint hint*? ;)
![](https://secure.gravatar.com/avatar/e1554622707bedd9202884900430b838.jpg?s=120&d=mm&r=g)
This is a very useful data point. I do not have any concrete experience with it and I was kind of wondering about this.
2. PyCA has a workflow for Jenkins & GitHub by telling a bot to vet changes. You can see it here in action: https://github.com/pyca/cryptography/pull/2914#issuecomment-220592167 AFAIK that’s been mostly Paul’s work. Aren’t you kind of his boss or something *hint hint*? ;)
Thanks for the promotion; I'll be sure to let him know on Monday. However, it's because I know Paul and I know what a complete nightmare it is to set up and maintain infrastructure like that that I was hoping to cheat and get away with it. But what I'm hearing from you in this thread is pretty compelling to me that we are going to need to follow a mostly Cryptography-like workflow after all, asking a bot to run some buildbots. -glyph
![](https://secure.gravatar.com/avatar/a93db92c60e9d5435d204407264457f2.jpg?s=120&d=mm&r=g)
Glyph <glyph@twistedmatrix.com> writes:
This is a very useful data point. I do not have any concrete experience with it and I was kind of wondering about this.
FWIW, Tahoe-LAFS *just* started using AppVeyor too, and I also find it horrifically slow. That said, the Tahoe tests run pretty slowly on a VirtualBox windows VM as well, but not nearly as slowly as AppVeyor. Or, at least that's my impression so far. p.s. If anyone is interested in running a Windows buildbot slave for Tahoe I'm very sure it would be appreciated -- please speak up on the tahoe mailing list :) (my understanding of the setup is that the buildbot slaves don't run random PR code, only post-merged-to-master code). -- meejah
![](https://secure.gravatar.com/avatar/e1554622707bedd9202884900430b838.jpg?s=120&d=mm&r=g)
On May 22, 2016, at 12:15 AM, Hynek Schlawack <hs@ox.cx> wrote:
Ah finally a fine bike shedding thread that gets everyone involved. ;)
OKAY NOW THAT I'VE GOT YOU ALL HERE LET'S TALK ABOUT https://twistedmatrix.com/trac/ticket/288 *slams a metal grating shut over the only exit from the mailing list* -glyph
![](https://secure.gravatar.com/avatar/469df05f5dfd5b75fb3cb3a0868d36bf.jpg?s=120&d=mm&r=g)
On Sat, May 21, 2016 at 6:04 PM, Glyph <glyph@twistedmatrix.com> wrote:
This is quite useful actually. We would need a tool to do this. For example, if I want to build this pr: https://github.com/twisted/twisted/pull/63 Then the tool could poke the buildbots to do something like: git clone https://github.com/twisted/twisted testspace cd testspace git fetch origin pull/62/head:pr/62 git checkout pr/62 [run the tests] Are there enough scripts in the buildbot infrastructure which could be extended to do this? -- Craig
![](https://secure.gravatar.com/avatar/e1554622707bedd9202884900430b838.jpg?s=120&d=mm&r=g)
The only new line would be fetching the test ref. Everything else on the buildbots basically works that way already, just checking out branches. (Please nobody try to do the clever thing where you configure buildbot to automatically pull all PRs, that would effectively negate any security protections...) I've been assuming that in the worst-case scenario, we'd do what Cryptography does and have a bot that polls for special comments and then triggers buildbot in exactly this way. Perhaps I should have made that assumption explicit, I thought it was ticketed somewhere in Braid already. -glyph
![](https://secure.gravatar.com/avatar/b973c2a161019ba60c4f30ad598ced4d.jpg?s=120&d=mm&r=g)
On May 22, 2016 9:36:28 AM GMT+02:00, Glyph <glyph@twistedmatrix.com> wrote:
The Jenkins plugin for GitHub PR triggers has this feature, too. However, it also has a feature to whitelist users and GitHub teams so that PRs/commits can trigger automatically for them. Maybe that's a thing for us, too? -- ralphm
![](https://secure.gravatar.com/avatar/869963bdf99b541c9f0bbfb04b0320f1.jpg?s=120&d=mm&r=g)
On Sun, 22 May 2016 at 10:12 Ralph Meijer <ralphm@ik.nu> wrote:
I don't think we need a whitelist, we can just automatically build branches that are pushed to the twisted/twisted repository. If you can push a branch there, you can also push a change directly to trunk, so you can already execute arbitrary code on the buildbots.
![](https://secure.gravatar.com/avatar/469df05f5dfd5b75fb3cb3a0868d36bf.jpg?s=120&d=mm&r=g)
On Sat, May 21, 2016 at 3:12 PM, Glyph <glyph@twistedmatrix.com> wrote:
Hooray! We're on github now. Next: there's the question of how to deal with pull requests?
A few people, including myself modified the text with Git instructions: https://twistedmatrix.com/trac/wiki/TwistedDevelopment#SubmittingaPatch The basic idea is, "for now, if you submit a GitHub PR, you must file a Trac ticket. You must reference the PR in the Trac ticket, and you must reference the Trac ticket in the PR". If you want to change that workflow somehow, go ahead, but for starters that should be good enough to get going. These two pull requests followed that: https://github.com/twisted/twisted/pull/62 https://github.com/twisted/twisted/pull/63
I don't agree with this. If a PR is reviewed, and the result of the review is "NO WAY", then I am OK with the PR being closed. However, if a the result of the review is "needs more work before being accepted", then it should be possible for the submitter to add more commits to that PR, and even "git rebase" and squash commits in that PR. That's the process that I followed when I submitted https://github.com/twisted/twisted/pull/62 By the way, PR 62 is the first pull request that has been successfully submitted to Twisted, and incorporated into the code: https://github.com/twisted/twisted/commit/95c49d74136eef420fabdeea85f650da2b... It is a rather trivial documentation fix, but I will still take credit as the first Pull Request successful submitter! :) -- Craig
![](https://secure.gravatar.com/avatar/e93e18f71a5821a54c233690506bdbf7.jpg?s=120&d=mm&r=g)
Dear all, While I'm just a lurker, having used Twisted and Github for some time in a moderately sized team I would like to offer a couple comments:
This is a real problem for my team, and I think it happens because we don't respect the value of issues enough. We have two kinds of branches 1) Those which associate to a specific issue. If issue #123 is about fixing the quizzwopper, you can make a branch called 123-fix-quizzwopper. This is a Good Thing. 2) Those which associate to a person. We name these things like u/danielsank/fix-quizzwopper. The point of that originally was to make things more "lightweight" and "dynamic" by allowing folks to make a branch and write some code asap. While this does often work, it also means that we have a mountain of near unidentifiable branches with no obvious place to look for that branch's motivation/history/status/whatever. Type 1 branches have the extra advantage that they're more likely to attract useful input from people at the right time, i.e. before the code is written. It's very frustrating to have a PR representing many hours of someone's work come in only to realize that you want to respond with "Oh for the love of all that is good in this world please don't do that". Discussion in an issue helps avoid that.
Again, agreed.
One thing that pull requests encourage is discussion
It's better for that to happen in issues. I have written code, submitted it as a PR, realized (via feedback and otherwise) that the code is total crap, deleted the branch and closed the PR, and then gone back to the issue to re-evaluate my life choices. This is really useful.
![](https://secure.gravatar.com/avatar/e1554622707bedd9202884900430b838.jpg?s=120&d=mm&r=g)
Sorry, I guess I wasn't clear. I know that PRs are presently a potential alternative to a diff, and that we are still using Trac for ticketing. I want to make it possible to avoid using Trac for ticketing; perhaps switching to github issues entirely. Right now, PRs are still ignored; the thing reviewers are paying attention to is the review queue in Trac, and that is sub-optimal, since it requires new contributors to do something unfamiliar.
Reviewing: This is the potentially slightly odd part. I believe a review that doesn't result in acceptance should close the PR. We need to be careful to always include some text that explains that closing a PR does not mean that the change is rejected, just that the submitter must re-submit. Initially this would just mean opening a new PR, but Mark Williams is working on a bot to re-open pull requests when a submitter posts a "please review" comment: https://github.com/markrwilliams/txghbot <https://github.com/markrwilliams/txghbot>
I don't agree with this. If a PR is reviewed, and the result of the review is "NO WAY", then I am OK with the PR being closed.
I understand that this is your feeling, but do you have any reasoning as to why you believe that this should be the case? "I don't feel like it" isn't selling me.
However, if a the result of the review is "needs more work before being accepted", then it should be possible for the submitter to add more commits to that PR,
Technically speaking, "PRs" are not things that you add commits to. You add commits to branches, and the PR points to a branch. Closing your PR will not - can not, if it's on your own fork - delete your branch.
and even "git rebase" and squash commits in that PR.
Please do not use squash commits. See http://mjg59.dreamwidth.org/42759.html.
First is first, trivial or not :-). -glyph
![](https://secure.gravatar.com/avatar/3d37232726396a1d3c7412dd915095ea.jpg?s=120&d=mm&r=g)
On 22 May 2016, at 14:15, Glyph <glyph@twistedmatrix.com> wrote:
Sorry, I guess I wasn't clear. I know that PRs are presently a potential alternative to a diff, and that we are still using Trac for ticketing. I want to make it possible to avoid using Trac for ticketing; perhaps switching to github issues entirely.
This is an optimistic idea but one that, unfortunately, won't happen yet ;) The things GitHub Issues need to be competitive with Trac as it stands: - Allowing triage by people without write. - Useful search (GitHub search is kind of abysmal) - Assigning to non-committers. Without these things (and quite a few more), it's unlikely that GitHub Issues will be as useful to us. - Amber
![](https://secure.gravatar.com/avatar/e1554622707bedd9202884900430b838.jpg?s=120&d=mm&r=g)
How are things currently "triaged"? Do you mean "review"? If so, I think it would be acceptable to come up with a magic comment for a non-commiter to use to signify that they've fully reviewed a PR. As it stands, we need committers to "accept" a review by deciding to merge, the only difference here is that it would remain in the review queue until they did so, which I think is acceptable (since if the review isn't accepted, it should have remained in the queue anyway). We could also have a bot address this edge-case somehow.
- Useful search (GitHub search is kind of abysmal)
I don't see how Trac's is better.
- Assigning to non-committers.
Honestly I'm not sure that the non-committer assignment part of the workflow is all that useful. I know I hardly ever look at report 7, and I very much doubt any non-committer does :). It's not like we're losing information, either; we still have a record of whose fork the PR points to.
Without these things (and quite a few more), it's unlikely that GitHub Issues will be as useful to us.
I am curious about the "quite a few more". There are things which we really need as a critical part of our workflow (primarily: the review queue) and then there are accidents of the way trac works. Nothing is graven in stone here :). -glyph
![](https://secure.gravatar.com/avatar/3d37232726396a1d3c7412dd915095ea.jpg?s=120&d=mm&r=g)
Creating a ticket, adding it to the relevant milestone (commit required on GitHub), setting the component (which would be a tag on github, requires commit)...
- Useful search (GitHub search is kind of abysmal)
I don't see how Trac's is better.
It has a GUI rather than being stringly typed ;)
- Assigning to non-committers.
Honestly I'm not sure that the non-committer assignment part of the workflow is all that useful. I know I hardly ever look at report 7, and I very much doubt any non-committer does :). It's not like we're losing information, either; we still have a record of whose fork the PR points to.
I look at report 7 :(
Without these things (and quite a few more), it's unlikely that GitHub Issues will be as useful to us.
I am curious about the "quite a few more". There are things which we really need as a critical part of our workflow (primarily: the review queue) and then there are accidents of the way trac works. Nothing is graven in stone here :).
I guess there's a lot of things that are an accident of trac, but the things above are useful.
-glyph
![](https://secure.gravatar.com/avatar/e1554622707bedd9202884900430b838.jpg?s=120&d=mm&r=g)
I don't see us getting a lot of non-committer triage of this kind. And I'm not sure that's really an important part of our workflow - do you really feel that it is? Frankly when non-committers try to put their changes into a milestone or start adding custom keywords, it's almost always wrong. We also don't use "component" for much. If we just got rid of it, would any part of our process change?
Oh, you're talking about "query", i.e. https://twistedmatrix.com/trac/query, not "search", i.e. https://twistedmatrix.com/trac/search ? GitHub's search is actually structured, not stringly typed; there's a bit of GUI here: https://github.com/search/advanced. It does resolve down to a string query, but so does the trac "custom query" eventually (in that it goes into a URL you can copy and paste). But, this is all sort of abstract: what kinds of queries would you do against our ticket database in Trac that are not possible in GitHub's query language? Personally after some exploration of GitHub's query language I have found it's actually more expressive for what I want to do most of the time; particularly querying across projects which is obviously not possible with trac...
That's not a counterexample: you are a committer :-). I do actually have a recurring personal to-do item to check report 7 nowadays, but almost all of my doing-stuff-on-the-tracker has to do with me getting emails about actionable state changes (somebody reviewed your change, you should merge it), rather than scanning that list.
I'm not disputing that, but I'm still a little confused about how exactly they're useful, rather than just different. Can you give some examples of things you do regularly? -glyph
![](https://secure.gravatar.com/avatar/e93e18f71a5821a54c233690506bdbf7.jpg?s=120&d=mm&r=g)
All,
Please do not use squash commits. See http://mjg59.dreamwidth.org/42759.html.
Squashing commits is essential to making useful commit histories. Are you just saying not to use Github's built-in feature which squashes everything into a _single_ commit? If so, note that you can turn that GUI feature off so nobody is tempted to use it. That does leave the question of whether you want people to use merge commits or to rebase their branch on the latest master before merging so that you always get a fast-forward.
![](https://secure.gravatar.com/avatar/3d37232726396a1d3c7412dd915095ea.jpg?s=120&d=mm&r=g)
The thing is that we don't want fast-forward; fast-forward merging creates awful commit histories with no regard to branches, especially for ones like Twisted where (near) every commit on trunk needs to be a deployable one, and where we may need to revert an entire branch. - Amber
![](https://secure.gravatar.com/avatar/e1554622707bedd9202884900430b838.jpg?s=120&d=mm&r=g)
On May 21, 2016, at 11:24 PM, Daniel Sank <sank.daniel@gmail.com> wrote:
Squashing commits is essential to making useful commit histories.
Nope. It's just a handy hack to work around the commonly-used, broken history viewers (like Github's own) that can't correctly present multi-parent commits. If you use SourceTree or bzr qlog or anything that correctly collapses merges, you don't need squashes. Furthermore, if you use squashes, you work around a temporary problem (crummy history viewers - which will probably eventually be fixed) by permanently destroying useful information (the sequence of changes which lead to a larger change). Of course, I can't stop you from doing this in any meaningful sense, you can always delete commits and just create bigger diffs and I won't be able to tell, but I would prefer it if you don't use squash commits or any other kind of history rewriting on Twisted. -glyph
![](https://secure.gravatar.com/avatar/e1554622707bedd9202884900430b838.jpg?s=120&d=mm&r=g)
On May 21, 2016, at 11:24 PM, Daniel Sank <sank.daniel@gmail.com> wrote:
That does leave the question of whether you want people to use merge commits or to rebase their branch on the latest master before merging so that you always get a fast-forward.
Github will never fast-forward, and I never want to see anyone using a client manually push a fast-forward to master, either. Every left-parent commit should be something that was code reviewed. If the changes on master are small, it's fine to just merge master into the branch. But, in the case of larger changes and resurrecting stale, old PRs, rather than merge 1000s of commits off of master, I actually prefer creating a new branch from current master and merging the old branch into it. Rebasing an entire branch on master creates new commits that were never tested by CI, and never present in anyone's working copy. I strongly prefer to avoid this. Rebase is great functionality, but IMHO there is only one correct way to use it: rebase --interactive, carefully vetting each commit to ensure it still says what you want it to. -glyph
![](https://secure.gravatar.com/avatar/3d37232726396a1d3c7412dd915095ea.jpg?s=120&d=mm&r=g)
I think this is reasonable. I believe txghbot is in a place where we can start using it for just that.
The one thing that this workflow is missing from trac is a convenient way for committers, having eyeballed a patch for any obvious malware, to send it to the buildbots.
We could also potentially just replace our buildbot build farm with a combination of appveyor and travis-ci; this would remove FreeBSD from our list of supported platforms, as well as eliminating a diversity of kernel integrations. However, for the stuff that doesn't work in containers (mostly inotify) we could run one builder on non-container-based infrastructure, and for everything else (integrating with different system toolchains) we can test using Docker: https://docs.travis-ci.com/user/docker/. I am very much on the fence about this, since I don't want to move backwards in terms of our test coverage, but this would accelerate the contribution process so much that it's probably worth discussing.
I don't think that this will do us any good. Having travis/appveyor to be a first line of review (giving a quick "builds fail/builds probably pass) so that contributors get an immediate "does this have a chance of being merged" is a good thing; but there is certainly value in having the various different platforms. All we need is a bit more tooling around it (and I think txghbot is a good starting place for that). We also need to look into some extra tooling around tox; mainly around the ratcheting quality checkers -- some form of fetching the latest build from buildbot and downloading it locally to compare, or something.
10 years ago or so, we would routinely discover kernel bugs in our integration test harness and they would be different on different platforms. But today's platform realities are considerably less harsh, since there are a lot more entities in the ecosystem that have taken responsibility for testing that layer of the stack; I couldn't find anything since 2008 or so where we really saw a difference between Fedora and Ubuntu at the kernel level, for example.
The issue in 2016 is not actually the kernel; but OpenSSL and OpenSSH, among other system libraries. Every new version of Fedora has been red on the buildbots for this reason; OpenSSL changed, and we needed to fix our use of it. Worth also noting is that Travis is so horrendously behind in all things Python+Ubuntu (do they even have a current PyPy yet?) that we're not actually testing the platforms people are *using*, which is something I think is valuable that the current system gives us.
Thoughts?
-glyph
- Amber
![](https://secure.gravatar.com/avatar/e1554622707bedd9202884900430b838.jpg?s=120&d=mm&r=g)
On May 21, 2016, at 11:08 PM, Amber Hawkie Brown <hawkowl@atleastfornow.net> wrote:
The issue in 2016 is not actually the kernel; but OpenSSL and OpenSSH, among other system libraries. Every new version of Fedora has been red on the buildbots for this reason; OpenSSL changed, and we needed to fix our use of it. Worth also noting is that Travis is so horrendously behind in all things Python+Ubuntu (do they even have a current PyPy yet?) that we're not actually testing the platforms people are *using*, which is something I think is valuable that the current system gives us.
The reason I specifically mention the kernel is that different userlands can be tested by using Docker in Travis: <https://docs.travis-ci.com/user/docker/>. The travis official images are a little faster, because they cache more stuff, but we can build our own base images to try to offset that somewhat. -glyph
![](https://secure.gravatar.com/avatar/d37c7104c024b78dc451e3a6b733df9d.jpg?s=120&d=mm&r=g)
FWIW, I just noticed that the CONTRIBUTING message wasn't showing up when trying to make a PR. github is supposed to make a warning box that shows the contents of /CONTRIBUTING or /CONTRIBUTING.md when opening an issue or PR
![](https://secure.gravatar.com/avatar/469df05f5dfd5b75fb3cb3a0868d36bf.jpg?s=120&d=mm&r=g)
Hi, Thanks for bringing up these points. See: https://twistedmatrix.com/trac/ticket/8352 Feel free to add any thoughts you may have on how to improve this. -- Craig On Tue, May 24, 2016 at 10:37 AM, Jonathan Vanasco <twisted-python@2xlp.com> wrote:
![](https://secure.gravatar.com/avatar/d37c7104c024b78dc451e3a6b733df9d.jpg?s=120&d=mm&r=g)
just a few thoughts: The current system as-explained seems to use an "Issue" as a queue item that is either a "bug report" or a "notice of a pull request, which may also reference another bug report". that is weird. IMHO, a quality contributor will never get turned off by handling the docs, tests, and other requirements - but may get confused/turned off by this. anyways: 1. github's PRs are peculiar in that they're "branch to branch" and not "branch@commit to branch@commit". If I were to submit a PR, I could still make changes on it between my submission and someone finally doing a merge. Personally, I find this infuriating. In any event, I suggest *requiring* something like the "git flow" model for submissions. Outright rejecting anything that isn't in a dedicated fix/feature branch. Everyone is currently doing the right thing, and there are contribution docs that suggest this – I would just make it a stated policy to automatically reject any PRs from someone using "master/trunk". 2. regarding Issues vs PRs and working between Trac and Github, I suggest you take the approach of "Trac Issue = Idea" and "Github PR = Implementation of Idea". Under this concept, one or more PRs might be attempts to address a given Issue. The core maintainers can address the highlevel concepts and requirements under Issue, while grounds for rejection / feedback on each PR are listed there. As a github/bitbucket submitter, it's really useful when someone looks at a diff and uses inline comments to note their rejection for a section. This is how most people use Github. (and it's incompatible with the current queue system, I KNOW). 3. Initially I didn't like the idea of rejecting so fast, but after thinking it over, now I do. In addition to boilerplate text about "please resubmit", I think it might be beneficial to standardize some labels for this though, as it helps future contributors who may want to address an issue. If the "IDEA" is approved but the implementation is not, then noting "Resubmission Pending" suggests this is still active. If the underlying IDEA or approach is rejected, then noting "Rejection Final" can suggest it's just not going to happen. 4. I understand that "review" hold special significance within the twisted team's workflow, but outside of Twisted it is awkward to see something bounce in and out of "review". At least on the github side, using some sort of label to state where in the process (ie, what type of review) something is under would help. The caveat is that this breaks the current use of Issue as a "queue" item. But Twisted is using "issues" less as an Issue and more as a queue item, while github "Issues" are less of a queue item and amore of an actual issue. This is a bit confusing and different to how many people will use github. Just to illustrate what a typical github contributor flow might be: Scenario A- There is a trac Issue #1001 for "F is broken" bob generates github pr 11, and submits. He comments in #1001 ted generates github pr 12, and submits. He creates a trac issue #1002. When #1002 is first looked at, it is a dupe of 1001 and copied over. carol generates github pr 13 and submits. she comments in #1003 There are 3 PRs for a single Issue. Feedback on the PRs occurs on Github. Feedback on the Ideas and status of PRs is on trac. Scenario B- Bob notes that "X is broken" Bob generates github PR 11 and submits. He goes to trac and creates issue #1002 Bob's PR is rejected because the fundamental approach is not acceptable, however X is still broken. PR11 is closed, #1002 is open. Ted generates pr 12 and submits. he comments in #1002. Ted is rejected but the approach is compatible. it can be implemented with fixes. Carol forks Ted's approach , fixes it, and generates pr 13. she comments in #1002 and it is accepted. again, this breaks the current use of queues. I'm not sure how to reconcile everything together, however I think you're going to find non-core maintainers naturally fall into the 2 scenarios above.
![](https://secure.gravatar.com/avatar/e1554622707bedd9202884900430b838.jpg?s=120&d=mm&r=g)
In the current system, we have "tickets" (in Trac) not "issues" (on Github). And in that system, an issue is put into review when it has an attached branch or patch that needs to be considered for inclusion. In the Github system.
that is weird.
IMHO, a quality contributor will never get turned off by handling the docs, tests, and other requirements - but may get confused/turned off by this.
Literally everyone's predictions about this have always been wrong, so I don't like speculating ;). Especially classifying people as "quality contributors" or not. I will sometimes say something like "if you don't do X, you probably won't do Y" but being a "quality contributor" is a complex, multi-dimensional, and situationally-dependent personality characteristic that I don't think we can predict.
anyways:
1. github's PRs are peculiar in that they're "branch to branch" and not "branch@commit to branch@commit". If I were to submit a PR, I could still make changes on it between my submission and someone finally doing a merge. Personally, I find this infuriating.
I KNOW, RIGHT!!! However, protected statuses somewhat reduce the potential race-condition here. And contributing to a couple dozen Github projects I have to say that practically this has never been an issue, even though I find it aesthetically appalling, design-wise.
In any event, I suggest *requiring* something like the "git flow" model for submissions. Outright rejecting anything that isn't in a dedicated fix/feature branch. Everyone is currently doing the right thing, and there are contribution docs that suggest this – I would just make it a stated policy to automatically reject any PRs from someone using "master/trunk".
Honestly I don't think this is worth the effort. git-flow is, abstractly, a reasonably good idea (if people don't know what we're talking about, see <http://jeffkreeftmeijer.com/2010/why-arent-you-using-git-flow/>) since it encapsulates the precise semantics desired by certain commits. But sometimes people make a fork and their fork branch just happens to be called 'patch-1', or 'master', but this doesn't really affect our workflow as long as they observe proper PR etiquette and don't push any unrelated revisions to that branch in the meanwhile.
2. regarding Issues vs PRs and working between Trac and Github, I suggest you take the approach of "Trac Issue = Idea" and "Github PR = Implementation of Idea". Under this concept, one or more PRs might be attempts to address a given Issue.
That's ... exactly what I was trying to express. (Except I was thinking we'd gradually move from Trac Tickets to Github Issues).
The core maintainers can address the highlevel concepts and requirements under Issue, while grounds for rejection / feedback on each PR are listed there. As a github/bitbucket submitter, it's really useful when someone looks at a diff and uses inline comments to note their rejection for a section. This is how most people use Github. (and it's incompatible with the current queue system, I KNOW).
I don't see how it's incompatible. It seems perfectly fine, as long as we shift from 'review queue == report 25' to 'review queue == open PRs with non-failing status'.
3. Initially I didn't like the idea of rejecting so fast, but after thinking it over, now I do. In addition to boilerplate text about "please resubmit", I think it might be beneficial to standardize some labels for this though, as it helps future contributors who may want to address an issue. If the "IDEA" is approved but the implementation is not, then noting "Resubmission Pending" suggests this is still active. If the underlying IDEA or approach is rejected, then noting "Rejection Final" can suggest it's just not going to happen.
Yeah, adding some informative labels to the closed issues might be a good way of letting contributors know what is going on. I just wanted to make sure that they are an optional part of the workflow which, if the automation around them breaks down in some way, we can still make progress, and we don't end up with inconsistent or lost information.
4. I understand that "review" hold special significance within the twisted team's workflow, but outside of Twisted it is awkward to see something bounce in and out of "review".
I think this is because, generally speaking, most teams do a poor job structuring their communications about what state issues and pull requests are in :).
At least on the github side, using some sort of label to state where in the process (ie, what type of review) something is under would help.
What do you mean it "would help"? What problem would it solve?
The caveat is that this breaks the current use of Issue as a "queue" item. But Twisted is using "issues" less as an Issue and more as a queue item, while github "Issues" are less of a queue item and amore of an actual issue. This is a bit confusing and different to how many people will use github.
I don't know where you're getting this from. Only tickets in review are queue items. I have no idea what "actual issue" means here.
What use? What are you referring to with the plural "queues"? There's only ever been a single queue, the review queue. In both of your scenarios, the list of open PRs is the review queue, and in both of those scenarios, discussion of the abstract idea can happen on the tickets without treating them directly as queue items. I appreciate you taking the time to think about our workflow and the review queue, but I would ask that you carefully consider what problem you're trying to address with your suggestions. The practical issue I'm trying to address by switching to github PRs as opposed to trac tickets with the 'review' keyword is the speed and ease of submitting a change and getting CI feedback on that change, in addition to lowering training overhead since many people are familiar with github. Faster feedback = happier contributors = more maintenance effort productively expended. The issue I'm trying to address with the slightly odd close-a-PR-to-signify-finished-review workflow is to avoid the common problem of reviewers not knowing which things to review and therefore leaving certain contributions mouldering until their submitters lose interest. Beyond that, I don't see that we have substantial workflow issues that need solving. -glyph
![](https://secure.gravatar.com/avatar/d37c7104c024b78dc451e3a6b733df9d.jpg?s=120&d=mm&r=g)
On May 24, 2016, at 6:49 PM, Glyph wrote:
I KNOW, RIGHT!!! However, protected statuses somewhat reduce the potential race-condition here. And contributing to a couple dozen Github projects I have to say that practically this has never been an issue, even though I find it aesthetically appalling, design-wise.
But sometimes people make a fork and their fork branch just happens to be called 'patch-1', or 'master', this doesn't really affect our workflow as long as they observe proper PR etiquette and don't push any unrelated revisions to that branch in the meanwhile.
In terms of "git flow" i didn't mean the exact names, just the concept that every fix has it's own dedicated branch. This is something that almost every github-experienced person does automatically (and all current twisted PRs do). People with less experience on github itself (not a given package) will often just edit the "master" branch and submit. then they'll forget they did that and edit something else... because of that github peculiarity, and that everything is acting "centralized" on github and not under a "decentralized" model where a particular commit was queued that ends up in the review. those types of changes can also end up triggering CI tests, which complicates things further. twisted contributors are likely a bit more experienced and it's a self-selecting pool... i've just seen an increasing number of projects require a dedicated branch and politely reject + ask for anything against 'master' to be resubmitted via a dedicated branch.
I don't see how it's incompatible. It seems perfectly fine, as long as we shift from 'review queue == report 25' to 'review queue == open PRs with non-failing status'.
ah, I did not know that was possible. this ties into me not understanding the content of the queue being PR only (not "all tickets").
At least on the github side, using some sort of label to state where in the process (ie, what type of review) something is under would help.
What do you mean it "would help"? What problem would it solve?
This would help non-maintainers understand what that actual progress was in that 5 stage review.
The caveat is that this breaks the current use of Issue as a "queue" item. But Twisted is using "issues" less as an Issue and more as a queue item, while github "Issues" are less of a queue item and amore of an actual issue. This is a bit confusing and different to how many people will use github.
I don't know where you're getting this from. Only tickets in review are queue items. I have no idea what "actual issue" means here.
Ok. From here to the end I believe I understand my confusion. The "queue" looked to me like it was "All Trac Tickets" not just "Certain types of Trac Tickets". I was looking at a few reports and some raw views that showed track tickets that were both bug reports and attempts to fix a specific problem. It looked like the normal use of github would have run antagonistic to your workflow , but his all seems fine now.
The practical issue I'm trying to address by switching to github PRs as opposed to trac tickets with the 'review' keyword is the speed and ease of submitting a change and getting CI feedback on that change, in addition to lowering training overhead since many people are familiar with github. Faster feedback = happier contributors = more maintenance effort productively expended. The issue I'm trying to address with the slightly odd close-a-PR-to-signify-finished-review workflow is to avoid the common problem of reviewers not knowing which things to review and therefore leaving certain contributions mouldering until their submitters lose interest. Beyond that, I don't see that we have substantial workflow issues that need solving.
I was mostly concerned with the proposed implementation creating workflow issues.
![](https://secure.gravatar.com/avatar/c8f4d4d1a8fc53c13ed05c202e0257fe.jpg?s=120&d=mm&r=g)
Hi Glyph On Sun, May 22, 2016 at 12:12 AM, Glyph <glyph@twistedmatrix.com> wrote:
- Reviewing: This is the potentially slightly odd part. I believe a review that doesn't result in acceptance should *close* the PR.
This feels wrong to me. I find github pull requests very useful, in ways that it sounds like your suggestion would cut off - if I understand you right. Do you mean that the one person who decides to do the formal review would set themself as the "assignee" and, following their review, close the ticket if the change isn't up to standard? And in the meantime others (who are not the assignee) would be free to comment at will, with the pr staying open? If so, that's good, but I still don't like to think that all discussion around a pr then gets shut down on the say so of the single reviewer. One thing that pull requests encourage is discussion - sometimes it's a way to ask for input on how to proceed, sometimes it's just people chipping in a little bit with a code optimization, sometimes people saying "if you do this then this other thing will happen" etc. They help people learn, to easily make small contributions (that can lead to larger ones), to see what's going on, to judge the health of a project, etc. I like how dynamic and lightweight that process can be in a Github pull request. It feels to me that closing pull requests is a step in the other direction - back towards something that feels more monolithic and more old fashioned. As usual, I'm sure that there's absolutely nothing I can say on any tech subject that you don't know already :-) But those are my thoughts... Terry
![](https://secure.gravatar.com/avatar/e1554622707bedd9202884900430b838.jpg?s=120&d=mm&r=g)
The "assignee" field isn't all that useful; really, it should be set to the submitter, but github has weird rules about who can be assigned (last I checked, only contributors).
And in the meantime others (who are not the assignee) would be free to comment at will, with the pr staying open?
Drive-by comments on a PR are sometimes helpful, but should be used sparingly. Mostly, discussion should happen on issues, not PRs. A PR is a suggested resolution to a problem, and we might reject one solution, but an issue should describe the problem itself.
If so, that's good, but I still don't like to think that all discussion around a pr then gets shut down on the say so of the single reviewer.
The PR gets closed, not deleted. People can still comment if they like.
One thing that pull requests encourage is discussion - sometimes it's a way to ask for input on how to proceed, sometimes it's just people chipping in a little bit with a code optimization, sometimes people saying "if you do this then this other thing will happen" etc. They help people learn, to easily make small contributions (that can lead to larger ones), to see what's going on, to judge the health of a project, etc. I like how dynamic and lightweight that process can be in a Github pull request. It feels to me that closing pull requests is a step in the other direction - back towards something that feels more monolithic and more old fashioned.
You see "dynamic and lightweight", I see "unfocused and noisy" :). One of the things I still don't like about github is the tendency for projects to build up huge piles of open PRs, which nobody wants to reject because they're maybe possibly useful, but they won't accept because they don't meet quality standards. Contributors don't get clear feedback about whether they're expected to do more or whether the project will take it over, and people frequently get mad about their stuff not being merged. I think by closing PRs that we aren't going to merge as-is we can send a much clearer signal about what the project is taking on versus what it expects external contributors to do more work on.
As usual, I'm sure that there's absolutely nothing I can say on any tech subject that you don't know already :-) But those are my thoughts...
While I disagree, I also think that this is a very common perception, and another one of the functions of the github bot could perform would be a form comment after the PR is closed, to always submit a form comment to explain what closing the PR means and how to reopen it. -glyph
![](https://secure.gravatar.com/avatar/61b1aad7d8e6c28832ecdffe8036a5f0.jpg?s=120&d=mm&r=g)
To qualify my comments, I've yet to contribute to Twisted because I don't have a good enough grasp of its internals, but I have contributed to a variety of Git-based OSS projects. I definitely get uneasy with the general idea that we're trying to "replicate workflow A from Trac in tangentially related Git PR feature". We're in Git. We're hoping to solicit PRs from Git users. Git users will be used to the way PRs are used in other OSS Git projects. Glyph has some valid criticisms of the situation in some projects, but it should still be the starting point. For example, closing a PR strikes me as a bad idea -- for lack of a better word, it feels "hostile" to me and certainly unwelcoming. In several of the projects I've seen, Git tags fill these roles. Piwik has a "needs review" tag -- the short list for reviewers. Looks like it's a manual add, but maybe it could be automated. Once reviewed, Piwik has tags like "Tests & QA". ZendFramework has a generic "awaiting author update". To address Glyph's concerns about lingering PRs, perhaps the combination of: - A policy like "a reviewer must accept, close, or tag with one of the next step tags" - A short list of common next steps like "code quality", "needs tests", "second opinion", "not review ready"... plus a generic "other author action" - Auto-close tickets except those with "needs review" or "second opinion" (say) 30 days after the last action. Drive-by comments on a PR are sometimes helpful, but should be used
While an Issue is a good place for discussion about a problem, it lacks the reference code often included in a PR. You can't ask "how about this approach" without showing the approach. As an added bonus, most systems run travis on PRs so you get a sense of "this approach is thorough" or "this idea still breaks something". Clayton Daley On Sat, May 21, 2016 at 8:29 PM, Glyph <glyph@twistedmatrix.com> wrote:
![](https://secure.gravatar.com/avatar/e1554622707bedd9202884900430b838.jpg?s=120&d=mm&r=g)
On May 21, 2016, at 7:25 PM, Clayton Daley <clayton.daley@gmail.com> wrote:
To qualify my comments, I've yet to contribute to Twisted because I don't have a good enough grasp of its internals, but I have contributed to a variety of Git-based OSS projects. I definitely get uneasy with the general idea that we're trying to "replicate workflow A from Trac in tangentially related Git PR feature".
The workflow is not "from Trac". The instantiation in Trac is not optimal either, which is why I described the abstract desired workflow separately from our existing instantiation.
We're in Git. We're hoping to solicit PRs from Git users. Git users will be used to the way PRs are used in other OSS Git projects.
I think you mean "GitHub". Git PRs don't work at all like GitHub PRs. :).
Glyph has some valid criticisms of the situation in some projects, but it should still be the starting point. For example, closing a PR strikes me as a bad idea -- for lack of a better word, it feels "hostile" to me and certainly unwelcoming.
The thing is, if you perceive it as "hostile" that a project closes a PR - i.e. "says that they're not going to do more work on it" - that is a cultural problem; it suggests a certain implicit level of passive aggression in opening a PR which I don't want to assume. It's sort of like having a culture where you can just send anybody an email asking them to do whatever and it would be "hostile" for them to refuse. In such a culture people don't say "yes", but they do start to ignore messages Closing the PR is a more accurate reflection of reality - the project (twisted) is not going to do anything about the PR in its current state, so it shouldn't be left open. It also clearly demarcates the completion of a review. People feel very differently about workflow, of course, but I've definitely heard from other OSS maintainers that the average workflow of volunteer projects often devolves into a huge backlog of un-reviewed stuff, which obscures the new stuff, and if you want something to actually get reviewed and move along you need to know the maintainers of the project and ask them personally. I'd much rather our new contributors get a little confused about the culturally unusual step of closing a PR than to have their work be accidentally but systematically discriminated against in favor of people who know how to bug the right people in IRC or email.
In several of the projects I've seen, Git tags fill these roles. Piwik has a "needs review" tag -- the short list for reviewers. Looks like it's a manual add, but maybe it could be automated. Once reviewed, Piwik has tags like "Tests & QA". ZendFramework has a generic "awaiting author update".
This was my original idea. The problem with GitHub labels ("Git tags" are something completely different) is that they can't be applied by external contributors. You need write access to the repository to be able to manipulate them. It's very important to our workflow that external contributors be able to re-submit their PRs. We could have a bot for that (again, this was the original plan). But it seems like using the open / closed state to reflect the we will do some work on this / we won't do any more work on this is actually closer to the "native" state of github.
Drive-by comments on a PR are sometimes helpful, but should be used sparingly. Mostly, discussion should happen on issues, not PRs. A PR is a suggested resolution to a problem, and we might reject one solution, but an issue should describe the problem itself.
While an Issue is a good place for discussion about a problem, it lacks the reference code often included in a PR. You can't ask "how about this approach" without showing the approach. As an added bonus, most systems run travis on PRs so you get a sense of "this approach is thorough" or "this idea still breaks something".
This is exactly why issues and PRs should be separated. If you only have one artifact - the PR - to represent both the issue and the potential solution, then you can't get rid of the potential solution (reject the PR) without also getting rid of the description of the problem. Github already automatically shows anywhere that your PR or issue is mentioned, so all you need to do to say "how about this approach" is to put the words "fixes #NNN" in your PR description. As a bonus, that will make it automatically close the issue when the PR is merged. -glyph
![](https://secure.gravatar.com/avatar/61b1aad7d8e6c28832ecdffe8036a5f0.jpg?s=120&d=mm&r=g)
... I'd much rather our new contributors get a little confused about the
It's your party, but I think this vastly undervalues first impressions in OSS engagement. From all the projects I've contributed to on Github -- yes technically Github not Git though Bitbucket has equivalent features (and unlimited private repos for free!) -- closing means "this is off the table". It's a nice clear signal that there's nothing the contributor can do to fix it -- e.g. I had an LSP-valid change that passed all tests run afoul of some obscure PHP method signature limitation when mixed with other packages. I'll wager only the active contributors will remember the proposed subtlety a month from now. As a big break from Github norms, It's going to hit everyone else at the worst time... a first PR submission. That's the moment when a contributor is trying to get a sense of the culture of the team that manages the project. They're at their most vulnerable and violating *their* norms significantly increases the odds that they'll leave the fix in a private fork and disengage. Twisted operates at a different level so this may not be a bad thing. You may benefit from actively discouraging dabblers -- especially given resource constraints. But there aren't going to be a lot of "first PR" folks on this list to point out the effect of this break from norms. Clayton Daley On Sun, May 22, 2016 at 12:12 AM, Glyph <glyph@twistedmatrix.com> wrote:
![](https://secure.gravatar.com/avatar/e1554622707bedd9202884900430b838.jpg?s=120&d=mm&r=g)
This is roughly the same story I've been getting for the last decade and a half: "You can't: require test coverage, require documentation, require coding standard compliance, require people to file a ticket before sending a patch to the mailing list, that's a terrible burden to put on new contributors!" Somehow we've survived much longer than most projects, and while some would say "in spite of" these restrictions, I think it's "because of". So, we are not trying to "discourage dabblers"; we would like new contributors who want to contribute only a little bit. So while I don't want to throw up arbitrary barriers, if you aren't willing to invest the effort to even read a single comment on your PR explaining why it was closed and how to reopen it, I cannot imagine that chances are good that you'll read the rest of the comments explaining what changes need to be made and make them effectively. Further, people who contribute trivial changes that can be immediately merged, like documentation typos, won't need to deal with this, because they won't see the intermediary "needs feedback" closed state; they'll just get their PRs accepted immediately. -glyph
![](https://secure.gravatar.com/avatar/61b1aad7d8e6c28832ecdffe8036a5f0.jpg?s=120&d=mm&r=g)
this caliber and the 4th doesn't make sense in a Github/Bitbucket/Gitlab world (and I fortunately avoided its predecessors). In fact, I'd be a little worried if the first there weren't required in a project. ResourceSpace is the only one I can think of that doesn't require them and I wouldn't touch it with a 10 foot pole if there were acceptable (free) alternatives. So I don't see the link between your success enforcing three norms of good OSS projects and your desire to break a 4th. === The idea that PRs are a substantial burden has caused me pause. Normally, a PR is a chance to give back to a project rather than freeload. In most projects, new features are part of a virtuous cycle: a new feature tends to benefit a large fraction of the user base and expand the user base... which draws more contributors. Especially given the recent deprecations (old, unmaintained protocols), it seems that Twisted doesn't work like this. If another user adds another protocol, it doesn't make the system better for most users. In fact, it makes sense that it actually increases the maintenance burden. I tried to look for myself, but was reminded that I don't know the internals well enough... are patches on non-central protocols a big part of the backlog? Or is the backlog mostly core features (like reactors or IO infrastructure) that most projects depend upon? Clayton Daley On Sun, May 22, 2016 at 4:22 PM, Glyph <glyph@twistedmatrix.com> wrote:
![](https://secure.gravatar.com/avatar/ebf132362b622423ed5baca2988911b8.jpg?s=120&d=mm&r=g)
Twisted has been enforcing these rules since before they were considered part of the norm and I believe that Glyph was referencing is that back then people said that Twisted was going to fail or w/e because of requiring those things. Sent from my iPhone
On May 22, 2016, at 9:12 PM, Clayton Daley <clayton.daley@gmail.com> wrote:
The first three of these are *already* norms in all of the OSS projects of this caliber
![](https://secure.gravatar.com/avatar/e1554622707bedd9202884900430b838.jpg?s=120&d=mm&r=g)
To make a long story short: we pretty much invented these norms, and were doing them a long time before other projects got on the bandwagon, over many, very loud objections. That said, I don't want to overstate the case. Test coverage was a huge deal. Doc coverage was a huge deal. This idea with PRs is a very small thing, maybe not the best idea, and not something we're unanimous about or dead set on. But the "all your friends are jumping off a bridge" argument against doing it just isn't very compelling.
The idea that PRs are a substantial burden has caused me pause. Normally, a PR is a chance to give back to a project rather than freeload. In most projects, new features are part of a virtuous cycle: a new feature tends to benefit a large fraction of the user base and expand the user base... which draws more contributors.
The issue is not that PRs are, inherently, a "burden". But, before I try to rephrase the issue with too many simultaneously open PRs, I should probably describe the tremendous asymmetry between core maintainers and external contributors. This asymmetry is general to all open source projects, not particularly specific to Twisted at all. If you listen to Nadia Eghbal's various comments about funding open source (motivated by <https://medium.com/@nayafia/how-i-stumbled-upon-the-internet-s-biggest-blind...>) you hear this complaint repeated by lots of project maintainers. External contributors typically have an initial interest in landing one "contribution", which is something that benefits them (whether personally or their company) specifically, rather than something that benefits the community at large. Transforming that initial interest into a long-term commitment to the project is very challenging for maintainers. Even if an improvement is specific to the contributor who is making it, of course, there will be others like them who want similar improvements, so it's never a purely selfish or purely altruistic motivation for making the contribution; it's a mix. And general benefits are good for the core maintainers, of course, since it makes their long-term maintenance job easier, and it does attract more people to the project. But that benefit has to be balanced with a cost. The cost is that it takes time and energy to code-review contributions. This is the source of a lot of friction between core maintainers and external contributors: contributors feel that they are generously giving their time and energy to the project, and it's rude of the project to make them jump through any hoops to get it accepted, whether that's test coverage, documentation, pre-commit code review, coding standard compliance, or, in this case, learning a slightly weird workflow. External contributors can affect this balance a lot, by making sure that their contributions are already as close as possible to acceptable. But even if they do, the ratio of external contributors to core maintainers is almost always far greater than 1; the only way that external contributors can _really_ affect this balance is to become core maintainers :). And this asymmetry brings us to why it's important to keep the 'Review Queue' short. To rephrase what I said earlier in this thread, if there are so many open PRs that reviewers don't know which ones to look at first, then the sorting algorithm will be "whichever ones my friends want me to look at first", which means we need to maintain a clear division between things-we-are-looking-at and things-we-are-not-looking-at, to maximize the effectiveness of the limiting factor of code reviewer time for impact on the latency of time it takes to get feedback on a potential change. Sorry that this is a bit long-winded but I really just object to the premise that the point is that I don't care about contributors. I care about contributors a great deal; this is why I want a carefully designed process that doesn't shut them out or waste their time.
Especially given the recent deprecations (old, unmaintained protocols), it seems that Twisted doesn't work like this. If another user adds another protocol, it doesn't make the system better for most users. In fact, it makes sense that it actually increases the maintenance burden.
This is true of all features for all projects, though. More code == more maintenance.
I tried to look for myself, but was reminded that I don't know the internals well enough... are patches on non-central protocols a big part of the backlog? Or is the backlog mostly core features (like reactors or IO infrastructure) that most projects depend upon?
I don't think we have any good metrics on the backlog, unfortunately. But certainly most of the maintenance work has been maintenance, like porting existing code to python 3, improving test coverage, and implementing improvements to TLS. -glyph
![](https://secure.gravatar.com/avatar/e93e18f71a5821a54c233690506bdbf7.jpg?s=120&d=mm&r=g)
FWIW I thought of another "open source" community which uses a similar idea to closing pull requests if they won't be accepted in their current form: Stack Exchange. I mostly frequent Physics Stack Exchange <http://physics.stackexchange.com/>. At any time some number of the questions on the front page are either "closed" or "on hold" for not living up to site standards in some way. We've asked almost exactly the same question as is being asked in this thread: does closing unfit questions discourage new users so much as to outweigh the benefits to site quality? I won't pretend to have an authoritative answer to this question, but I will mention the steps we've taken to help make sure new users aren't put off. 1. We have a help center (like all other Stack Exchange sites) explaining the rules and system. It's not great though, and its shortcomings show up all the time as new users do get confused about the rules. I think if anything this is just an indication that contributing guidelines need to be really clear about what the various signals from the maintainers mean. 2. It's really helpful to leave a comment on unfit questions explaining what the problem is and *how the poster can improve it*. Stack Exchange has canned close reasons, but they're rarely sufficient in my opinion. I leave comments like "Welcome to Physics Stack Exchange! I think there's an interesting question here but we have some rules...Please see the [help center](link)...". Starting the comment with something positive and directing the user to official guidelines seems rather helpful, although I can't provide metrics. 3. Our chat room is extremely easy to access and people there are friendly. IRC is great but I just clicked around the Twisted website for a minute and couldn't find any indication that the twisted IRC channel exists.
![](https://secure.gravatar.com/avatar/61b1aad7d8e6c28832ecdffe8036a5f0.jpg?s=120&d=mm&r=g)
... +1 for some guidelines from a similar policy Note that we're actually discussing one step more extreme. Closing poor questions is common across SE so the issue only affects new OPs. We're talking about a break from community norms... more like Physics deciding to close questions after the first answer is added. We should also consider any unintended side-effects due to Github's default searches behavior ("open"): [image: Inline image 1] Closing PRs will make them less likely to be found by searchers. If every PR has an issue (common for bug fixes, less common for new features), this is less of a problem -- is this something the bot would need to verify/fix? Clayton Daley On Mon, May 23, 2016 at 1:08 PM, Daniel Sank <sank.daniel@gmail.com> wrote:
![](https://secure.gravatar.com/avatar/e1554622707bedd9202884900430b838.jpg?s=120&d=mm&r=g)
On May 23, 2016, at 11:56 AM, Clayton Daley <clayton.daley@gmail.com> wrote:
Closing PRs will make them less likely to be found by searchers. If every PR has an issue (common for bug fixes, less common for new features), this is less of a problem -- is this something the bot would need to verify/fix?
The bot is just for the contributors to be able to re-open PRs to respond to review feedback. When a PR is closed, the reviewers themselves will ask the authors to open an issue separately if that is appropriate. -glyph
![](https://secure.gravatar.com/avatar/a93db92c60e9d5435d204407264457f2.jpg?s=120&d=mm&r=g)
Personally, I find closing PRs that aren't going to be merged "soon" mostly-beneficial. Even if it *might* be perceived as "hostile" by some contributers, a simple explanation should suffice. (And, if simply closing a PR with a nice note explaining, "please re-open when X is fixed/changed" scares away a potential contributer I have my doubts as to whether they would fix X if you *didn't* close it...) There's nothing worse than trolling through open PRs only to find that the last comment is "fix up X, Y, and Z and we'll merge" because then you have to (re-)figure out if X, Y and Z have been done etc. On the flip side, it's nice to know if your PR has problems or not. The other plus of closing is that it's way more obvious when the PR is once again considered ready for merging (even without 'completely baked' workflows like Twisted's) and keeps the "open PRs" list (hopefully!) shorter. -- meejah
![](https://secure.gravatar.com/avatar/469df05f5dfd5b75fb3cb3a0868d36bf.jpg?s=120&d=mm&r=g)
I submitted this PR, which is now closed: https://github.com/twisted/twisted/pull/62 I don't want to re-open that PR, but I am using that as an example As an example, if I wanted to re-open that, how would I go about doing it? I am not an administrator of the Twisted GitHub project, so on that web link, there is no option for me to re-open the PR. Are you suggesting that I would need to -> create a new branch in my repo with new commits -> create a new pull request ? -- Craig On Sun, May 22, 2016 at 9:39 PM, meejah <meejah@meejah.ca> wrote:
![](https://secure.gravatar.com/avatar/3d37232726396a1d3c7412dd915095ea.jpg?s=120&d=mm&r=g)
Mark has been working on a bot which would reopen it with a comment: https://github.com/markrwilliams/txghbot - Amber
![](https://secure.gravatar.com/avatar/e1554622707bedd9202884900430b838.jpg?s=120&d=mm&r=g)
On May 22, 2016, at 10:51 PM, Tristan Seligmann <mithrandi@mithrandi.net> wrote:
Note that even without the bot, I believe you can just create a new PR for the same branch, so it's not *too* bad, but definitely a little clunky.
Many of my comments have had to do why we want this kind of process generally, rather than why specifically closing PRs is the way I'd prefer to go; this is one benefit, which is that even if our bot infrastructure breaks down, there's a clear workaround available to contributors. If resubmitting requires a label change on an existing *open* PR there's not really any way to do that if the bot is temporarily offline. -glyph
![](https://secure.gravatar.com/avatar/e1554622707bedd9202884900430b838.jpg?s=120&d=mm&r=g)
While FreeBSD support is important, there are no CI-as-a-service platforms that support it, that I'm aware of. Right now, we need to manually vet each change before sending it to buildbots, because they are shared mutable environments that we can't afford to have running untrusted code automatically. If we could switch to Travis and Appveyor, then we could let them worry about malicious code, which would allow contributors to get instant feedback, rather than waiting for reviewers to manually run the builders. Travis does support OS X, which means that some level of BSD coverage would still be maintained. And of course if someone could find an equivalent service that supports FreeBSD we could add it to the list of pull request statuses :). -glyph
![](https://secure.gravatar.com/avatar/c194a4d2f2f8269aa052942e87985198.jpg?s=120&d=mm&r=g)
On 22 May 2016 at 02:04, Glyph <glyph@twistedmatrix.com> wrote:
I am not sure that Travis supports Python on OSX.... and it might take some time until there is support for Python on Windows and I am not sure if Travis-ci.org will support this free of charge. I think that we can have both... and in the first instance automatically trigger Travis-CI builds and manually trigger buildbot builds. ---------- GitLab is an option, as I think that they allow you to bring your own build slaves.... but I think that the current buildbots are fine. -- Adi Roiban
![](https://secure.gravatar.com/avatar/e1554622707bedd9202884900430b838.jpg?s=120&d=mm&r=g)
On May 21, 2016, at 11:04 PM, Adi Roiban <adi@roiban.ro> wrote:
I am not sure that Travis supports Python on OSX....
It does. We test on OS X quite extensively on <https://github.com/rackerlabs/mimic>, including a py2app app bundle.
and it might take some time until there is support for Python on Windows and I am not sure if Travis-ci.org <http://travis-ci.org/> will support this free of charge.
Travis-CI doesn't need to support it; you can use both Travis (for OS X / Linux) and Appveyor (for Windows) separately. You can check it out here: http://www.appveyor.com I know some projects are using this quite successfully.
I think that we can have both... and in the first instance automatically trigger Travis-CI builds and manually trigger buildbot builds.
A lot of projects do follow this workflow, and maybe it will be fine for us. The real question is; is FreeBSD support really worth it for the cost to contributors, since that's the only platform we currently support but can't test?
GitLab is an option, as I think that they allow you to bring your own build slaves.... but I think that the current buildbots are fine.
I don't understand.
![](https://secure.gravatar.com/avatar/3d37232726396a1d3c7412dd915095ea.jpg?s=120&d=mm&r=g)
On 22 May 2016, at 14:23, Glyph <glyph@twistedmatrix.com> wrote:
A lot of projects do follow this workflow, and maybe it will be fine for us. The real question is; is FreeBSD support really worth it for the cost to contributors, since that's the only platform we currently support but can't test?
I'm guessing that we have more FreeBSD users than Windows users ;) - Amber
![](https://secure.gravatar.com/avatar/e1554622707bedd9202884900430b838.jpg?s=120&d=mm&r=g)
I realize it can feel like that sometimes, but Google Analytics suggests the large majority of our visitors (45%) are on Windows. By contrast, 0.05% are on FreeBSD. Granted, that's a very high percentage of FreeBSD clients for the Internet at large, but nevertheless, I think your perspective may be slightly statistically skewed. -glyph
![](https://secure.gravatar.com/avatar/e93e18f71a5821a54c233690506bdbf7.jpg?s=120&d=mm&r=g)
I can speak for ~20 scientific research groups who use Twisted via LabRAD's python API <https://github.com/labrad/pylabrad>. A lot of us use or deploy to Windows at least some times. So that's around 200 people you've never heard of who use Twisted on Windows :)
![](https://secure.gravatar.com/avatar/e1554622707bedd9202884900430b838.jpg?s=120&d=mm&r=g)
On May 21, 2016, at 11:42 PM, Daniel Sank <sank.daniel@gmail.com> wrote:
I can speak for ~20 scientific research groups who use Twisted via LabRAD's python API <https://github.com/labrad/pylabrad>. A lot of us use or deploy to Windows at least some times. So that's around 200 people you've never heard of who use Twisted on Windows :)
Wow, you are just knocking it out of the park with useful feedback in this thread :-). Any chance I could cajole you into submitting a success story to success@twistedmatrix.com? -glyph
![](https://secure.gravatar.com/avatar/469df05f5dfd5b75fb3cb3a0868d36bf.jpg?s=120&d=mm&r=g)
On Sat, May 21, 2016 at 11:37 PM, Glyph <glyph@twistedmatrix.com> wrote:
Isn't Google Analytics just telling you what type of OS + web browser is being used to access the twistedmatrix.com web site? That isn't the same as telling you who actually uses Twisted in a project or a piece of code. Unfortunately, FreeBSD isn't well represented in the third party CI systems out there such as Travis. It would be nice if it was, but it isn't. That's a judgment call that the Twisted project needs to make whether to support its own buildbots or not, in order to support configurations not supported by third party CI systems. -- Craig
![](https://secure.gravatar.com/avatar/e1554622707bedd9202884900430b838.jpg?s=120&d=mm&r=g)
Sure, it's not perfect. But even assuming only 1% of Windows desktop users are actually running any of their Twisted code on Windows, and 100% of FreeBSD users are, that's still roughly 9x as many Windows users as FreeBSD. I think that's probably a pretty conservative estimate.
One of the other interesting questions here, beyond "do we want to support it or not" (I think we do want to continue supporting it, in the sense that we want to fix bugs that affect it) is how often FreeBSD breaks; if 99.99% of the time, a change that works on Windows, OS X, and Linux works on FreeBSD, maybe we can still have some FreeBSD CI, but not make it a gating part of the review process. We can run it periodically, before each release, and in the exceedingly unlikely case that FreeBSD broke, we can roll back the change after the fact. -glyph
![](https://secure.gravatar.com/avatar/e1554622707bedd9202884900430b838.jpg?s=120&d=mm&r=g)
The existence of Netflix & WhatsApp doesn't indicate no correlation, just not a perfect correlation :). Although I suppose I'm being unfair there in that some of the Windows users represent actual FreeBSD users as well. Nevertheless, I think Windows consumers are less frequently acculturated to speaking up in open-source land, so if anything we are underestimating their prevalence. Unrelatedly, the existence of such high-profile FreeBSD users is encouraging :). Perhaps this is more important to keep in the critical path than I was assuming. -glyph
![](https://secure.gravatar.com/avatar/174e7b0ff60963f821d0b9a4f1a3ef52.jpg?s=120&d=mm&r=g)
Ah finally a fine bike shedding thread that gets everyone involved. ;)
Right now, we need to manually vet each change before sending it to buildbots, because they are shared mutable environments that we can't afford to have running untrusted code automatically. If we could switch to Travis and Appveyor, then we could let them worry about malicious code, which would allow contributors to get instant feedback, rather than waiting for reviewers to manually run the builders.
I have two points to add: 1. Appveyor is terribly slow and sometimes a bit flaky. I use it for argon2_cffi’s wheels and it drives me bonkers. It should never become an essential part of anything. As a first line of defense it’s fine of course. 2. PyCA has a workflow for Jenkins & GitHub by telling a bot to vet changes. You can see it here in action: https://github.com/pyca/cryptography/pull/2914#issuecomment-220592167 AFAIK that’s been mostly Paul’s work. Aren’t you kind of his boss or something *hint hint*? ;)
![](https://secure.gravatar.com/avatar/e1554622707bedd9202884900430b838.jpg?s=120&d=mm&r=g)
This is a very useful data point. I do not have any concrete experience with it and I was kind of wondering about this.
2. PyCA has a workflow for Jenkins & GitHub by telling a bot to vet changes. You can see it here in action: https://github.com/pyca/cryptography/pull/2914#issuecomment-220592167 AFAIK that’s been mostly Paul’s work. Aren’t you kind of his boss or something *hint hint*? ;)
Thanks for the promotion; I'll be sure to let him know on Monday. However, it's because I know Paul and I know what a complete nightmare it is to set up and maintain infrastructure like that that I was hoping to cheat and get away with it. But what I'm hearing from you in this thread is pretty compelling to me that we are going to need to follow a mostly Cryptography-like workflow after all, asking a bot to run some buildbots. -glyph
![](https://secure.gravatar.com/avatar/a93db92c60e9d5435d204407264457f2.jpg?s=120&d=mm&r=g)
Glyph <glyph@twistedmatrix.com> writes:
This is a very useful data point. I do not have any concrete experience with it and I was kind of wondering about this.
FWIW, Tahoe-LAFS *just* started using AppVeyor too, and I also find it horrifically slow. That said, the Tahoe tests run pretty slowly on a VirtualBox windows VM as well, but not nearly as slowly as AppVeyor. Or, at least that's my impression so far. p.s. If anyone is interested in running a Windows buildbot slave for Tahoe I'm very sure it would be appreciated -- please speak up on the tahoe mailing list :) (my understanding of the setup is that the buildbot slaves don't run random PR code, only post-merged-to-master code). -- meejah
![](https://secure.gravatar.com/avatar/e1554622707bedd9202884900430b838.jpg?s=120&d=mm&r=g)
On May 22, 2016, at 12:15 AM, Hynek Schlawack <hs@ox.cx> wrote:
Ah finally a fine bike shedding thread that gets everyone involved. ;)
OKAY NOW THAT I'VE GOT YOU ALL HERE LET'S TALK ABOUT https://twistedmatrix.com/trac/ticket/288 *slams a metal grating shut over the only exit from the mailing list* -glyph
![](https://secure.gravatar.com/avatar/469df05f5dfd5b75fb3cb3a0868d36bf.jpg?s=120&d=mm&r=g)
On Sat, May 21, 2016 at 6:04 PM, Glyph <glyph@twistedmatrix.com> wrote:
This is quite useful actually. We would need a tool to do this. For example, if I want to build this pr: https://github.com/twisted/twisted/pull/63 Then the tool could poke the buildbots to do something like: git clone https://github.com/twisted/twisted testspace cd testspace git fetch origin pull/62/head:pr/62 git checkout pr/62 [run the tests] Are there enough scripts in the buildbot infrastructure which could be extended to do this? -- Craig
![](https://secure.gravatar.com/avatar/e1554622707bedd9202884900430b838.jpg?s=120&d=mm&r=g)
The only new line would be fetching the test ref. Everything else on the buildbots basically works that way already, just checking out branches. (Please nobody try to do the clever thing where you configure buildbot to automatically pull all PRs, that would effectively negate any security protections...) I've been assuming that in the worst-case scenario, we'd do what Cryptography does and have a bot that polls for special comments and then triggers buildbot in exactly this way. Perhaps I should have made that assumption explicit, I thought it was ticketed somewhere in Braid already. -glyph
![](https://secure.gravatar.com/avatar/b973c2a161019ba60c4f30ad598ced4d.jpg?s=120&d=mm&r=g)
On May 22, 2016 9:36:28 AM GMT+02:00, Glyph <glyph@twistedmatrix.com> wrote:
The Jenkins plugin for GitHub PR triggers has this feature, too. However, it also has a feature to whitelist users and GitHub teams so that PRs/commits can trigger automatically for them. Maybe that's a thing for us, too? -- ralphm
![](https://secure.gravatar.com/avatar/869963bdf99b541c9f0bbfb04b0320f1.jpg?s=120&d=mm&r=g)
On Sun, 22 May 2016 at 10:12 Ralph Meijer <ralphm@ik.nu> wrote:
I don't think we need a whitelist, we can just automatically build branches that are pushed to the twisted/twisted repository. If you can push a branch there, you can also push a change directly to trunk, so you can already execute arbitrary code on the buildbots.
![](https://secure.gravatar.com/avatar/469df05f5dfd5b75fb3cb3a0868d36bf.jpg?s=120&d=mm&r=g)
On Sat, May 21, 2016 at 3:12 PM, Glyph <glyph@twistedmatrix.com> wrote:
Hooray! We're on github now. Next: there's the question of how to deal with pull requests?
A few people, including myself modified the text with Git instructions: https://twistedmatrix.com/trac/wiki/TwistedDevelopment#SubmittingaPatch The basic idea is, "for now, if you submit a GitHub PR, you must file a Trac ticket. You must reference the PR in the Trac ticket, and you must reference the Trac ticket in the PR". If you want to change that workflow somehow, go ahead, but for starters that should be good enough to get going. These two pull requests followed that: https://github.com/twisted/twisted/pull/62 https://github.com/twisted/twisted/pull/63
I don't agree with this. If a PR is reviewed, and the result of the review is "NO WAY", then I am OK with the PR being closed. However, if a the result of the review is "needs more work before being accepted", then it should be possible for the submitter to add more commits to that PR, and even "git rebase" and squash commits in that PR. That's the process that I followed when I submitted https://github.com/twisted/twisted/pull/62 By the way, PR 62 is the first pull request that has been successfully submitted to Twisted, and incorporated into the code: https://github.com/twisted/twisted/commit/95c49d74136eef420fabdeea85f650da2b... It is a rather trivial documentation fix, but I will still take credit as the first Pull Request successful submitter! :) -- Craig
![](https://secure.gravatar.com/avatar/e93e18f71a5821a54c233690506bdbf7.jpg?s=120&d=mm&r=g)
Dear all, While I'm just a lurker, having used Twisted and Github for some time in a moderately sized team I would like to offer a couple comments:
This is a real problem for my team, and I think it happens because we don't respect the value of issues enough. We have two kinds of branches 1) Those which associate to a specific issue. If issue #123 is about fixing the quizzwopper, you can make a branch called 123-fix-quizzwopper. This is a Good Thing. 2) Those which associate to a person. We name these things like u/danielsank/fix-quizzwopper. The point of that originally was to make things more "lightweight" and "dynamic" by allowing folks to make a branch and write some code asap. While this does often work, it also means that we have a mountain of near unidentifiable branches with no obvious place to look for that branch's motivation/history/status/whatever. Type 1 branches have the extra advantage that they're more likely to attract useful input from people at the right time, i.e. before the code is written. It's very frustrating to have a PR representing many hours of someone's work come in only to realize that you want to respond with "Oh for the love of all that is good in this world please don't do that". Discussion in an issue helps avoid that.
Again, agreed.
One thing that pull requests encourage is discussion
It's better for that to happen in issues. I have written code, submitted it as a PR, realized (via feedback and otherwise) that the code is total crap, deleted the branch and closed the PR, and then gone back to the issue to re-evaluate my life choices. This is really useful.
![](https://secure.gravatar.com/avatar/e1554622707bedd9202884900430b838.jpg?s=120&d=mm&r=g)
Sorry, I guess I wasn't clear. I know that PRs are presently a potential alternative to a diff, and that we are still using Trac for ticketing. I want to make it possible to avoid using Trac for ticketing; perhaps switching to github issues entirely. Right now, PRs are still ignored; the thing reviewers are paying attention to is the review queue in Trac, and that is sub-optimal, since it requires new contributors to do something unfamiliar.
Reviewing: This is the potentially slightly odd part. I believe a review that doesn't result in acceptance should close the PR. We need to be careful to always include some text that explains that closing a PR does not mean that the change is rejected, just that the submitter must re-submit. Initially this would just mean opening a new PR, but Mark Williams is working on a bot to re-open pull requests when a submitter posts a "please review" comment: https://github.com/markrwilliams/txghbot <https://github.com/markrwilliams/txghbot>
I don't agree with this. If a PR is reviewed, and the result of the review is "NO WAY", then I am OK with the PR being closed.
I understand that this is your feeling, but do you have any reasoning as to why you believe that this should be the case? "I don't feel like it" isn't selling me.
However, if a the result of the review is "needs more work before being accepted", then it should be possible for the submitter to add more commits to that PR,
Technically speaking, "PRs" are not things that you add commits to. You add commits to branches, and the PR points to a branch. Closing your PR will not - can not, if it's on your own fork - delete your branch.
and even "git rebase" and squash commits in that PR.
Please do not use squash commits. See http://mjg59.dreamwidth.org/42759.html.
First is first, trivial or not :-). -glyph
![](https://secure.gravatar.com/avatar/3d37232726396a1d3c7412dd915095ea.jpg?s=120&d=mm&r=g)
On 22 May 2016, at 14:15, Glyph <glyph@twistedmatrix.com> wrote:
Sorry, I guess I wasn't clear. I know that PRs are presently a potential alternative to a diff, and that we are still using Trac for ticketing. I want to make it possible to avoid using Trac for ticketing; perhaps switching to github issues entirely.
This is an optimistic idea but one that, unfortunately, won't happen yet ;) The things GitHub Issues need to be competitive with Trac as it stands: - Allowing triage by people without write. - Useful search (GitHub search is kind of abysmal) - Assigning to non-committers. Without these things (and quite a few more), it's unlikely that GitHub Issues will be as useful to us. - Amber
![](https://secure.gravatar.com/avatar/e1554622707bedd9202884900430b838.jpg?s=120&d=mm&r=g)
How are things currently "triaged"? Do you mean "review"? If so, I think it would be acceptable to come up with a magic comment for a non-commiter to use to signify that they've fully reviewed a PR. As it stands, we need committers to "accept" a review by deciding to merge, the only difference here is that it would remain in the review queue until they did so, which I think is acceptable (since if the review isn't accepted, it should have remained in the queue anyway). We could also have a bot address this edge-case somehow.
- Useful search (GitHub search is kind of abysmal)
I don't see how Trac's is better.
- Assigning to non-committers.
Honestly I'm not sure that the non-committer assignment part of the workflow is all that useful. I know I hardly ever look at report 7, and I very much doubt any non-committer does :). It's not like we're losing information, either; we still have a record of whose fork the PR points to.
Without these things (and quite a few more), it's unlikely that GitHub Issues will be as useful to us.
I am curious about the "quite a few more". There are things which we really need as a critical part of our workflow (primarily: the review queue) and then there are accidents of the way trac works. Nothing is graven in stone here :). -glyph
![](https://secure.gravatar.com/avatar/3d37232726396a1d3c7412dd915095ea.jpg?s=120&d=mm&r=g)
Creating a ticket, adding it to the relevant milestone (commit required on GitHub), setting the component (which would be a tag on github, requires commit)...
- Useful search (GitHub search is kind of abysmal)
I don't see how Trac's is better.
It has a GUI rather than being stringly typed ;)
- Assigning to non-committers.
Honestly I'm not sure that the non-committer assignment part of the workflow is all that useful. I know I hardly ever look at report 7, and I very much doubt any non-committer does :). It's not like we're losing information, either; we still have a record of whose fork the PR points to.
I look at report 7 :(
Without these things (and quite a few more), it's unlikely that GitHub Issues will be as useful to us.
I am curious about the "quite a few more". There are things which we really need as a critical part of our workflow (primarily: the review queue) and then there are accidents of the way trac works. Nothing is graven in stone here :).
I guess there's a lot of things that are an accident of trac, but the things above are useful.
-glyph
![](https://secure.gravatar.com/avatar/e1554622707bedd9202884900430b838.jpg?s=120&d=mm&r=g)
I don't see us getting a lot of non-committer triage of this kind. And I'm not sure that's really an important part of our workflow - do you really feel that it is? Frankly when non-committers try to put their changes into a milestone or start adding custom keywords, it's almost always wrong. We also don't use "component" for much. If we just got rid of it, would any part of our process change?
Oh, you're talking about "query", i.e. https://twistedmatrix.com/trac/query, not "search", i.e. https://twistedmatrix.com/trac/search ? GitHub's search is actually structured, not stringly typed; there's a bit of GUI here: https://github.com/search/advanced. It does resolve down to a string query, but so does the trac "custom query" eventually (in that it goes into a URL you can copy and paste). But, this is all sort of abstract: what kinds of queries would you do against our ticket database in Trac that are not possible in GitHub's query language? Personally after some exploration of GitHub's query language I have found it's actually more expressive for what I want to do most of the time; particularly querying across projects which is obviously not possible with trac...
That's not a counterexample: you are a committer :-). I do actually have a recurring personal to-do item to check report 7 nowadays, but almost all of my doing-stuff-on-the-tracker has to do with me getting emails about actionable state changes (somebody reviewed your change, you should merge it), rather than scanning that list.
I'm not disputing that, but I'm still a little confused about how exactly they're useful, rather than just different. Can you give some examples of things you do regularly? -glyph
![](https://secure.gravatar.com/avatar/e93e18f71a5821a54c233690506bdbf7.jpg?s=120&d=mm&r=g)
All,
Please do not use squash commits. See http://mjg59.dreamwidth.org/42759.html.
Squashing commits is essential to making useful commit histories. Are you just saying not to use Github's built-in feature which squashes everything into a _single_ commit? If so, note that you can turn that GUI feature off so nobody is tempted to use it. That does leave the question of whether you want people to use merge commits or to rebase their branch on the latest master before merging so that you always get a fast-forward.
![](https://secure.gravatar.com/avatar/3d37232726396a1d3c7412dd915095ea.jpg?s=120&d=mm&r=g)
The thing is that we don't want fast-forward; fast-forward merging creates awful commit histories with no regard to branches, especially for ones like Twisted where (near) every commit on trunk needs to be a deployable one, and where we may need to revert an entire branch. - Amber
![](https://secure.gravatar.com/avatar/e1554622707bedd9202884900430b838.jpg?s=120&d=mm&r=g)
On May 21, 2016, at 11:24 PM, Daniel Sank <sank.daniel@gmail.com> wrote:
Squashing commits is essential to making useful commit histories.
Nope. It's just a handy hack to work around the commonly-used, broken history viewers (like Github's own) that can't correctly present multi-parent commits. If you use SourceTree or bzr qlog or anything that correctly collapses merges, you don't need squashes. Furthermore, if you use squashes, you work around a temporary problem (crummy history viewers - which will probably eventually be fixed) by permanently destroying useful information (the sequence of changes which lead to a larger change). Of course, I can't stop you from doing this in any meaningful sense, you can always delete commits and just create bigger diffs and I won't be able to tell, but I would prefer it if you don't use squash commits or any other kind of history rewriting on Twisted. -glyph
![](https://secure.gravatar.com/avatar/e1554622707bedd9202884900430b838.jpg?s=120&d=mm&r=g)
On May 21, 2016, at 11:24 PM, Daniel Sank <sank.daniel@gmail.com> wrote:
That does leave the question of whether you want people to use merge commits or to rebase their branch on the latest master before merging so that you always get a fast-forward.
Github will never fast-forward, and I never want to see anyone using a client manually push a fast-forward to master, either. Every left-parent commit should be something that was code reviewed. If the changes on master are small, it's fine to just merge master into the branch. But, in the case of larger changes and resurrecting stale, old PRs, rather than merge 1000s of commits off of master, I actually prefer creating a new branch from current master and merging the old branch into it. Rebasing an entire branch on master creates new commits that were never tested by CI, and never present in anyone's working copy. I strongly prefer to avoid this. Rebase is great functionality, but IMHO there is only one correct way to use it: rebase --interactive, carefully vetting each commit to ensure it still says what you want it to. -glyph
![](https://secure.gravatar.com/avatar/3d37232726396a1d3c7412dd915095ea.jpg?s=120&d=mm&r=g)
I think this is reasonable. I believe txghbot is in a place where we can start using it for just that.
The one thing that this workflow is missing from trac is a convenient way for committers, having eyeballed a patch for any obvious malware, to send it to the buildbots.
We could also potentially just replace our buildbot build farm with a combination of appveyor and travis-ci; this would remove FreeBSD from our list of supported platforms, as well as eliminating a diversity of kernel integrations. However, for the stuff that doesn't work in containers (mostly inotify) we could run one builder on non-container-based infrastructure, and for everything else (integrating with different system toolchains) we can test using Docker: https://docs.travis-ci.com/user/docker/. I am very much on the fence about this, since I don't want to move backwards in terms of our test coverage, but this would accelerate the contribution process so much that it's probably worth discussing.
I don't think that this will do us any good. Having travis/appveyor to be a first line of review (giving a quick "builds fail/builds probably pass) so that contributors get an immediate "does this have a chance of being merged" is a good thing; but there is certainly value in having the various different platforms. All we need is a bit more tooling around it (and I think txghbot is a good starting place for that). We also need to look into some extra tooling around tox; mainly around the ratcheting quality checkers -- some form of fetching the latest build from buildbot and downloading it locally to compare, or something.
10 years ago or so, we would routinely discover kernel bugs in our integration test harness and they would be different on different platforms. But today's platform realities are considerably less harsh, since there are a lot more entities in the ecosystem that have taken responsibility for testing that layer of the stack; I couldn't find anything since 2008 or so where we really saw a difference between Fedora and Ubuntu at the kernel level, for example.
The issue in 2016 is not actually the kernel; but OpenSSL and OpenSSH, among other system libraries. Every new version of Fedora has been red on the buildbots for this reason; OpenSSL changed, and we needed to fix our use of it. Worth also noting is that Travis is so horrendously behind in all things Python+Ubuntu (do they even have a current PyPy yet?) that we're not actually testing the platforms people are *using*, which is something I think is valuable that the current system gives us.
Thoughts?
-glyph
- Amber
![](https://secure.gravatar.com/avatar/e1554622707bedd9202884900430b838.jpg?s=120&d=mm&r=g)
On May 21, 2016, at 11:08 PM, Amber Hawkie Brown <hawkowl@atleastfornow.net> wrote:
The issue in 2016 is not actually the kernel; but OpenSSL and OpenSSH, among other system libraries. Every new version of Fedora has been red on the buildbots for this reason; OpenSSL changed, and we needed to fix our use of it. Worth also noting is that Travis is so horrendously behind in all things Python+Ubuntu (do they even have a current PyPy yet?) that we're not actually testing the platforms people are *using*, which is something I think is valuable that the current system gives us.
The reason I specifically mention the kernel is that different userlands can be tested by using Docker in Travis: <https://docs.travis-ci.com/user/docker/>. The travis official images are a little faster, because they cache more stuff, but we can build our own base images to try to offset that somewhat. -glyph
![](https://secure.gravatar.com/avatar/d37c7104c024b78dc451e3a6b733df9d.jpg?s=120&d=mm&r=g)
FWIW, I just noticed that the CONTRIBUTING message wasn't showing up when trying to make a PR. github is supposed to make a warning box that shows the contents of /CONTRIBUTING or /CONTRIBUTING.md when opening an issue or PR
![](https://secure.gravatar.com/avatar/469df05f5dfd5b75fb3cb3a0868d36bf.jpg?s=120&d=mm&r=g)
Hi, Thanks for bringing up these points. See: https://twistedmatrix.com/trac/ticket/8352 Feel free to add any thoughts you may have on how to improve this. -- Craig On Tue, May 24, 2016 at 10:37 AM, Jonathan Vanasco <twisted-python@2xlp.com> wrote:
![](https://secure.gravatar.com/avatar/d37c7104c024b78dc451e3a6b733df9d.jpg?s=120&d=mm&r=g)
just a few thoughts: The current system as-explained seems to use an "Issue" as a queue item that is either a "bug report" or a "notice of a pull request, which may also reference another bug report". that is weird. IMHO, a quality contributor will never get turned off by handling the docs, tests, and other requirements - but may get confused/turned off by this. anyways: 1. github's PRs are peculiar in that they're "branch to branch" and not "branch@commit to branch@commit". If I were to submit a PR, I could still make changes on it between my submission and someone finally doing a merge. Personally, I find this infuriating. In any event, I suggest *requiring* something like the "git flow" model for submissions. Outright rejecting anything that isn't in a dedicated fix/feature branch. Everyone is currently doing the right thing, and there are contribution docs that suggest this – I would just make it a stated policy to automatically reject any PRs from someone using "master/trunk". 2. regarding Issues vs PRs and working between Trac and Github, I suggest you take the approach of "Trac Issue = Idea" and "Github PR = Implementation of Idea". Under this concept, one or more PRs might be attempts to address a given Issue. The core maintainers can address the highlevel concepts and requirements under Issue, while grounds for rejection / feedback on each PR are listed there. As a github/bitbucket submitter, it's really useful when someone looks at a diff and uses inline comments to note their rejection for a section. This is how most people use Github. (and it's incompatible with the current queue system, I KNOW). 3. Initially I didn't like the idea of rejecting so fast, but after thinking it over, now I do. In addition to boilerplate text about "please resubmit", I think it might be beneficial to standardize some labels for this though, as it helps future contributors who may want to address an issue. If the "IDEA" is approved but the implementation is not, then noting "Resubmission Pending" suggests this is still active. If the underlying IDEA or approach is rejected, then noting "Rejection Final" can suggest it's just not going to happen. 4. I understand that "review" hold special significance within the twisted team's workflow, but outside of Twisted it is awkward to see something bounce in and out of "review". At least on the github side, using some sort of label to state where in the process (ie, what type of review) something is under would help. The caveat is that this breaks the current use of Issue as a "queue" item. But Twisted is using "issues" less as an Issue and more as a queue item, while github "Issues" are less of a queue item and amore of an actual issue. This is a bit confusing and different to how many people will use github. Just to illustrate what a typical github contributor flow might be: Scenario A- There is a trac Issue #1001 for "F is broken" bob generates github pr 11, and submits. He comments in #1001 ted generates github pr 12, and submits. He creates a trac issue #1002. When #1002 is first looked at, it is a dupe of 1001 and copied over. carol generates github pr 13 and submits. she comments in #1003 There are 3 PRs for a single Issue. Feedback on the PRs occurs on Github. Feedback on the Ideas and status of PRs is on trac. Scenario B- Bob notes that "X is broken" Bob generates github PR 11 and submits. He goes to trac and creates issue #1002 Bob's PR is rejected because the fundamental approach is not acceptable, however X is still broken. PR11 is closed, #1002 is open. Ted generates pr 12 and submits. he comments in #1002. Ted is rejected but the approach is compatible. it can be implemented with fixes. Carol forks Ted's approach , fixes it, and generates pr 13. she comments in #1002 and it is accepted. again, this breaks the current use of queues. I'm not sure how to reconcile everything together, however I think you're going to find non-core maintainers naturally fall into the 2 scenarios above.
![](https://secure.gravatar.com/avatar/e1554622707bedd9202884900430b838.jpg?s=120&d=mm&r=g)
In the current system, we have "tickets" (in Trac) not "issues" (on Github). And in that system, an issue is put into review when it has an attached branch or patch that needs to be considered for inclusion. In the Github system.
that is weird.
IMHO, a quality contributor will never get turned off by handling the docs, tests, and other requirements - but may get confused/turned off by this.
Literally everyone's predictions about this have always been wrong, so I don't like speculating ;). Especially classifying people as "quality contributors" or not. I will sometimes say something like "if you don't do X, you probably won't do Y" but being a "quality contributor" is a complex, multi-dimensional, and situationally-dependent personality characteristic that I don't think we can predict.
anyways:
1. github's PRs are peculiar in that they're "branch to branch" and not "branch@commit to branch@commit". If I were to submit a PR, I could still make changes on it between my submission and someone finally doing a merge. Personally, I find this infuriating.
I KNOW, RIGHT!!! However, protected statuses somewhat reduce the potential race-condition here. And contributing to a couple dozen Github projects I have to say that practically this has never been an issue, even though I find it aesthetically appalling, design-wise.
In any event, I suggest *requiring* something like the "git flow" model for submissions. Outright rejecting anything that isn't in a dedicated fix/feature branch. Everyone is currently doing the right thing, and there are contribution docs that suggest this – I would just make it a stated policy to automatically reject any PRs from someone using "master/trunk".
Honestly I don't think this is worth the effort. git-flow is, abstractly, a reasonably good idea (if people don't know what we're talking about, see <http://jeffkreeftmeijer.com/2010/why-arent-you-using-git-flow/>) since it encapsulates the precise semantics desired by certain commits. But sometimes people make a fork and their fork branch just happens to be called 'patch-1', or 'master', but this doesn't really affect our workflow as long as they observe proper PR etiquette and don't push any unrelated revisions to that branch in the meanwhile.
2. regarding Issues vs PRs and working between Trac and Github, I suggest you take the approach of "Trac Issue = Idea" and "Github PR = Implementation of Idea". Under this concept, one or more PRs might be attempts to address a given Issue.
That's ... exactly what I was trying to express. (Except I was thinking we'd gradually move from Trac Tickets to Github Issues).
The core maintainers can address the highlevel concepts and requirements under Issue, while grounds for rejection / feedback on each PR are listed there. As a github/bitbucket submitter, it's really useful when someone looks at a diff and uses inline comments to note their rejection for a section. This is how most people use Github. (and it's incompatible with the current queue system, I KNOW).
I don't see how it's incompatible. It seems perfectly fine, as long as we shift from 'review queue == report 25' to 'review queue == open PRs with non-failing status'.
3. Initially I didn't like the idea of rejecting so fast, but after thinking it over, now I do. In addition to boilerplate text about "please resubmit", I think it might be beneficial to standardize some labels for this though, as it helps future contributors who may want to address an issue. If the "IDEA" is approved but the implementation is not, then noting "Resubmission Pending" suggests this is still active. If the underlying IDEA or approach is rejected, then noting "Rejection Final" can suggest it's just not going to happen.
Yeah, adding some informative labels to the closed issues might be a good way of letting contributors know what is going on. I just wanted to make sure that they are an optional part of the workflow which, if the automation around them breaks down in some way, we can still make progress, and we don't end up with inconsistent or lost information.
4. I understand that "review" hold special significance within the twisted team's workflow, but outside of Twisted it is awkward to see something bounce in and out of "review".
I think this is because, generally speaking, most teams do a poor job structuring their communications about what state issues and pull requests are in :).
At least on the github side, using some sort of label to state where in the process (ie, what type of review) something is under would help.
What do you mean it "would help"? What problem would it solve?
The caveat is that this breaks the current use of Issue as a "queue" item. But Twisted is using "issues" less as an Issue and more as a queue item, while github "Issues" are less of a queue item and amore of an actual issue. This is a bit confusing and different to how many people will use github.
I don't know where you're getting this from. Only tickets in review are queue items. I have no idea what "actual issue" means here.
What use? What are you referring to with the plural "queues"? There's only ever been a single queue, the review queue. In both of your scenarios, the list of open PRs is the review queue, and in both of those scenarios, discussion of the abstract idea can happen on the tickets without treating them directly as queue items. I appreciate you taking the time to think about our workflow and the review queue, but I would ask that you carefully consider what problem you're trying to address with your suggestions. The practical issue I'm trying to address by switching to github PRs as opposed to trac tickets with the 'review' keyword is the speed and ease of submitting a change and getting CI feedback on that change, in addition to lowering training overhead since many people are familiar with github. Faster feedback = happier contributors = more maintenance effort productively expended. The issue I'm trying to address with the slightly odd close-a-PR-to-signify-finished-review workflow is to avoid the common problem of reviewers not knowing which things to review and therefore leaving certain contributions mouldering until their submitters lose interest. Beyond that, I don't see that we have substantial workflow issues that need solving. -glyph
![](https://secure.gravatar.com/avatar/d37c7104c024b78dc451e3a6b733df9d.jpg?s=120&d=mm&r=g)
On May 24, 2016, at 6:49 PM, Glyph wrote:
I KNOW, RIGHT!!! However, protected statuses somewhat reduce the potential race-condition here. And contributing to a couple dozen Github projects I have to say that practically this has never been an issue, even though I find it aesthetically appalling, design-wise.
But sometimes people make a fork and their fork branch just happens to be called 'patch-1', or 'master', this doesn't really affect our workflow as long as they observe proper PR etiquette and don't push any unrelated revisions to that branch in the meanwhile.
In terms of "git flow" i didn't mean the exact names, just the concept that every fix has it's own dedicated branch. This is something that almost every github-experienced person does automatically (and all current twisted PRs do). People with less experience on github itself (not a given package) will often just edit the "master" branch and submit. then they'll forget they did that and edit something else... because of that github peculiarity, and that everything is acting "centralized" on github and not under a "decentralized" model where a particular commit was queued that ends up in the review. those types of changes can also end up triggering CI tests, which complicates things further. twisted contributors are likely a bit more experienced and it's a self-selecting pool... i've just seen an increasing number of projects require a dedicated branch and politely reject + ask for anything against 'master' to be resubmitted via a dedicated branch.
I don't see how it's incompatible. It seems perfectly fine, as long as we shift from 'review queue == report 25' to 'review queue == open PRs with non-failing status'.
ah, I did not know that was possible. this ties into me not understanding the content of the queue being PR only (not "all tickets").
At least on the github side, using some sort of label to state where in the process (ie, what type of review) something is under would help.
What do you mean it "would help"? What problem would it solve?
This would help non-maintainers understand what that actual progress was in that 5 stage review.
The caveat is that this breaks the current use of Issue as a "queue" item. But Twisted is using "issues" less as an Issue and more as a queue item, while github "Issues" are less of a queue item and amore of an actual issue. This is a bit confusing and different to how many people will use github.
I don't know where you're getting this from. Only tickets in review are queue items. I have no idea what "actual issue" means here.
Ok. From here to the end I believe I understand my confusion. The "queue" looked to me like it was "All Trac Tickets" not just "Certain types of Trac Tickets". I was looking at a few reports and some raw views that showed track tickets that were both bug reports and attempts to fix a specific problem. It looked like the normal use of github would have run antagonistic to your workflow , but his all seems fine now.
The practical issue I'm trying to address by switching to github PRs as opposed to trac tickets with the 'review' keyword is the speed and ease of submitting a change and getting CI feedback on that change, in addition to lowering training overhead since many people are familiar with github. Faster feedback = happier contributors = more maintenance effort productively expended. The issue I'm trying to address with the slightly odd close-a-PR-to-signify-finished-review workflow is to avoid the common problem of reviewers not knowing which things to review and therefore leaving certain contributions mouldering until their submitters lose interest. Beyond that, I don't see that we have substantial workflow issues that need solving.
I was mostly concerned with the proposed implementation creating workflow issues.
participants (14)
-
Adi Roiban
-
Amber "Hawkie" Brown
-
Clayton Daley
-
Craig Rodrigues
-
Daniel Sank
-
Donald Stufft
-
Glyph
-
Hynek Schlawack
-
Jonathan Vanasco
-
meejah
-
Ralph Meijer
-
Terry Jones
-
Tristan Seligmann
-
Werner Thie