IGT contributions and reviews

IGT contributions and reviews

[Petri Latvala]

The current contributing docs for IGT state:

<<
  There is no formal review requirement and regular contributors with
  commit access can push patches right after submitting them to the
  mailing lists. But invasive changes, new helper libraries and
  contributions from newcomers should go through a proper review to
  ensure overall consistency in the codebase.
>>


While not requiring reviews or acks has definitely increased the
speed of development, I feel the time for slowing down a bit has
come.

At the very least I would like to see all commits have a visit to the
mailing list before pushing, as the current docs already ask for. The
"right after" part would be changed to a $period of quarantine, maybe
24 hours?

As for requiring reviews or acks before pushing, how do the developers
at large feel about that? Different rules for different parts of IGT?
(Benchmarks, tools, tests, CI test sets, lib....)

The goal with this discussion is to reach a suitable tradeoff between
stability from CI point of view and fruitful use of programmer time.


-- 
Petri Latvala
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: IGT contributions and reviews

[Jani Nikula]

On Tue, 18 Oct 2016, Petri Latvala <petri.latvala@intel.com> wrote:
> The current contributing docs for IGT state:
>
> <<
>   There is no formal review requirement and regular contributors with
>   commit access can push patches right after submitting them to the
>   mailing lists. But invasive changes, new helper libraries and
>   contributions from newcomers should go through a proper review to
>   ensure overall consistency in the codebase.
>>>
>
>
> While not requiring reviews or acks has definitely increased the
> speed of development, I feel the time for slowing down a bit has
> come.

Agreed. (Though a more rigorous review requirement doesn't necessarily
slow things down in the big picture.)

> At the very least I would like to see all commits have a visit to the
> mailing list before pushing, as the current docs already ask for. The
> "right after" part would be changed to a $period of quarantine, maybe
> 24 hours?

Sounds good to me.

> As for requiring reviews or acks before pushing, how do the developers
> at large feel about that? Different rules for different parts of IGT?
> (Benchmarks, tools, tests, CI test sets, lib....)

I think there are two big buckets here:

* Tests in BAT and the BAT set list. I think we need r-b/ack on the
  mailing list on these changes before pushing. (In the long run, I'd
  like to have these go through a CI run with everything else unchanged
  too.)

* Everything else. Other tests and tools. I'd be happy with requiring
  the patches are sent to the list, and either receiving r-b/ack or 24
  hrs during weekdays without negative feedback.

> The goal with this discussion is to reach a suitable tradeoff between
> stability from CI point of view and fruitful use of programmer time.

Thanks for starting the discussion.

BR,
Jani.


-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: IGT contributions and reviews

[Daniel Vetter]

On Tue, Oct 18, 2016 at 07:33:10PM +0300, Jani Nikula wrote:
> On Tue, 18 Oct 2016, Petri Latvala <petri.latvala@intel.com> wrote:
> > The current contributing docs for IGT state:
> >
> > <<
> >   There is no formal review requirement and regular contributors with
> >   commit access can push patches right after submitting them to the
> >   mailing lists. But invasive changes, new helper libraries and
> >   contributions from newcomers should go through a proper review to
> >   ensure overall consistency in the codebase.
> >>>
> >
> >
> > While not requiring reviews or acks has definitely increased the
> > speed of development, I feel the time for slowing down a bit has
> > come.
> 
> Agreed. (Though a more rigorous review requirement doesn't necessarily
> slow things down in the big picture.)
> 
> > At the very least I would like to see all commits have a visit to the
> > mailing list before pushing, as the current docs already ask for. The
> > "right after" part would be changed to a $period of quarantine, maybe
> > 24 hours?
> 
> Sounds good to me.

We've already had this, and people stopped bothering. What will be
different this time around? I feel a bit like we do need to be a bit more
formal here, to really make this stick ...

Also, who's going to be the annoying maintainer who reminds everyone every
time they break the rules? It'll take some serious effort here to get
folks off their well-trodded paths onto a new one I think.
-Daniel

