Re: development process

[Han-Wen Nienhuys]

>> We don't need to rehash that the current system sucks.
>
> This would also be my comment on the initial message: It's again saying
> how bad the current process is. It would be far more constructive to
> make a concrete proposal about how to do it instead.

I want us to come to consensus what the problems are and before we try to
fix them. If we have no consensus about where we're trying to go, then a
concrete proposal will also generate controversy.

It is useful to observe how the current system sucks: if we agree on what
the problems are, we can have a shot at spelling out our requirements. If
there is consensus of what we want from a process (the requirements), we
can evaluate options (eg. github, gitlab, gerrit etc.) against our
requirements, and select tradeoffs that suit ourselves best.

There is a list at the bottom of properties of what I think a better
process looks like.

I am sufficiently versed with github and gerrit to be able to build
something that could work, but doing so is a lot more work than discussing
things, so I like to be sure I am building something that solves the
problem.

I get the feeling you don't like where this is going, but maybe you could
tell us what you think are problems, and what you would like to see in a
different process.

if you can only work with concrete proposals, I guess you'll have to wait
until the rest of us come to that point.

On Wed, Feb 5, 2020 at 9:31 AM Jonas Hahnfeld <address@hidden> wrote:

> Am Mittwoch, den 05.02.2020, 00:11 +0100 schrieb David Kastrup:
> > Han-Wen Nienhuys <
> > address@hidden
> > > writes:
> >
> > > 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.
> >
> > Changes belonging to the same topic should be committed to the same
> > Rietveld review and Sourceforge issue.  One can commit them in sequence
> > to Rietveld when it is desirable to view them independently.  This does
> > not allow to view fixes resulting from discussion in the context of the
> > ultimately resulting commit chain (which will usually be fixed
> > per-commit with git rebase -i).
> >
> > For a sequence of commits belonging to one logical change, this is the
> > somewhat defective way of doing stuff.  It's not as bad as you happened
> > to use it, but it definitely is a tool that grew on Subversion and added
> > Git as an afterthought.
> >
> > Where commits do not belong to the same logical issue but are still
> > interdependent, they cannot be logically disentangled even using a
> > Git-specific tool like Gerrit.
> >
> > >    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.
> >
> > Recompiles tend to be very fast unless you "make clean" in between or
> > check out, in the same work tree, vastly differing branches, like
> > switching between stable and unstable branches.
> >
> > Or bisecting across a version change.  It's annoying how much just
> > depends on the VERSION file, but not actually something that the review
> > tool will help with.
> >
> > >    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?
> >
> > The change is ok to go in when it has been approved for pushing.  I find
> > the idea of ignoring instead of addressing complaints weird.
> >
> > >    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.
> >
> > We don't need to rehash that the current system sucks.  We had to amend
> > our process after relying on a proprietary tool, namely Google Code,
> > ended up hanging us out to dry and we had to replace the parts removed.
> > Our available knowledge and volunteer power ended up with the current
> > setup which was not intended as a keeper.  It kept the project working,
> > but I doubt many people will be sad to see it replaced.
> >
> > >    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).
> >
> > Then you have set it up incompletely or use it wrong.
> >
> > git cl upload
> >
> > will copy the current change set to Rietveld.  I am impressed at the
> > rate of change you manage to churn out without relying on the commands
> > we use for that purpose, but this certainly can be done less painfully.
> >
> >
> > > 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.
> >
> > They get feedback when the code is getting reviewed.  If code does not
> > get reviewed, having their changes dropped on the floor is not going to
> > increase their enthusiasm.
> >
> > And just above you wanted to know when you are free to ignore feedback.
> >
> > >    It is harmful to the project, because we can end up adopting code
> > >    that we don’t understand.  -
> >
> > Most contributors leave the code in a better documented state than they
> > got to work with.  I am probably guilty for most contributions of "code
> > that we don't understand" by condensing complexity into few places in
> > order to create large user-accessible swaths of code people _can_
> > understand, like making music functions much more powerful and generic
> > than they were, making large amounts of LilyPond accessible to Scheme
> > programming and extension, at the cost of making lily/parser.yy quite
> > more complex.  In contrast to previous coding practice, my changes are
> > quite thoroughly documented, but it's still a real piece of work.
> >
> > Much of that work got accepted not because reviewers understood it (few
> > reviewers are into Bison) but because people trusted me.  In the end,
> > that tends to be a considerable part of the currency of work, but new
> > contributors cannot rely on it.
> >
> > With regard to "code that we don't understand": I had to completely
> > redesign the internals of several corners of LilyPond because code was
> > entered that not even the committer did understand but that had become
> > rather popular.
> >
> > Chord repeats come to mind, nested property overrides and reverts,
> > overrides inside of grace passages and a few other things.
> >
> > I cursed a lot having to come up with replacements for things that
> > I could prove not workable.
> >
> > >    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.
> >
> > The really big problem of many free software projects is that the people
> > going all-in as developer do not have enough time left in their life to
> > be serious users of the software they are working with.  Rietveld et al
> > are not helping, but power users on our forums still got drawn into
> > contributing.  And much of their contributions could bypass any central
> > vetting process if we had a package repository where people can take
> > other people's code and combine it effortlessly, or leave it.
> >
> > > 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).
> >
> > Nope.  It is an Andy-only domain, and Andy does not respond.  That's all
> > there is to it.  You don't see any "wontfix" tag or other form of
> > rejection.  The issue remains open.
> >
> > > I imagine that David was not happy about this particular decision, but
> > > I see a process working as expected.
> >
> > There was no decision.  There were some comments which I addressed and
> > put into context.
> >
> > > 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.
> >
> > Uh, I am banished from the Guile developer list.  There is no way to
> > build consensus.  And I was merely implementing what Andy Wingo stated
> > in a comment should be implemented, in the manner he stated it should be
> > done.
> >
> > But that's a side track.  As I already stated: my initial experience
> > with contributing involved patches to LilyPond was that they were
> > ignored because nobody considered themselves able or motivated to review
> > them.  Even simple changes took weeks to get accepted.  For better or
> > worse, there just aren't people responsible for large parts of the code
> > that would be able or willing to judge it on its merits in the context
> > of the LilyPond code base.
> >
> > I can on occasion work with active complex projects: you'll find that
> > the bulk of git-blame's internals has been rewritten by me (a dumb
> > promise I made to the Emacs developer list when the non-scaling
> > performance of git-blame was a large impediment to moving from Bzr to
> > Git) and I got it in.  But the project is much more modular and active
> > than LilyPond, including a hierarchy of developers that we just don't
> > have.
> >
> > > David suggests that I like getting patches in by fiat/countdown, but I
> > > disagree.
> >
> > That was likely inappropriate by me, sorry for that.  I just pointed out
> > that what you considered detrimental would work in your interest here.
> >
> > >    Uncontroversial changes can be submitted immediately after a
> maintainer
> > >    has LGTM’d it,
> >
> > Two problems with that: what is uncontroversial?  And who is a
> > maintainer?  You want less opportunity for people to raise objections,
> > but how can you decide about something being uncontroversial when people
> > don't get to review a patch and consider objections?
> >
> > >    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.  -
> >
> > Like which patch?
> >
> > >    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.
> > >    -
> >
> > It's a good thing, I agree on that.  "We should keep it" sounds like it
> > is a mechanical thing and a decision we can make.  It involves
> > significant work currently done by James.  And the automation he has
> > available to that purpose is in a decrepit state.  That's really an
> > embarrassment.  So we should not just "keep it" but hopefully also fix
> > significant parts of it to make them less manual.  This visual
> > comparison is something that is unique to LilyPond as a typesetter, and
> > there may be some effort involved getting a good workflow designed and
> > implemented working with a different tool.
> >
> > The current scripts were designed to work with Google Code, and the
> > migration to Sourceforge has not really been anywhere to complete.
> > Whatever we end up with, I hope it takes a lot more of the mechanical
> > workload off whoever does the visual comparisons than what we have now.
> >
> > >    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.
> >
> > Who is we?
> >
> > >    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.  -
> >
> > Who are maintainers?
> >
> > >    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.
> >
> > That is not their current job description.
> Jonas
>


-- 
Han-Wen Nienhuys - address@hidden - http://www.xs4all.nl/~hanwen