Re: [Spice-devel] [PATCH spice-protocol] build-sys: simplify autogen

2014-12-08 Thread Christophe Fergeau
On Fri, Dec 05, 2014 at 07:18:50PM +0100, Marc-André Lureau wrote:
 Where did you see that all patches have to be mandatorily reviewed in
 Spice? We always had a trivial push rule, and you always complained
 about it. That's all I know.

What I remember from my first few years in SPICE was that all patches
were going to the mailing lists before push. I don't remember any patch
being pushed without prior review.
Then it was implicitly agreed that a trivial push rule would not hurt,
and I started complaining about some dubious patches you pushed abusing
that rule. I don't remember complaining about anyone else's patch, nor
seeing a lot of trivial pushes from anyone else (I've done a few myself,
most of the time one liners or so).

Christophe


pgpZnVPHbePHD.pgp
Description: PGP signature
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel


Re: [Spice-devel] [PATCH spice-protocol] build-sys: simplify autogen

2014-12-08 Thread Uri Lublin

On 12/06/2014 04:04 AM, Fabiano Fidêncio wrote:

On Fri, Dec 5, 2014 at 11:08 PM, Marc-André Lureau wrote:

On Fri, Dec 5, 2014 at 10:38 PM, Jonathon Jongsma wrote:

For what it's worth, I basically agree with Christophe and Jeremy.

I agree with Marc-André here.


I too agree with Marc-Andre about _trivial_ patches (such as typo fixing).
For any non-trivial patch, a review should be performed.


Just to finish, IMHO, this patch, specifically, was not a trivial one.


The problem here is that trivial is subjective.
I view this specific patch as not trivial, as it practically replaces 
the whole file.
I understand why Marc-Andre thinks it is trivial as the result of it is 
a common autogen.sh.



Thanks,
Uri.

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel


Re: [Spice-devel] [PATCH spice-protocol] build-sys: simplify autogen

2014-12-08 Thread Uri Lublin

On 12/05/2014 05:52 PM, Marc-André Lureau wrote:

On Fri, Dec 5, 2014 at 4:12 PM, Christophe Fergeau cferg...@redhat.com wrote:

On Fri, Dec 05, 2014 at 03:57:29PM +0100, Marc-André Lureau wrote:

The blame will be anyway on the one who
typed it forever.

I have absolutely no interest in blaming people after the fact, I prefer
to fix things before the mistake happens ;)

By that I mean that they are aware of the responsability of doing
unreview commit.


Nothing like replacing a crufted autogen with an obvious autoreconf.

Is this what you did? This is not what I read in the commit log.

Use autoreconf, allow out of tree autogen.sh run.


Quick for me is a matter of minutes.

Even if it's a few hours, or a few days, is it a big deal?

That's a big factor, yes.


There are a lot of trivial patches that have been pending for days.

This means we need to improve on reviews :) Any pointers?

I can point you to patches, but you should try to remember your own for a start.

Also, you can see that other projects have troubles with that, ex
http://wiki.qemu.org/Contribute/TrivialPatches: they are moving the
problem to me, but perhaps it helps.


Note that those trivial patches are being reviewed by the trivial 
patches team





Improving the change can be done immediately upstream or by a
after-commit review. That's not a valid argument.

With pre-commit review, you ensure that at least one person read the
patch. With post-commit review, you have no such guarantee.

With current workflow, you have no guaratee that unreviewed patch go
there by mistake or maliciously. We would need a tool for that.
For me this is the job of maintainer to quickly review each commit
before release.


I disagree.
When doing a release, a maintainer should _not_ review all commits.
Those commits should have been reviewed before being committed.
The number of patches added since the previous release can be large,
and re-reviewing all of them can be too much work for little gain.

Regards,
Uri.

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel


Re: [Spice-devel] [PATCH spice-protocol] build-sys: simplify autogen

2014-12-08 Thread Marc-André Lureau
On Mon, Dec 8, 2014 at 1:47 PM, Uri Lublin u...@redhat.com wrote:
 With current workflow, you have no guaratee that unreviewed patch go
 there by mistake or maliciously. We would need a tool for that.
 For me this is the job of maintainer to quickly review each commit
 before release.


 I disagree.
 When doing a release, a maintainer should _not_ review all commits.
 Those commits should have been reviewed before being committed.
 The number of patches added since the previous release can be large,
 and re-reviewing all of them can be too much work for little gain.

This contradicts a bit the fact that you can do commit without review.

 I said quickly, doing thorough review of all commits before a
release is not doable. But it is the role of the maintainer to check
all the commits that go in a repository, because we don't have ways to
enforce that all patches are the one that are ack on ML. (and I don't
think we need that, to me the process works well so far)


-- 
Marc-André Lureau
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel


Re: [Spice-devel] [PATCH spice-protocol] build-sys: simplify autogen

2014-12-08 Thread Christophe Fergeau
On Mon, Dec 08, 2014 at 01:53:43PM +0100, Marc-André Lureau wrote:
 This contradicts a bit the fact that you can do commit without review.
 
  I said quickly, doing thorough review of all commits before a
 release is not doable. But it is the role of the maintainer to check
 all the commits that go in a repository, because we don't have ways to
 enforce that all patches are the one that are ack on ML. (and I don't
 think we need that, to me the process works well so far)

