[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: development process
From: |
Jonas Hahnfeld |
Subject: |
Re: development process |
Date: |
Wed, 05 Feb 2020 09:31:26 +0100 |
User-agent: |
Evolution 3.34.3 |
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
Re: development process, Han-Wen Nienhuys, 2020/02/05