Re: [Spice-devel] [PATCH spice-protocol] build-sys: simplify autogen
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
[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
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
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
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
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
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
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
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
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