If you send all patches to spice-devel either for ACKing, or as a
notice that you pushed a patch as trivial, then this kind of commits can
be caught through spice-commits@
Then maybe the person doing the release won't trust people with commit
access to commit the patches that were ACKed unmodified, which is
another can of worms ;)

Christophe


pgpHv2SkVfW5w.pgp
Description: PGP signature
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel


Re: [Spice-devel] [PATCH spice-protocol] build-sys: simplify autogen

2014-12-08 Thread Uri Lublin

On 12/08/2014 02:53 PM, Marc-André Lureau wrote:

On Mon, Dec 8, 2014 at 1:47 PM, Uri Lublin u...@redhat.com wrote:

With current workflow, you have no guaratee that unreviewed patch go
there by mistake or maliciously. We would need a tool for that.
For me this is the job of maintainer to quickly review each commit
before release.


I disagree.
When doing a release, a maintainer should _not_ review all commits.
Those commits should have been reviewed before being committed.
The number of patches added since the previous release can be large,
and re-reviewing all of them can be too much work for little gain.

This contradicts a bit the fact that you can do commit without review.
  I said quickly, doing thorough review of all commits before a
release is not doable. But it is the role of the maintainer to check
all the commits that go in a repository, because we don't have ways to
enforce that all patches are the one that are ack on ML. (and I don't
think we need that, to me the process works well so far)


OK.

I agree that a maintainer should quick review the patches.
At least to know what's new in the release and update some files (e.g. 
NEWS).


