>> 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 <[email protected]> wrote: > 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. > Jonas > -- Han-Wen Nienhuys - [email protected] - http://www.xs4all.nl/~hanwen
