[PATCH wayland] contributing: review rules for bugs

[Pekka Paalanen]

On Wed, 27 Jun 2018 16:47:09 +0300
Pekka Paalanen <ppaalanen at gmail.com> wrote:

> From: Pekka Paalanen <pekka.paalanen at collabora.co.uk>
> 
> Half of the ideas came from Daniel but most of them are reworded, the
> rest are my thoughts.
> 
> Mention compiler warnings specifically, and be more explicit on what
> kind of code or bugs or bug fixes are acceptable or not. Clarify commit
> scope.
> 
> Cc: Daniel Stone <daniels at collabora.com>
> Signed-off-by: Pekka Paalanen <pekka.paalanen at collabora.co.uk>
> ---
>  CONTRIBUTING.md | 19 ++++++++++++++++---
>  1 file changed, 16 insertions(+), 3 deletions(-)
> 

Hi,

this is a bit more than already discussed. Questions below.

> diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md
> index 70d0eca..51bef89 100644
> --- a/CONTRIBUTING.md
> +++ b/CONTRIBUTING.md
> @@ -223,11 +223,24 @@ include tests excercising the additions in the test suite.
>  - The code fits the existing software architecture, e.g. no layering
>  violations.
>  
> -- The code is correct and does not ignore corner-cases.
> +- The code is correct and does not introduce new failures for existing users,
> +does not add new corner-case bugs, and does not introduce new compiler
> +warnings.
>  
> -- In a patch series, every intermediate step produces correct code as well.
> +- In a patch series, every intermediate step produces correct and warning-free
> +code as well.
>  
> -- The code does what it says in the commit message and changes nothing else.
> +- The patch does what it says in the commit message and changes nothing else.
> +
> +- The patch is a single logical change. If the commit message addresses
> +multiple points, it is a hint that the commit might need splitting up.
> +
> +- A bug fix should target the underlying root cause instead of hiding symptoms.
> +If a complete fix is not practical, partial fixes are acceptable if they come
> +with code comments and filed Gitlab issues for the remaining bugs.
> +
> +- The bug root cause rule applies to external software components as well, e.g.
> +do not work around kernel driver issues in userspace.

This last item is written with Weston in mind. We definitely do not
want to collect a pile of per-driver workarounds or features. However,
sometimes a workaround has its place, drm_output_pick_crtc() works
around bad possible_crtcs and possible_clones, for instance.

https://gitlab.freedesktop.org/wayland/weston/blob/78a42116ae92f93a01539a785ce95cc478189608/libweston/compositor-drm.c#L4809

Is there a better wording to allow that?

Or does this workaround have its place?

Seeing that Ville Syrjälä went on a journey to actually fix the
possible_*, I have filed an issue to remind us about it:
https://gitlab.freedesktop.org/wayland/weston/issues/120


Thanks,
pq
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20180627/fdfbca61/attachment-0001.sig>