Re: Patch queue and reviewing (Was Re: Generator programs can only be built with optimization enabled?)
Hi, On Wed, 14 Jun 2006, Joe Buck wrote: On Wed, Jun 14, 2006 at 11:34:33AM -0700, Mike Stump wrote: I'd welcome the issue be addressed by the SC. I'd favor more timely reviews. Maybe auto approval for a patch that sits for more than a week? :-) I see your smilie, Mike, but GCC would rapidly decay into a pile of random bytes, unable to build itself, much less anything else, under such a policy. Not necessarily. There are other (bigger but perhaps not as complicated) open source projects, which work under a much more open scheme. It doesn't need to be an everything-goes. It could be something like trusted people can commit after some time, even without formal review. If it breaks something it goes out again. Trusted people would basically be long time committers. I.e. a mix of maintainers and perhaps active write-after-approvals. The special thing would be that they can write to the whole tree by default, if nobody cares enough to prevent that. It's not like the patch authors are a bunch of unresponsible people randomly hacking crap together ;-) (I know that nobody is saying that) I would predict that GCC would gain quality by a much more open and accepting policy. I know it's frustrating for people when their hard work doesn't get reviewed for long periods of time. But GCC is a mature compiler, it's stable, and while it has bugs and could be better, I'm not sure I *want* GCC to start changing much more rapidly than it changes today. Bugs will be fixed, yes. New features will be introduced, yes. But will the quality level be maintained? That's the whole reason we insist on patch review, so any process that speeds it up has to guarantee that will still get a decent review of all patches (other than the truly obvious ones). All patches still would have to be sent to [EMAIL PROTECTED] We have version control systems to back out whole patches at once. For some things reviewing can also happen after applying the patch (for most self-contained things I would argue). When the patch author is not sure about his way of tackling the problem he still is able to not check in without review or discussion. Trust is the key. Ciao, Michael.
Re: Generator programs can only be built with optimization enabled?
I actually like the existing behaviour, which I'm pretty sure hasn't changed for many years. I often find myself typing Hum, I think I need another patch. Paolo
Re: Generator programs can only be built with optimization enabled?
Eric Botcazou writes: I actually like the existing behaviour, which I'm pretty sure hasn't changed for many years. Eric It has, at least for make quickstrap. Yes, exactly. Prior to the top-level bootstrap changes, I explicitly would need to use make CFLAGS=-g to recompile a particular object file without optimization to enhance debugging. David
Re: Patch queue and reviewing (Was Re: Generator programs can only be built with optimization enabled?)
Dan == Daniel Berlin [EMAIL PROTECTED] writes: Dan It can also tell you who to copy on a ping email to make sure it Dan actually goes to a maintainer. Dan the interface is under construction, but okay for casual use. Dan http://www.dberlin.org/patches/patches/maintainer_list/745 would be the Dan one for this patch. This one is funny because many of the maintainers on that list are very inactive. I think we ought to retire inactive maintainers, by moving them to a Maintainer Emeritus section -- they wouldn't lose any rights, but at the same time we wouldn't be giving patch submitters false hope that there is a large number of maintainers available. Dan I could try generating the ping mails for single patches automatically, Dan and try to randomly disperse them so that you can't just ignore some Dan email bomb of ping emails, but this seems like it should be unnecessary. This would help me, except that java folks don't seem to use the patch queue. For Classpath we're much more aggressive about giving people patch approval status. Of course, our situation is a bit different, since Classpath is more modular than GCC. A look through the write-after-approval list to see if anybody there really could be promoted would help... if nobody there stands out, then our problem is just lack of developers, which requires a different sort of solution. Tom
Re: {Spam?} Re: Patch queue and reviewing (Was Re: Generator programs can only be built with optimization enabled?)
You can't put new features and bug fixes in the same basket. They can even be viewed as steering the compiler in opposite directions quality-wise. If you don't want to increase the patches-per-day ratio, the only solution is to prioritize bug fixes over new features. For example we could introduce secondary maintainers with approval rights for bug fixes only or something along these lines. I agree wholeheartedly. There could be tweaks like s/approval/review/ (in the sense that they cannot approve their own fixes) or restricting these reviewers to regressions. But in general I think it's a very good starting point! Paolo
Re: Patch queue and reviewing (Was Re: Generator programs can only be built with optimization enabled?)
On 15/06/06, Mike Stump [EMAIL PROTECTED] wrote: For example, would be nice to have a batch tester that would bootstrap and regression test on 2-5 platforms for all patch submitters post approval but pre-checkin. If any regressions, dump all patches and move on to the next set, repeat as fast as possible. I am new to the project so please don't take me too seriously if I am saying some dumb thing: why review a patch that doesn't pass bootstrap and check? I mean, there is a patch queue, you put your patch or patch set in the queue, it gets bootstrapped and tested as you said on 2-5 patforms, then either it passes and a notification is send to the potential reviewers or either it doesn't and it is rejected automatically by sending all the information to the submitter and being dropped from the queue. Maintainers said that they are overwhelmed by the amount of work required to review. Post-approval testing seems just a waste of time to me. Cheers, Manuel.
Re: Patch queue and reviewing (Was Re: Generator programs can only be built with optimization enabled?)
On 14/06/06, Eric Botcazou [EMAIL PROTECTED] wrote: For example we could introduce secondary maintainers with approval rights for bug fixes only or something along these lines. Or the secondary maintainers could review patches and reject them but not approve them for commit. They may add comments to help the main maintainer. This may also help in the particular situation when a person is good enough to be a main reviewer but cannot commit herself (perhaps temporally) to be responsive all the time. Any little help from such person will improve the review process and keeping her away just because she cannot dedicate all the required time to be main maintainer is a waste of resources. Such person should be allowed to reject patches and comment them, although the last decision would be taken by the main maintainer. (or maybe I am missing the point of the discussion...)
Re: Patch queue and reviewing (Was Re: Generator programs can only be built with optimization enabled?)
On Jun 15, 2006, at 2:34 AM, Manuel López-Ibáñez wrote: I am new to the project so please don't take me too seriously if I am saying some dumb thing: why review a patch that doesn't pass bootstrap and check? This is not usually an issue. Yes most people will only test one target but that is only because it is takes a long time to bootstrap and test on just one target. -- Pinski
Re: Patch queue and reviewing (Was Re: Generator programs can only be built with optimization enabled?)
On 15/06/06, Andrew Pinski [EMAIL PROTECTED] wrote: On Jun 15, 2006, at 2:34 AM, Manuel López-Ibáñez wrote: I am new to the project so please don't take me too seriously if I am saying some dumb thing: why review a patch that doesn't pass bootstrap and check? This is not usually an issue. Yes most people will only test one target but that is only because it is takes a long time to bootstrap and test on just one target. Sorry Andrew, I don't understand what you mean by this in this is not usually an issue. Mike Stump proposed a batch tester that would bootstrap and regression test on 2-5 platforms for all patch submitters post approval but pre-checkin. My point is that a batch tester post-approval is just wasting reviewers time, which seems to be one of the reasons why it takes so much time to approve patches. If there is a need for a batch tester, then it should be pre-review. Of course, this is just one little step. I don't expect that there is an ultimate solution, but a number of little steps that will, with time, improve the current situation. One would be batch testing pre-approval, another may be secondary maintainers who can reject and comment patches but need the approval of a maintainer to commit them.
Re: Patch queue and reviewing (Was Re: Generator programs can only be built with optimization enabled?)
Manuel López-Ibáñez wrote on 06/15/06 05:34: I mean, there is a patch queue, you put your patch or patch set in the queue, it gets bootstrapped and tested as you said on 2-5 patforms, then either it passes and a notification is send to the Actually, the patch queue only stores the patches. This idea of bootstrapping on 2-5 platforms automatically has not been implemented.
Re: Patch queue and reviewing (Was Re: Generator programs can only be built with optimization enabled?)
On Thu, Jun 15, 2006 at 03:57:05PM +0100, Manuel López-Ibáñez wrote: Mike Stump proposed a batch tester that would bootstrap and regression test on 2-5 platforms for all patch submitters post approval but pre-checkin. My point is that a batch tester post-approval is just wasting reviewers time, which seems to be one of the reasons why it takes so much time to approve patches. If there is a need for a batch tester, then it should be pre-review. Agreed; it makes more sense not to waste reviewers' time with patches that fail the bootstrap/regression test. If we do have a batch tester, it makes sense to run it before asking for review. Otherwise, the reviewer will have to diagnose portability problems that could have been found by the batch tester. For a bug fix patch, if it hits a reviewer after going through a system that shows it works on our five most important platforms, in most cases the reviewer can just give a quick OK and we're done. The reviewer might ask for a rewrite if the fix is done in a kludgy way, or there's a coding style problem.
Re: Patch queue and reviewing (Was Re: Generator programs can only be built with optimization enabled?)
On Thu, Jun 15, 2006 at 01:03:17PM -0400, Diego Novillo wrote: Manuel López-Ibáñez wrote on 06/15/06 05:34: I mean, there is a patch queue, you put your patch or patch set in the queue, it gets bootstrapped and tested as you said on 2-5 patforms, then either it passes and a notification is send to the Actually, the patch queue only stores the patches. This idea of bootstrapping on 2-5 platforms automatically has not been implemented. Right, but Manuel was commenting on Mike Stump's proposal, wondering why Mike proposed to run the bootstrap tests *after* reviewer approval instead of before.
Re: Patch queue and reviewing (Was Re: Generator programs can only be built with optimization enabled?)
On 6/15/06, Joe Buck [EMAIL PROTECTED] wrote: On Thu, Jun 15, 2006 at 01:03:17PM -0400, Diego Novillo wrote: Manuel López-Ibáñez wrote on 06/15/06 05:34: I mean, there is a patch queue, you put your patch or patch set in the queue, it gets bootstrapped and tested as you said on 2-5 patforms, then either it passes and a notification is send to the Actually, the patch queue only stores the patches. This idea of bootstrapping on 2-5 platforms automatically has not been implemented. Right, but Manuel was commenting on Mike Stump's proposal, wondering why Mike proposed to run the bootstrap tests *after* reviewer approval instead of before. It might be a question of resources, though I agree that it makes sense to do it before patch review. Like testing on all primary release platforms - which makes resources more of a problem, as it is not simply throwing a bunch of cheap boxes at the problem this way. Richard.
RE: Patch queue and reviewing (Was Re: Generator programs can only be built with optimization enabled?)
On 15 June 2006 18:09, Joe Buck wrote: On Thu, Jun 15, 2006 at 01:03:17PM -0400, Diego Novillo wrote: Manuel López-Ibáñez wrote on 06/15/06 05:34: I mean, there is a patch queue, you put your patch or patch set in the queue, it gets bootstrapped and tested as you said on 2-5 patforms, then either it passes and a notification is send to the Actually, the patch queue only stores the patches. This idea of bootstrapping on 2-5 platforms automatically has not been implemented. Right, but Manuel was commenting on Mike Stump's proposal, wondering why Mike proposed to run the bootstrap tests *after* reviewer approval instead of before. Well, running them before and short-circuiting the review would be an *optimisation*; those come later, after the /design/ stage! :) cheers, DaveK -- Can't think of a witty .sigline today
Re: Patch queue and reviewing (Was Re: Generator programs can only be built with optimization enabled?)
On 6/15/06, Joe Buck [EMAIL PROTECTED] wrote: Right, but Manuel was commenting on Mike Stump's proposal, wondering why Mike proposed to run the bootstrap tests *after* reviewer approval instead of before. On Thu, Jun 15, 2006 at 07:12:38PM +0200, Richard Guenther wrote: It might be a question of resources, though I agree that it makes sense to do it before patch review. Like testing on all primary release platforms - which makes resources more of a problem, as it is not simply throwing a bunch of cheap boxes at the problem this way. Well, maybe we should decide on what is a primary release platform based at least partly on who will make adequate testing resources available for GCC testing. Skilled people are a more valuable resource than hardware is.
Re: Patch queue and reviewing (Was Re: Generator programs can only be built with optimization enabled?)
Diego Novillo wrote: Manuel López-Ibáñez wrote on 06/15/06 05:34: I mean, there is a patch queue, you put your patch or patch set in the queue, it gets bootstrapped and tested as you said on 2-5 patforms, then either it passes and a notification is send to the Actually, the patch queue only stores the patches. This idea of bootstrapping on 2-5 platforms automatically has not been implemented. I know, for example, SuSE has such a build farm that is accessible by email (IE you email patches to it). If they were willing to let the patchapp submit emails (or xmlrpc or whatever), and there was a way for it to notify the patchapp about the results (xmlrpc or http post would be fine), that would be ideal. I could just add a status column saying what happened with the bootstrap. --Dan
Re: Patch queue and reviewing (Was Re: Generator programs can only be built with optimization enabled?)
Hello, Diego Novillo wrote: Manuel López-Ibá?ez wrote on 06/15/06 05:34: I mean, there is a patch queue, you put your patch or patch set in the queue, it gets bootstrapped and tested as you said on 2-5 patforms, then either it passes and a notification is send to the Actually, the patch queue only stores the patches. This idea of bootstrapping on 2-5 platforms automatically has not been implemented. I know, for example, SuSE has such a build farm that is accessible by email (IE you email patches to it). If they were willing to let the patchapp submit emails (or xmlrpc or whatever), and there was a way for it to notify the patchapp about the results (xmlrpc or http post would be fine), that would be ideal. I could just add a status column saying what happened with the bootstrap. there are several problems with the idea. With the current setup, we cannot make the testers available to public for security reasons. Testing each patch takes several hours, so for larger-scale usage we would need to dedicate more machines to the task than we do now. Also, the testers need some maintenance -- 1-2 hours each month for me, but this would obviously go up with the more intensive usage. I do not think it would be possible for SuSE by itself to accomodate for these demands. Perhaps in the cooperation with other companies interested in gcc, we could put together enough hardware and developer time to realize this idea (don't take it as promise of anything from the SuSE side, though -- I have very little say regarding that). I think it might be good to discuss it at gcc summit. Zdenek
Re: Patch queue and reviewing (Was Re: Generator programs can only be built with optimization enabled?)
I know, for example, SuSE has such a build farm that is accessible by email (IE you email patches to it). If they were willing to let the patchapp submit emails (or xmlrpc or whatever), and there was a way for it to notify the patchapp about the results (xmlrpc or http post would be fine), that would be ideal. On Thu, Jun 15, 2006 at 07:55:30PM +0200, Zdenek Dvorak wrote: there are several problems with the idea. With the current setup, we cannot make the testers available to public for security reasons. Testing each patch takes several hours, so for larger-scale usage we would need to dedicate more machines to the task than we do now. Also, the testers need some maintenance -- 1-2 hours each month for me, but this would obviously go up with the more intensive usage. I do not think it would be possible for SuSE by itself to accomodate for these demands. This is understandable. In any case, it would probably best to have a human in the loop before submitting patches to autobuilders, both for security reasons and as a sanity check, to avoid wasting resources on an unacceptable patch. Machine donors (maybe SuSE?) would authorize a small number of people to submit patches to the auto-builder after an initial review. I'm now starting to get a clue as to why Mike talked about a review before the auto-builder instead of after. Maybe the pre-review would just be a very cursory sanity check, with the real review to happen after it builds. Perhaps in the cooperation with other companies interested in gcc, we could put together enough hardware and developer time to realize this idea (don't take it as promise of anything from the SuSE side, though -- I have very little say regarding that). I think it might be good to discuss it at gcc summit. If people like the concept, but there's a shortage of dedicated equipment, perhaps donations can be solicited.
Re: Patch queue and reviewing (Was Re: Generator programs can only be built with optimization enabled?)
On Jun 15, 2006, at 2:34 AM, Manuel López-Ibáñez wrote: Maintainers said that they are overwhelmed by the amount of work required to review. Post-approval testing seems just a waste of time to me. It is, well, unless you want mainline to build and pass a regression suite. No amount of pre-testing can ensure that. Maybe you've never seen: http://gcc.gnu.org/regtest/HEAD/log.txt.gz see http://gcc.gnu.org/regtest/HEAD/ for context.
Re: Patch queue and reviewing (Was Re: Generator programs can only be built with optimization enabled?)
On 15/06/06, Joe Buck [EMAIL PROTECTED] wrote: This is understandable. In any case, it would probably best to have a human in the loop before submitting patches to autobuilders, both for security reasons and as a sanity check, to avoid wasting resources on an unacceptable patch. Machine donors (maybe SuSE?) would authorize a small number of people to submit patches to the auto-builder after an initial review. I guess I understood wrongly what the problem was. I believed that there was a shortage of human reviewers and that by wasting resources you meant someone reviewing a patch that doesn't even pass the testsuite, and thus, the pre-review batch test was a way to avoid wasting (human) resources on an unacceptable patch. Sorry for the confusion.
Re: Patch queue and reviewing (Was Re: Generator programs can only be built with optimization enabled?)
thus, the pre-review batch test was a way to avoid wasting (human) resources on an unacceptable patch. You are confusing the review process with whether the patch works. To a large extent, those two issues are very different. A reviewer will always presume the patch works and is looking a stylistic and methodological issues. Even if the patch doesn't work in its present form, if there are such issues, they will need to be fixed at such time when the patch *does* work. So it would be relatively rare for a reviewer to be wasting time commenting on a patch that has a technical problem.
Re: Generator programs can only be built with optimization enabled?
David Edelsohn [EMAIL PROTECTED] writes: Daniel Jacobowitz writes: Daniel On Mon, Jun 12, 2006 at 10:22:17AM -0400, David Edelsohn wrote: Typing make in the gcc subdirectory does not do what I expect. Daniel Then could you clarify what happens, and what you expect, please? The behavior prior to the top-level bootstrap changes that I and others repeatedly have mentioned in email and IRC: if I type make cc1 in the gcc subdirectory, the build should be invoked with the appropriate options from the current build stage. In other words, if I have a completely bootstrapped compiler, change a source file, enter $objdir/gcc, and type make cc1, I expect cc1 to be rebuilt with CFLAGS=-O2 -g. Instead, if I type make or make quickstrap, the compilation uses CFLAGS=-g. I actually like the existing behaviour, which I'm pretty sure hasn't changed for many years. I often find myself typing touch $srcdir/gcc/some_file.c make -C gcc cc1 to get a cc1 built with that file with -O0 in the hope that debugging will work better.
Re: Generator programs can only be built with optimization enabled?
I actually like the existing behaviour, which I'm pretty sure hasn't changed for many years. It has, at least for make quickstrap. -- Eric Botcazou
Re: Patch queue and reviewing (Was Re: Generator programs can only be built with optimization enabled?)
Ian Lance Taylor wrote: Daniel Berlin [EMAIL PROTECTED] writes: It can also tell you who to copy on a ping email to make sure it actually goes to a maintainer. the interface is under construction, but okay for casual use. http://www.dberlin.org/patches/patches/maintainer_list/745 would be the one for this patch. I'm on that list, but I can't approve this patch. It needs approval from a build system maintainer, not a middle-end maintainer. The maintenance area was listed by the submitter asbuild and middle-end hence the reason the middle end maintainers are listed.
Re: Patch queue and reviewing (Was Re: Generator programs can only be built with optimization enabled?)
Daniel Berlin [EMAIL PROTECTED] writes: It can also tell you who to copy on a ping email to make sure it actually goes to a maintainer. the interface is under construction, but okay for casual use. http://www.dberlin.org/patches/patches/maintainer_list/745 would be the one for this patch. I'm on that list, but I can't approve this patch. It needs approval from a build system maintainer, not a middle-end maintainer. The maintenance area was listed by the submitter as build and middle-end hence the reason the middle end maintainers are listed. Yeah, I did see that. I guess that naturally leads to the next question: how do I edit that field? There ought to be some nice little AJAX editor to let me adjust all the fields, assuming of course that I have a valid SSL enabled login. Maybe you could work on that this afternoon if you have a free hour. Ian
Re: Patch queue and reviewing (Was Re: Generator programs can only be built with optimization enabled?)
Ian Lance Taylor wrote: Daniel Berlin [EMAIL PROTECTED] writes: It can also tell you who to copy on a ping email to make sure it actually goes to a maintainer. the interface is under construction, but okay for casual use. http://www.dberlin.org/patches/patches/maintainer_list/745 would be the one for this patch. I'm on that list, but I can't approve this patch. It needs approval from a build system maintainer, not a middle-end maintainer. The maintenance area was listed by the submitter as build and middle-end hence the reason the middle end maintainers are listed. Yeah, I did see that. I guess that naturally leads to the next question: how do I edit that field? There ought to be some nice little AJAX editor to let me adjust all the fields, assuming of course that I have a valid SSL enabled login. Maybe you could work on that this afternoon if you have a free hour. I'm already ahead of you. For the moment, i'm just going to add an edit button. I have to read up on the in-place editor API before i can just make it editable. Anyway, the editor will only adjust the maintenance and url fields. The rest is auto-grabbed from the url. Ian
Re: Patch queue and reviewing (Was Re: Generator programs can only be built with optimization enabled?)
Daniel Berlin wrote on 06/13/06 23:24: Does anyone believe this would help make sure patches stop dropping through the cracks? Not really. Technical solutions to social problems rarely work. Patch review is mostly a social problem. I am frequently part of the problem, unfortunately. Mostly this is a time constraint problem, there are only so many hours in a working day. I don't really have a good idea on how to address the core problem, other than to encourage adding more maintainers. A couple of Summits ago I think we discussed the idea of having secondary maintainers: folks who may not feel fully confident about an area, but may want to chime in with an initial review which the primary maintainer could then use to help with the final review. That doesn't mean that the patch queue is a bad idea. On the contrary, but if a patch is destined to fall through the cracks, no amount of technical infrastructure will prevent it from doing so.
Re: Patch queue and reviewing (Was Re: Generator programs can only be built with optimization enabled?)
On 6/14/06, Diego Novillo [EMAIL PROTECTED] wrote: Daniel Berlin wrote on 06/13/06 23:24: Does anyone believe this would help make sure patches stop dropping through the cracks? Not really. Technical solutions to social problems rarely work. Patch review is mostly a social problem. I am frequently part of the problem, unfortunately. Mostly this is a time constraint problem, there are only so many hours in a working day. I don't really have a good idea on how to address the core problem, other than to encourage adding more maintainers. A couple of Summits ago I think we discussed the idea of having secondary maintainers: folks who may not feel fully confident about an area, but may want to chime in with an initial review which the primary maintainer could then use to help with the final review. That doesn't mean that the patch queue is a bad idea. On the contrary, but if a patch is destined to fall through the cracks, no amount of technical infrastructure will prevent it from doing so. Some time ago I invented the obvious-because-nobody-cares rule under which you can apply a patch as obvious if nobody objected after two weeks. At least this is a workaround for the social problem (and creates another). ;) Richard.
Re: Patch queue and reviewing (Was Re: Generator programs can only be built with optimization enabled?)
Diego Novillo [EMAIL PROTECTED] writes: Daniel Berlin wrote on 06/13/06 23:24: Does anyone believe this would help make sure patches stop dropping through the cracks? Not really. Technical solutions to social problems rarely work. Patch review is mostly a social problem. I am frequently part of the problem, unfortunately. Mostly this is a time constraint problem, there are only so many hours in a working day. You are of course correct in general. However, I believe there are specific cases in which generating reminder messages would help. The gcc-patches mailing list is a busy one. It's easy for people to miss a patch. It's also easy for people to assume that a patch to a particular area will be handled by a maintainer who tends to focus on that area. I think a reminder message that some patch is still outstanding could be helpful to catch these sorts of cases. Ian
Re: Patch queue and reviewing (Was Re: Generator programs can only be built with optimization enabled?)
On Jun 13, 2006, at 8:24 PM, Daniel Berlin wrote: Past the above, I have no better ideas for getting patches reviewed other than appointing more maintainers. I'd welcome the issue be addressed by the SC. I'd favor more timely reviews. Maybe auto approval for a patch that sits for more than a week? :-)
Re: Patch queue and reviewing (Was Re: Generator programs can only be built with optimization enabled?)
On Jun 13, 2006, at 8:24 PM, Daniel Berlin wrote: Past the above, I have no better ideas for getting patches reviewed other than appointing more maintainers. On Wed, Jun 14, 2006 at 11:34:33AM -0700, Mike Stump wrote: I'd welcome the issue be addressed by the SC. I'd favor more timely reviews. Maybe auto approval for a patch that sits for more than a week? :-) I see your smilie, Mike, but GCC would rapidly decay into a pile of random bytes, unable to build itself, much less anything else, under such a policy. The SC mainly has negative power, it can't make people do work. There have been a number of proposals that basically amount to threatening the patch reviewers with negative consequences, but I'm not for that. Certainly we can talk about mechanisms to help the reviewers, or try to recruit new help. I know it's frustrating for people when their hard work doesn't get reviewed for long periods of time. But GCC is a mature compiler, it's stable, and while it has bugs and could be better, I'm not sure I *want* GCC to start changing much more rapidly than it changes today. Bugs will be fixed, yes. New features will be introduced, yes. But will the quality level be maintained? That's the whole reason we insist on patch review, so any process that speeds it up has to guarantee that will still get a decent review of all patches (other than the truly obvious ones).
Re: Patch queue and reviewing (Was Re: Generator programs can only be built with optimization enabled?)
On Jun 13, 2006, at 8:24 PM, Daniel Berlin wrote: Past the above, I have no better ideas for getting patches reviewed other than appointing more maintainers. I'd welcome the issue be addressed by the SC. I'd favor more timely reviews. Maybe auto approval for a patch that sits for more than a week? :-) Auto approving is the wrong approach except in the case where the patch is small, even then questionable. Maybe a system where you can trade reviews for patches. Like if you want a patch to be reviewed and you make a promise to also do another patch for the review. Yes this might not always work but it might help the current situation. -- Pinski
Re: Patch queue and reviewing (Was Re: Generator programs can only be built with optimization enabled?)
But GCC is a mature compiler, it's stable, and while it has bugs and could be better, I'm not sure I *want* GCC to start changing much more rapidly than it changes today. Bugs will be fixed, yes. New features will be introduced, yes. But will the quality level be maintained? You can't put new features and bug fixes in the same basket. They can even be viewed as steering the compiler in opposite directions quality-wise. If you don't want to increase the patches-per-day ratio, the only solution is to prioritize bug fixes over new features. For example we could introduce secondary maintainers with approval rights for bug fixes only or something along these lines. -- Eric Botcazou
Re: Patch queue and reviewing (Was Re: Generator programs can only be built with optimization enabled?)
Joe Buck [EMAIL PROTECTED] writes: The SC mainly has negative power, it can't make people do work. There have been a number of proposals that basically amount to threatening the patch reviewers with negative consequences, but I'm not for that. Certainly we can talk about mechanisms to help the reviewers, or try to recruit new help. Yeah, we need positive feedback, not negative feedback. I think that every approved review (as tracked by some program, e.g., Danny's patch queue e-mail reader) the approver should get a gooble. And, just to make it more fun, the submitter should get one also. Whoever has the most goobles in any month gets to be Gcc Poobah of the Month. Whoever has the most in a year gets to be Gcc Grand Poobah for the next year. If we find a sponsor, or we all chip in a dollar, then the Gcc Grand Poobah can get a T-shirt and a funny hat. This may sound kind of dumb, but, hey, in an insular community people can care about the most trivial of things, even if they don't take them seriously. E.g., Slashdot Karma. Or, this may sound extremely dumb, in which case, never mind. Ian
Re: Patch queue and reviewing (Was Re: Generator programs can only be built with optimization enabled?)
On Wed, Jun 14, 2006 at 10:16:38PM +0200, Eric Botcazou wrote: But GCC is a mature compiler, it's stable, and while it has bugs and could be better, I'm not sure I *want* GCC to start changing much more rapidly than it changes today. Bugs will be fixed, yes. New features will be introduced, yes. But will the quality level be maintained? You can't put new features and bug fixes in the same basket. They can even be viewed as steering the compiler in opposite directions quality-wise. If you don't want to increase the patches-per-day ratio, the only solution is to prioritize bug fixes over new features. For example we could introduce secondary maintainers with approval rights for bug fixes only or something along these lines. That might make sense. I'm not saying I don't want to increase the patches-per-day ratio, particularly with respect to getting bug fixes in. However, bug fixes carry their own risk: I had a project where we found that one out of five fixes to critical bugs introduced another critical bug, though the number was so high because the project required critical bugs reported by customers to be fixed under severe time pressure so GCC would not see so high a ratio. Sometimes you need time to check whether a bug fix is really correct, or if it just patches over a symptom.
Re: Patch queue and reviewing (Was Re: Generator programs can only be built with optimization enabled?)
On Jun 14, 2006, at 11:51 AM, Joe Buck wrote: There have been a number of proposals that basically amount to threatening the patch reviewers with negative consequences, but I'm not for that. I too think that would be the wrong direction to go. I'm not sure I *want* GCC to start changing much more rapidly than it changes today. But, is change alone the basis to drive decisions? I hope not. gcc has always been driven by users. Fewer users, less change. More users, more change. I think there are ways to manage it and still keep the quality up. But will the quality level be maintained? Yes, we ensure the observable quality in part by the testsuite, this gets us a generally monotonic baseline and from the user's perspective, this is an important part of quality. This helps us scale in terms of the change load. I think there are other opportunities to help scale that have a beneficial impact on quality. For example, would be nice to have a batch tester that would bootstrap and regression test on 2-5 platforms for all patch submitters post approval but pre-checkin. If any regressions, dump all patches and move on to the next set, repeat as fast as possible. If they all pass the entire regression suite, all languages, then put them all in. In this fashion, at all points, for all the tested platforms, there could never be any regressions. This lends to stability for the developers in general and edges the quality up. This also can lesson the testing burden on individual contributers and free them of that task more often. That's the whole reason we insist on patch review, so any process that speeds it up has to guarantee that will still get a decent review of all patches (other than the truly obvious ones). When the patches are to fix regressions and bugs in the compiler, one can argue that quality isn't served by not fixing the problem. By making the process more efficient, we enable more bugs to be fixed and we further encourage more people to send in patches to fix more bugs, more often. I feel being responsive to patch submitters is being responsive to users, and being responsive to users it one way to get more users and a better reputation. More users, means in part, more testers and more diversity in testing. Discouraging patch submitters, and in the end that discourages users, and that results in less testing and less diverse code. I'm not advocating a free for all, but I do think we need to ensure that there are enough maintainers to ensure timely reviews. I'd argue that 1 month indicates that we don't have enough reviewer bandwidth. I'd argue that 10 months indicates the same thing, only more. The SC could ask for volunteers to be patch reviewers in areas where we have a backlog. If the existing maintainers don't want the help, this can motivate them to never have a backlog.
Re: Generator programs can only be built with optimization enabled?
An untested patch to do so is attached. You can try it and, if it fails, there is also Rainer Orth's patch in comment #14 of the PR. Sure, but read the date of the comment. :-) I'm really wondering what the Patch URL field of the PR is for... IMHO this PR is a striking example of the *major* problems we have been having in the patch reviewing department for quite some time. -- Eric Botcazou
Re: Generator programs can only be built with optimization enabled?
Eric Botcazou wrote: An untested patch to do so is attached. You can try it and, if it fails, there is also Rainer Orth's patch in comment #14 of the PR. Sure, but read the date of the comment. :-) Yes, OTOH it is the patch that I like the most... Thanks for chiming in this discussion. You've clearly given a good deal of thought to the problem, and if you have suggestions I'm all ears. Paolo
Re: {Spam?} Re: Generator programs can only be built with optimization enabled?
I didn't understand the purpose of: (build/gencondmd.o): Filter out -fkeep-inline-functions. Read the comment? -- Eric Botcazou
Re: Generator programs can only be built with optimization enabled?
Eric Botcazou wrote: I didn't understand the purpose of: (build/gencondmd.o): Filter out -fkeep-inline-functions. Read the comment? It can help indeed. However, the audit trail of the PR seems to say that now -fkeep-inline-functions is sort of implied by -O0; I can build insn-conditions.md with -O0 -fkeep-inline-functions so I'm not affected by the PR. In this case, will your patch still fix the bug when using GCC with BOOT_CFLAGS=-O0? Paolo
Re: Generator programs can only be built with optimization enabled?
However, the audit trail of the PR seems to say that now -fkeep-inline-functions is sort of implied by -O0; I can build insn-conditions.md with -O0 -fkeep-inline-functions so I'm not affected by the PR. Comment #36 seems to say that we're back to the initial state. -- Eric Botcazou
Patch queue and reviewing (Was Re: Generator programs can only be built with optimization enabled?)
IMHO this PR is a striking example of the *major* problems we have been having in the patch reviewing department for quite some time. I don't disagree in this case. Not only was this patch submitted in march and not reviewed, it was even pinged on march 29th by someone *else*. http://gcc.gnu.org/ml/gcc-patches/2006-03/msg01693.html It seems most undirected pings go ignored, probably because nobody realizes they are supposed to be looking at this patch. At least, i hope this is why. The patch queue can now tell who can review a given patch (it guesses matching the maintenance area you say the patch is for when adding to the queue against regexps for each maintenance area). It knows the difference between global and non-global maintainers. Given this, it would be trivial to make it able to generate, for example, an RSS feed (Or it could send them emails) for a maintainer that they can subscribe to which will have new items when they have new patches they can review. Does anyone believe this would help make sure patches stop dropping through the cracks? It can also tell you who to copy on a ping email to make sure it actually goes to a maintainer. the interface is under construction, but okay for casual use. http://www.dberlin.org/patches/patches/maintainer_list/745 would be the one for this patch. I already tried generating mass-ping emails for patches that have been outstanding 2 weeks on the patch queue, but this didn't seem to help. I could try generating the ping mails for single patches automatically, and try to randomly disperse them so that you can't just ignore some email bomb of ping emails, but this seems like it should be unnecessary. Past the above, I have no better ideas for getting patches reviewed other than appointing more maintainers. --Dan
Re: Patch queue and reviewing (Was Re: Generator programs can only be built with optimization enabled?)
Daniel Berlin [EMAIL PROTECTED] writes: It can also tell you who to copy on a ping email to make sure it actually goes to a maintainer. the interface is under construction, but okay for casual use. http://www.dberlin.org/patches/patches/maintainer_list/745 would be the one for this patch. I'm on that list, but I can't approve this patch. It needs approval from a build system maintainer, not a middle-end maintainer. Ian
Re: Generator programs can only be built with optimization enabled?
David Edelsohn wrote: Mark Mitchell writes: Mark That seems unfortunate, but so be it. Yes it is very unfortunate and not very convenient for the way that most developers want to use the build infrastructure. There no longer is an equivalent to make quickstrap. To rebuild only GCC, one can use make all-stageN-gcc at the top-level. Andrew pointed out PR18058, where you can read: - Comment #33 From Andrew Pinski 2006-06-11 13:10 [reply] --- ... This was caused by: 2006-01-22 Zack Weinberg [EMAIL PROTECTED] * genautomata.c: Include vec.h, not varray.h. The problem that Mark reported happens because (since always) the CFLAGS of the gcc directory are just -g, not -O2 -g. Optimized builds have (since always) been done only because the toplevel overrides the -g CFLAGS. So, when you type make in the gcc directory, it triggers a non-optimized build of everything (generator programs, compiler, driver), which in turn triggers PR18058. So, let's please not confuse issues. I work in the GCC directory daily. I type make there and it just works. You can even type make quickstrap if you want: ifeq ($(BOOTSTRAPPING),yes) # Provide quickstrap as a target that people can type into the gcc # directory, and that fails if you're not into it. quickstrap: all else ... endif I think this was your suggestion, and it was implemented. If there are requests for improvements to the build infrastructure, please tell me clearly about them. However, I don't see how this is related to the problem that Mark reported. Paolo
Re: Generator programs can only be built with optimization enabled?
Paolo Bonzini writes: Paolo So, let's please not confuse issues. I work in the GCC directory daily. Paolo I type make there and it just works. You can even type make Paolo quickstrap if you want: Paolo I think this was your suggestion, and it was implemented. Typing make in the gcc subdirectory does not do what I expect. Also, if make quickstrap is implemented, you should update the Top-Level Bootstrap page in the GCC Wiki to document the current functionality. David
Re: Generator programs can only be built with optimization enabled?
On Mon, Jun 12, 2006 at 10:22:17AM -0400, David Edelsohn wrote: Typing make in the gcc subdirectory does not do what I expect. Then could you clarify what happens, and what you expect, please? -- Daniel Jacobowitz CodeSourcery
Re: Generator programs can only be built with optimization enabled?
Daniel Jacobowitz writes: Daniel On Mon, Jun 12, 2006 at 10:22:17AM -0400, David Edelsohn wrote: Typing make in the gcc subdirectory does not do what I expect. Daniel Then could you clarify what happens, and what you expect, please? The behavior prior to the top-level bootstrap changes that I and others repeatedly have mentioned in email and IRC: if I type make cc1 in the gcc subdirectory, the build should be invoked with the appropriate options from the current build stage. In other words, if I have a completely bootstrapped compiler, change a source file, enter $objdir/gcc, and type make cc1, I expect cc1 to be rebuilt with CFLAGS=-O2 -g. Instead, if I type make or make quickstrap, the compilation uses CFLAGS=-g. Thanks, David
Re: Generator programs can only be built with optimization enabled?
Paolo Bonzini wrote: This was caused by: 2006-01-22 Zack Weinberg [EMAIL PROTECTED] * genautomata.c: Include vec.h, not varray.h. The problem that Mark reported happens because (since always) the CFLAGS of the gcc directory are just -g, not -O2 -g. Optimized builds have (since always) been done only because the toplevel overrides the -g CFLAGS. So, when you type make in the gcc directory, it triggers a non-optimized build of everything (generator programs, compiler, driver), which in turn triggers PR18058. I think that, after Zack's change, the generator programs that include rtl.h should be linked with build/vec.o. That may not be necessary when optimizing, but it would avoid this problem. Do you agree? -- Mark Mitchell CodeSourcery [EMAIL PROTECTED] (650) 331-3385 x713
Generator programs can only be built with optimization enabled?
Typing make in $objdir/gcc (after a bootstrap) sometimes results in errors like: build/gencondmd.o: In function `VEC_rtx_heap_reserve': /net/sparrowhawk/scratch/mitchell/src/lto/gcc/rtl.h:195: undefined reference to `vec_heap_p_reserve' For an ordinary make the generator programs are built without optimization. But, rtl.h use DEF_VEC_*, the expansion of which includes inline functions. With optimization disabled, the compiler apparently emits the inline functions. The inline functions reference symbols (like vec_heap_p_reserve) that are not included in the build objects linked into the generator program. I'm using a version of mainline that's a few weeks old; is this something that has been recently fixed? Thanks, -- Mark Mitchell CodeSourcery [EMAIL PROTECTED] (650) 331-3385 x713
Re: Generator programs can only be built with optimization enabled?
This is part of the new build infrastructure. One cannot simply go into $objdir/gcc and type make. One either needs to use the appropriate incantation at the top-level build directory or go into $objdir/gcc and type make CFLAGS='xxx', where 'xxx' matches the optimization options for the current bootstrap phase. David
Re: Generator programs can only be built with optimization enabled?
Mark Mitchell [EMAIL PROTECTED] writes: | Typing make in $objdir/gcc (after a bootstrap) sometimes results in | errors like: | | build/gencondmd.o: In function `VEC_rtx_heap_reserve': | /net/sparrowhawk/scratch/mitchell/src/lto/gcc/rtl.h:195: undefined | reference to `vec_heap_p_reserve' | | For an ordinary make the generator programs are built without | optimization. But, rtl.h use DEF_VEC_*, the expansion of which includes | inline functions. With optimization disabled, the compiler apparently | emits the inline functions. The inline functions reference symbols | (like vec_heap_p_reserve) that are not included in the build objects | linked into the generator program. this must be a bug, not a feature. I think we already have PRs about it. -- Gaby
Re: Generator programs can only be built with optimization enabled?
David Edelsohn wrote: This is part of the new build infrastructure. One cannot simply go into $objdir/gcc and type make. One either needs to use the appropriate incantation at the top-level build directory or go into $objdir/gcc and type make CFLAGS='xxx', where 'xxx' matches the optimization options for the current bootstrap phase. That seems unfortunate, but so be it. Thanks, -- Mark Mitchell CodeSourcery [EMAIL PROTECTED] (650) 331-3385 x713
Re: Generator programs can only be built with optimization enabled?
Mark Mitchell writes: Mark That seems unfortunate, but so be it. Yes it is very unfortunate and not very convenient for the way that most developers want to use the build infrastructure. There no longer is an equivalent to make quickstrap. To rebuild only GCC, one can use make all-stageN-gcc at the top-level. David