Hopefully the maintainer would catch bad commits, pushed by mistake.
(Let's (naively?) assume no malicious commits are pushed).
That does not happen often (or at least not enough data about 
bad-commits is available).


A maintainer can push a commit without review, but one should not for 
most patches.
And those trivial patches, by definition, do not need to get reviewed by 
the maintainer

doing the release.

Uri
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel


Re: [Spice-devel] [PATCH spice-protocol] build-sys: simplify autogen

2014-12-08 Thread Jonathon Jongsma
On Sat, 2014-12-06 at 00:14 +0200, Alon Levy wrote:
 On 12/06/2014 12:00 AM, Jonathon Jongsma wrote:
  On Fri, 2014-12-05 at 23:41 +0200, Alon Levy wrote:
  [snip]
 
  At the same time, I'm not sure mailing lists are the right tool for code
  review.  It's difficult to track which patches have been reviewed and
  which haven't.
 
  http://patchwork.freedesktop.org/project/Spice/list/ can help, linked
  from the wiki btw (http://www.spice-space.org/page/Main_Page)
 
  (not getting into this otherwise :)
 
  
  Thanks, I had forgotten about that. I notice now that all patches listed
  on that site are in state New. Is there a magical incantation we can
  insert into our review that will change the patch state to Approved or
  Needs Work or something? If so, can we start using this incantation
  rather than our traditional ACK, so that patchwork will be able to
  track the true state of these patches?
 
 I don't know that magic, but it does sound handy.


Hrm, after trawling the patchwork mailing list archives, it appears that
it's not actually possible to change the patch state via email. The
authors consider this to be insecure since anybody could change the
review state:
https://lists.ozlabs.org/pipermail/patchwork/2012-April/000700.html

That's too bad since I think it makes it significantly less useful as a
passive tool for tracking patches. However we could at least set up the
provided git post-receive hook that sets a patch status to Accepted
after it is pushed to the repository.





___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel


Re: [Spice-devel] [PATCH spice-protocol] build-sys: simplify autogen

2014-12-08 Thread Christophe Fergeau
Hey,

On Mon, Dec 08, 2014 at 10:06:13AM -0600, Jonathon Jongsma wrote:
 Hrm, after trawling the patchwork mailing list archives, it appears that
 it's not actually possible to change the patch state via email. The
 authors consider this to be insecure since anybody could change the
 review state:
 https://lists.ozlabs.org/pipermail/patchwork/2012-April/000700.html
 
 That's too bad since I think it makes it significantly less useful as a
 passive tool for tracking patches. However we could at least set up the
 provided git post-receive hook that sets a patch status to Accepted
 after it is pushed to the repository.

For what it's worth, Daniel Veillard hacked on patchchecker at some
point for libvirt patch tracking:
https://www.redhat.com/archives/libvir-list/2011-June/msg00418.html
http://libvirt.org/git/?p=patchchecker.git;a=summary

Christophe


pgpqBj5p2Mu0X.pgp
Description: PGP signature
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel


Re: [Spice-devel] [PATCH spice-protocol] build-sys: simplify autogen

2014-12-08 Thread Jonathon Jongsma
On Fri, 2014-12-05 at 23:08 +0100, Marc-André Lureau wrote:
 On Fri, Dec 5, 2014 at 10:38 PM, Jonathon Jongsma jjong...@redhat.com wrote:
  For what it's worth, I basically agree with Christophe and Jeremy.
  (Although I think that describing it as mandatory code review is
  over-stating the case a bit -- there is nothing but peer pressure and
  polite requests preventing contributors from pushing unreviewed code).
 
 Peer pressure is precisely what one should try to avoid. If you think
 your change does not require second look, because it is trivial, then
 why would you do it? Now you will have to bother others repeatedly for
 the most basic thing that they might not even care about. I would
 rather work differently as I explained before, and as we did until
 now.
 

By peer pressure, I simply mean the expectations that are shared by
contributors of the project. And I think that some set of shared
expectations are essential in a well-functioning project. So I disagree
that peer pressure is something that should always be avoided. If
somebody violates the shared expectations of the project, there *should*
be some pressure to either stop violating those expectations, or
re-negotiate a new set of shared expectations. Which is basically what
we're doing now, right?

Jonathon

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel


Re: [Spice-devel] [PATCH spice-protocol] build-sys: simplify autogen

2014-12-05 Thread Jeremy White

Yes, that it should not have been pushed.


I agree.

I would prefer we eliminated the trivial push rule altogether.

Cheers,

Jeremy
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel


Re: [Spice-devel] [PATCH spice-protocol] build-sys: simplify autogen

2014-12-05 Thread Marc-André Lureau
Hi

- Original Message -
  Yes, that it should not have been pushed.
 
 I agree.
 
 I would prefer we eliminated the trivial push rule altogether.

I agree, if it can help to avoid this kind of endless discussion.

But to me, it makes the project less friendly if people have no trust to each 
other for the most basic and obvious improvements. I am not talking about 
controversial or complicated fixes. But doc addition, build-sys, cleaning, 
spelling: this all qualifies to something that is an obvious improvement that I 
can trust people who have commit access to do the right call. This is to me 
more healthy than having to bother and wait for each other through a mailing 
list. It also allows to prioritize, trivial things shouldn't be at the same 
level as critical bug fix or protocol changes.

I am first a GNOME developper, where anyone can push changes without review. 
This is based on a trust and meritocratic relationship too, and I like it, and 
afaik it works well. In fact, most projects I know follow that rule. It makes 
sense to me that maintainers and main contributors can decide to push changes 
without bothering and waiting on others. It will be looked over by other people 
anyway. If they don't, they should try to keep looking at recent changes. 
Because we are not self contain project, we need to do that anyway for many 
other projects we depend on. But there is no need to force people to check and 
review every single minor improvements. There are more important pieces of our 
stack where changes can go without review (all of them?).

I think this rule should be left to the maintainer, and as a maintainer of some 
of the Spice project, I prefer to have a trustful relationship and let people 
commit directly. It's really not much, if the change is wrong, it can be 
reverted, not a big deal.

With a bit more perspective, a project where every single change has to be 
approved is not an open project to me. I'd prefer to work elsewhere.
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel


Re: [Spice-devel] [PATCH spice-protocol] build-sys: simplify autogen

2014-12-05 Thread Jeremy White

I agree, if it can help to avoid this kind of endless discussion.


:-/.  Yes, a good point.



But to me, it makes the project less friendly if people have no trust to each 
other for the most basic and obvious improvements. I am not talking about 
controversial or complicated fixes. But doc addition, build-sys, cleaning, 
spelling: this all qualifies to something that is an obvious improvement that I 
can trust people who have commit access to do the right call. This is to me 
more healthy than having to bother and wait for each other through a mailing 
list. It also allows to prioritize, trivial things shouldn't be at the same 
level as critical bug fix or protocol changes.


I don't think it's about trust.  In my mind, it's about simplicity and 
rigor.  If every patch requires two reviews, that's easy to understand 
and sets clear expectations.


Anything else is subjective, which can lead to anger.

I have screwed up so many times in the past, and it always seems my 
biggest goofs are when I'm at my most confident.  So I am always 
grateful for a review, and I like the ideal that every patch, no matter 
how small, is reviewed by at least two people.




I am first a GNOME developper, where anyone can push changes without review. 
This is based on a trust and meritocratic relationship too, and I like it, and 
afaik it works well. In fact, most projects I know follow that rule. It makes 
sense to me that maintainers and main contributors can decide to push changes 
without bothering and waiting on others. It will be looked over by other people 
anyway. If they don't, they should try to keep looking at recent changes. 
Because we are not self contain project, we need to do that anyway for many 
other projects we depend on. But there is no need to force people to check and 
review every single minor improvements. There are more important pieces of our 
stack where changes can go without review (all of them?).


My instincts come from Wine, Xorg, and Linux.  I believe Xorg is trying 
for a rigorous signed off by / reviewed by policy, similar to what the 
Kernel strives for.  Wine is more dictatorial; everything is reviewed by 
at least two people, unless it's Alexandre's work :-/.




I think this rule should be left to the maintainer, and as a maintainer of some 
of the Spice project, I prefer to have a trustful relationship and let people 
commit directly. It's really not much, if the change is wrong, it can be 
reverted, not a big deal.


I'm espousing an ideal.  It may be that practical realities of patch 
flow, and review bandwidth make my ideal undesirable for Spice.


And I think the people with the most credibility in this conversation 
are the maintainers who do a great deal of patch review.  That's not me; 
so I don't think I deserve a particularly large 'vote'.


Cheers,

Jeremy
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel


Re: [Spice-devel] [PATCH spice-protocol] build-sys: simplify autogen

2014-12-05 Thread Christophe Fergeau
Hey,

On Fri, Dec 05, 2014 at 08:46:16AM -0500, Marc-André Lureau wrote:
 - Original Message -
   Yes, that it should not have been pushed.
  
  I agree.
  
  I would prefer we eliminated the trivial push rule altogether.
 
 I agree, if it can help to avoid this kind of endless discussion.
 
 But to me, it makes the project less friendly if people have no trust
 to each other for the most basic and obvious improvements.

Basic, obvious is very subjective...  It's not a matter of trusting
people or not (I'm the person I trust the least, even for the most
trivial change btw), it's a matter of improving the quality of commits
with reviews. Even the most basic commit can have a typo in its commit
message.

 I am not talking about controversial or complicated fixes. But doc
 addition, build-sys, cleaning, spelling: this all qualifies to
 something that is an obvious improvement that I can trust people who
 have commit access to do the right call.

The spice-protocol submodule removal is an obvious improvement (to me),
and is only making build-sys related changes, but pushing it without
review would have been a very bad call.

 This is to me more healthy than having to bother
 and wait for each other through a mailing list.

Turnaround should be fairly quick with a trivial patch. The patch is not
going to get worse by waiting on the mailing list, but it may end up
being better, so it's a win-win situation.


 I am first a GNOME developper, where anyone can push changes without review.

This is not true, this depends on the module, the maintainer, your
relationship with that maintainer and the module, ...

 I think this rule should be left to the maintainer, and as a
 maintainer of some of the Spice project, I prefer to have a trustful
 relationship and let people commit directly.

Since you are playing the maintainer card, hopefully as a maintainer you
are going to listen how people (at least 2 ) in your project community
want to work, and not unilaterally change the way commits have been
handled in the past.

 It's really not much, if
 the change is wrong, it can be reverted, not a big deal.

Not a big deal save for history cluttering, the need to be careful when
backporting patches if the commit was followed by a fixup commit, no way
for fixing commit log typos, or for adding missing information, ...

Christophe



pgpM0gVuQoeVS.pgp
Description: PGP signature
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel


Re: [Spice-devel] [PATCH spice-protocol] build-sys: simplify autogen

2014-12-05 Thread Marc-André Lureau
On Fri, Dec 5, 2014 at 3:37 PM, Christophe Fergeau cferg...@redhat.com wrote:
 But to me, it makes the project less friendly if people have no trust
 to each other for the most basic and obvious improvements.

 Basic, obvious is very subjective...  It's not a matter of trusting
 people or not (I'm the person I trust the least, even for the most
 trivial change btw), it's a matter of improving the quality of commits
 with reviews. Even the most basic commit can have a typo in its commit
 message.

Ok, but a commit message is not as important as the change itself,
although it's not reversible. The blame will be anyway on the one who
typed it forever.

 I am not talking about controversial or complicated fixes. But doc
 addition, build-sys, cleaning, spelling: this all qualifies to
 something that is an obvious improvement that I can trust people who
 have commit access to do the right call.

 The spice-protocol submodule removal is an obvious improvement (to me),
 and is only making build-sys related changes, but pushing it without
 review would have been a very bad call.

It's obvious but it's controversial and it is a change in workflow, so
that doesn't fall in this rule. Nothing like replacing a crufted
autogen with an obvious autoreconf.

 This is to me more healthy than having to bother
 and wait for each other through a mailing list.

 Turnaround should be fairly quick with a trivial patch. The patch is not
 going to get worse by waiting on the mailing list, but it may end up
 being better, so it's a win-win situation.

Quick for me is a matter of minutes. There are a lot of trivial
patches that have been pending for days. Improving the change can be
done immediately upstream or by a after-commit review. That's not a
valid argument.

 I am first a GNOME developper, where anyone can push changes without review.

 This is not true, this depends on the module, the maintainer, your
 relationship with that maintainer and the module, ...

And yet, there is no ACL per project for the reason that we trust each
other doing the right thing and it works well.

 I think this rule should be left to the maintainer, and as a
 maintainer of some of the Spice project, I prefer to have a trustful
 relationship and let people commit directly.

 Since you are playing the maintainer card, hopefully as a maintainer you
 are going to listen how people (at least 2 ) in your project community
 want to work, and not unilaterally change the way commits have been
 handled in the past.

Don't you see that I actually discuss the matter here?

 It's really not much, if
 the change is wrong, it can be reverted, not a big deal.

 Not a big deal save for history cluttering, the need to be careful when
 backporting patches if the commit was followed by a fixup commit, no way
 for fixing commit log typos, or for adding missing information, ...

It's also cluttering the mailing list, you moved the problem. You have
to weight the cost of applying strict rules. I have a different
opinion on that.


-- 
Marc-André Lureau
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel


Re: [Spice-devel] [PATCH spice-protocol] build-sys: simplify autogen

2014-12-05 Thread Christophe Fergeau
On Fri, Dec 05, 2014 at 03:57:29PM +0100, Marc-André Lureau wrote:
 Ok, but a commit message is not as important as the change itself,
 although it's not reversible.

But it's far from being unimportant.

 The blame will be anyway on the one who
 typed it forever.

I have absolutely no interest in blaming people after the fact, I prefer
to fix things before the mistake happens ;)

 Nothing like replacing a crufted autogen with an obvious autoreconf.

Is this what you did? This is not what I read in the commit log.

 Quick for me is a matter of minutes.

Even if it's a few hours, or a few days, is it a big deal?

 There are a lot of trivial patches that have been pending for days.

This means we need to improve on reviews :) Any pointers?

 Improving the change can be done immediately upstream or by a
 after-commit review. That's not a valid argument.

