Re: [PATCH wayland] contributing: review rules for bugs
On Thu, 28 Jun 2018 10:41:43 +0100 Emil Velikov wrote: > On 28 June 2018 at 10:12, Pekka Paalanen wrote: > > It's the spirit of the guidelines that matters, they are not to be > > interpreted by the letter, and there may always be special > > circumstances where the reasonable action is to overlook something. > > Even with the guidelines, we still rely very much on the individual > > judgement. The important thing is to get people to the right mindset in > > general. > > > > Perhaps the above would be worth a paragraph in CONTRUBUTING.md? > > > It was meant as food for thought, inspired by some long threads I've > seen where devs discuss various compiler warnings. > You're spot on being terse and providing a feel (or spirit as you call > it) is the key point. > > Apologies for the distraction this has caused :-\ Oh, no worries, these are important things to discuss, so we set up the right expectations. I think I should add that paragraph to Review Section, exactly to set the right mindset. Thanks, pq pgp2r_Iw61XAE.pgp Description: OpenPGP digital signature ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH wayland] contributing: review rules for bugs
On 28 June 2018 at 10:12, Pekka Paalanen wrote: > On Wed, 27 Jun 2018 17:49:34 +0100 > Emil Velikov wrote: > >> Hi Pekka, >> >> A couple small ideas come to mind: >> >> On 27 June 2018 at 14:47, Pekka Paalanen wrote: >> > From: Pekka Paalanen >> > >> > 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 >> > Signed-off-by: Pekka Paalanen >> > --- >> > CONTRIBUTING.md | 19 --- >> > 1 file changed, 16 insertions(+), 3 deletions(-) >> > >> > 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. >> Compiler warnings vary greatly on the compiler and it's version. It >> does become a nuisance when committer/reviewer is using a newer >> version which flags dozens of warnings. > > Yes. There is no need to get out of one's way to hunt for possible > warnings, but if someone does notice new warnings, they should be > fixed. If a reviewer gets them but the author/submitter doesn't, the > reviewer can help with them. It's common sense to me. > > I've been wondering if the CI build should actually turn -Werror on, > but that's better to discuss after if/when we migrate to MR-based work > flow. > >> That aside, worth loosening this for patches where existing code is >> copied or moved? > > It is already loosened: they are not new warnings if they were there > before. If code motion results in new warnings, they should ideally be > fixed. If the fix makes the code motion harder to review, it's ok to > fix it in another patch before or after. > > I think we also need to be picky on how much details we are going to > write down here. It is very easy to extend the guidelines with details > so far that it becomes too long to read. That is why I try to be as > terse as possible. > > It's the spirit of the guidelines that matters, they are not to be > interpreted by the letter, and there may always be special > circumstances where the reasonable action is to overlook something. > Even with the guidelines, we still rely very much on the individual > judgement. The important thing is to get people to the right mindset in > general. > > Perhaps the above would be worth a paragraph in CONTRUBUTING.md? > It was meant as food for thought, inspired by some long threads I've seen where devs discuss various compiler warnings. You're spot on being terse and providing a feel (or spirit as you call it) is the key point. Apologies for the distraction this has caused :-\ -Emil ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH wayland] contributing: review rules for bugs
On Wed, 27 Jun 2018 17:49:34 +0100 Emil Velikov wrote: > Hi Pekka, > > A couple small ideas come to mind: > > On 27 June 2018 at 14:47, Pekka Paalanen wrote: > > From: Pekka Paalanen > > > > 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 > > Signed-off-by: Pekka Paalanen > > --- > > CONTRIBUTING.md | 19 --- > > 1 file changed, 16 insertions(+), 3 deletions(-) > > > > 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. > Compiler warnings vary greatly on the compiler and it's version. It > does become a nuisance when committer/reviewer is using a newer > version which flags dozens of warnings. Yes. There is no need to get out of one's way to hunt for possible warnings, but if someone does notice new warnings, they should be fixed. If a reviewer gets them but the author/submitter doesn't, the reviewer can help with them. It's common sense to me. I've been wondering if the CI build should actually turn -Werror on, but that's better to discuss after if/when we migrate to MR-based work flow. > That aside, worth loosening this for patches where existing code is > copied or moved? It is already loosened: they are not new warnings if they were there before. If code motion results in new warnings, they should ideally be fixed. If the fix makes the code motion harder to review, it's ok to fix it in another patch before or after. I think we also need to be picky on how much details we are going to write down here. It is very easy to extend the guidelines with details so far that it becomes too long to read. That is why I try to be as terse as possible. It's the spirit of the guidelines that matters, they are not to be interpreted by the letter, and there may always be special circumstances where the reasonable action is to overlook something. Even with the guidelines, we still rely very much on the individual judgement. The important thing is to get people to the right mindset in general. Perhaps the above would be worth a paragraph in CONTRUBUTING.md? > > > > -- 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. > > > It makes sense to move this as the final bullet-point and let it refer > to the whole section. > Namely: > > - In a patch series, every intermediate step adheres to the guidelines above. Yes, a good suggestion. Thanks, pq pgpjg3DhtCnXZ.pgp Description: OpenPGP digital signature ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH wayland] contributing: review rules for bugs
Hi Pekka, A couple small ideas come to mind: On 27 June 2018 at 14:47, Pekka Paalanen wrote: > From: Pekka Paalanen > > 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 > Signed-off-by: Pekka Paalanen > --- > CONTRIBUTING.md | 19 --- > 1 file changed, 16 insertions(+), 3 deletions(-) > > 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. Compiler warnings vary greatly on the compiler and it's version. It does become a nuisance when committer/reviewer is using a newer version which flags dozens of warnings. That aside, worth loosening this for patches where existing code is copied or moved? > > -- 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. > It makes sense to move this as the final bullet-point and let it refer to the whole section. Namely: - In a patch series, every intermediate step adheres to the guidelines above. HTH Emil ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH wayland] contributing: review rules for bugs
On Wed, 27 Jun 2018 16:47:09 +0300 Pekka Paalanen wrote: > From: Pekka Paalanen > > 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 > Signed-off-by: Pekka Paalanen > --- > 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 pgpYvYB3NLWHH.pgp Description: OpenPGP digital signature ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
[PATCH wayland] contributing: review rules for bugs
From: Pekka Paalanen 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 Signed-off-by: Pekka Paalanen --- CONTRIBUTING.md | 19 --- 1 file changed, 16 insertions(+), 3 deletions(-) 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. - The test suite passes. -- 2.16.4 ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
