[Twisted-Python] major changes, release engineering, and learning cost

[Laurens Van Houtven]

For clarity: I think Launchpad replacing Trac is a good thing. I realize
that's a huge ordeal. However, I don't think the basic ideas are so
different that it'd be impossible. As discussed on IRC, the main
downside (aka why we can't do it right now) is lack of notifications, so
it's hard to integrate stuff like buildbot yet, but that's being worked
on.

The idea I'm proposing is probably doable without Launchpad, but it's
definitely much harder without bzr. Mixing bzr and svn, might work, but
the developers definitely need to be using bzr because branching really
can't be a pain for it to work.

I have diagrammed the quantum-transmogrifier example that I tried to
explain in the last email.

http://bit.ly/aA20Qs

On Thu, May 27, 2010 at 7:02 AM, Glyph Lefkowitz
<glyph at twistedmatrix.com> wrote:
>
> On May 26, 2010, at 5:41 AM, Laurens Van Houtven wrote:
>
> So, while I can definitely sympathize with a certain animosity towards
> trac, and I can appreciate the goals and sensibilities of launchpad, I
> will probably flat-out veto any required / process-driven usage of
> Launchpad blueprints. Bugs, features, enhancements, etc, are all units
> of work that need to be tracked, and it's better to have one kind of
> crummy interface for tracking _everything_ than three interfaces, even
> three good interfaces, for tracking little bits stuff in different
> ways according to arbitrary distinctions. (As someone recently opined
> to me, Blueprints are a giant complicated interface for pasting the
> URL to a Google Wave into a text field. We might as well skip the text
> field and just link straight to the conversation from a Trac ticket.)

(was it dash? ;-))

I understand your point of view, but I don't think blueprints are that
bad. I'm not saying blueprints aren't fat pointers to URLs, but I just
don't think that would necessarily make them less useful. As far as
arbitrary distinctions go: I'd think new features are blueprints, and
bugs are bugs. It's not very arbitrary in my mind -- which is just a
different way of saying "I can't think of any grey areas". (Yes, this
means there are very few blueprints. I think that's a good thing :))

I think I understand the reasoning behind your opinion from a project
lead/release management/developer perspective: both bugs and blueprints
are jobs that still need to be done, similarly tracked for releases, and
they both take developer time to be resolved. I don't think this
reasoning is wrong.

For both users and developers, I think thinking of bugs and new features
as separate things makes sense. Furthermore, Launchpad has stuff like
milestones and targeted releases, so I don't think the
three-good-interfaces thing is really that prohibitive. Personally, I
don't feel that split is bad for developers either.

(FWIW: yes, I think Launchpad's Whiteboard feature needs extending and
it probably needs a comment system. And once you do that, you might
indeed wonder what the difference with bugs still is -- but I'm not
arguing Launchpad is perfect, I'm arguing it's better than Trac ;-))

Even if blueprints are non-negotiable, I think most of what I said could
just as well be applied to Launchpad bugs: you'd treat Launchpad bugs
like you treat Trac tickets now. Merge proposals and the reviews they
come with are properties of branches in Launchpad, and not of blueprints
or bugs (IIRC). So, feel free to scrap blueprints, it's not that big a
deal :)

> lp:~lvh... isn't a verb. What do you do with that string? :)

Sorry, bad emacs VC mode habit. I meant 'create a branch lp:~lvh/...'

> > and it turns out FTL travel is really really hard so I need two
> > smaller branches lp:~lvh/twisted/quantum-transmogrification-tunnels
> > and lp:~lvh/twisted/quantum-transmogrification-ansible. Both are
> > good, so they get put back into
> > ~lvh/twisted/quantum-transmogrification-ftl-travel.
> >
> > Each review would verify that all children (if any) have also been
> > reviewed. So, the final review is pretty small, as suggested :-)
>
> The review wouldn't verify that the parent had been reviewed, though.
> If you started this process by writing a bunch of code in the q-t-f-t
> branch, *that* code would never have been reviewed; unless q-t-f-t
> needs to be reviewed in its entirety before landing on trunk. Which is
> precisely what I'm trying to avoid.

Yeah, this is sort-of fixed in practice by my point (1) below, but
requires some conscious effort and discipline from the developer. An
alternative idea to just having merges of reviewed branches in q-t,
would be to have the review of q-t be "all of the commits that aren't
reviewed merges from other branches". That sounds really, really
annoying, so I'd rather do it the first way. Specifically, that means
"don't do that, branch early and often, merging is easy but branching
halfway is confusing".

>
> > This does not limit a developer's freedom to branch at will, because
> > code review is opt-in (merge proposal), not opt-out. If you don't do
> > it, that code in that branch isn't covered by a previous review, and
> > must be reviewed later.
>
> This strikes me as placing a pretty nasty burden on the reviewer. The
> reviewer has to figure out if there are any commits that went only to
> the integration branch, isolate them, review them, get that branch
> into an OK-it's-reviewed state, while meanwhile other developers might
> be committing stuff to that branch and changing its contents, both
> from regular working commits and from reviewed merges. It sounds like
> a nightmare. Maybe bzr makes it easier than it sounds, but it sounds
> bad enough that even a big improvement would still be pretty bad ;).