With pre-commit review, you ensure that at least one person read the
patch. With post-commit review, you have no such guarantee.

 And yet, there is no ACL per project for the reason that we trust each
 other doing the right thing and it works well.

Yes, for example modules who want mandatory reviews trust others not to
push without a review ;)

  It's really not much, if
  the change is wrong, it can be reverted, not a big deal.
 
  Not a big deal save for history cluttering, the need to be careful when
  backporting patches if the commit was followed by a fixup commit, no way
  for fixing commit log typos, or for adding missing information, ...
 
 It's also cluttering the mailing list, you moved the problem. You have
 to weight the cost of applying strict rules. I have a different
 opinion on that.

If the clutter on mailing lists is that bad, there's an easy solution,
a spice-users mailing list in addition to spice-devel. git history is
what you have to look at everyday, mailing lists history, not so often.

Christophe


pgpjMzlgEPphc.pgp
Description: PGP signature
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel


Re: [Spice-devel] [PATCH spice-protocol] build-sys: simplify autogen

2014-12-05 Thread Marc-André Lureau
On Fri, Dec 5, 2014 at 5:43 PM, Christophe Fergeau cferg...@redhat.com wrote:
 On Fri, Dec 05, 2014 at 04:52:49PM +0100, Marc-André Lureau wrote:
 Use autoreconf, allow out of tree autogen.sh run.

 Nowhere it says that you just got rid of the existing autogen.sh and
 replaced it with something else, you only say 'simplify'

