[Spice-devel] SPICE: contributor guidelines
Hi SPICE community! After thinking a while on the discussion in the thread: "SPICE: changing the merge rules - a proposal" I would like to come back on the topic and make explicit the way merge requests, changes and reviews are expected to be managed in our community. The core points that seems to me all of us want and agree upon are: * Each merge request / patch should get a review (trivial patches may be an exception and be merged without review). * Each merge request / patch should get a review in a timely manner. Note that the meaning of "timely" and what a "trivial" patch is are not defined: I think this is exactly the point for an open source community. We don't want strict rules that force people to contribute in a specific way or under a strict time-frame and I (we) trust each other to do the right thing in respect of the project and the community. This is enough once there are clear guidelines. There could be errors from time to time, sure... but as long as we stay together and collaborate as a community, they can be easily fixed. Unfortunately enforcing some strict rules was exactly what I tried to do in my previous mail. It may have brought tension to our community and that was not my intention, sorry for that. So, for the sake of clarity for actual and future contributors, if we want to write down a short and simple list of "rules" (guidelines) for contributing to the SPICE project, I would say they could be: 1) Get a review before merging a non-trivial patch 2) Keep asking reviews more and more if not getting one 3) Ensure each one asking a review gets one in a timely manner I would love to read your comments, opinions and ideas. Francesco ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] SPICE: changing the merge rules - a proposal
Hi, On 18/05/20 12:56, Daniel P. Berrangé wrote: On Fri, May 15, 2020 at 07:05:50PM +0200, Francesco Giudici wrote: Hi, the community around the SPICE project always tried to follow one fundamental, implicit rule for accepting code contributions to the project: every merge request (beside trivial patches) should be reviewed and acked at least by one before getting merged. While everyone agrees with this fundamental rule, the actual status of some SPICE projects makes the rule impractical to let the project move forward. Let's consider the spice/spice project as an example: the number of contributions is very low, both on the commit side (only 4 different contributors with more than 1 commit from the beginning of the year, and a single contributor with 90% of commits) and on the review side (in the last 40 merge requests before the C++ switch one, 21 had no comments). The x11spice project is another example: we have only 4 contributors from the beginning of the year (and a single contributor holding 70% of the commits) and the reviews on the gitlab merge requests have been provided by two people only, each one reviewing the merge requests of the other. I think these paragraphs here are the highlighting the key issue. There just is not sufficient contribution to SPICE in general. I think the most important thing is to consider why the SPICE project has such a low rate of contribution, and more importantly what steps can be done to make SPICE more interesting to attract contributors. I like your analysis, thanks for sharing it. I agree the best solution would be to attract contributors (we need to find some ideas on how to do this and act on them btw!) but my point is also: let's try to keep the contributors we already have in the meanwhile. I would like to avoid issues about contributions not merged (or merged without agreement) causing us to start arguing among us. This could led to a not-so-happy environment that may harm the project and the community. For a project that has such a large codebase and featureset, it is not healthy to have such a small community. The "bus factor" is waay too low here, such that SPICE would be in serious trouble as a project if one or two main contributors moved onto to different work. So for any changes in process, it is wise to consider what effects those change have on people SPICE would like to attract as new contributors. Agreed. I would also add we don't want any change that may also make current contributors lose love or even leave the project. Hope this is not the effect of this draft "proposal" on anyone, as my intent is exactly the opposite! For example, if new feature work by an existing contributor is discussed out of band between two contributors, instead of via merge request comments this sets up new contributors up as second class citizenships - the out of band discussions are essentially invisible to them, so they can't see or understand the rationale for decisions. This makes it harder for them to learn how to effectively contribute to the project. This is the big thing that concerned me about the merge request that adopted C++. Reading between the lines I get the impression this was indeed discussed between the several contributors but out of band, instead of on the merge request which had no comments. Thus that discussion is invisible to any 3rd party contributor interested in SPICE. I think this risks discouraging people from contributed to SPICE, so it is something to be wary of doing too often, especially for really big technical changes. Thanks for sharing this! AFAIK, this did not happened: no out of band discussion at all about the C++ branch merging. Glad you brought this up. There were two legitimate competing desires in that situation: one was to have a big rework merged by the main contributor to the project. The other was to have proper review and agreement before performing such a merge. My personal point is that such a rework, moving to C++, with so low review effort on the spice/spice project, would have caused the branch to stay there without getting a fair review for a very long time... btw, a warning that it was going to be merged if no reviews would have had not harmed. This is why I think would be good to have rules that help on what to do in such cases: committer asks for reviews for few weeks before merging. The other contributors will know that if they don't reply, the merge will happen. They will have at least to put a comment there asking for more time if they are interested in reviewing. Wouldn't be a fair "game"? ;-) For the sake of having the projects being able to move forward with a reduced number of contributors/reviewers, the proposal is to *allow* a maintainer to merge a Merge Request without an explicit ack if the three following conditions are met: 1) The Merge Request has been pending for at least 3 weeks without getting new comments 2) The Me
Re: [Spice-devel] SPICE: changing the merge rules - a proposal
Hi, On 15/05/20 23:58, Marc-André Lureau wrote: Hi On Fri, May 15, 2020 at 7:06 PM Francesco Giudici <mailto:fgiud...@redhat.com>> wrote: Hi, the community around the SPICE project always tried to follow one fundamental, implicit rule for accepting code contributions to the project: every merge request (beside trivial patches) should be reviewed and acked at least by one before getting merged. While everyone agrees with this fundamental rule, the actual status of some SPICE projects makes the rule impractical to let the project move forward. I wasn't aware of a maintenance problem. Perhaps we should first list the projects that have maintenance issues & discuss our options, before changing the common rule. The idea of this e-mail is exactly this: let's discuss. But starting with a proposal and not only the issue seems to me the best way to try to move things forward (also if in the end the proposal my be completely changed it would have reached the goal). Let's consider the spice/spice project as an example: the number of contributions is very low, both on the commit side (only 4 different contributors with more than 1 commit from the beginning of the year, and a single contributor with 90% of commits) and on the review side (in the last 40 merge requests before the C++ switch one, 21 had no comments). You are omitting the passive reviewers. I consider myself as one of them. If you need people to be more proactive, you could at least reach me & probably others past contributors. Great to know that you are looking at the contributions, also if not sure what a "passive" reviewer does. In this case anyway I think if you add a comment to the branch you looked at it will be much better. I'm pretty sure the submitter will be happy that someone looked at it. So, reaching out right now: if there are people looking at the branches and not commenting, please start doing. Also partial comments (not full reviews) may help I would say. The x11spice project is another example: we have only 4 contributors from the beginning of the year (and a single contributor holding 70% of the commits) and the reviews on the gitlab merge requests have been provided by two people only, each one reviewing the merge requests of the other. As projects become more specific/marginal, it's clear that the number of maintainers is lower. Yet, we have those projects under the same umbrella, and I don't think they should be treated differently. Sorry, maybe I was not clear: projects under the same umbrella should be treated equally. So, the rules will be the same for every project, and can be used to address situations where contributions (reviews) are missing. This is something that the number above suggests. 2 active developers/maintainers can be very healthy, I would say. So do we have a maintenance issue with x11spice? Do we want to move the project out of the "Spice-space" projects for ex? What is the problem exactly? I think you are not getting the point, maybe I was not clear: as long as we have at least 2 contributors we get a review there. Perfect, nothing changes. What if one of the two developers/maintainers gets sick? For months? Or stops for his own reasons to contribute to the project? Should the project stop? Problem is: if there is low contribution we don't want that active contributors face even more obstacles to keep contributing. We want to keep the project healthy and alive as much as possible. For the sake of having the projects being able to move forward with a reduced number of contributors/reviewers, the proposal is to *allow* a maintainer to merge a Merge Request without an explicit ack if the three following conditions are met: 1) The Merge Request has been pending for at least 3 weeks without getting new comments 2) The Merge Request submitter has kept asking a review on a weekly basis 3) There are no pending nacks on the Merge Request It's hard to define a delay to bypass a reviewing & consensus rule. In general, it should really be frowned upon. I understand your feeling and I feel the same at least in part: but the rules we are talking about apply only when a project doesn't get reviews and gets paralyzed. Also notice that it *allows* a *maintainer* to merge the branch, it is not automatic. We give more freedom and trust to maintainers over strict rules when contributions are low. There is clearly more than one person interested and using Spice. If the issue is important, it should be fairly easy to get someone else to look at the issue in a timely manner. I like your optimism here, but honestly I don't think this is true. But anyway, checking if someone will give the review is already part of the proposal: the "asking a review on a weekly basis"
[Spice-devel] SPICE: changing the merge rules - a proposal
Hi, the community around the SPICE project always tried to follow one fundamental, implicit rule for accepting code contributions to the project: every merge request (beside trivial patches) should be reviewed and acked at least by one before getting merged. While everyone agrees with this fundamental rule, the actual status of some SPICE projects makes the rule impractical to let the project move forward. Let's consider the spice/spice project as an example: the number of contributions is very low, both on the commit side (only 4 different contributors with more than 1 commit from the beginning of the year, and a single contributor with 90% of commits) and on the review side (in the last 40 merge requests before the C++ switch one, 21 had no comments). The x11spice project is another example: we have only 4 contributors from the beginning of the year (and a single contributor holding 70% of the commits) and the reviews on the gitlab merge requests have been provided by two people only, each one reviewing the merge requests of the other. For the sake of having the projects being able to move forward with a reduced number of contributors/reviewers, the proposal is to *allow* a maintainer to merge a Merge Request without an explicit ack if the three following conditions are met: 1) The Merge Request has been pending for at least 3 weeks without getting new comments 2) The Merge Request submitter has kept asking a review on a weekly basis 3) There are no pending nacks on the Merge Request Note that having patches reviewed would still be the preferred way. If at any time the number of contributors would raise again, we can switch back to the mandatory review rule. Until then the priority is to allow the project to move forward. What do you think? Please share your thoughts and/or contribute with your own ideas. Thanks Francesco ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] About decisions and reviews
On 12/05/20 19:13, Marc-André Lureau wrote: Hi On Tue, May 12, 2020 at 5:07 PM Francesco Giudici wrote: On 12/05/20 11:24, Daniel P. Berrangé wrote: On Mon, May 11, 2020 at 12:16:24PM +0200, Marc-André Lureau wrote: Hi, About "Move code to C++": https://gitlab.freedesktop.org/spice/spice/-/merge_requests/62 I would like to know how the decision happened. As long as I have been involved in the Spice project, we had open discussions and did mandatory review for anything non-trivial. This change is substantial, and impacts the work and contribution of others. Where did the discussion happen? Who reviewed the code changes? Looking at that merge request, it is pretty surprising to see a 100 patch long series merged with no review comments visible on the code from other maintainers. Regards, Daniel I see your points: a proper discussion and fair review on the branch would have been the way to go. To have a fair overall picture btw, I think we should also consider some other points: $> git shortlog --since 01/01/2019 -s -n 411 Frediano Ziglio 29 Victor Toso 20 Uri Lublin 14 Francois Gouget 11 Christophe Fergeau 10 Snir Sheriber 6 Eduardo Lima (Etrunko) 6 Jonathon Jongsma 6 Kevin Pouget 6 Lukáš Hrázký 5 James Le Cuirot 3 Thiago Mendes 2 Rosen Penev 1 Benjamin Tissoires 1 Christian Ehrhardt 1 Douglas Paul 1 Gilmar Santos Jr 1 Jeremy White 1 worldofpeace 1 谢 昆明 To be "fair", the number of commits in spice server alone over 1.5y is not a very good metric. Certainly, Frediano's work is consequent, thank you, but it's also part of his job afaik. My point here is that in the last 1.5y there was very little contribution to the project apart from Frediano. Same on the review side. In this context, I can understand why the branch was merged. I have my opinion, but here I don't want to say he did it right or he did it wrong: I'm just saying I can understand why he did. Frediano's branches don't get much reviews (if any... see the full list of merged/closed MR in gitlab for the spice/spice project). I think we all agree that his intention is good, which is to just move the project forward. Wondering who would have looked into his 100 patches branch to do a fair review in a reasonable time-frame. I feel (at least partially) guilty for this situation. That said... at this point the branch has been merged. What are the proposals now? Draft a more formal review/commit policy? What if a branch doesn't get a fair review in an acceptable time-frame? Who will have the last word if unanimity is not reached? What does an acceptable time-frame mean? If the work is important, the project maintainers should do a review in a timely manner. If nobody has enough time to review changes, isn't it because we, collectively, have more important things to do? The contributor who proposed such a change in the first place should realize that. Well, agreeing on what is "important" is not easy, as it is personal. Any contribution is important to the submitter ;-) But my point is, let's make the rules clearer, so to try to avoid similar situations in the future. Do you want to do a post-merge review to consider reverting the commits? Do you want to have a detached "C" branch with the former code to be kept as a "stable C" one? Or what else? I am a past contributor to the Spice server, but I regularly have to read or debug the code. I keep co-maintaining the spice-gtk client, although it seems others are doing a pretty good job at helping me. Is there a problem with the Spice server maintenance? I don't know. What do you want me to say... I am disappointed by this change, the nature of the change, switching to c++, and the way it happened. It doesn't reflect practices I like in open-source projects and the way the Spice project has been developed in the past. Overall, the way it happened is detrimental to the project imho, and I would indeed revert the change if nobody reviewed it openly. The spice server should not to be taken so lightly, it doesn't deserve it either. I like the love you have for the project. Really, great to see it. It is also ok to disagree and argue. But when you say "taken so lightly, it doesn't deserve it" you are putting blame there... Hey, we have all the same goal to make SPICE better and better. Frediano merged the branch to improve the project, not to make it worse... and he proved to love the project too. We are all on the same side! So, let's move forward... Frediano merged it, this is done. What would you propose to do now? Let's move the discussion on what to do now :-) Francesco ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] About decisions and reviews
On 12/05/20 11:24, Daniel P. Berrangé wrote: On Mon, May 11, 2020 at 12:16:24PM +0200, Marc-André Lureau wrote: Hi, About "Move code to C++": https://gitlab.freedesktop.org/spice/spice/-/merge_requests/62 I would like to know how the decision happened. As long as I have been involved in the Spice project, we had open discussions and did mandatory review for anything non-trivial. This change is substantial, and impacts the work and contribution of others. Where did the discussion happen? Who reviewed the code changes? Looking at that merge request, it is pretty surprising to see a 100 patch long series merged with no review comments visible on the code from other maintainers. Regards, Daniel I see your points: a proper discussion and fair review on the branch would have been the way to go. To have a fair overall picture btw, I think we should also consider some other points: $> git shortlog --since 01/01/2019 -s -n 411 Frediano Ziglio 29 Victor Toso 20 Uri Lublin 14 Francois Gouget 11 Christophe Fergeau 10 Snir Sheriber 6 Eduardo Lima (Etrunko) 6 Jonathon Jongsma 6 Kevin Pouget 6 Lukáš Hrázký 5 James Le Cuirot 3 Thiago Mendes 2 Rosen Penev 1 Benjamin Tissoires 1 Christian Ehrhardt 1 Douglas Paul 1 Gilmar Santos Jr 1 Jeremy White 1 worldofpeace 1 谢 昆明 Frediano's branches don't get much reviews (if any... see the full list of merged/closed MR in gitlab for the spice/spice project). I think we all agree that his intention is good, which is to just move the project forward. Wondering who would have looked into his 100 patches branch to do a fair review in a reasonable time-frame. I feel (at least partially) guilty for this situation. That said... at this point the branch has been merged. What are the proposals now? Draft a more formal review/commit policy? What if a branch doesn't get a fair review in an acceptable time-frame? Who will have the last word if unanimity is not reached? Do you want to do a post-merge review to consider reverting the commits? Do you want to have a detached "C" branch with the former code to be kept as a "stable C" one? Or what else? best, Francesco ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
