Machine-scope maintainers for OpenBMC

[Andrew Jeffery]

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.
> 
> 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?

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: This is a digitally signed message part
URL: <http://lists.ozlabs.org/pipermail/openbmc/attachments/20171018/af3a2de4/attachment.sig>