It is a summary, use autoreconf mean we don't have much to do in
autogen.sh, this should be obvious to you too.

Looking at the change or the result is still the most important thing.
A commit message wouldn't be a summary otherwise, you have to look at
the diff to know the details, as always.

 That's a big factor, yes.

 What is so bad with having a commit delayed for a few hours while it's
 waiting for reviews?

It is mainly the difference between asking someone to take an action
or not. And asking someone take time often longer than necessary.

 Also, you can see that other projects have troubles with that, ex
 http://wiki.qemu.org/Contribute/TrivialPatches: they are moving the
 problem to me, but perhaps it helps.

 QEMU is a much bigger project though. I don't think there is much point
 looking at other projects, some are doing mandatory reviews, some are
 doing free for all commits, some are using mailing lists, others are
 using tools for patch tracking, ... And some are doing exactly what we
 do currently, with everyone agreeing to send all their patches for
 review before pushing.

And Spice is no such a big project with so many contributors, your
argument can be reverted.

  With pre-commit review, you ensure that at least one person read the
  patch. With post-commit review, you have no such guarantee.

 With current workflow, you have no guaratee that unreviewed patch go
 there by mistake or maliciously. We would need a tool for that.
 For me this is the job of maintainer to quickly review each commit
 before release.

 Not sure what you are getting at, 'quickly review' here could mean
 glance at the shortlog given the context. And this definitely does not
 scale, and you don't want to discover issues at release time. We also
 have spice-commits for this scenario.

I mean that we need to do post-commit review nonetheless unless you do
signed commit with a tool and ACL. I really don't think our project
deserve that treatment.

 Btw, do you apply your own rules in your own projects?

 I don't really have a project of mine in mind where a second person is
 actively involved, which is a prerequisite to set up such a system.
 I'd be happy to switch to that model if I had the opportunity. If
 someone was to take over libgovirt maintainance, I'd be happy to send
 patches before committing.

libgovirt is used by virt-viewer, and it means the Spice and
virt-tools team are affected by your changes. You can always ask for
help, but you trust enough your own work to do it alon and provide it
to those projects that will rely on it. Those projects trust you to do
the right thing by yourslef. Similarly, I would like you trust
contributors to do the right thing for Spice, without mandatory code
review. There has been no big issues so far, only you complaining for
no valid technical reasons. Let's stick to that before enforcing
rules.

-- 
Marc-André Lureau
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel


Re: [Spice-devel] [PATCH spice-protocol] build-sys: simplify autogen

2014-12-05 Thread Marc-André Lureau
Hi

On Fri, Dec 5, 2014 at 6:24 PM, Christophe Fergeau cferg...@redhat.com wrote:
 On Fri, Dec 05, 2014 at 06:03:42PM +0100, Marc-André Lureau wrote:
 On Fri, Dec 5, 2014 at 5:43 PM, Christophe Fergeau cferg...@redhat.com 
 wrote:
  What is so bad with having a commit delayed for a few hours while it's
  waiting for reviews?

 It is mainly the difference between asking someone to take an action
 or not. And asking someone take time often longer than necessary.

 I don't understand why it's bad for the project, or why it makes things
 worse.