> > As for requiring reviews or acks before pushing, how do the developers
> > at large feel about that? Different rules for different parts of IGT?
> > (Benchmarks, tools, tests, CI test sets, lib....)
> 
> I think there are two big buckets here:
> 
> * Tests in BAT and the BAT set list. I think we need r-b/ack on the
>   mailing list on these changes before pushing. (In the long run, I'd
>   like to have these go through a CI run with everything else unchanged
>   too.)
> 
> * Everything else. Other tests and tools. I'd be happy with requiring
>   the patches are sent to the list, and either receiving r-b/ack or 24
>   hrs during weekdays without negative feedback.
> 
> > The goal with this discussion is to reach a suitable tradeoff between
> > stability from CI point of view and fruitful use of programmer time.
> 
> Thanks for starting the discussion.
> 
> BR,
> Jani.
> 
> 
> -- 
> Jani Nikula, Intel Open Source Technology Center
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: IGT contributions and reviews

[Jani Nikula]

On Wed, 19 Oct 2016, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Tue, Oct 18, 2016 at 07:33:10PM +0300, Jani Nikula wrote:
>> On Tue, 18 Oct 2016, Petri Latvala <petri.latvala@intel.com> wrote:
>> > The current contributing docs for IGT state:
>> >
>> > <<
>> >   There is no formal review requirement and regular contributors with
>> >   commit access can push patches right after submitting them to the
>> >   mailing lists. But invasive changes, new helper libraries and
>> >   contributions from newcomers should go through a proper review to
>> >   ensure overall consistency in the codebase.
>> >>>
>> >
>> >
>> > While not requiring reviews or acks has definitely increased the
>> > speed of development, I feel the time for slowing down a bit has
>> > come.
>> 
>> Agreed. (Though a more rigorous review requirement doesn't necessarily
>> slow things down in the big picture.)
>> 
>> > At the very least I would like to see all commits have a visit to the
>> > mailing list before pushing, as the current docs already ask for. The
>> > "right after" part would be changed to a $period of quarantine, maybe
>> > 24 hours?
>> 
>> Sounds good to me.
>
> We've already had this, and people stopped bothering. What will be
> different this time around? I feel a bit like we do need to be a bit more
> formal here, to really make this stick ...
>
> Also, who's going to be the annoying maintainer who reminds everyone every
> time they break the rules? It'll take some serious effort here to get
> folks off their well-trodded paths onto a new one I think.

What's your concrete proposal?

BR,
Jani.


> -Daniel
>
>> > As for requiring reviews or acks before pushing, how do the developers
>> > at large feel about that? Different rules for different parts of IGT?
>> > (Benchmarks, tools, tests, CI test sets, lib....)
>> 
>> I think there are two big buckets here:
>> 
>> * Tests in BAT and the BAT set list. I think we need r-b/ack on the
>>   mailing list on these changes before pushing. (In the long run, I'd
>>   like to have these go through a CI run with everything else unchanged
>>   too.)
>> 
>> * Everything else. Other tests and tools. I'd be happy with requiring
>>   the patches are sent to the list, and either receiving r-b/ack or 24
>>   hrs during weekdays without negative feedback.
>> 
>> > The goal with this discussion is to reach a suitable tradeoff between
>> > stability from CI point of view and fruitful use of programmer time.
>> 
>> Thanks for starting the discussion.
>> 
>> BR,
>> Jani.
>> 
>> 
>> -- 
>> Jani Nikula, Intel Open Source Technology Center
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: IGT contributions and reviews

[Daniel Vetter]

On Wed, Oct 19, 2016 at 1:26 PM, Jani Nikula
<jani.nikula@linux.intel.com> wrote:
> On Wed, 19 Oct 2016, Daniel Vetter <daniel@ffwll.ch> wrote:
>> On Tue, Oct 18, 2016 at 07:33:10PM +0300, Jani Nikula wrote:
>>> On Tue, 18 Oct 2016, Petri Latvala <petri.latvala@intel.com> wrote:
>>> > The current contributing docs for IGT state:
>>> >
>>> > <<
>>> >   There is no formal review requirement and regular contributors with
>>> >   commit access can push patches right after submitting them to the
>>> >   mailing lists. But invasive changes, new helper libraries and
>>> >   contributions from newcomers should go through a proper review to
>>> >   ensure overall consistency in the codebase.
>>> >>>
>>> >
>>> >
>>> > While not requiring reviews or acks has definitely increased the
>>> > speed of development, I feel the time for slowing down a bit has
>>> > come.
>>>
>>> Agreed. (Though a more rigorous review requirement doesn't necessarily
>>> slow things down in the big picture.)
>>>
>>> > At the very least I would like to see all commits have a visit to the
>>> > mailing list before pushing, as the current docs already ask for. The
>>> > "right after" part would be changed to a $period of quarantine, maybe
>>> > 24 hours?
>>>
>>> Sounds good to me.
>>
>> We've already had this, and people stopped bothering. What will be
>> different this time around? I feel a bit like we do need to be a bit more
>> formal here, to really make this stick ...
>>
>> Also, who's going to be the annoying maintainer who reminds everyone every
>> time they break the rules? It'll take some serious effort here to get
>> folks off their well-trodded paths onto a new one I think.
>
> What's your concrete proposal?

1. Bring up the topic.
2. Discuss it to death until suitable rough consensus emerges.
3. Roll out new rules and have a semi-evil maintainer (I can help with
that) enforce them.

I think anything up to full review starndards like we have them for
the kernel (for all of igt, for simplicity) should be on the table,
and would in my opinion be an adequate choice. Personally I'm leaning
more towards the process-heavy side. Replied to Paulo with more of my
reasons for that.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: IGT contributions and reviews

[Paulo Zanoni]

Em Qua, 2016-10-19 às 09:50 +0200, Daniel Vetter escreveu:
> On Tue, Oct 18, 2016 at 07:33:10PM +0300, Jani Nikula wrote:
> > 
> > On Tue, 18 Oct 2016, Petri Latvala <petri.latvala@intel.com> wrote:
> > > 
> > > The current contributing docs for IGT state:
> > > 
> > > <<
> > >   There is no formal review requirement and regular contributors
> > > with
> > >   commit access can push patches right after submitting them to
> > > the
> > >   mailing lists. But invasive changes, new helper libraries and
> > >   contributions from newcomers should go through a proper review
> > > to
> > >   ensure overall consistency in the codebase.
> > > > 
> > > > > 
> > > > > 
> > > 
> > > 
> > > While not requiring reviews or acks has definitely increased the
> > > speed of development, I feel the time for slowing down a bit has
> > > come.
> > 
> > Agreed. (Though a more rigorous review requirement doesn't
> > necessarily
> > slow things down in the big picture.)
> > 
> > > 
> > > At the very least I would like to see all commits have a visit to
> > > the
> > > mailing list before pushing, as the current docs already ask for.
> > > The
> > > "right after" part would be changed to a $period of quarantine,
> > > maybe
> > > 24 hours?
> > 
> > Sounds good to me.
> 
> We've already had this, and people stopped bothering. What will be
> different this time around? I feel a bit like we do need to be a bit
> more
> formal here, to really make this stick ...

I think the problem is that if only one single person doesn't follow
the rules, a few others will notice and start thinking: "If this guy
doesn't need to follow the rules, why do I need to? Why is Daniel
asking me to submit patches to the mailing list if that guy doesn't
seem to need to follow the same rule? Is this some sort of double-
standard?". Then person #2 stops following the rule, which makes more
people realize the same thing, which makes person #3 and #4 stop, etc.
So please, whatever rule we decide, let's make sure *everybody has to
follow it*, no exception for special cases, no allowing-people-to-get-
away-with-it. No double standards. </rant>

As a consequence of what I said, we do need to think: what if someone
doesn't follow the rule we decide? What will we *actually* do? Will it
really work if all we do is to politely ask them on IRC? Maybe the lack
of consequences is that degraded our previous rule?


