FAQ Search Today's Posts Mark Forums Read
» Video Reviews

» Linux Archive

Linux-archive is a website aiming to archive linux email lists and to make them easily accessible for linux users/developers.


» Sponsor

» Partners

» Sponsor

Go Back   Linux Archive > Gentoo > Gentoo Development

 
 
LinkBack Thread Tools
 
Old 04-25-2010, 10:06 AM
Ryan Hill
 
Default Requiring two sets of eyes for all eclass commits

On Sat, 24 Apr 2010 20:40:54 +0300
Petteri Räty <betelgeuse@gentoo.org> wrote:

> What do you think about not allowing commits to eclasses without
> mentioning an another developer who has reviewed and approved the diff
> in the commit message? There's enough people on gentoo-dev for urgent
> stuff too.

I think it's a good idea to strongly encourage it, but actually forcing it
through cvs? No thanks. I'm not tracking down another dev just to fix a
spelling mistake. :P


--
fonts, by design, by neglect
gcc-porting, for a fact or just for effect
wxwidgets @ gentoo EFFD 380E 047A 4B51 D2BD C64F 8AA8 8346 F9A4 0662
 
Old 04-25-2010, 10:11 AM
Petteri Räty
 
Default Requiring two sets of eyes for all eclass commits

On 04/25/2010 01:06 PM, Ryan Hill wrote:
> On Sat, 24 Apr 2010 20:40:54 +0300
> Petteri Räty <betelgeuse@gentoo.org> wrote:
>
>> What do you think about not allowing commits to eclasses without
>> mentioning an another developer who has reviewed and approved the diff
>> in the commit message? There's enough people on gentoo-dev for urgent
>> stuff too.
>
> I think it's a good idea to strongly encourage it, but actually forcing it
> through cvs? No thanks. I'm not tracking down another dev just to fix a
> spelling mistake. :P
>
>

How did the spelling mistake get there in the first place? A review
system should reduce having them in the first place.

Regards,
Petteri
 
Old 04-25-2010, 10:14 AM
Petteri Räty
 
Default Requiring two sets of eyes for all eclass commits

On 04/24/2010 09:14 PM, Alexis Ballier wrote:
> On Sat, 24 Apr 2010 20:40:54 +0300
> Petteri Räty <betelgeuse@gentoo.org> wrote:
>
>> 17:34 < Betelgeuse> robbat2|na: how easy to it to prevent commits to
>> CVS if the commit message doesn't match a certain pattern?
>> 17:36 <@robbat2|na> go and checkout the CVSROOT and there should be an
>> example there
>> 17:37 < Betelgeuse> robbat2|na: Ok so doable then. Thanks.
>>
>> What do you think about not allowing commits to eclasses without
>> mentioning an another developer who has reviewed and approved the diff
>> in the commit message? There's enough people on gentoo-dev for urgent
>> stuff too.
>
> no thanks; we already have the policy to require that major changes to
> broad impact eclasses have gone through -dev, no need to add more
> bureaucracy.
>

But the policy is not tested by the quizzes and we have had cases lately
where large diffs have been committed without gentoo-dev review. With
peer review it's likely that the reviewer is familiar with what should
be sent to gentoo-dev as hesitant/new people won't give their approval
that easily.

Regards,
Petteri
 
Old 04-25-2010, 10:54 AM
Roy Bamford
 
Default Requiring two sets of eyes for all eclass commits

On 2010.04.24 18:40, Petteri Räty wrote:
> 17:34 < Betelgeuse> robbat2|na: how easy to it to prevent commits to
> CVS
> if the commit message doesn't match a certain pattern?
> 17:36 <@robbat2|na> go and checkout the CVSROOT and there should be
> an
> example there
> 17:37 < Betelgeuse> robbat2|na: Ok so doable then. Thanks.
>
> What do you think about not allowing commits to eclasses without
> mentioning an another developer who has reviewed and approved the
> diff
> in the commit message? There's enough people on gentoo-dev for urgent
> stuff too.
>
> Regards,
> Petteri
>
>
In industry, the practice is called peer review. Its generally thought
to be a GoodThing as its part of the process of trapping errors as
early as possible in the process, where they have lowest cost.

We cannot easily attribute cost in terms of money, so think about it in
developer and user hours wasted as errors 'escape'.

Industry also recognises the need that any process needs to be tailored
to the circumstance so the peer review process is not enforced. Project
groups are permitted to assess the risk of screwing up against the cost
of a fix. (That's overly simplistic).

In short, following industry best practice, the peer review process
should be strongly encouraged but we should stop short of using tools
to enforce it.

--
Regards,

Roy Bamford
(Neddyseagoon) a member of
gentoo-ops
forum-mods
trustees
 
Old 04-25-2010, 11:36 AM
Ryan Hill
 
Default Requiring two sets of eyes for all eclass commits

On Sun, 25 Apr 2010 13:11:11 +0300
Petteri Räty <betelgeuse@gentoo.org> wrote:

> On 04/25/2010 01:06 PM, Ryan Hill wrote:

> > I think it's a good idea to strongly encourage it, but actually forcing it
> > through cvs? No thanks. I'm not tracking down another dev just to fix a
> > spelling mistake. :P
>
> How did the spelling mistake get there in the first place? A review
> system should reduce having them in the first place.

People make mistakes. Anyways my point is that requiring a peer review for
trivial changes is just unneeded bureaucracy. Even for non-trivial changes,
it doesn't make sense to force someone who knows their eclass inside out and
knows what they're doing to get a review from someone else who may not have a
clue. I'm not saying that peer-review shouldn't be done; it's a very good
idea, especially if you're new, or unsure of your changes, or you have a team
consisting of more than one person. In fact I would support a policy that
said eclasses need to be reviewed before committing. But enforcing it through
cvs is never going to fly. Just use common sense.

If we were having ongoing issues with people breaking eclasses then I could
see where you're coming from. But as it is, it's a non-problem.


--
fonts, by design, by neglect
gcc-porting, for a fact or just for effect
wxwidgets @ gentoo EFFD 380E 047A 4B51 D2BD C64F 8AA8 8346 F9A4 0662
 
Old 04-25-2010, 12:04 PM
Richard Freeman
 
Default Requiring two sets of eyes for all eclass commits

On 04/25/2010 07:36 AM, Ryan Hill wrote:

People make mistakes.


Agreed - at work I've often found a quality mindset that is 100% focused
on preventing mistakes, and I've found that these kinds of systems are
almost equally as focused on preventing them from being fixed (three
minutes to fix a bug, three weeks to document and release the fix - then
we wonder why the system has hundreds of trivial open bugs with no ROI
for fixing them).


A good quality system isn't just about preventing mistakes - it needs to
be about fixing them too. The system that prevents typos from getting
into the tree shouldn't get in the way of those typos being fixed.
There needs to be a balance.


Scripts running on repository servers don't have a sense of balance, so
they aren't the answer. Nor is cutting off hands any time a dev messes
up unless it becomes a pattern or there is malicious intent.


However, a systematic review process is probably a good thing most of
the time, and published policies supporting this are a good thing.


Rich
 
Old 04-25-2010, 01:10 PM
Petteri Räty
 
Default Requiring two sets of eyes for all eclass commits

On 04/26/2010 01:42 AM, Alistair Bush wrote:
>> On 04/24/2010 09:14 PM, Alexis Ballier wrote:
>>> On Sat, 24 Apr 2010 20:40:54 +0300
>>>
>>> Petteri Räty <betelgeuse@gentoo.org> wrote:
>>>> 17:34 < Betelgeuse> robbat2|na: how easy to it to prevent commits to
>>>> CVS if the commit message doesn't match a certain pattern?
>>>> 17:36 <@robbat2|na> go and checkout the CVSROOT and there should be an
>>>> example there
>>>> 17:37 < Betelgeuse> robbat2|na: Ok so doable then. Thanks.
>>>>
>>>> What do you think about not allowing commits to eclasses without
>>>> mentioning an another developer who has reviewed and approved the diff
>>>> in the commit message? There's enough people on gentoo-dev for urgent
>>>> stuff too.
>>>
>>> no thanks; we already have the policy to require that major changes to
>>> broad impact eclasses have gone through -dev, no need to add more
>>> bureaucracy.
>>
>> But the policy is not tested by the quizzes and we have had cases lately
>> where large diffs have been committed without gentoo-dev review. With
>> peer review it's likely that the reviewer is familiar with what should
>> be sent to gentoo-dev as hesitant/new people won't give their approval
>> that easily.
>
> 1) Why is it of any relevance whether or not the quizzes test this policy?

I doubt recruits read all of our documentation while answering the
quizzes. This would just enforce behavior.

> 2) Where is this policy recorded, and why does devmanual.g.o seem to
> (possibly) contradict it? [1] I'm not sure of the nature of the commits but
> were they non-general?
>

http://devmanual.gentoo.org/eclass-writing/index.html

"Before updating eutils or a similar widely used eclass, it is best to
email the gentoo-dev list."

>
> [1] "It is not usually necessary to email the gentoo-dev list before making
> changes to a non-general eclass which you maintain. Use common sense here."

Yeah it's not spelled that clearly there.

Regards,
Petteri
 
Old 04-25-2010, 03:22 PM
"Jorge Manuel B. S. Vicetto"
 
Default Requiring two sets of eyes for all eclass commits

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 25-04-2010 13:10, Petteri Räty wrote:
> On 04/26/2010 01:42 AM, Alistair Bush wrote:
>>> On 04/24/2010 09:14 PM, Alexis Ballier wrote:
>>>> On Sat, 24 Apr 2010 20:40:54 +0300
>>>>
>>>> Petteri Räty <betelgeuse@gentoo.org> wrote:
>>>>> 17:34 < Betelgeuse> robbat2|na: how easy to it to prevent commits to
>>>>> CVS if the commit message doesn't match a certain pattern?
>>>>> 17:36 <@robbat2|na> go and checkout the CVSROOT and there should be an
>>>>> example there
>>>>> 17:37 < Betelgeuse> robbat2|na: Ok so doable then. Thanks.
>>>>>
>>>>> What do you think about not allowing commits to eclasses without
>>>>> mentioning an another developer who has reviewed and approved the diff
>>>>> in the commit message? There's enough people on gentoo-dev for urgent
>>>>> stuff too.
>>>>
>>>> no thanks; we already have the policy to require that major changes to
>>>> broad impact eclasses have gone through -dev, no need to add more
>>>> bureaucracy.
>>>
>>> But the policy is not tested by the quizzes and we have had cases lately
>>> where large diffs have been committed without gentoo-dev review. With
>>> peer review it's likely that the reviewer is familiar with what should
>>> be sent to gentoo-dev as hesitant/new people won't give their approval
>>> that easily.

>> 2) Where is this policy recorded, and why does devmanual.g.o seem to
>> (possibly) contradict it? [1] I'm not sure of the nature of the commits but
>> were they non-general?
>>
> http://devmanual.gentoo.org/eclass-writing/index.html
>
> "Before updating eutils or a similar widely used eclass, it is best to
> email the gentoo-dev list."

The important bit there is "widely used eclass". I agree with having
peer review, but as others argued I don't agree with the recent trend to
make it increasingly harder to commit stuff to the tree.
To expand on the above, by "widely used eclass" I interpret eutils,
autotools, base and similar eclasses because even though some eclasses
like the kde4 eclasses are used by many ebuilds, it's mostly a team's
eclass and not a general eclass shared by everyone.

- --
Regards,

Jorge Vicetto (jmbsvicetto) - jmbsvicetto at gentoo dot org
Gentoo- forums / Userrel / Devrel / KDE / Elections
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.15 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iQIcBAEBAgAGBQJL1F5OAAoJEC8ZTXQF1qEPNf4QAIoyr8IELe ixBJDHvv2ACaMw
omUQj5nbjFB1Xc7Die4dT7TkUfJ2/QddMcG1I/CNTNEdRtJAR31UDS2Lbm7gOj11
wpd+g7mDQuJZdW3873YXkThoynqS3xzfpZocxb2s+adxyXF6Mh 65+N+ZT515HMh8
DujqxGHxjA4Cqn/zGe6ClqfRwxfGZkYkA/eQfX9m7TSJHTwxK4sijhFNphSsA89E
qyVW0Y18mrf0pVBpUaQ4kfCuwp0HWOIoubSCIo47KFINfL4Tte aX1NTOP9JzEIDH
saCUURHQw2nWTsPDNjvL6euvriyTZpm0lhHR86j87maDVeGFDn 0PZ8Cs/ypCVhxJ
1MxdL7NvyUptHO6UGUvluqZzh4zTDEsotCRWzyshEPAy51q+rW bMLPVeDZfkVgl8
/0VlhNFgdoxuowOEK4AiTWifp5oa5RO03K5Hyfze2IfFXArva7Z nb5oCGeHoEBqn
Kr5trpgIqyW+v3XifurbuOSoU7BDTlzH3WrkCRJq0pP5Hogtod 0wf1tAy/wQ+F+3
yUphi3tzMFMlFoqBE5pfjyTa22vi/RjNVgH3sie6HD8qZsmKJIILb2Y3NrloCzlB
WYdCP0+dfFOafqFMpAFUuI3E7zrKacQK7mshLE9vTNa+dh6LVA Sn2WIaLdtse1GK
sUN2ESxQteslTvu7v6r/
=GLm6
-----END PGP SIGNATURE-----
 