It makes the general process more complicated and slower, it prevents
taking responsability. It make us more rely on each other instead of
steping up and doing the right thing without asking each other what
and how to do it. It prioritizes a bit our work. We don't necessarily
need each other for all the thing we do.


 Actually I don't and often feel uncomfortable making changes without
 anyone looking at them.

s/uncomfortable/comfortable I presume. The problem is this is not just
libgovirt and your work but a lot of more important pieces. There are
a lot of projects we rely on without mandatory review, and it's not
that bad. Imho they are more healthy that way.

 Similarly, I would like you trust contributors to do the right thing
 for Spice, without mandatory code review.

 I'd prefer if we did not frame this in term of trusting people or not,
 this is not what this is about. We are all humans, we all make mistakes,
 and we all have different skills. Patch reviews are just a way to tap
 into other people skillsets in order to improve the project overall, and
 to try to avoid these human mistakes as much as possible.

Isn't it trust when you give resonsabiity to people? If you remove
responsability, and forcefully double check everything they do, you
remove trust.

 There has been no big issues so far, only you complaining for no valid
 technical reasons. Let's stick to that before enforcing rules.

 Well, I did complain, Jeremy did too. Coming getting pushed because they
 are trivial is something newish and mostly done by you. I don't think the
 obvious conclusion to this discussion is to just do whatever you

May be the other raison Jeremy step up is because of this noisy and
endless discussion.

So let's bother more people and ask them what they think, good chances
are that opinions will be divided.

 decided, quite the contrary. Let's stop trying to be smart in deciding
 what is trivial or not, and let's keep sending all patches to the ML,
 this is not a big constraint, and will result in less frictions.

Where did you see that all patches have to be mandatorily reviewed in
Spice? We always had a trivial push rule, and you always complained
about it. That's all I know.

I invite more people to do more patches in general, and if mandatory
review is what slowing them down, I would happily repeat that we don't
have mandatory review in the Spice project and we trust your call for
making the changes without review. If you do something bad or
controversial, some (at least me), will notice it and it will be
discussed.

-- 
Marc-André Lureau
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel


Re: [Spice-devel] [PATCH spice-protocol] build-sys: simplify autogen

2014-12-05 Thread Marc-André Lureau
Hi

On Fri, Dec 5, 2014 at 8:59 PM, Jeremy White jwh...@codeweavers.com wrote:
 To be clear, I am an advocate of mandatory review.  I, personally, would not
 want to ever push a patch unless someone else has at least glanced at it.

Thanks, it was pretty clear already. Nevertheless, you didn't bring
this before (did you?), so I think you took an opportunity here to
express your opinion. There was already a lot of similar endless
threads on our ML: there is no mandatory review of all commits in the
Spice community, but Christophe, and now you, would like to apply it.
I disagree for the reason I expressed before but if the majority of
the contributors think it's better, I will follow with despite my
disagreement. But I will be against such policy to be applied on the
project I maintain for as long as I maintain them.

regards

-- 
Marc-André Lureau
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel


Re: [Spice-devel] [PATCH spice-protocol] build-sys: simplify autogen

2014-12-05 Thread Alon Levy
[snip]
 
 At the same time, I'm not sure mailing lists are the right tool for code
 review.  It's difficult to track which patches have been reviewed and
 which haven't.

http://patchwork.freedesktop.org/project/Spice/list/ can help, linked
from the wiki btw (http://www.spice-space.org/page/Main_Page)

(not getting into this otherwise :)

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel


Re: [Spice-devel] [PATCH spice-protocol] build-sys: simplify autogen

2014-12-05 Thread Marc-André Lureau
On Fri, Dec 5, 2014 at 10:38 PM, Jonathon Jongsma jjong...@redhat.com wrote:
 For what it's worth, I basically agree with Christophe and Jeremy.
 (Although I think that describing it as mandatory code review is
 over-stating the case a bit -- there is nothing but peer pressure and
 polite requests preventing contributors from pushing unreviewed code).

Peer pressure is precisely what one should try to avoid. If you think
your change does not require second look, because it is trivial, then
why would you do it? Now you will have to bother others repeatedly for
the most basic thing that they might not even care about. I would
rather work differently as I explained before, and as we did until
now.

-- 
Marc-André Lureau
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel


Re: [Spice-devel] [PATCH spice-protocol] build-sys: simplify autogen

2014-12-05 Thread Alon Levy
On 12/06/2014 12:00 AM, Jonathon Jongsma wrote:
 On Fri, 2014-12-05 at 23:41 +0200, Alon Levy wrote:
 [snip]

 At the same time, I'm not sure mailing lists are the right tool for code
 review.  It's difficult to track which patches have been reviewed and
 which haven't.

 http://patchwork.freedesktop.org/project/Spice/list/ can help, linked
 from the wiki btw (http://www.spice-space.org/page/Main_Page)

 (not getting into this otherwise :)

 
 Thanks, I had forgotten about that. I notice now that all patches listed
 on that site are in state New. Is there a magical incantation we can
 insert into our review that will change the patch state to Approved or
 Needs Work or something? If so, can we start using this incantation
 rather than our traditional ACK, so that patchwork will be able to
 track the true state of these patches?

I don't know that magic, but it does sound handy.

 
 Jonathon
 
 

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel


[Spice-devel] [PATCH spice-protocol] build-sys: simplify autogen

2014-12-04 Thread Marc-André Lureau
Use autoreconf, allow out of tree autogen.sh run.
---
Pushed upstream as trivial build fix.

autogen.sh | 167 -
 1 file changed, 10 insertions(+), 157 deletions(-)

diff --git a/autogen.sh b/autogen.sh
index 7d7c534..de6881d 100755
--- a/autogen.sh
+++ b/autogen.sh
@@ -1,166 +1,19 @@
 #! /bin/sh
 
-srcdir=`dirname $0`
-test -z $srcdir  srcdir=.
-
-ORIGDIR=`pwd`
-cd $srcdir
-
-# FIXME: can replace this entire script with
-# the following line if we can require autoconf 2.60:
-# autoreconf -v --force --install || exit 1
-
-PACKAGE=spice-protocol
-
-ACLOCAL_FLAGS=
-AUTOHEADER=${AUTOHEADER-autoheader}
-AUTOMAKE_FLAGS=--add-missing --gnu
-AUTOCONF=${AUTOCONF-autoconf}
-
-# automake 1.8 requires autoconf 2.58
-# automake 1.7 requires autoconf 2.54
-automake_min_vers=1.7
-aclocal_min_vers=$automake_min_vers
-autoconf_min_vers=2.54
+set -e # exit on errors
 
-# The awk-based string-number conversion we use needs a C locale to work
-# as expected. Setting LC_ALL overrides whether the user set LC_ALL,
-# LC_NUMERIC, or LANG.
-LC_ALL=C
-
-ARGV0=$0
-
-# Allow invocation from a separate build directory; in that case, we change
-# to the source directory to run the auto*, then change back before running 
configure
-srcdir=`dirname $ARGV0`
+srcdir=`dirname $0`
 test -z $srcdir  srcdir=.
 
-ORIGDIR=`pwd`
-
-# Not all echo versions allow -n, so we check what is possible. This test is
-# based on the one in autoconf.
-case `echo testing\c; echo 1,2,3`,`echo -n testing; echo 1,2,3` in
-  *c*,-n*) ECHO_N= ;;
-  *c*,*  ) ECHO_N=-n ;;
-  *)   ECHO_N= ;;
-esac
-
-
-# some terminal codes ...
-boldface=`tput bold 2/dev/null || true`
-normal=`tput sgr0 2/dev/null || true`
-printbold() {
-echo $ECHO_N $boldface
-echo $@
-echo $ECHO_N $normal
-}
-printerr() {
-echo $@ 2
-}
-
 
-# Usage:
-# compare_versions MIN_VERSION ACTUAL_VERSION
-# returns true if ACTUAL_VERSION = MIN_VERSION
-compare_versions() {
-ch_min_version=$1
-ch_actual_version=$2
-ch_status=0
-IFS=${IFS= }; ch_save_IFS=$IFS; IFS=.
-set $ch_actual_version
-for ch_min in $ch_min_version; do
-ch_cur=`echo $1 | sed 's/[^0-9].*$//'`; shift # remove letter suffixes
-if [ -z $ch_min ]; then break; fi
-if [ -z $ch_cur ]; then ch_status=1; break; fi
-if [ $ch_cur -gt $ch_min ]; then break; fi
-if [ $ch_cur -lt $ch_min ]; then ch_status=1; break; fi
-done
-IFS=$ch_save_IFS
-return $ch_status
-}
+(
+cd $srcdir
+autoreconf --verbose --force --install
+)
 
