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 Merge Request submitter has kept asking a review on a weekly basis
Note since spice is using gitlab, it is possible to explicitly tag people
by name in the comments to ask for reviews, and/or assign people as a
reviewer. I'd suggest to make use of such tagging, so that people can be
aware they they are being explicitly asked for feedback if something has
been sitting for a long time.
This is a good idea! Let's add adding explicit people tagged in the
project! How to pick them? At least 2 maintainers? The 2 most active
contributors? What else? This could be done.
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.
Contributors are unlikely to magically rise again on their own, without
proactive work to make SPICE a more attractive project to contribute to.
This is a very difficult problem for projects in general, not just SPICE,
to which I don't have any magic answers. At least try to think about what
you like to see from projects when you are approaching them as a new
contributor, and then make sure SPICE follows these practices where ever
possible / practical.
I agree, addressing this may help reducing or even solving the issue in
the middle/long term if successful. But this is another point, IMHO. I'm
in favor of it, but it is harder and will require longer planning and
effort with no immediate return. What we do in the meanwhile?
Code being pushed with no review is a turn-off to
me as a external contributor, unless the project is clearly setup as a
single-author personal project, rather than a team project.
I don't like the idea of commits merged without reviews too. And what do
you think about a project where your merge requests doesn't get any
comment for three weeks or more? The idea here is to put some pressure
(and responsibility) on reviewers too. Remember, one single comment is
enough. And the committer has to keep asking for reviews (and maybe
tagging specific people).
Not saying that the draft proposal here must be the way to go and I
agree that improving the participation to the project is important and
may prevent the issue from happening again.
My point here is, let's not dismiss the problem we are facing right now:
we don't want MR abandoned without even a comment or merged without an
agreed process. How do we deal with this?
Thanks
Francesco
Regards,
Daniel
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel