Machine-scope maintainers for OpenBMC

[Brad Bishop]

> 
> On Oct 17, 2017, at 8:56 PM, Andrew Jeffery <andrew at aj.id.au> wrote:
> 
> On Tue, 2017-10-17 at 09:23 +0800, Lei YU wrote:
>> Hi Andrew, Brad,
>> 
>> I would like to bring a question about how do we welcome system
>> vendors' systems.
>> 
>> Background: in gerrit we already have some system vendors' machines
>> under review:
>> * meta-inventec/meta-lanyang: https://gerrit.openbmc-project.xyz/#/c/3888/
>> * meta-yadro/meta-vesnin: https://gerrit.openbmc-project.xyz/#/c/6429/
>> 
>> They both contain a "large" commit that introduce a machine.
>> 
>> I know we have suggestions to split the commit into separated smaller
>> ones, but this
>> seems a bit hard:
>> * The work is based (referenced) on a similar machine, e.g palmetto or
>> zaius, which
>>    means the base code (meta-xxx) is already big;
>> * The change is submitted as a "squashed" one after it's built and
>> tested, which means
>>    not each "internal" commit can be built successfully.

I understand.  I remember the first time someone asked me to split
a bunch of my work into small pieces.  My first thought was, why!?! It
works as-is.

But I think this view is just an artifact of not having your code peer
reviewed (or not properly) in the past, or having to do peer review yourself.
(I’m not referring to you personally Lei, just in general).

It really doesn’t take very long participating in an environment where
peer review is primary to understand the benefit for both the author
and the reviewer, of that extra effort.


>> 
>> In such case, how do we handle this case? Is it possible to accept a
>> large squashed commit
>> if and *only* if the change is to introduce a new meta-machine?

For the new machine case, we may have an opportunity to ease the
transition for new contributors with a new machine cookbook or wizard…
one that includes help constructing a series of patches rather than just one.

> 
> I think that depends. What are you comfortable reviewing? Personally I
> *much* prefer small, obvious patches than a hulking diff in the
> magnitude of thousands of lines. I find it *much* harder to track
> what's going on in the latter type of change. Bear in mind that this is
> code that could/will be executing on any number of people's machines,
> and we have a responsibility to the community to ensure it's not doing
> anything too crazy.
> 
> If you're comfortable reviewing large patches like that and signing off
> on them then you may have just won yourself a lot of work :D Otherwise,
> my view is contributors should help themselves by helping those
> reviewing the code say yes to their contribution, which means at least
> self-contained changes which are hopefully small.
> 
> However, we do run into one of my favourite topics: essential vs
> accidental complexity[1]. Sometimes it's not possible to break things
> down further. In this case reviewers might have to just deal with large
> patches, but contributors should be willing to bend to their judgement
> on whether the patch has been reduced to deal with only the essential
> complexity of the problem. If it hasn't, the patch should probably be
> split.
> 
> I'm hesitant to provide a blanket rule like "all new system additions
> can be a patch bomb" without ruling out accidental complexity from the
> problem.
> 
> [1] https://en.wikipedia.org/wiki/No_Silver_Bullet
> 
> Sure, this all raises the bar to contribution a bit. However I feel it
> does so in a way that leads to positive long-term contributions to the
> community by demonstrating some commitment, rather than just having
> code dumped over the wall with the risk that it gets neglected.
> 
> Maybe this could be seen as being exclusive. That's true to an extent,
> but I feel it's principled exclusion: That we should weight exclusion
> for the quality of the implementation of the contribution (i.e. whether
> the patch can be understood with confidence, the quality of the design,
> quality of the code code, whether it has tests, etc), but we should
> heavily weight *inclusion* for the requirements and concepts driving a
> contribution (adding support for previously unsupported systems,
> alternative implementations of DBus interfaces if they meet new
> requirements, etc).
> 
> To clarify, saying no to a patch adding a new system should probably
> not be interpreted as "no we don't want to support your system", rather
> "thanks for your effort to contribute support for your system! However
> I think you can help us help you by arranging the patch differently".
> 
> To finish, these are my personal opinions on the matter, and may not be
> reflected by the community at large. I'm keen to hear what other people
> think. Hopefully it addressed your query somewhere in the wall of text
> :)
> 
> Andrew


I’m pretty much in complete agreement with all of Andrew’s points.

-brad