Re: development process

[Janek Warchoł]

I've just went through the patch upload process (for proposed Code of
Conduct https://codereview.appspot.com/575620043/) and I agree that it
should be changed. It was obscure and confusing even for me.

cheers,
Janek

wt., 4 lut 2020 o 22:57 Han-Wen Nienhuys <address@hidden> napisał(a):

> As promised in several code reviews, here my take on the current
> development process. I wrote it as a google doc first, so you can also go
> here
>
> https://docs.google.com/document/d/1BSffzjiQKMTTmr988ezMbsJyfwT9S-rxGRbYSBTv3PM/edit
> for
> inline comments.
>
>
> Context:
>
>    -
>
>    https://codereview.appspot.com/561390043/#msg32
>    -
>
>
>
> https://docs.google.com/document/d/1zYwygWEKJt21rl-bKL41IpjFmiBjlfob6eS63jd-62I/edit#
>
>
> My experiences with the current Lilypond development process.
>
> For context, I have a busy daytime job. I work 80% so I can set aside a
> couple of hours of concentrated hacking time on Friday. When I am in my
> element, I can easily churn out a dozen commits in a day. Those often touch
> the same files, because a fix often needs a preparatory cleanup (“dependent
> changes”).
>
> My annoyances so far are especially with the review/countdown process :
>
>
>    -
>
>    Rietveld + git-cl doesn’t support dependent changes. So to make do, I
>    explode my series of changes in individual changes to be reviewed (I
>    currently have 34 branches each with a different commit so git-cl can
> match
>    up branch names and reviews). For dependent changes, I have to shepherd
> the
>    base change through review, wait for it to be submitted, and then rebase
>    the next change in the series on origin/master.
>    -
>
>    Because the review happens on the split-out patches, I now switch back
>    and forth between 34 different versions of LilyPond. Every time I
> address a
>    review comment, I go through a lengthy recompile. The large number of
>    changes clogs up my git branch view.
>    -
>
>    The countdown introduces an extra delay of 2 days in this already
>    cumbersome process.
>    -
>
>    The review process leaves changes in an unclear state. If Werner says
>    LGTM, but then Dan and David have complaints, do I have to address the
>    comments, or is change already OK to go in?
>    -
>
>    We track the status of each review in a different system (the bug
>    tracker), but the two aren’t linked in an obvious way: I can’t navigate
>    from the review to the tracker, for example. Some things (eg. LGTM) are
> to
>    be done in the review tool, but some other things should be in the
>    bugtracker.
>    -
>
>    Using the bug tracker to track reviews is new to me. It is common for a
>    bug to need multiple changes to be fixed. It also adds another hurdle to
>    new developers (setting a sourceforge account and getting that added to
> the
>    project).
>    -
>
>    I have to keep track of my own dashboard: once changes are pushed, I
>    have to look them up in Rietveld to click the ‘close’ button.
>    -
>
>    Rietveld and my local commits are not linked. If I change my commits, I
>    update my commit message. I have to copy those changes out to Rietveld
> by
>    hand, and it took me quite a while to find the right button (“edit
> issue”,
>    somewhere in the corner).
>
>
> Some of my complaints come from having to manage a plethora of changes, but
> I suspect the process will trip new contributors up too, to note:
>
>
>    -
>
>    Seeing your code submitted to a project is what makes coders tick. It is
>    the Dopamine hit Facebook exploits so well, and so should we. The key to
>    exploiting it is instant gratification, so we should get rid of
> artificial
>    delays
>    -
>
>    We use “we’ll push if there are no complaints” for contributions. I
>    think this is harmful to contributors, because it doesn’t give
> contributors
>    a clear feedback mechanism if they should continue down a path. It is
>    harmful to the project, because we can end up adopting code that we
> don’t
>    understand.
>    -
>
>    The whole constellation of tools and processes (bug tracker for managing
>    patch review, patch testing that seems to involve humans, Rietveld for
>    patches, countdown, then push to staging) is odd and different from
>    everything else. For contributing, you need to be very passionate about
>    music typography, because otherwise, you won’t have the energy to
> invest in
>    how the process works.
>
>
> David uses a patch <https://debbugs.gnu.org/cgi/bugreport.cgi?bug=17474>
> he
> made to GUILE as an example that code can be stuck in a limbo. I disagree
> with this assessment. To me it shows the GUILE community considering and
> then rejecting a patch (comment #26 and #40). I imagine that David was not
> happy about this particular decision, but I see a process working as
> expected. If anything, it took too long and was not explicit enough in
> rejecting the patch. Also, in cases like these, one would normally build
> consensus about the plan before going off and writing a patch.
>
> David suggests that I like getting patches in by fiat/countdown, but I
> disagree. If you look at the past 2 weeks, you can see that I have actually
> tried to address all code review comments so far, and again, it is more
> important for the project to have explicit consensus than that individual
> contributors that go off in possibly conflicting directions.
>
> Here is what a development process should look to me
>
>
>    -
>
>    Uncontroversial changes can be submitted immediately after a maintainer
>    has LGTM’d it, and automated tests pass. Merging such a change should
> be an
>    effortless single click, and should not involve mucking with the git
>    command line. Because submitting requires no effort, we won’t have
> patches
>    stuck because we’re too lazy to do the work of merging them.
>    -
>
>    There is a central place where I can see all my outstanding code
>    reviews, my own, but also from other people. This means that I can do
>    reviews if I have some time left.
>    -
>
>    The review tool supports chains of commits natively.
>    -
>
>    The review tool supports painless reverts. When it is easy to fix up
>    mistakes, contributors will feel less afraid to make changes.
>    -
>
>    Right now, results from build/test are available during review. This is
>    a good thing, and we should keep it.
>    -
>
>    There is no “lack of feedback means it goes in”. By accepting a code
>    contribution we implicitly take on the duty to maintain it and fix bugs
> in
>    it. If no maintainer can be convinced a piece of code is worth taking it
>    on, then that is a long-term maintenance risk, and it should be
> rejected.
>    -
>
>    Coordinated, larger efforts across the code base should start out with a
>    discussion. The mailing list could work here, but I find discussion in
> an
>    issue tracker is often easier to manage, because it is easier to search,
>    and by its nature, discussion in an issue tracker drifts less off-topic.
>    -
>
>    We have a patch meister whose job it is to hound the project maintainer
>    to look at patches that don’t find traction or otherwise.
>
>
>
> --
> Han-Wen Nienhuys - address@hidden - http://www.xs4all.nl/~hanwen
>