Re: development process

[Jonas Hahnfeld]

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.

Sorry for quoting the response in full, but I fully agree with every
single point that David describes.

> 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.

Jonas

signature.asc
Description: This is a digitally signed message part