[Twisted-Python] Re: [Twisted-commits] r16287 - Revert r16241; tests are passing.
On Mon, 13 Mar 2006 12:33:30 -0700, Wilfredo Sanchez <wsanchez@wolfwood.twistedmatrix.com> wrote:
Author: wsanchez Date: Mon Mar 13 12:33:27 2006 New Revision: 16287
Modified: trunk/twisted/web2/dav/acl.py trunk/twisted/web2/dav/element/base.py trunk/twisted/web2/dav/element/rfc3744.py trunk/twisted/web2/dav/http.py trunk/twisted/web2/dav/idav.py trunk/twisted/web2/dav/method/propfind.py trunk/twisted/web2/dav/resource.py trunk/twisted/web2/dav/static.py Log: Revert r16241; tests are passing.
This does not actually appear to be the case. The quick build which ran as a result of this commit had these results: Running 2 tests. twisted.web2.dav.test.test_prop.PROP.test_PROPFIND ... [OK] twisted.web2.dav.test.test_prop.PROP.test_PROPFIND ... [ERROR] =============================================================================== [ERROR]: twisted.web2.dav.test.test_prop.PROP.test_PROPFIND File "/home/buildbot/BuildBot/slave/quick/Twisted/twisted/web2/dav/http.py", line 287, in statusForFailure failure.raiseException() File "/home/buildbot/BuildBot/slave/quick/Twisted/twisted/web2/dav/method/propfind.py", line 119, in gotXML properties_by_status[responsecode.OK].append(resource.readProperty(property, request)) File "/home/buildbot/BuildBot/slave/quick/Twisted/twisted/web2/dav/resource.py", line 129, in readProperty return davxml.GETContentLength(self.contentLength()) File "/home/buildbot/BuildBot/slave/quick/Twisted/twisted/web2/dav/element/base.py", line 107, in __init__ assert isinstance(child, (WebDAVElement, PCDATAElement)), "Not an element: %r" % (child,) exceptions.AssertionError: Not an element: 2717L ------------------------------------------------------------------------------- Ran 2 tests in 0.371s FAILED (errors=1, successes=1) This is slightly challenging to reproduce due to the fact that "trial -u twisted.web2.dav" crashes horribly after the first run. Jean-Paul
On Tue, 14 Mar 2006 11:14:18 -0500, Jean-Paul Calderone <exarkun@divmod.com> wrote:
On Mon, 13 Mar 2006 12:33:30 -0700, Wilfredo Sanchez <wsanchez@wolfwood.twistedmatrix.com> wrote:
[snip] Log: Revert r16241; tests are passing.
This does not actually appear to be the case.
Okay, after a couple hours of trying to track down the problem here, I noticed that you fixed this bug with the very next commit. This has caused me to realize that I don't think use of branches for all development on Twisted should be optional. Unless anyone can present a compelling case to the contrary, I think that as of now we should "officially" switch to the development procedures described by <http://divmod.org/trac/wiki/BranchBasedDevelopment>. Jean-Paul
On Tue, 2006-03-14 at 11:31 -0500, Jean-Paul Calderone wrote:
Unless anyone can present a compelling case to the contrary, I think that as of now we should "officially" switch to the development procedures described by <http://divmod.org/trac/wiki/BranchBasedDevelopment>.
We should probably use Combinator, so people have an easy time with this. 1. Is Combinator documented? 2. Maybe Combinator should be in Twisted trunk, if it's a hard dependency for development?
On Tue, 14 Mar 2006 11:41:55 -0500, Itamar Shtull-Trauring <itamar@itamarst.org> wrote:
On Tue, 2006-03-14 at 11:31 -0500, Jean-Paul Calderone wrote:
Unless anyone can present a compelling case to the contrary, I think that as of now we should "officially" switch to the development procedures described by <http://divmod.org/trac/wiki/BranchBasedDevelopment>.
We should probably use Combinator, so people have an easy time with this.
True enough.
1. Is Combinator documented?
Not the best doc in the world (I think it actually describes the configuration that non-Divmod Twisted devs might want to use, i.e. without the rest of the Divmod code) but it gets the general idea across: http://divmod.org/trac/wiki/CombinatorTutorial
2. Maybe Combinator should be in Twisted trunk, if it's a hard dependency for development?
There used to be problems with putting Combinator into the same sys.path entry as code it's managing - I'm not sure if they're all fixed. It might work, but one of the reasons I'd held off making this pronouncement so long is that it would be handy to reorganize Twisted trunk first. If someone wants to make a branch to move Combinator into Twisted though, I will review it :).
Jean-Paul Calderone:
Unless anyone can present a compelling case to the contrary, I think that as of now we should "officially" switch to the development procedures described by <http://divmod.org/trac/wiki/BranchBasedDevelopment>.
Itamar Shtull-Trauring:
We should probably use Combinator, so people have an easy time with this.
glyph@divmod.com wrote:
True enough.
I like Combinator, and would like to use it to hack on Twisted, in addition to hacking with it. mkbranch seems to need write privileges to the SVN repo, though. Is there a way around this, or do I just create a fake local branch by copying the trunk in my local copy? Actually, it would be better to have the branch up there in the repo, so that the ongoing work is visible. Is there a way to get write access to the branches and sandbox trees of the Twisted repo, without having it in the tags and trunk trees? -- Nicola Larosa - http://www.tekNico.net/ We are not seats or eyeballs or end users or consumers. We are human beings - and our reach exceeds your grasp: deal with it. -- Chris Locke, Cluetrain
On 3/27/06, Nicola Larosa <nico@teknico.net> wrote:
I like Combinator, and would like to use it to hack on Twisted, in addition to hacking with it. mkbranch seems to need write privileges to the SVN repo, though. Is there a way around this, or do I just create a fake local branch by copying the trunk in my local copy?
This is a limitation of SVN, not Combinator. There's no way to write branches to the server without having write access to the server. Actually, it would be better to have the branch up there in the repo, so
that the ongoing work is visible. Is there a way to get write access to the branches and sandbox trees of the Twisted repo, without having it in the tags and trunk trees?
We don't have an actual policy yet on what it takes to get write access restricted to sandbox, and branches. We have given access to people on the agreement that they only keep their stuff in /branches or /sandbox for a while and someone has to review before merging, but this is exactly the process we've switched to for everyone now, anyway. It'd still be nice to get some access restrictions in place, though. But really, the suggested workflow for offline Twisted hacking is this: Hack on your checkout of trunk, do a "svn diff > my_patch", file a bug on the tracker, and attach the patch. Do this successfully a few times, and real commit access is imminent. -- Christopher Armstrong International Man of Twistery http://radix.twistedmatrix.com/ http://twistedmatrix.com/ http://canonical.com/
On Mon, 27 Mar 2006 08:48:23 -0500, Christopher Armstrong <radeex@gmail.com> wrote:
On 3/27/06, Nicola Larosa <nico@teknico.net> wrote:
Actually, it would be better to have the branch up there in the repo, so
that the ongoing work is visible. Is there a way to get write access to the branches and sandbox trees of the Twisted repo, without having it in the tags and trunk trees?
But really, the suggested workflow for offline Twisted hacking is this: Hack on your checkout of trunk, do a "svn diff > my_patch", file a bug on the tracker, and attach the patch. Do this successfully a few times, and real commit access is imminent.
It's worth noting that since we have a real review process now, and everything has to go through it (instead of just submitted patches) it's more likely that bugs filed in this manner will be addressed promptly. Make sure to attach the 'review' keyword when you submit a patch this way.
On Mon, Mar 27, 2006, Christopher Armstrong wrote:
We don't have an actual policy yet on what it takes to get write access restricted to sandbox, and branches. We have given access to people on the agreement that they only keep their stuff in /branches or /sandbox for a while and someone has to review before merging, but this is exactly the process we've switched to for everyone now, anyway. It'd still be nice to get some access restrictions in place, though.
Is it the case that this should be the default process for everyone and that something special needs to be done for merging? Andrew might have already mentioned, but I know that for his project you need to tag things in your commit message as "r=[reviewer's IRC name]" (or [trivial]) and that the merging software actually checks this. We could do something similar, or possibly even stricter where most "Twisted committers" do not in fact actually have merge capability, only reviewers do. It depends whether our reviewers are going to be pretty much all of us who already have access, or some trusted subset. -Mary -- <skreech> I'm sorry. I forgot that in #twisted, all suggestions are taken seriously.
On 3/27/06, Mary Gardiner <mary-twisted@puzzling.org> wrote:
We could do something similar, or possibly even stricter where most "Twisted committers" do not in fact actually have merge capability, only reviewers do. It depends whether our reviewers are going to be pretty much all of us who already have access, or some trusted subset.
No, I think that's a big difference between launchpad's model and Twisted's/Divmod's model. For example, Divmod sometimes has people who aren't even divmod committers reviewing branches. At the very least, any committer can review a branch. -- Christopher Armstrong International Man of Twistery http://radix.twistedmatrix.com/ http://twistedmatrix.com/ http://canonical.com/
On Mon, 2006-03-27 at 16:38 -0500, Christopher Armstrong wrote:
On 3/27/06, Mary Gardiner <mary-twisted@puzzling.org> wrote: We could do something similar, or possibly even stricter where most "Twisted committers" do not in fact actually have merge capability, only reviewers do. It depends whether our reviewers are going to be pretty much all of us who already have access, or some trusted subset.
No, I think that's a big difference between launchpad's model and Twisted's/Divmod's model. For example, Divmod sometimes has people who aren't even divmod committers reviewing branches. At the very least, any committer can review a branch.
I think you're conflating reviewers with people who do merges/commits to trunk?
On Mon, Mar 27, 2006, Itamar Shtull-Trauring wrote:
I think you're conflating reviewers with people who do merges/commits to trunk?
In the Launchpad model, the reviewers are a small group of people (a proper subset of committers, actually), and anyone can merge to trunk (or more specifically, since they use Bazaar, instruct a bot to merge to trunk) as long as they present a "has been reviewed" token. But in some alternative model, we could potentially have a really large group of (potential) reviewers and a small group of people who actually execute merges. Is there a reason to do that though? -Mary -- <Tv> thomasvs: why not just talk some authentication protocol to the other host? <thomasvs> Tv: why use something arcane and difficult when I have THE POWER OF TWISTED ?
On 3/27/06, Itamar Shtull-Trauring <itamar@itamarst.org> wrote:
On Mon, 2006-03-27 at 16:38 -0500, Christopher Armstrong wrote:
On 3/27/06, Mary Gardiner <mary-twisted@puzzling.org> wrote: We could do something similar, or possibly even stricter where most "Twisted committers" do not in fact actually have merge capability, only reviewers do. It depends whether our reviewers are going to be pretty much all of us who already have access, or some trusted subset.
No, I think that's a big difference between launchpad's model and Twisted's/Divmod's model. For example, Divmod sometimes has people who aren't even divmod committers reviewing branches. At the very least, any committer can review a branch.
I think you're conflating reviewers with people who do merges/commits to trunk?
Mary said "It depends whether our reviewers are going to be pretty much all of us who already have access, or some trusted subset.". Reviewers in Launchpad are a trusted subset. They're not a trusted subset in Twisted. I guess the Launchpad Reviewers are *also* those who merge (is this true?), but the difference remains. -- Christopher Armstrong International Man of Twistery http://radix.twistedmatrix.com/ http://twistedmatrix.com/ http://canonical.com/
On Mon, Mar 27, 2006, Christopher Armstrong wrote:
I guess the Launchpad Reviewers are *also* those who merge (is this true?), but the difference remains.
No, all committers are also mergers, it's just that they need to say "approved by a reviewer!" when merging. We don't want all committers to be mergers, it seems. -Mary -- <strib> Sorry, I'm just in the middle of a paradigm warp right now. <dash> strib: welcome to twisted
On Tue, 28 Mar 2006 08:32:14 +1100, Mary Gardiner <mary-twisted@puzzling.org> wrote:
Is it the case that this should be the default process for everyone and that something special needs to be done for merging? Andrew might have already mentioned, but I know that for his project you need to tag things in your commit message as "r=[reviewer's IRC name]" (or [trivial]) and that the merging software actually checks this.
I could swear I wrote this down somewhere, but I can't find it now. The format of commit messages to trunk is supposed to be something like this: <one-line summary> Author: <author> Reviewer: <reviewer> Fixes #<ticket number> <long description of changes, at least 2 full sentences, mentioning anything user-visible>
On Mon, 27 Mar 2006 17:31:09 -0500, glyph@divmod.com wrote:
On Tue, 28 Mar 2006 08:32:14 +1100, Mary Gardiner <mary- twisted@puzzling.org> wrote:
Is it the case that this should be the default process for everyone and that something special needs to be done for merging? Andrew might have already mentioned, but I know that for his project you need to tag things in your commit message as "r=[reviewer's IRC name]" (or [trivial]) and that the merging software actually checks this.
I could swear I wrote this down somewhere, but I can't find it now.
It's partially documented under the _Generate a Meaningful Changelog_ section at <http://divmod.org/trac/wiki/BranchBasedDevelopment>.
The format of commit messages to trunk is supposed to be something like this:
<one-line summary>
Author: <author>
Reviewer: <reviewer>
Fixes #<ticket number>
<long description of changes, at least 2 full sentences, mentioning anything user-visible>
Jean-Paul
On Mon, 27 Mar 2006 12:19:01 +0200, Nicola Larosa <nico@teknico.net> wrote:
Jean-Paul Calderone:
Unless anyone can present a compelling case to the contrary, I think that as of now we should "officially" switch to the development procedures described by <http://divmod.org/trac/wiki/BranchBasedDevelopment>.
Itamar Shtull-Trauring:
We should probably use Combinator, so people have an easy time with this.
glyph@divmod.com wrote:
True enough.
I like Combinator, and would like to use it to hack on Twisted, in addition to hacking with it. mkbranch seems to need write privileges to the SVN repo, though. Is there a way around this, or do I just create a fake local branch by copying the trunk in my local copy?
You can create a fake local branch, except it will only work for 'chbranch', not 'unbranch', because SVN's merge functionality needs to talk to the server.
On Tue, 14 Mar 2006 11:31:19 -0500, Jean-Paul Calderone <exarkun@divmod.com> wrote:
Okay, after a couple hours of trying to track down the problem here, I noticed that you fixed this bug with the very next commit. This has caused me to realize that I don't think use of branches for all development on Twisted should be optional.
I assume that you meant "should *not* be optional".
Unless anyone can present a compelling case to the contrary, I think that as of now we should "officially" switch to the development procedures described by <http://divmod.org/trac/wiki/BranchBasedDevelopment>.
As the resident malevolent power here, I have performed the appropriate ritual to convert our policy. We have officially switched. Now, anyone who checks code into trunk without a branch or a review will be cursed, yea, unto the seventh generation! Documentation to this effect on the website or in the repository should show up within the next few days. There will be a few days while we make sure that everyone understands the ramifications of this new policy, but in the near future, code that is checked in without having its review documented on a ticket may be reverted immediately even if it does not cause any tests to fail. There may be other consequences as well, such as loss of commit access. It's also worth noting that if code fails the buildbot after review (or lacks adequate tests and thereby breaks critical existing functionality), it's the reviewer's fault, not the author's! Therefore, it is suggested that you make heavy use of buildbot's "build this branch" feature before OKing a merge.
* glyph@divmod.com <glyph@divmod.com> [2006-03-14 11:47:26 -0500]:
caused me to realize that I don't think use of branches for all ^^^^^ development on Twisted should be optional.
I assume that you meant "should *not* be optional".
Probably not. -- mithrandi, i Ainil en-Balandor, a faer Ambar
glyph@divmod.com wrote: [...]
It's also worth noting that if code fails the buildbot after review (or lacks adequate tests and thereby breaks critical existing functionality), it's the reviewer's fault, not the author's! Therefore, it is suggested that you make heavy use of buildbot's "build this branch" feature before OKing a merge.
Or, more likely, is a pre-existing intermittent failure. :P -Andrew.
On Fri, 24 Mar 2006 05:29:08 +1100, Andrew Bennetts <andrew-twisted@puzzling.org> wrote:
glyph@divmod.com wrote: [...]
It's also worth noting that if code fails the buildbot after review (or lacks adequate tests and thereby breaks critical existing functionality), it's the reviewer's fault, not the author's! Therefore, it is suggested that you make heavy use of buildbot's "build this branch" feature before OKing a merge.
Or, more likely, is a pre-existing intermittent failure. :P
Everyone who commits to Twisted should be familiar with the intermittent test failures present in the suite. For anyone who isn't, I recommend you spend some time looking at the buildbot waterfall before making any further commits. Jean-Paul
On 3/24/06, Jean-Paul Calderone <exarkun@divmod.com> wrote:
On Fri, 24 Mar 2006 05:29:08 +1100, Andrew Bennetts <andrew-twisted@puzzling.org> wrote:
glyph@divmod.com wrote: [...]
It's also worth noting that if code fails the buildbot after review (or lacks adequate tests and thereby breaks critical existing functionality), it's the reviewer's fault, not the author's! Therefore, it is suggested that you make heavy use of buildbot's "build this branch" feature before OKing a merge.
Or, more likely, is a pre-existing intermittent failure. :P
Everyone who commits to Twisted should be familiar with the intermittent test failures present in the suite. For anyone who isn't, I recommend you spend some time looking at the buildbot waterfall before making any further commits.
That's too much work. Sorry.
On Thu, 23 Mar 2006 13:39:07 -0500, Jean-Paul Calderone <exarkun@divmod.com> wrote:
On Fri, 24 Mar 2006 05:29:08 +1100, Andrew Bennetts <andrew- twisted@puzzling.org> wrote:
glyph@divmod.com wrote:
Everyone who commits to Twisted should be familiar with the intermittent test failures present in the suite. For anyone who isn't, I recommend you spend some time looking at the buildbot waterfall before making any further commits.
I think we should probably start flagging intermittently-failing tests as 'todo' or something. Having them periodically mess up the buildbot display makes the red- or green-ness of the page misleading.
glyph@divmod.com wrote: [...]
I think we should probably start flagging intermittently-failing tests as 'todo' or something. Having them periodically mess up the buildbot display makes the red- or green-ness of the page misleading.
Something like this sounds like a very good idea. -Andrew.
[..]
Unless anyone can present a compelling case to the contrary, I think that as of now we should "officially" switch to the development procedures described by <http://divmod.org/trac/wiki/BranchBasedDevelopment>.
I've been trying this out. Not sure how the review process works from the moment you add the 'review' keyword, though. -- Groetjes, ralphm
On Tue, 14 Mar 2006 17:49:48 +0100, Ralph Meijer <twisted@ralphm.ik.nu> wrote:
[..]
Unless anyone can present a compelling case to the contrary, I think that as of now we should "officially" switch to the development procedures described by <http://divmod.org/trac/wiki/BranchBasedDevelopment>.
I've been trying this out. Not sure how the review process works from the moment you add the 'review' keyword, though.
1. Yell on IRC for a reviewer. If someone responds, assign it to them. If nobody responds, assign it to someone at random. 2. Set the priority to "highest". 3. The reviewer accepts the ticket. 4. The reviewer reviews, adds a comment. (The comment should always include at least one positive and one negative comment about the branch, as well as an indication of whether it should be merged.) 5. If it's mergeable, and you're a committer, the reviewer then reassigns it to you for merging (for more accurate stat tracking). If you're not a committer, the reviewer just merges it. If it needs work, it's reassigned to you regardless. 6. Repeat as desired. If you can't get past step 3, feel free to keep reassigning until you find someone who is responsive enough to accept / review it. This does also assume that people will check their "my tickets" page on a regular basis, but that's just a good habit.
On Tue, Mar 14, 2006, glyph@divmod.com wrote:
1. Yell on IRC for a reviewer. If someone responds, assign it to them. If nobody responds, assign it to someone at random.
Documentation can work a similar way, except just assign it to me. -Mary -- <moshez> jml: but euphemisms for sex are common in all languages :) <exarkun> moshez: what about lojban? <jml> exarkun: there's no record of any lojban speakers having sex. :)
On 3/15/06, Jean-Paul Calderone <exarkun@divmod.com> wrote:
Unless anyone can present a compelling case to the contrary, I think that as of now we should "officially" switch to the development procedures described by <http://divmod.org/trac/wiki/BranchBasedDevelopment>.
I doubt this is a compelling case, but here we go. I find that strict branch-based development greatly decreases the likelihood that I will fix more than one bug in a day. This means that its likely that I'll max out at about one bug a week. I presume that the way to fix many different bugs is to branch from trunk, fix first bug then wait for review. While waiting, branch again from trunk, fix second bug then wait for review and so on. If there is a better way, let me know. To do this requires a context switching that is difficult for me. I find it unpleasant to work on code which has a bug in it that I've already fixed. The bigger the changes, the more difficult it becomes. I'm not opposing branch-based development. This is my sole problem with it. As a side point, if we have switched to strict branch-based development, then we should document the procedure on the Twisted web site, not on the Divmod web site. jml
On Thu, 30 Mar 2006 22:20:12 +1000, Jonathan Lange <jml@mumak.net> wrote:
I presume that the way to fix many different bugs is to branch from trunk, fix first bug then wait for review. While waiting, branch again from trunk, fix second bug then wait for review and so on. If there is a better way, let me know.
It is perfectly acceptable to resolve multiple tickets with one review cycle. If you are going to work for only a few hours, and close multiple tickets, the tickets are probably not big enough to each require their own branch, or their own individual review. Feel free to reassign and resolve en-masse. The only reason this practice is not already documented (and more strongly recommended) is that it isn't good to mix multiple tickets together in one branch or one patch if the reviewer is likely to agree with the code for resolving some tickets, but not others. In some cases, this is unavoidable, because the same code needs to change for two issues; in still others, if the reviewer is unlikely to require major changes, and will be able to review all the changes in a reasonable amount of time, there's no particular reason not to bundle them together -- and convenience of limited-time development is a good reason *to* bundle them together. Still - remember to write a descriptive commit message for each ticket being resolved when you merge to trunk :).
As a side point, if we have switched to strict branch-based development, then we should document the procedure on the Twisted web site, not on the Divmod web site.
At the very least, there should be a link on the Twisted website, on a page explaining how we do development. I think that "branch-based development" is a crummy name for this process, since it isn't very exciting and it's really about tickets, reviews, and changesets, not branches -- branches are an implementation detail to allow developers to continue to use the VCS as a file server. I think I am going to change the name to "ultimate quality development system" and then we can abbreviate it "UQDS".
On 3/30/06, glyph@divmod.com <glyph@divmod.com> wrote:
At the very least, there should be a link on the Twisted website, on a page explaining how we do development.
It's been there since a few days ago, FYI: http://twistedmatrix.com/trac/wiki/TwistedDevelopment -- Christopher Armstrong International Man of Twistery http://radix.twistedmatrix.com/ http://twistedmatrix.com/ http://canonical.com/
Glyph wrote:
I think that "branch-based development" is a crummy name for this process, since it isn't very exciting and it's really about tickets, reviews, and changesets, not branches -- branches are an implementation detail to allow developers to continue to use the VCS as a file server.
We're considering using a process inspired by yours at Allmydata, but we don't call it "branch-based", since the word "branch" means something else in our workflow. Also we use darcs instead of SVN, and there is no particular sense in which "branches" are important to our version of this process.
I think I am going to change the name to "ultimate quality development system" and then we can abbreviate it "UQDS".
Okay. Regards, Zooko
participants (11)
-
Andrew Bennetts -
Christopher Armstrong -
glyph＠divmod.com -
Itamar Shtull-Trauring -
Jean-Paul Calderone -
Jonathan Lange -
Mary Gardiner -
Nicola Larosa -
Ralph Meijer -
Tristan Seligmann -
zooko＠zooko.com