-# Usage:
-# version_check PACKAGE VARIABLE CHECKPROGS MIN_VERSION SOURCE
-# checks to see if the package is available
-version_check() {
-vc_package=$1
-vc_variable=$2
-vc_checkprogs=$3
-vc_min_version=$4
-vc_source=$5
-vc_status=1
+CONFIGURE_ARGS=--enable-maintainer-mode
 
-vc_checkprog=`eval echo \\$$vc_variable`
-if [ -n $vc_checkprog ]; then
-   printbold using $vc_checkprog for $vc_package
-   return 0
-fi
-
-printbold checking for $vc_package = $vc_min_version...
-for vc_checkprog in $vc_checkprogs; do
-   echo $ECHO_N   testing $vc_checkprog... 
-   if $vc_checkprog --version  /dev/null  /dev/null 21; then
-   vc_actual_version=`$vc_checkprog --version | head -n 1 | \
-   sed 's/^.*[ ]\([0-9.]*[a-z]*\).*$/\1/'`
-   if compare_versions $vc_min_version $vc_actual_version; then
-   echo found $vc_actual_version
-   # set variable
-   eval $vc_variable=$vc_checkprog
-   vc_status=0
-   break
-   else
-   echo too old (found version $vc_actual_version)
-   fi
-   else
-   echo not found.
-   fi
-done
-if [ $vc_status != 0 ]; then
-   printerr ***Error***: You must have $vc_package = $vc_min_version 
installed
-   printerr   to build $PROJECT.  Download the appropriate package for
-   printerr   from your distribution or get the source tarball at
-printerr $vc_source
-   printerr
-fi
-return $vc_status
-}
-
-version_check autoconf AUTOCONF $AUTOCONF $autoconf_min_vers \
-http://ftp.gnu.org/pub/gnu/autoconf/autoconf-${autoconf_min_vers}.tar.gz; 
|| DIE=1
-version_check automake AUTOMAKE $AUTOMAKE automake automake-1.10 automake-1.9 
automake-1.8 automake-1.7 $automake_min_vers \
-http://ftp.gnu.org/pub/gnu/automake/automake-${automake_min_vers}.tar.gz; 
|| DIE=1
-ACLOCAL=`echo $AUTOMAKE | sed s/automake/aclocal/`
-
-if test -n $DIE; then
-  exit 1
+if [ -z $NOCONFIGURE ]; then
+echo Running configure with $CONFIGURE_ARGS $@
+$srcdir/configure $CONFIGURE_ARGS $@
 fi
-
-
-if test -z $*; then
-  echo $ARGV0:Note: \`./configure' will be run with no arguments.
-  echoIf you wish to pass any to it, please specify them on 
the
-  echo\`$0' command line.
- 

Re: [Spice-devel] [PATCH spice-protocol] build-sys: simplify autogen

2014-12-04 Thread Christophe Fergeau
On Thu, Dec 04, 2014 at 05:42:18PM +0100, Marc-André Lureau wrote:
 Use autoreconf, allow out of tree autogen.sh run.
 ---
 Pushed upstream as trivial build fix.

Replacing autogen.sh with a totally different one is _not_ something
trivial.

Christophe


pgpF0gZjXIhYD.pgp
Description: PGP signature
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel


Re: [Spice-devel] [PATCH spice-protocol] build-sys: simplify autogen

2014-12-04 Thread Marc-André Lureau
On Thu, Dec 4, 2014 at 5:53 PM, Christophe Fergeau cferg...@redhat.com wrote:
 Replacing autogen.sh with a totally different one is _not_ something
 trivial.

It is to me, it took me about a minute to do all that. It is also
trivial to remove it or change it. Do you have something to say about
the change itself?

-- 
Marc-André Lureau
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel


Re: [Spice-devel] [PATCH spice-protocol] build-sys: simplify autogen

2014-12-04 Thread Christophe Fergeau
On Thu, Dec 04, 2014 at 05:59:07PM +0100, Marc-André Lureau wrote:
 On Thu, Dec 4, 2014 at 5:53 PM, Christophe Fergeau cferg...@redhat.com 
 wrote:
  Replacing autogen.sh with a totally different one is _not_ something
  trivial.
 
 It is to me, it took me about a minute to do all that. It is also
 trivial to remove it or change it. Do you have something to say about
 the change itself?

Yes, that it should not have been pushed.

Christophe


pgp8BKVVZVyGN.pgp
Description: PGP signature
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel


Re: [Spice-devel] [PATCH spice-protocol] build-sys: simplify autogen

2014-12-04 Thread Marc-André Lureau
Hi

- Original Message -
 On Thu, Dec 04, 2014 at 05:59:07PM +0100, Marc-André Lureau wrote:
  On Thu, Dec 4, 2014 at 5:53 PM, Christophe Fergeau cferg...@redhat.com
  wrote:
   Replacing autogen.sh with a totally different one is _not_ something
   trivial.
  
  It is to me, it took me about a minute to do all that. It is also
  trivial to remove it or change it. Do you have something to say about
  the change itself?
 
 Yes, that it should not have been pushed.

Can you please say why?
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel


Re: [Spice-devel] [PATCH spice-protocol] build-sys: simplify autogen

2014-12-04 Thread Christophe Fergeau
On Thu, Dec 04, 2014 at 12:09:15PM -0500, Marc-André Lureau wrote:
 Hi
 
 - Original Message -
  On Thu, Dec 04, 2014 at 05:59:07PM +0100, Marc-André Lureau wrote:
   On Thu, Dec 4, 2014 at 5:53 PM, Christophe Fergeau cferg...@redhat.com
   wrote:
Replacing autogen.sh with a totally different one is _not_ something
trivial.
   
   It is to me, it took me about a minute to do all that. It is also
   trivial to remove it or change it. Do you have something to say about
   the change itself?
  
  Yes, that it should not have been pushed.
 
 Can you please say why?

Your question seems to be based on the assumption that If something is
pushed with no review, and if noone has anything to say about it, then
it was fine to push it in the first place. Since this patch does not
qualify for the trivial rule or whatever, it had no reason to be pushed
in the first place.
Whether there are review comments or not is irrelevat to the point I'm
making.

Christophe


pgp_blYQIOB1e.pgp
Description: PGP signature
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel