Re: [Sugar-devel] Code Review process changes
Sorry for being late in the discussion, just a few words from a Sugar module maintainer's point of few. I don't think the tool we used was the bottleneck. Of course there could have been some enhancements (like reminders to the ml), but in general it was ok. I personally had just too many things to do, Feature process, Release process etc. So we are in need of more man power, like laid out here several times. Mainly people taking responsibilities. I don't think we want to cut away the maintainers, this is important to keep the quality high. Thanks, Simon ___ Sugar-devel mailing list Sugar-devel@lists.sugarlabs.org http://lists.sugarlabs.org/listinfo/sugar-devel
Re: [Sugar-devel] Code Review process changes
I have been trying to keep quite about this until we have had results flowing in Over the past couple of months Bernie, Caroline, and I have been looking at businesses around Sugar and OLPC. One of the ideas that we are exploring is service and support for deployments. As such, we are contracting developers to fix specific bugs that deployments hire us to fix or issues that Bernie identifies as high priority at the deployment in Paraguay. There are several organizations and companies already working in this field. The key difference between our company and others, in the field, is that our issues are not considered _complete_ until 1) the fix the customer's problem and 2) the fixes have been accepted upstream by Sugar Labs, OLPC, or Debian. I don't understand the pros and cons of the various of maintainer workflows to have a opinion. I would like to inform the community that: 1) You will be seeing a continued increase of deployment related bug fixes. 2) As these new developers become more experienced I will be happy to fund them to to maintain portions of Sugar. david On Tue, May 4, 2010 at 2:13 AM, Simon Schampijer si...@schampijer.de wrote: Sorry for being late in the discussion, just a few words from a Sugar module maintainer's point of few. I don't think the tool we used was the bottleneck. Of course there could have been some enhancements (like reminders to the ml), but in general it was ok. I personally had just too many things to do, Feature process, Release process etc. So we are in need of more man power, like laid out here several times. Mainly people taking responsibilities. I don't think we want to cut away the maintainers, this is important to keep the quality high. Thanks, Simon ___ Sugar-devel mailing list Sugar-devel@lists.sugarlabs.org http://lists.sugarlabs.org/listinfo/sugar-devel ___ Sugar-devel mailing list Sugar-devel@lists.sugarlabs.org http://lists.sugarlabs.org/listinfo/sugar-devel
Re: [Sugar-devel] Code Review process changes
On Fri, Apr 30, 2010 at 22:00, Bernie Innocenti ber...@codewiz.org wrote: El Fri, 30-04-2010 a las 10:49 +0200, Tomeu Vizoso escribió: What's the problem with plain email reviews that we're trying to solve with bug trackers and fancy review tools? It's useful to have the patches tracked and related to the bug report. Yes, but not all patches are related to bug reports. I'd like a fast path for simple patches that took 1 minute to write and shouldn't take more than 1 minute to review and apply. With a smooth development process, such trivial patches dominate the overall volume of patches. But if the overhead gets too high, they're lost. Are you saying you cannot have a fast track that keeps patches tracked? In GNOME and Freedesktop it takes a few seconds to submit a patch for review to bugzilla: $ git bz file my-product/some-component HEAD I don't have time to waste discussing how our system could be completely different. After we changed to use the same system as Linux, someone would start complaining we should switch to Mozilla's. Changing or refining the development process is as important as writing code. Now we have a problem that code already written is stuck because the current process is failing. I'm obviously ok with refining, I just don't have time energy nor time to switch to a completely different system now. For the sake of keeping some focus in this discussion, seems like you are saying that we cannot get enough resources to keep a process in which maintainers play a strong role. I actually think we can and I'm proposing a plan in another thread to get those resources. If SLs decides not to push for such a plan, then I would agree with you in that we need to make without maintainers. On IRC, you said you were in favor of doing reviews directly on the mailing list. Everyone else agreed, so I this part of the change should be already settled. It should be obvious, but just in case, we still need to agree on specific changes to the process and need to update the documentation in the wiki before we start using any different process than what we have documented now. We have a system modelled after GNOME's and it used to work when we had maintainers, and of course it works for the dozens of modules in GNOME and Freedesktop. Sure, we can make changes to adapt to the specifics of Sugar's community, but that request for change should come from the maintainers, who are the ones that will be most directly affected by them. The difference between us and GNOME seems to be in the availability of maintainers. Why it seems like that to you? For example, PyGObject is minimally maintained right now and it's affecting our work on Python 3. People are frustrated about it and we are discussing what to do, but I have still to hear from anyone that the maintainer-based system must be dropped. We are asking downstreams to contribute back with resources so PyGObject is maintained and next week I will be flying to the Ubuntu Developer Summit for that. Resourcing maintenance is often a problem, but somehow, not all projects think it's a good idea just to do without it. Maybe they have a reason? We can go back to the GNOME model when we'll have solved this issue, but at this time this model is clearly not working for us. Good luck with that experiment, I unfortunately won't have time to spend on it. Can peers also approve patches? If so, then I think we've already solved our issue. Well, we don't have enough peers, many of the listed them are not active any more. Having unresponsive people listed as maintainers/peers creates a false expectation. Let's remove those that did not post to the list or Trac over the last 2 months. We can quickly add them back if they come back. Sounds good to me, any volunteer to go through the list and propose the changes? Being so tightly coupled, these 4 modules should probably share the same set of maintainers and peers. That doesn't match my actual experience maintaining Sugar. You are guessing about what some people have actually experienced, please stop doing that. I'm not guessing. I've actually heard this complaint from several people who shall speak up for themselves publicly if they want to. This is the first item in Michael's experimental fork of Sugar lists as the first item: You are guessing because I doubt you have heard that complaint from anybody who has _maintained_ sugar (please read my words with more attention before replying). * combines all six of the sugar, sugar-toolkit, sugar-base, sugar-artwork, sugar-datastore, and sugar-presence-service repos into a single repo [1], So I'm certainly the only one who thinks that the current shredding of Sugar into 6 projects sucks. I'd be curious to know: who actually likes it this way and why? We need to split them conceptually because are different things. Having each of those conceptual units in their own repo with their own
Re: [Sugar-devel] Code Review process changes
On Thu, Apr 29, 2010 at 18:49, Bernie Innocenti ber...@codewiz.org wrote: El Wed, 28-04-2010 a las 18:41 +0200, Tomeu Vizoso escribió: Several contributors are hindered or even put off by the current process. It often takes more time to handle mere technicalities (save patch to file, create ticket in Trac, attach patch, wait for review, push) than it takes to fix the bug. We talked before of using XML-RPC to automate it, what happened to that? I'm using this for GNOME and Freedesktop projects and works very well: http://git.fishsoup.net/man/git-bz.html I also heard about a trac-email gateway, any news? What's the problem with plain email reviews that we're trying to solve with bug trackers and fancy review tools? It's useful to have the patches tracked and related to the bug report. On every release cycle (lasting ~2 months), the Linux kernel merges some 1 (ten thousands!) patches totaling 100 (one million!) lines of code, making it by far the fastest growing software project on the planet. See http://lwn.net/Articles/348445/ for more details. One may look at these numbers and wonder: what special tools are being used to manage such a gargantuan volume of patches? 1) git 2) email There's no 3. Projects much bigger than us, such as the Linux kernel and GCC have similarly been doing email reviews for the past 15 years without any trouble. The fear of loosing patches is really a red herring: if a patch got ignored, the poster (or anyone else) can simply ping it after 2-3 days. In fact, we're loosing more patches *now* by hiding them in Trac, away from the many eyeballs of all sugar-devel subscribers. I don't have time to waste discussing how our system could be completely different. After we changed to use the same system as Linux, someone would start complaining we should switch to Mozilla's. We have a system modelled after GNOME's and it used to work when we had maintainers, and of course it works for the dozens of modules in GNOME and Freedesktop. Sure, we can make changes to adapt to the specifics of Sugar's community, but that request for change should come from the maintainers, who are the ones that will be most directly affected by them. If we have such a thing as module peers is solely for helping out with reviews: http://wiki.sugarlabs.org/go/Development_Team/Release/Modules Can peers also approve patches? If so, then I think we've already solved our issue. Well, we don't have enough peers, many of the listed them are not active any more. But we also should have more than one maintainer per module. If peers can approve patches, I'd not be concerned if we have only one maintainer. Actually, two maintainers on the same module may even get into fights. Regarding sugar, sugar-toolkit, sugar-base and sugar-artwork... I don't see them as really separate modules: you can't really use patches often span across multiple modules and you can't really use them independently. Being so tightly coupled, these 4 modules should probably share the same set of maintainers and peers. That doesn't match my actual experience maintaining Sugar. You are guessing about what some people have actually experienced, please stop doing that. This seems to imply that the sole purpose of reviews is QA, but I think it has two more important purposes: transferring the burden of maintenance and sharing best practices and conventions. Indeed! This would make another good argument for carrying on reviews in the direct sunlight: so everyone would get to learn from each other's code. Sure, I'm a bit tired of repeating to you that I'm in favor of that. In practical terms, to me this means that reviews need to come from those who can appreciate the actual impact on maintenance of a patch, and that also have a direct interest in keeping the maintenance burden low. Let's distinguish the act of *reviewing* a patch, which anyone could do regardless of experience, from the act of approving it. I'm all running patches through maintainers for approval, IF THEY ARE RESPONSIVE. If not, we can't afford to block the entire development pipeline of Sugar due to lack of maintainer bandwidth. There are many healthy projects out there which managed to do well even without clear maintainers in the way of all patches. One very well known example of the anarchic model is Xorg. Others I participated in were uClinux, BDM and AROS. I'm not saying it's ideal, I'm just saying it's better than pissing off all new contributors by ignoring their patches. Note that what we're proposing is not totally anarchic: we're just proposing to enable more people to approve patches when the maintainer is unavailable or too busy. Upstream being unresponsive is the #1 complaint I hear about Sugar Labs lately, from many many people. We have a very serious problem that needs to be addressed ASAP. Hopefully we'll grow fresh new full-time maintainers from today's
Re: [Sugar-devel] Code Review process changes
El Fri, 30-04-2010 a las 10:49 +0200, Tomeu Vizoso escribió: What's the problem with plain email reviews that we're trying to solve with bug trackers and fancy review tools? It's useful to have the patches tracked and related to the bug report. Yes, but not all patches are related to bug reports. I'd like a fast path for simple patches that took 1 minute to write and shouldn't take more than 1 minute to review and apply. With a smooth development process, such trivial patches dominate the overall volume of patches. But if the overhead gets too high, they're lost. I don't have time to waste discussing how our system could be completely different. After we changed to use the same system as Linux, someone would start complaining we should switch to Mozilla's. Changing or refining the development process is as important as writing code. Now we have a problem that code already written is stuck because the current process is failing. On IRC, you said you were in favor of doing reviews directly on the mailing list. Everyone else agreed, so I this part of the change should be already settled. We have a system modelled after GNOME's and it used to work when we had maintainers, and of course it works for the dozens of modules in GNOME and Freedesktop. Sure, we can make changes to adapt to the specifics of Sugar's community, but that request for change should come from the maintainers, who are the ones that will be most directly affected by them. The difference between us and GNOME seems to be in the availability of maintainers. We can go back to the GNOME model when we'll have solved this issue, but at this time this model is clearly not working for us. Can peers also approve patches? If so, then I think we've already solved our issue. Well, we don't have enough peers, many of the listed them are not active any more. Having unresponsive people listed as maintainers/peers creates a false expectation. Let's remove those that did not post to the list or Trac over the last 2 months. We can quickly add them back if they come back. Being so tightly coupled, these 4 modules should probably share the same set of maintainers and peers. That doesn't match my actual experience maintaining Sugar. You are guessing about what some people have actually experienced, please stop doing that. I'm not guessing. I've actually heard this complaint from several people who shall speak up for themselves publicly if they want to. This is the first item in Michael's experimental fork of Sugar lists as the first item: * combines all six of the sugar, sugar-toolkit, sugar-base, sugar-artwork, sugar-datastore, and sugar-presence-service repos into a single repo [1], So I'm certainly the only one who thinks that the current shredding of Sugar into 6 projects sucks. I'd be curious to know: who actually likes it this way and why? Hopefully we'll grow fresh new full-time maintainers from today's newbies, but only if we make it possible for them to contribute now. I'm not interested in discussing changes to the system as long as we have Sugar in such unmaintained state. I have tried at least a dozen of times to start a discussion on this resourcing problem and have been ignored. Probably nobody knows what to answer! Free software projects like us often have to get going with the resources that happen to become available. I'm convinced the current volunteers base would expand dramatically if we'd let them contribute without demanding them full-time commitment. From my POV, changing the system to depend on less on maintainers is just side stepping the actual problem: nobody wants to do the boring maintenance work. I'm going to start one more thread about it and it will be my last try to get the SLs community to care about maintenance. I'm discussing with a few people to see if we could fill-in the gaps. Meanwhile, what shall we do? Do we halt development and give up? What I'm proposing would solve our most urgent issue without making it harder to find maintainers later on. Actually, it would make it a lot easier. We should at least try it before giving up. Giving more visibility to the review queue has nothing to do with where patches are posted and where the review happens. It has all to do with it! Patches in trac have been ignored for months, while all patches posted to this list immediately generated threads with interesting ideas. The difference is so evident that there should be no doubt about what's working best. The only thing that's missing now is giving the *current* reviewers also the authority to approve patches. We can call them maintainers or peers if you like. No, as I wrote before, people approving patches should be those who are going to be taking responsibility on maintaining the new code and also those who know what is a good patch in that module's context, which means spending time triaging and bug fixing. This is where we loose
Re: [Sugar-devel] Code Review process changes
El Wed, 28-04-2010 a las 18:41 +0200, Tomeu Vizoso escribió: Several contributors are hindered or even put off by the current process. It often takes more time to handle mere technicalities (save patch to file, create ticket in Trac, attach patch, wait for review, push) than it takes to fix the bug. We talked before of using XML-RPC to automate it, what happened to that? I'm using this for GNOME and Freedesktop projects and works very well: http://git.fishsoup.net/man/git-bz.html I also heard about a trac-email gateway, any news? What's the problem with plain email reviews that we're trying to solve with bug trackers and fancy review tools? On every release cycle (lasting ~2 months), the Linux kernel merges some 1 (ten thousands!) patches totaling 100 (one million!) lines of code, making it by far the fastest growing software project on the planet. See http://lwn.net/Articles/348445/ for more details. One may look at these numbers and wonder: what special tools are being used to manage such a gargantuan volume of patches? 1) git 2) email There's no 3. Projects much bigger than us, such as the Linux kernel and GCC have similarly been doing email reviews for the past 15 years without any trouble. The fear of loosing patches is really a red herring: if a patch got ignored, the poster (or anyone else) can simply ping it after 2-3 days. In fact, we're loosing more patches *now* by hiding them in Trac, away from the many eyeballs of all sugar-devel subscribers. If we have such a thing as module peers is solely for helping out with reviews: http://wiki.sugarlabs.org/go/Development_Team/Release/Modules Can peers also approve patches? If so, then I think we've already solved our issue. But we also should have more than one maintainer per module. If peers can approve patches, I'd not be concerned if we have only one maintainer. Actually, two maintainers on the same module may even get into fights. Regarding sugar, sugar-toolkit, sugar-base and sugar-artwork... I don't see them as really separate modules: you can't really use patches often span across multiple modules and you can't really use them independently. Being so tightly coupled, these 4 modules should probably share the same set of maintainers and peers. This seems to imply that the sole purpose of reviews is QA, but I think it has two more important purposes: transferring the burden of maintenance and sharing best practices and conventions. Indeed! This would make another good argument for carrying on reviews in the direct sunlight: so everyone would get to learn from each other's code. In practical terms, to me this means that reviews need to come from those who can appreciate the actual impact on maintenance of a patch, and that also have a direct interest in keeping the maintenance burden low. Let's distinguish the act of *reviewing* a patch, which anyone could do regardless of experience, from the act of approving it. I'm all running patches through maintainers for approval, IF THEY ARE RESPONSIVE. If not, we can't afford to block the entire development pipeline of Sugar due to lack of maintainer bandwidth. There are many healthy projects out there which managed to do well even without clear maintainers in the way of all patches. One very well known example of the anarchic model is Xorg. Others I participated in were uClinux, BDM and AROS. I'm not saying it's ideal, I'm just saying it's better than pissing off all new contributors by ignoring their patches. Note that what we're proposing is not totally anarchic: we're just proposing to enable more people to approve patches when the maintainer is unavailable or too busy. Upstream being unresponsive is the #1 complaint I hear about Sugar Labs lately, from many many people. We have a very serious problem that needs to be addressed ASAP. Hopefully we'll grow fresh new full-time maintainers from today's newbies, but only if we make it possible for them to contribute now. I'm confused, how are these systems better than the patch review report we used to have? For those who weren't with us back then: http://lists.sugarlabs.org/archive/sugar-devel/2008-July/006903.html That's nice, but I'd rather let everyone see the actual code right in the list without delays and reply with their comments inline. As a bonus, this lightweight process saves a lot of time to developers posting many small patches. OTOH, if a developer *likes* to go through Trac for posting a patch, they're still free to file tickets and then find a reviewer who would look at it. I've often done it myself for tickets that are already open, for informational purposes. But of course nobody reviewed my patches until I posted them to the list. (*) We defined Sugar developer as anybody who has made at least one change that entered mainline. To move forward: do you agree with this definition or would you prefer a stricter criteria for people who can approve
Re: [Sugar-devel] Code Review process changes
Hi! Bernie, Tomeu and I had a nice discussion regarding the Code Review process on #sugar yesterday. To sum it up: Several contributors are hindered or even put off by the current process. It often takes more time to handle mere technicalities (save patch to file, create ticket in Trac, attach patch, wait for review, push) than it takes to fix the bug. We talked before of using XML-RPC to automate it, what happened to that? I'm using this for GNOME and Freedesktop projects and works very well: http://git.fishsoup.net/man/git-bz.html I also heard about a trac-email gateway, any news? In addition there's a large backlog because reviews are currently handled only by module maintainers. The latter issue turned out to be at least partly due to misunderstandings about who can do reviews. If we have such a thing as module peers is solely for helping out with reviews: http://wiki.sugarlabs.org/go/Development_Team/Release/Modules But we also should have more than one maintainer per module. We'd like to try a different approach that's used by many successful projects - both small and large ones. Patches are sent to sugar-devel for review. Every Sugar developer (*) can review patches (and multiple reviews are quite welcome). Since the number of developers with commit access is limited, we have a sufficient level of QA even without limiting who can do reviews. This seems to imply that the sole purpose of reviews is QA, but I think it has two more important purposes: transferring the burden of maintenance and sharing best practices and conventions. In practical terms, to me this means that reviews need to come from those who can appreciate the actual impact on maintenance of a patch, and that also have a direct interest in keeping the maintenance burden low. There are a number of systems to track the status of patches sent to the list (e.g. Patchwork [1]), but as this adds complexity (and yet another system to maintain) again we'd like to try without at first. I'm confused, how are these systems better than the patch review report we used to have? For those who weren't with us back then: http://lists.sugarlabs.org/archive/sugar-devel/2008-July/006903.html Personal note: Instead of using a patch tracker, we could also ask patch submitters to file tickets at bugs.sugarlabs.org if there has been no review for, say, 3 days. This gives a streamlined process for most patches while still ensuring nothing gets lost. So we need to look for patches in two places? Regards, Tomeu (*) We defined Sugar developer as anybody who has made at least one change that entered mainline. [1] http://patchwork.ozlabs.org/ CU Sascha ___ Sugar-devel mailing list Sugar-devel@lists.sugarlabs.org http://lists.sugarlabs.org/listinfo/sugar-devel
Re: [Sugar-devel] Code Review process changes
On Fri, Apr 23, 2010 at 09:37:54AM -0400, Bernie Innocenti wrote: On Fri, 2010-04-23 at 15:17 +1000, James Cameron wrote: Activities too? I've been tracking #1571 for months now, and if posting the patch here will work, I'm all for it. ;-) If you were just asking whether it's ok to post patches for activities here, sure: we have no separate mailing list for activities (*). If, instead, you were proposing to relax the rules for approving patches to activities, I think we should discuss this carefully. Many activities would probably be better off with their only maintainer reviewing and approving each patch personally. What about the fructose activities? What about orphaned activities? The question of how to handle the case of an unresponsive maintainer came up on #sugar last week: shall we define a formal procedure for taking over projects in ASLO and Gitorious? This is how Fedora handles it: In my mind activities on ASLO are not something like packages in regular GNU/Linux distribution. ASLO is not one centralized product, as already was mentioned several times it is (or should) palace to share your work (maybe in trash) with others thus we (will)have bunch of outdated/ forgotten projects. And it is ok for sugar (but not for GNU/Linux distributions). And also most of activities come from individuals i.e. it's their masterpieces. So, lets talking about forking not about taking over. http://fedoraproject.org/wiki/Package_maintainer_policy#What_to_do_if_a_Maintainer_is_absent If it seems reasonable, we could adopt the very same procedure for ASLO and Gitorious, of course with some obvious changes: s/bugzilla/trac/, s/FESco/SLOBs/, s/CVS/Gitorious or ASLO/. In case of forks we don't need all this bureaucracy by design. If someone has any idea about improvement in activity, he can: * fork it in gitorius (no need in asking someone) * request fork for applying to master (ask only author not mailing lists/SLOBs etc) * if there is no feedback from current maintainer/author, just publish forked code as a fork in Activities Library (no need in asking someone) Yes, current ASLO can't handle all these forks in gitorius like scheme to let users at least see if there are forks of particular activity. Unfortunately it sounds pretty overkill to implement such features in ASLO hack (until appears someone who will implement them). In my mind instead of increasing level of bureaucracy in sugar, for now, we could ping authors/nd publish forks on ASLO (with new bundle_id) and start implementing new Activities Library with taking into account all sugar specific (ASLO in case of AMO is not such, since Mozilla controls AMO addons and it is mostly centralized repo w/ QA etc). Also don't read this text like absolute. It is not about deployment sugar distributions like OLPC, they have their list of stable/buggy activities and QA team to check them. This post only about having one repo for activities in gitorius free style. In my mind, ASLO was intended to be such repo, not list of blessed activities. I'm working on Activities Library (on early stage). It will look like: * server in the person of buddy in F1 view with shared instance of Library activity * any user can join this instance to see what activities are on the server * can just click to launch * can share new activity or fork of existed * any user can be such server, he just need to follow regular sugar practice, create new instance of Library and share it Any ideas are welcome http://wiki.sugarlabs.org/go/Activities/Library#Activity_Library -- Aleksey ___ Sugar-devel mailing list Sugar-devel@lists.sugarlabs.org http://lists.sugarlabs.org/listinfo/sugar-devel
Re: [Sugar-devel] Code Review process changes
On Fri, Apr 23, 2010 at 03:46, Bernie Innocenti ber...@codewiz.org wrote: On Tue, 2010-04-20 at 18:26 +0200, Sascha Silbe wrote: We'd like to try a different approach that's used by many successful projects - both small and large ones. Patches are sent to sugar-devel for review. Every Sugar developer (*) can review patches (and multiple reviews are quite welcome). Since the number of developers with commit access is limited, we have a sufficient level of QA even without limiting who can do reviews. Cannot find the rest of Sascha's email, was it sent to a list? The bits from Sascha that Bernie quoted refers to some context I cannot find, can someone repost? Or better, can we do what we agreed on #sugar and have a coherent proposal sent to the list? About Bernie's account on the kernel review practices, is it something that is being proposed for Sugar or is just food for thought? Thanks, Tomeu I noticed that we're already getting into this habit spontaneously, but it might be good to mention how the various tags are used in the Linux kernel. Here's the relevant excerpt from the Linux kernel documentation on submitting patches [1] [2]: -8--8--8--8--8--8- - Signed-off-by: this is a developer's certification that he or she has the right to submit the patch for inclusion into the kernel. It is an agreement to the Developer's Certificate of Origin, the full text of which can be found in Documentation/SubmittingPatches. Code without a proper signoff cannot be merged into the mainline. - Acked-by: indicates an agreement by another developer (often a maintainer of the relevant code) that the patch is appropriate for inclusion into the kernel. - Tested-by: states that the named person has tested the patch and found it to work. - Reviewed-by: the named developer has reviewed the patch for correctness; see the reviewer's statement in Documentation/SubmittingPatches for more detail. - Reported-by: names a user who reported a problem which is fixed by this patch; this tag is used to give credit to the (often underappreciated) people who test our code and let us know when things do not work correctly. - Cc: the named person received a copy of the patch and had the opportunity to comment on it. -8--8--8--8--8--8- Git handles some of these automatically. When you save a patch, you could simply do: # Export a patch with your Signed-off-by git format-patch -1 -s # Send patch to a maintainer, and cc the mailing list git send-email --to some...@sugarlabs.org --cc sugar-devel@lists.sugarlabs.org Committers should record in the patch who reviewed and ack'd a patch. This can be easily done by editing the comment in interactive mode: git am -i foo.patch (then use e to edit the patch) Alternatively, one could also edit the comment after the fact with git commit --amend. Attributing due credit to reviewers and testers is important because they're a scarce resource. A healthy development process depends on them as much as developers. Personal note: Instead of using a patch tracker, we could also ask patch submitters to file tickets at bugs.sugarlabs.org if there has been no review for, say, 3 days. This gives a streamlined process for most patches while still ensuring nothing gets lost. Another possibility is pinging on the list after a few days, perhaps adding on Cc people who are known to have worked in the area affected by the patch. for informational purposes, I continued to attach some patches to existing tickets. The difference is that it's no longer a required step for getting a patch in the mainline tree. [1] http://kernel.org/doc/Documentation/SubmittingPatches [2] http://kernel.org/doc/Documentation/development-process/5.Posting -- // Bernie Innocenti - http://codewiz.org/ \X/ Sugar Labs - http://sugarlabs.org/ ___ Sugar-devel mailing list Sugar-devel@lists.sugarlabs.org http://lists.sugarlabs.org/listinfo/sugar-devel ___ Sugar-devel mailing list Sugar-devel@lists.sugarlabs.org http://lists.sugarlabs.org/listinfo/sugar-devel
Re: [Sugar-devel] Code Review process changes
On Fri, Apr 23, 2010 at 12:43:33PM +0200, Tomeu Vizoso wrote: Cannot find the rest of Sascha's email, was it sent to a list? Yes, see [1]. Did gmail filter me out as SPAM again? Maybe you should deactivate the gmail SPAM filter for sugar-devel; we seem to have rather good filters on the list side already. The bits from Sascha that Bernie quoted refers to some context I cannot find, can someone repost? Does the archive link suffice or should I send you a copy privately? [1] http://lists.sugarlabs.org/archive/sugar-devel/2010-April/023410.html CU Sascha -- http://sascha.silbe.org/ http://www.infra-silbe.de/ signature.asc Description: Digital signature ___ Sugar-devel mailing list Sugar-devel@lists.sugarlabs.org http://lists.sugarlabs.org/listinfo/sugar-devel
Re: [Sugar-devel] Code Review process changes
On Fri, 2010-04-23 at 15:17 +1000, James Cameron wrote: Activities too? I've been tracking #1571 for months now, and if posting the patch here will work, I'm all for it. ;-) If you were just asking whether it's ok to post patches for activities here, sure: we have no separate mailing list for activities (*). If, instead, you were proposing to relax the rules for approving patches to activities, I think we should discuss this carefully. Many activities would probably be better off with their only maintainer reviewing and approving each patch personally. What about the fructose activities? What about orphaned activities? The question of how to handle the case of an unresponsive maintainer came up on #sugar last week: shall we define a formal procedure for taking over projects in ASLO and Gitorious? This is how Fedora handles it: http://fedoraproject.org/wiki/Package_maintainer_policy#What_to_do_if_a_Maintainer_is_absent If it seems reasonable, we could adopt the very same procedure for ASLO and Gitorious, of course with some obvious changes: s/bugzilla/trac/, s/FESco/SLOBs/, s/CVS/Gitorious or ASLO/. (*) this was a deliberate choice: this way, core developers would get a sense of what the infrastructure needs of activities really are, while activity writers would learn about Sugar internals and eventually become core developers. -- // Bernie Innocenti - http://codewiz.org/ \X/ Sugar Labs - http://sugarlabs.org/ ___ Sugar-devel mailing list Sugar-devel@lists.sugarlabs.org http://lists.sugarlabs.org/listinfo/sugar-devel
Re: [Sugar-devel] Code Review process changes
On Tue, 2010-04-20 at 18:26 +0200, Sascha Silbe wrote: We'd like to try a different approach that's used by many successful projects - both small and large ones. Patches are sent to sugar-devel for review. Every Sugar developer (*) can review patches (and multiple reviews are quite welcome). Since the number of developers with commit access is limited, we have a sufficient level of QA even without limiting who can do reviews. I noticed that we're already getting into this habit spontaneously, but it might be good to mention how the various tags are used in the Linux kernel. Here's the relevant excerpt from the Linux kernel documentation on submitting patches [1] [2]: -8--8--8--8--8--8- - Signed-off-by: this is a developer's certification that he or she has the right to submit the patch for inclusion into the kernel. It is an agreement to the Developer's Certificate of Origin, the full text of which can be found in Documentation/SubmittingPatches. Code without a proper signoff cannot be merged into the mainline. - Acked-by: indicates an agreement by another developer (often a maintainer of the relevant code) that the patch is appropriate for inclusion into the kernel. - Tested-by: states that the named person has tested the patch and found it to work. - Reviewed-by: the named developer has reviewed the patch for correctness; see the reviewer's statement in Documentation/SubmittingPatches for more detail. - Reported-by: names a user who reported a problem which is fixed by this patch; this tag is used to give credit to the (often underappreciated) people who test our code and let us know when things do not work correctly. - Cc: the named person received a copy of the patch and had the opportunity to comment on it. -8--8--8--8--8--8- Git handles some of these automatically. When you save a patch, you could simply do: # Export a patch with your Signed-off-by git format-patch -1 -s # Send patch to a maintainer, and cc the mailing list git send-email --to some...@sugarlabs.org --cc sugar-devel@lists.sugarlabs.org Committers should record in the patch who reviewed and ack'd a patch. This can be easily done by editing the comment in interactive mode: git am -i foo.patch (then use e to edit the patch) Alternatively, one could also edit the comment after the fact with git commit --amend. Attributing due credit to reviewers and testers is important because they're a scarce resource. A healthy development process depends on them as much as developers. Personal note: Instead of using a patch tracker, we could also ask patch submitters to file tickets at bugs.sugarlabs.org if there has been no review for, say, 3 days. This gives a streamlined process for most patches while still ensuring nothing gets lost. Another possibility is pinging on the list after a few days, perhaps adding on Cc people who are known to have worked in the area affected by the patch. for informational purposes, I continued to attach some patches to existing tickets. The difference is that it's no longer a required step for getting a patch in the mainline tree. [1] http://kernel.org/doc/Documentation/SubmittingPatches [2] http://kernel.org/doc/Documentation/development-process/5.Posting -- // Bernie Innocenti - http://codewiz.org/ \X/ Sugar Labs - http://sugarlabs.org/ ___ Sugar-devel mailing list Sugar-devel@lists.sugarlabs.org http://lists.sugarlabs.org/listinfo/sugar-devel
Re: [Sugar-devel] Code Review process changes
Activities too? I've been tracking #1571 for months now, and if posting the patch here will work, I'm all for it. ;-) -- James Cameron http://quozl.linux.org.au/ ___ Sugar-devel mailing list Sugar-devel@lists.sugarlabs.org http://lists.sugarlabs.org/listinfo/sugar-devel
[Sugar-devel] Code Review process changes
Hi! Bernie, Tomeu and I had a nice discussion regarding the Code Review process on #sugar yesterday. To sum it up: Several contributors are hindered or even put off by the current process. It often takes more time to handle mere technicalities (save patch to file, create ticket in Trac, attach patch, wait for review, push) than it takes to fix the bug. In addition there's a large backlog because reviews are currently handled only by module maintainers. The latter issue turned out to be at least partly due to misunderstandings about who can do reviews. We'd like to try a different approach that's used by many successful projects - both small and large ones. Patches are sent to sugar-devel for review. Every Sugar developer (*) can review patches (and multiple reviews are quite welcome). Since the number of developers with commit access is limited, we have a sufficient level of QA even without limiting who can do reviews. There are a number of systems to track the status of patches sent to the list (e.g. Patchwork [1]), but as this adds complexity (and yet another system to maintain) again we'd like to try without at first. Personal note: Instead of using a patch tracker, we could also ask patch submitters to file tickets at bugs.sugarlabs.org if there has been no review for, say, 3 days. This gives a streamlined process for most patches while still ensuring nothing gets lost. (*) We defined Sugar developer as anybody who has made at least one change that entered mainline. [1] http://patchwork.ozlabs.org/ CU Sascha -- http://sascha.silbe.org/ http://www.infra-silbe.de/ signature.asc Description: Digital signature ___ Sugar-devel mailing list Sugar-devel@lists.sugarlabs.org http://lists.sugarlabs.org/listinfo/sugar-devel
