| Message ID | 20221123104946.29480-1-mail@cbaines.net |
|---|---|
| State | New |
| Headers | show |
| Series | [bug#59513] doc: contributing: Tweak the Commit Policy. | expand |
Hi Chris, On Wed, 23 Nov 2022 at 10:49, Christopher Baines <mail@cbaines.net> wrote: > +For a minority of changes, it can be appropriate to push them directly > +without sending them for review. This includes both trivial changes > +(e.g. fixing typos) but also reverting problomatic changes and -^ > +addressing regressions. > -For patches that just add a new package, and a simple one, it's OK to > -commit, if you're confident (which means you successfully built it in a > -chroot setup, and have done a reasonable copyright and license > -auditing). Likewise for package upgrades, except upgrades that trigger > -a lot of rebuilds (for example, upgrading GnuTLS or GLib). We have a > -mailing list for commit notifications (@email{guix-commits@@gnu.org}), > -so people can notice. Before pushing your changes, make sure to run > -@code{git pull --rebase}. > +In general though, all changes should be posted to > +@email{guix-patches@@gnu.org}. This mailing list fills the > +patch-tracking database (@pxref{Tracking Bugs and Patches}). Leave time > +for a review, without committing anything (@pxref{Submitting Patches}). > +If you didn’t receive any reply after one week (two weeks for more > +significant changes), and if you're confident, it's OK to commit. I would write: … changes), and if you're confident (which means you successfully built it in a chroot setup, and have done a reasonable copyright and license auditing), it’s OK to commit. and I would keep the «two weeks» instead of the «one week except». I think it is also useful to provide the information about commit notifications (guix-commits mailing list). For what it is worth, I find clearer the structure, For patches that … For anything else, … or For a minority of changes, … For anything else, … than «In general though, all changes …». Cheers, simon
zimoun <zimon.toutoune@gmail.com> writes: > Hi Chris, Hey! Thanks for taking a look. > On Wed, 23 Nov 2022 at 10:49, Christopher Baines <mail@cbaines.net> wrote: > >> +For a minority of changes, it can be appropriate to push them directly >> +without sending them for review. This includes both trivial changes >> +(e.g. fixing typos) but also reverting problomatic changes and > -^ >> +addressing regressions. > > >> -For patches that just add a new package, and a simple one, it's OK to >> -commit, if you're confident (which means you successfully built it in a >> -chroot setup, and have done a reasonable copyright and license >> -auditing). Likewise for package upgrades, except upgrades that trigger >> -a lot of rebuilds (for example, upgrading GnuTLS or GLib). We have a >> -mailing list for commit notifications (@email{guix-commits@@gnu.org}), >> -so people can notice. Before pushing your changes, make sure to run >> -@code{git pull --rebase}. >> +In general though, all changes should be posted to >> +@email{guix-patches@@gnu.org}. This mailing list fills the >> +patch-tracking database (@pxref{Tracking Bugs and Patches}). Leave time >> +for a review, without committing anything (@pxref{Submitting Patches}). >> +If you didn’t receive any reply after one week (two weeks for more >> +significant changes), and if you're confident, it's OK to commit. > > I would write: > > … changes), and if you're confident (which means you > successfully built it in a chroot setup, and have done a > reasonable copyright and license auditing), it’s OK to commit. chroot setup doesn't really make sense to me, I'm not sure why that needs specifying (like do we not want things for the Hurd pushing, since the guix-daemon doesn't support build isolation there yet)? Also, this guidance is very general, and I think it should be applicable to all changes. We already trust people with commit access to know what needs doing, I see this documentation as more about how, so I'd prefer not to try and put a list here. > and I would keep the «two weeks» instead of the «one week except». My reason for changing this is that I think waiting two weeks after sending a simple patch is unreasonable. The value from the automated testing will come after one to two days, I just put a week to avoid changing it too much, but maybe the lower bound should be two days. > I think it is also useful to provide the information about commit > notifications (guix-commits mailing list). Why though? What do we expect people with commit access to do when they read about that here? > For what it is worth, I find clearer the structure, > > For patches that … > For anything else, … > > or > > For a minority of changes, … > For anything else, … > > than «In general though, all changes …». That seems fine to me, I think "everything" maybe carries more weight than "anything" though. Thanks, Chris
Hi, On Thu, 24 Nov 2022 at 08:40, Christopher Baines <mail@cbaines.net> wrote: >> On Wed, 23 Nov 2022 at 10:49, Christopher Baines <mail@cbaines.net> wrote: >> >>> +For a minority of changes, it can be appropriate to push them directly >>> +without sending them for review. This includes both trivial changes >>> +(e.g. fixing typos) but also reverting problomatic changes and >> -^ >>> +addressing regressions. To be sure you have not missed the typo here. :-) s/problomatic/problematic >> … changes), and if you're confident (which means you >> successfully built it in a chroot setup, and have done a >> reasonable copyright and license auditing), it’s OK to commit. > > chroot setup doesn't really make sense to me, I'm not sure why that > needs specifying (like do we not want things for the Hurd pushing, since > the guix-daemon doesn't support build isolation there yet)? Good point about chroot. :-) > Also, this guidance is very general, and I think it should be applicable > to all changes. We already trust people with commit access to know what > needs doing, I see this documentation as more about how, so I'd prefer > not to try and put a list here. Yes, we trust people. But a public and explicit policy reinforces the trust, IMHO. It also documents what commit access means. It is not because people with commit access already know what they need doing that all people know, I guess. >> and I would keep the «two weeks» instead of the «one week except». > > My reason for changing this is that I think waiting two weeks after > sending a simple patch is unreasonable. The value from the automated > testing will come after one to two days, I just put a week to avoid > changing it too much, but maybe the lower bound should be two days. Who is verifying the impact of a change? :-) Just a recent example to fix the ideas. The same situation is happening more than often but not that often neither. :-) (It is an example and no blame here. Or blame on me only, for not taking enough care of Julia packages.) Patch#58644 [1] submitted on 19 Oct and pushed on 8 Nov; which is 22 days. Unfortunately, this patch breaks julia-documenter [2], so it means many Julia packages are currently broken; since 17 days. Commit 83ede5a02e1fc531d912eb92eb0a22a4b897997c, gnu: git: Update to 2.38.1. Fixes CVE-2022-39253 and CVE-2022-39260. * gnu/packages/version-control.scm (git): Update to 2.38.1. Co-authored-by: Ludovic Courtès <ludo@gnu.org> 1 file changed, 3 insertions(+), 3 deletions(-) gnu/packages/version-control.scm | 6 +++--- from v2.38.0 to v2.38.1 seems inoffensive. :-) But, --8<---------------cut here---------------start------------->8--- $ ag 'inherit git' gnu/packages/version-control.scm 613: (inherit git) 676: (package/inherit git-minimal $ guix refresh -l git git-minimal | cut -f1 -d':' Building the following 292 packages would ensure 658 dependent packages are rebuilt --8<---------------cut here---------------end--------------->8--- (The one at line 676 is not impacted by the change, IIRC.) Who does check these 292 packages? For instance, this patch has an impact on Julia packages, --8<---------------cut here---------------start------------->8--- $ guix refresh -l git git-minimal | cut -f2 -d':' | sed 's/ /\n/g' | grep julia julia-geometrybasics@0.4.1 julia-configurations@0.16.4 julia-pyplot@2.10.0 julia-recipespipeline@0.3.4 julia-quadmath@0.5.5 julia-plotthemes@2.0.1 julia-infinity@0.2.4 julia-testimages@1.5.0 julia-optim@1.6.0 julia-referencetests@0.9.7 julia-imagemagick@1.2.1 --8<---------------cut here---------------end--------------->8--- As part of the Julia team, maybe I could have a look. Well, I had not had the time in these 2 weeks to fix it yet. For two reasons: 1. Because I noticed the failure just a couple of days ago. 2. Because I was busy elsewhere. About #1, I can follow a RSS feed by Cuirass. But somehow, it is too late then either I am working in a rush to minimize the breakage or either the package is broken… as today. I have not yet carefully looked at the new QA (neat!). Is it possible to follow some notifications? About #2, two weeks let the time to check the impact of a change. And even, depending on my schedule, sometime it is short. :-) But hey, I agree that the things need to move forward. :-) My point is: Considering leaf packages, yeah once submitted, the review can be fast (couple of days) especially with the new QA. Considering all the other packages, who is checking the impact of a change? Otherwise, we have again and again some broken packages. For sure, the QA is helping *a lot* for improving! Well, on one hand, I understand the willing to merge faster and, even I am not convinced that from two weeks to one week would be detrimental. On the other hand, using Guix, I replaced the pressure when running “apt-get upgrade” by an eternal annoyance of broken packages popping here or there. Somehow, <https://ci.guix.gnu.org/eval/4095/dashboard> should be always all green and faster the Git tree moves, harder it is to achieve, IMHO. 1: https://issues.guix.gnu.org/58644 2: <https://data.guix.gnu.org/repository/1/branch/master/package/julia-documenter/output-history> >> I think it is also useful to provide the information about commit >> notifications (guix-commits mailing list). > > Why though? What do we expect people with commit access to do when they > read about that here? Maybe it is a niche, I used it a couple of time. For instance, to comment a change already merged, see [3] I guess. 3: <https://lists.gnu.org/archive/html/guix-devel/2022-10/msg00099.html> Cheers, simon
zimoun <zimon.toutoune@gmail.com> writes: > On Thu, 24 Nov 2022 at 08:40, Christopher Baines <mail@cbaines.net> wrote: > >>> On Wed, 23 Nov 2022 at 10:49, Christopher Baines <mail@cbaines.net> wrote: >>> >>>> +For a minority of changes, it can be appropriate to push them directly >>>> +without sending them for review. This includes both trivial changes >>>> +(e.g. fixing typos) but also reverting problomatic changes and >>> -^ >>>> +addressing regressions. > > To be sure you have not missed the typo here. :-) > > s/problomatic/problematic Thanks, I've fixed that locally now. >> Also, this guidance is very general, and I think it should be applicable >> to all changes. We already trust people with commit access to know what >> needs doing, I see this documentation as more about how, so I'd prefer >> not to try and put a list here. > > Yes, we trust people. But a public and explicit policy reinforces the > trust, IMHO. It also documents what commit access means. It is not > because people with commit access already know what they need doing that > all people know, I guess. I don't disagree that we should make the expectations about functionality and testing explicit, but I want to see that separate from the commit policy. >>> and I would keep the «two weeks» instead of the «one week except». >> >> My reason for changing this is that I think waiting two weeks after >> sending a simple patch is unreasonable. The value from the automated >> testing will come after one to two days, I just put a week to avoid >> changing it too much, but maybe the lower bound should be two days. > > Who is verifying the impact of a change? :-) Just a recent example to > fix the ideas. The same situation is happening more than often but not > that often neither. :-) ... > My point is: Considering leaf packages, yeah once submitted, the review > can be fast (couple of days) especially with the new QA. Considering > all the other packages, who is checking the impact of a change? > > Otherwise, we have again and again some broken packages. For sure, the > QA is helping *a lot* for improving! Well, on one hand, I understand > the willing to merge faster and, even I am not convinced that from two > weeks to one week would be detrimental. On the other hand, using Guix, > I replaced the pressure when running “apt-get upgrade” by an eternal > annoyance of broken packages popping here or there. This is going a bit off topic I think. In general, the direction I'm trying to move the policy in here is one where more changes get sent to guix-patches rather than getting pushed straight to the repository. Checking the impact of changes is important, but you can't do that with a policy on committing. If however people send changes to guix-patches prior to pushing, then there's at least a chance that some automatic "verifying/checking" can take place.
Hi! Christopher Baines <mail@cbaines.net> skribis: > Add more examples of when it can be appropriate to push changes without > review, as I think this can be appropriate in the case of trivial changes (as > mentioned before), but also non-trivial fixes. > > No longer suggest pushing simple new packages or package upgrades (that don't > cause lots of rebuilds) without sending to guix-patches. Now there's some > automation for testing changes sent to guix-patches, sending changes there > before pushing can mean that more rigerious testing takes place and help speed > up substitutes becoming available. This is true, even if no human review takes > place. > > Only suggest waiting one week for review for simpler changes, wait two weeks > for more significant changes. > > Also, reorder some of the information in this section so it's grouped together > better. > > * doc/contributing.texi (Commit Policy): Tweak. FWIW I like the spirit of these changes. Now to the letter… :-) > @subsection Commit Policy > > -If you get commit access, please make sure to follow > -the policy below (discussions of the policy can take place on > +If you get commit access, please make sure to follow the policy below > +(discussions of the policy can take place on > @email{guix-devel@@gnu.org}). > > -Non-trivial patches should always be posted to > -@email{guix-patches@@gnu.org} (trivial patches include fixing typos, > -etc.). This mailing list fills the patch-tracking database > -(@pxref{Tracking Bugs and Patches}). > +For a minority of changes, it can be appropriate to push them directly > +without sending them for review. This includes both trivial changes > +(e.g. fixing typos) but also reverting problomatic changes and > +addressing regressions. > > -For patches that just add a new package, and a simple one, it's OK to Similar to zimoun’s first comment I think, I would like the beginning of the sentence to clearly tell you whether it’s the situation you’re interested in. “For a minority of changes” doesn’t fit the bill in my view. So I would suggest something along the lines of: Changes should be posted to @email{guix-patches@@gnu.org}. This mailing list […]. It also allows patches to be picked up and tested by the quality assurance robot; the result of that testing eventually shows up on the dashboard at @indicateurl{https://qa.guix.gnu.org/issue/@var{number}}, where @var{number} is the number assigned by the issue tracker. Leave time […] it’s OK to commit. As an exception, some changes considered consensual and ``trivial'' or ``obvious'' may instead be pushed directly. These include: fixing typos, and reverting commits that caused immediate problems. That way we state the general rule first, and the exception next. That also explicitly mentions how that relates to qa.guix. Regarding the list of exceptions, I feel that these two exceptions listed here may be less than what we may except on a day-to-day basis; perhaps there are other things to add there, but I’m not sure what. Would be nice to get feedback from a maintainer too. Thanks for working on this! Ludo’.
Hi, Christopher Baines <mail@cbaines.net> skribis: >> I would write: >> >> … changes), and if you're confident (which means you >> successfully built it in a chroot setup, and have done a >> reasonable copyright and license auditing), it’s OK to commit. > > chroot setup doesn't really make sense to me, I'm not sure why that > needs specifying The chroot requirement is specified because a non-isolated build is worth nothing: it might work on some machine and fail on another one for hard-to-find reasons. We could stop mentioning it if we think everyone keeps chroot enabled (that’s probably the case), but it doesn’t hurt to keep it. > (like do we not want things for the Hurd pushing, since the > guix-daemon doesn't support build isolation there yet)? Well yes, that’s the thing. For i586-gnu we’re currently stuck because we haven’t yet defined how to normalize the build environment: https://guix.gnu.org/en/blog/2020/childhurds-and-substitutes/ https://issues.guix.gnu.org/43857 And it’s not theoretical: some util-linux tests may or may not work depending on whether the Hurd box has /proc set up, the Texinfo test suite would pass if there happens to be a usable ‘pt_chown’ program around (see <https://issues.guix.gnu.org/59616>), etc. So I think the current situation is that we can build base packages for i586-gnu, that’s fine, but don’t have a solid foundation like we do on GNU/Linux, so there’s no point in going too far. Now, I don’t think the i586-gnu situation is an important consideration for the commit policy. Ludo’.
Ludovic Courtès <ludo@gnu.org> writes: >> @subsection Commit Policy >> >> -If you get commit access, please make sure to follow >> -the policy below (discussions of the policy can take place on >> +If you get commit access, please make sure to follow the policy below >> +(discussions of the policy can take place on >> @email{guix-devel@@gnu.org}). >> >> -Non-trivial patches should always be posted to >> -@email{guix-patches@@gnu.org} (trivial patches include fixing typos, >> -etc.). This mailing list fills the patch-tracking database >> -(@pxref{Tracking Bugs and Patches}). >> +For a minority of changes, it can be appropriate to push them directly >> +without sending them for review. This includes both trivial changes >> +(e.g. fixing typos) but also reverting problomatic changes and >> +addressing regressions. >> >> -For patches that just add a new package, and a simple one, it's OK to > > Similar to zimoun’s first comment I think, I would like the beginning of > the sentence to clearly tell you whether it’s the situation you’re > interested in. “For a minority of changes” doesn’t fit the bill in my > view. > > So I would suggest something along the lines of: > > Changes should be posted to @email{guix-patches@@gnu.org}. This > mailing list […]. It also allows patches to be picked up and tested > by the quality assurance robot; the result of that testing eventually I've gone for "tooling" rather than "robot" as I'm not sure we want to go the way of personifying it. I'm not against that, but the place to start is probably not here. > shows up on the dashboard at > @indicateurl{https://qa.guix.gnu.org/issue/@var{number}}, where > @var{number} is the number assigned by the issue tracker. Leave time > […] it’s OK to commit. > > As an exception, some changes considered consensual and ``trivial'' or I removed "consensual" here as I wasn't sure what was meant by that, or at least I'm not sure the phrasing fits the context here. Are you trying to say something about a belief that no one will object to the change being made? > ``obvious'' may instead be pushed directly. These include: fixing > typos, and reverting commits that caused immediate problems. > > That way we state the general rule first, and the exception next. That > also explicitly mentions how that relates to qa.guix. Yeah, I think that's better. I've sent a v2 patch now (for some reason I forgot to send this email until now). Thanks, Chris
Hi Chris, Christopher Baines <mail@cbaines.net> skribis: > Ludovic Courtès <ludo@gnu.org> writes: > >>> @subsection Commit Policy >>> >>> -If you get commit access, please make sure to follow >>> -the policy below (discussions of the policy can take place on >>> +If you get commit access, please make sure to follow the policy below >>> +(discussions of the policy can take place on >>> @email{guix-devel@@gnu.org}). BTW, we should follow that policy :-) and send a heads-up on guix-devel. > I've gone for "tooling" rather than "robot" as I'm not sure we want to > go the way of personifying it. I'm not against that, but the place to > start is probably not here. Fine with me! >> shows up on the dashboard at >> @indicateurl{https://qa.guix.gnu.org/issue/@var{number}}, where >> @var{number} is the number assigned by the issue tracker. Leave time >> […] it’s OK to commit. >> >> As an exception, some changes considered consensual and ``trivial'' or > > I removed "consensual" here as I wasn't sure what was meant by that, or > at least I'm not sure the phrasing fits the context here. > > Are you trying to say something about a belief that no one will object > to the change being made? Yes, exactly. With experience in the project (like committers usually have), you can often tell whether a given change might raise objections or not. Thanks for following up! Ludo’.
diff --git a/doc/contributing.texi b/doc/contributing.texi index 40ae33ecac..6f772961ea 100644 --- a/doc/contributing.texi +++ b/doc/contributing.texi @@ -1819,23 +1819,25 @@ It additionally calls @code{make check-channel-news} to be sure @subsection Commit Policy -If you get commit access, please make sure to follow -the policy below (discussions of the policy can take place on +If you get commit access, please make sure to follow the policy below +(discussions of the policy can take place on @email{guix-devel@@gnu.org}). -Non-trivial patches should always be posted to -@email{guix-patches@@gnu.org} (trivial patches include fixing typos, -etc.). This mailing list fills the patch-tracking database -(@pxref{Tracking Bugs and Patches}). +For a minority of changes, it can be appropriate to push them directly +without sending them for review. This includes both trivial changes +(e.g. fixing typos) but also reverting problomatic changes and +addressing regressions. -For patches that just add a new package, and a simple one, it's OK to -commit, if you're confident (which means you successfully built it in a -chroot setup, and have done a reasonable copyright and license -auditing). Likewise for package upgrades, except upgrades that trigger -a lot of rebuilds (for example, upgrading GnuTLS or GLib). We have a -mailing list for commit notifications (@email{guix-commits@@gnu.org}), -so people can notice. Before pushing your changes, make sure to run -@code{git pull --rebase}. +In general though, all changes should be posted to +@email{guix-patches@@gnu.org}. This mailing list fills the +patch-tracking database (@pxref{Tracking Bugs and Patches}). Leave time +for a review, without committing anything (@pxref{Submitting Patches}). +If you didn’t receive any reply after one week (two weeks for more +significant changes), and if you're confident, it's OK to commit. + +That last part is subject to being adjusted, allowing individuals to +commit directly on non-controversial changes on parts they’re familiar +with. When pushing a commit on behalf of somebody else, please add a @code{Signed-off-by} line at the end of the commit log message---e.g., @@ -1850,14 +1852,6 @@ right before pushing: make check-channel-news @end example -For anything else, please post to @email{guix-patches@@gnu.org} and -leave time for a review, without committing anything (@pxref{Submitting -Patches}). If you didn’t receive any reply after two weeks, and if -you're confident, it's OK to commit. - -That last part is subject to being adjusted, allowing individuals to commit -directly on non-controversial changes on parts they’re familiar with. - @subsection Addressing Issues Peer review (@pxref{Submitting Patches}) and tools such as