Am Mittwoch, den 05.02.2020, 00:11 +0100 schrieb David Kastrup: > Han-Wen Nienhuys < > [email protected] > > 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