Again, I think point (1) addresses this: yes, but not if you promise to
make branches off your first branch off trunk (wording is a bit off, but
look at the diagram for clarification). That way they only have merges
from other branches, and those merges are reviewed.

As long as you don't do that, and keep your development out of review
branches, there is no problem. That sounds like a very big caveat, but
we have found it to work in practice. I'm not sure why, but one
explanation would be that people sometimes hugely underestimate how much
time something takes to develop, but guesstimates about the complexity
of a particular feature tend to be much more accurate. Even if that goes
awry, there is quite some leeway here: the complexity of a review branch
has to really completely get out of hand before it wouldn't be okay for
it to be one code review anymore -- up to the point that it probably
wouldn't pass review anymore under the old design.

An added bonus is that there is reduced incentive to keep piling on
features in a single review branch, because all of it has to be
reviewable in one go. I think this is a good idea, because it encourages
proper planning and up-front specs of which features you want to
implement. This effect might be stronger in a small, tight development
team such as in a small development house than with a distributed
development team like Twisted (screwing over your reviewer just means
he'll be less friendly to you next time you have to do reviews, and you
still have to work with these people later on), but I'm going to be
optimistic and pretend we're all nice people :-)

> > How exactly code review coverage would work is somewhat of an open
> > question and it's the obvious failure in this system. We use it in
> > production and it turns out to not be a problem, because people
> > always end up doing two things:
>
> Who is "we"? What is "production"? Are you talking about Twisted or a
> hypothetical project which uses Twisted, or a fork of Twisted on
> Launchpad? Is this a hypothetical project or a real proejct? I am
> super confused.

This is a real project that uses (amongst other things) Twisted.


> > 1) always branch at least once from the first branch off trunk (so
> > branch off lp:~lvh/twisted/quantum-transmogrification). Net result:
> > lp:~lvh/twisted/quantum-transmogrification only introduces code in
> > the form of merges.
>
> That's pretty much what I'm proposing, except I don't actually care
> whether they're merges or patches or individual commits, as long as
> they've cycled through code-review properly.

Right, but wouldn't it then be hard for reviewers to know what has been
reviewed and what hasn't?

>
> > 2) always do code review on branches being merged into your first
> > branch off trunk (so everything merged into
> > lp:~lvh/twisted/quantum-transmogrification has to be reviewed
> > already)
>
> And this is what we already do.

Huh? I thought it got reviewed when it was put up for review for
suggested merging into trunk. My suggestion is the same thing, except
s/trunk/q-t/. There's a second review when q-t itself gets merged into
trunk, but as long as those are all merges of reviewed branches, that
review is trivial. See diagram, points 16 and 17.

> > Note that our merges into trunk are automagic.
>
> (Again, who is "we", and by what mechanism are they automated? Are you
> proposing that we do this, or are you stating that some other people
> do?)

The aforementioned project using Twisted. I'd prefer not going into a
lot of detail, maybe we should forget about it for purposes of keeping
the discussing focused.

Perhaps what _can_ be taken home from this for purposes of the
discussion is that this way of organizing branches does actually work
for at least one development team somewhere. I'm not sure to what extent
this carries over to Twisted. All I know is that the distributedness of
Twisted development isn't much of a problem, since I had no issues and I
spent 99% working from home/university.

> > If it's merged into a direct branch off of trunk and it satisfies
> > some qualities (such as full test coverage :)), it gets put into
> > trunk, and that gets pushed to production servers. No human
> > interaction. Scary at first, but then you realize humans were
> > already involved in the QC process extensively at every point --
> > doing it this way just makes them take testing more seriously :)
>
> Human interaction of some kind should definitely be required for
> Twisted. This is not just pushing some new widget to a web site; this
> is potentially pushing out new APIs that need to be documented and
> supported to a whole ton of developers. The whole point of the process
> modification I've proposed is to make sure that features arrive in
> releases as coherent, comprehensible whole pieces, not to allow things
> we can automatically verify (like docstring and test coverage) to be
> deferred to later merges. These properties of the code should still be
> verified on every merge to the integration branch; the interesting
> thing about the merge to trunk is the verification that the unit is a
> coherent whole (and in the case of a deprecation / replacement, that
> the replacement is a functionally adequate upgrade).

Yeah, I can see that. My point is not so much an argument for
implementing automatic merging into trunk for Twisted, but mostly that
this method, when properly implemented, results in a very high
confidence level of the quality of your implementation branches, up to
the point where people have successfully automated it :)

As far as the coherent, comprehensible releases, that's one of the
reasons I like Launchpad's milestones, series, blueprints... You want to
do all of that and I think it's a great idea, and I think that they're
good tools for making all of the specifics of that intent (coherent,
comprehensible releases) more transparent to the outside world.


> Thanks,
>
> -glyph

Thanks,
Laurens