Old 04-25-2010, 09:06 PM
Ryan Hill
 
Default Requiring two sets of eyes for all eclass commits

On Sun, 25 Apr 2010 05:01:17 -0700
Alec Warner <antarus@gentoo.org> wrote:

> On Sun, Apr 25, 2010 at 4:36 AM, Ryan Hill <dirtyepic@gentoo.org> wrote:
> > said eclasses need to be reviewed before committing. *But enforcing it through
> > cvs is never going to fly. *Just use common sense.
>
> Sure it will; you just need to create the tools with flexibility in
> mind. For instance:
>
> 1) Require peer review on all eclasses
> 2) Do not require peer review for changes less than N lines
> 3) enable a commiter to over-ride the review with some kind of option
> (--force or similar)
> 4) enable an eclass-owner to opt-out of the review process entirely
> with some kind of flag.
>
> I am not aware of how robust the pre-commit hooks in cvs are so I
> cannot comment on how easy these things are to implement.

Well, I didn't mean technically. But we could achieve the same general
effect of the above without installing padlocks on cvs. Just let projects or
teams establish their own review policies like they already do. For example,
if you commit changes to toolchain.eclass without consulting Mike, you'll
soon find an email in your inbox. If you mess with wxwidgets.eclass without
running it by me you'll find your commit reverted. On the other hand,
anyone in the font, X, or tex herds can modify font.eclass. Let people
establish rules suited to their project's workflow and enforce them as they
see fit.

Again, if there were some evidence this isn't working then it should be
presented. Otherwise I don't see any reason to change the status quo.


--
fonts, by design, by neglect
gcc-porting, for a fact or just for effect
wxwidgets @ gentoo EFFD 380E 047A 4B51 D2BD C64F 8AA8 8346 F9A4 0662
 
Old 04-25-2010, 10:42 PM
Alistair Bush
 
Default Requiring two sets of eyes for all eclass commits

> On 04/24/2010 09:14 PM, Alexis Ballier wrote:
> > On Sat, 24 Apr 2010 20:40:54 +0300
> >
> > Petteri Räty <betelgeuse@gentoo.org> wrote:
> >> 17:34 < Betelgeuse> robbat2|na: how easy to it to prevent commits to
> >> CVS if the commit message doesn't match a certain pattern?
> >> 17:36 <@robbat2|na> go and checkout the CVSROOT and there should be an
> >> example there
> >> 17:37 < Betelgeuse> robbat2|na: Ok so doable then. Thanks.
> >>
> >> What do you think about not allowing commits to eclasses without
> >> mentioning an another developer who has reviewed and approved the diff
> >> in the commit message? There's enough people on gentoo-dev for urgent
> >> stuff too.
> >
> > no thanks; we already have the policy to require that major changes to
> > broad impact eclasses have gone through -dev, no need to add more
> > bureaucracy.
>
> But the policy is not tested by the quizzes and we have had cases lately
> where large diffs have been committed without gentoo-dev review. With
> peer review it's likely that the reviewer is familiar with what should
> be sent to gentoo-dev as hesitant/new people won't give their approval
> that easily.

1) Why is it of any relevance whether or not the quizzes test this policy?
2) Where is this policy recorded, and why does devmanual.g.o seem to
(possibly) contradict it? [1] I'm not sure of the nature of the commits but
were they non-general?

- Alistair

[1] "It is not usually necessary to email the gentoo-dev list before making
changes to a non-general eclass which you maintain. Use common sense here."
 

Thread Tools




All times are GMT. The time now is 09:19 AM.

VBulletin, Copyright ©2000 - 2014, Jelsoft Enterprises Ltd.
Content Relevant URLs by vBSEO ©2007, Crawlability, Inc.
Copyright ©2007 - 2008, www.linux-archive.org