> 
> Also, who's going to be the annoying maintainer who reminds everyone
> every
> time they break the rules? It'll take some serious effort here to get
> folks off their well-trodded paths onto a new one I think.
> -Daniel
> 
> > 
> > > 
> > > As for requiring reviews or acks before pushing, how do the
> > > developers
> > > at large feel about that? Different rules for different parts of
> > > IGT?
> > > (Benchmarks, tools, tests, CI test sets, lib....)
> > 
> > I think there are two big buckets here:
> > 
> > * Tests in BAT and the BAT set list. I think we need r-b/ack on the
> >   mailing list on these changes before pushing. (In the long run,
> > I'd
> >   like to have these go through a CI run with everything else
> > unchanged
> >   too.)
> > 
> > * Everything else. Other tests and tools. I'd be happy with
> > requiring
> >   the patches are sent to the list, and either receiving r-b/ack or
> > 24
> >   hrs during weekdays without negative feedback.
> > 
> > > 
> > > The goal with this discussion is to reach a suitable tradeoff
> > > between
> > > stability from CI point of view and fruitful use of programmer
> > > time.
> > 
> > Thanks for starting the discussion.
> > 
> > BR,
> > Jani.
> > 
> > 
> > -- 
> > Jani Nikula, Intel Open Source Technology Center
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: IGT contributions and reviews

[Daniel Vetter]

On Wed, Oct 19, 2016 at 3:06 PM, Paulo Zanoni <paulo.r.zanoni@intel.com> wrote:
>> > > At the very least I would like to see all commits have a visit to
>> > > the
>> > > mailing list before pushing, as the current docs already ask for.
>> > > The
>> > > "right after" part would be changed to a $period of quarantine,
>> > > maybe
>> > > 24 hours?
>> >
>> > Sounds good to me.
>>
>> We've already had this, and people stopped bothering. What will be
>> different this time around? I feel a bit like we do need to be a bit
>> more
>> formal here, to really make this stick ...
>
> I think the problem is that if only one single person doesn't follow
> the rules, a few others will notice and start thinking: "If this guy
> doesn't need to follow the rules, why do I need to? Why is Daniel
> asking me to submit patches to the mailing list if that guy doesn't
> seem to need to follow the same rule? Is this some sort of double-
> standard?". Then person #2 stops following the rule, which makes more
> people realize the same thing, which makes person #3 and #4 stop, etc.
> So please, whatever rule we decide, let's make sure *everybody has to
> follow it*, no exception for special cases, no allowing-people-to-get-
> away-with-it. No double standards. </rant>
>
> As a consequence of what I said, we do need to think: what if someone
> doesn't follow the rule we decide? What will we *actually* do? Will it
> really work if all we do is to politely ask them on IRC? Maybe the lack
> of consequences is that degraded our previous rule?

Yeah I think that was kinda what happened. At first I did gently prod
people who bent the rules a bit into what I think would be the better
direction. But pretty much from the start I didn't want to be too much
of an annoyance, because when I put my maintainer dictator hammer down
and said I want to see testcases I think a few people were very
sceptical. Hence I was really happy with every engineer who discovered
that igts are useful and started hacking on them like mad, even if the
process went out of the window a bit.

But I do think that now igt and tests as a primary deliverable is
well-established, and we seem to have a few issues from the lack of
any kind of review or otherwise coordination:
- Duplicated tests, and sometimes duplicated test infrastructure that
probably should be better extracted into helper libraries.
- Much harder to ramp up new folks since they can't go watch the pros
do their work on the m-l like on the kernel side.
- Defacto double-standards pissing off contributors.
- Lack of sharing of useful testing tricks. E.g. I only learned by
accident of Chris' very clever delay-batch trick, and I think that's
something which should be used in a lot more places.
- And finally I think we have a bit a quality problem with tests
itself ("works on my machine, it must be perfect") which is causing
some serious hiccups and fun with CI.

Would love to here the thoughts of others here.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: IGT contributions and reviews

[Jani Nikula]

On Wed, 19 Oct 2016, Paulo Zanoni <paulo.r.zanoni@intel.com> wrote:
> Why is Daniel asking me to submit patches to the mailing list if that
> guy doesn't seem to need to follow the same rule? Is this some sort of
> double- standard?".

Not to take anything away from a perfectly good rant, but from now on
it's really s/Daniel/Petri/ for IGT.

BR,
Jani.



-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx