Re: Patches submission policy change
On 03/04/2024 14:23, Christophe Lyon via Gcc wrote: > On Wed, 3 Apr 2024 at 14:59, Joel Sherrill wrote: >> >> Another possible issue which may be better now than in years past >> is that the versions of autoconf/automake required often had to be >> installed by hand. I think newlib has gotten better but before the >> rework on its Makefile/configure, I had a special install of autotools >> which precisely matched what it required. >> >> And that led to very few people being able to successfully regenerate. >> >> Is that avoidable? >> >> OTOH the set of people touching these files may be small enough that >> pain isn't an issue. >> > > For binutils/gcc/gdb we still have to use specific versions which are > generally not the distro's ones. That's because at least some distros modify autoconf to their own taste/needs, so that it does not generate the same output as the officially released version. Furthermore, they provide no mechanism to make their version revert back to the original behaviour. R.
Re: Patches submission policy change
Hi Jonathan, On Sun, Apr 07, 2024 at 03:20:42PM +0100, Jonathan Wakely wrote: > On Sun, 7 Apr 2024, 15:02 Mark Wielaard, wrote: > > The jit mailinglist is the same. It only has one moderator > > (David). Having a second/backup one would probably be nice. Are you ok > > to be added there? > > Sure! I imagine that one is quiet as well. Yes, we like quiet :) It means the spam filters work correctly. > > (I see that both the libstdc++ and jit mailinglist just allow up to > > 5MB patches, which probably means they don't block any legitimate > > ones, but might get a bit more spam than necessary). > > > > It isn't a patches mailinglist, so slightly less urgent, but the > > gcc-help mailinglist doesn't have any moderators. Would you be willing > > to help out with that one? > > Sure, I'm subscribed and active there anyway. Thanks for volunteering, you have been added to both as admin. Cheers, Mark
Re: Patches submission policy change
On Sun, 7 Apr 2024, 15:02 Mark Wielaard, wrote: > Hi Jonathan, > > On Sun, Apr 07, 2024 at 01:32:11PM +0100, Jonathan Wakely via Gdb wrote: > > On Thu, 4 Apr 2024, 22:36 Mark Wielaard, wrote: > > > wrt to the mailinglists maybe getting larger patches, I think most > > > will still be under 400K and I wouldn't raise the limit (because most > > > such larger emails are really just spam). But we might want to get > > > more mailinglist moderators. > > > > > > gcc-patches, binutils and gdb-patches all have only one moderator > > > (Jeff, Ian and Thiago). It would probably be good if there were > > > more. > > > > > > Any volunteers? It shouldn't be more than 1 to 3 emails a week > > > (sadly most of them spam). > > > > I'm happy to help moderating/spambusting for the GCC lists. > > I see you are already doing that for the libstdc++ mailinglist, but > you are the only moderator. Maybe you want someone as backup there? > It wouldn't hurt to have backup if somebody wants to volunteer, but I don't remember ever having to moderate anything for that list. Maybe just once. > The jit mailinglist is the same. It only has one moderator > (David). Having a second/backup one would probably be nice. Are you ok > to be added there? > Sure! I imagine that one is quiet as well. > (I see that both the libstdc++ and jit mailinglist just allow up to > 5MB patches, which probably means they don't block any legitimate > ones, but might get a bit more spam than necessary). > > It isn't a patches mailinglist, so slightly less urgent, but the > gcc-help mailinglist doesn't have any moderators. Would you be willing > to help out with that one? > Sure, I'm subscribed and active there anyway.
Re: Patches submission policy change
Hi Jonathan, On Sun, Apr 07, 2024 at 01:32:11PM +0100, Jonathan Wakely via Gdb wrote: > On Thu, 4 Apr 2024, 22:36 Mark Wielaard, wrote: > > wrt to the mailinglists maybe getting larger patches, I think most > > will still be under 400K and I wouldn't raise the limit (because most > > such larger emails are really just spam). But we might want to get > > more mailinglist moderators. > > > > gcc-patches, binutils and gdb-patches all have only one moderator > > (Jeff, Ian and Thiago). It would probably be good if there were > > more. > > > > Any volunteers? It shouldn't be more than 1 to 3 emails a week > > (sadly most of them spam). > > I'm happy to help moderating/spambusting for the GCC lists. I see you are already doing that for the libstdc++ mailinglist, but you are the only moderator. Maybe you want someone as backup there? The jit mailinglist is the same. It only has one moderator (David). Having a second/backup one would probably be nice. Are you ok to be added there? (I see that both the libstdc++ and jit mailinglist just allow up to 5MB patches, which probably means they don't block any legitimate ones, but might get a bit more spam than necessary). It isn't a patches mailinglist, so slightly less urgent, but the gcc-help mailinglist doesn't have any moderators. Would you be willing to help out with that one? Thanks, Mark
Re: Patches submission policy change
On Thu, 4 Apr 2024, 22:36 Mark Wielaard, wrote: > wrt to the mailinglists maybe getting larger patches, I think most > will still be under 400K and I wouldn't raise the limit (because most > such larger emails are really just spam). But we might want to get > more mailinglist moderators. > > gcc-patches, binutils and gdb-patches all have only one moderator > (Jeff, Ian and Thiago). It would probably be good if there were > more. > > Any volunteers? It shouldn't be more than 1 to 3 emails a week > (sadly most of them spam). > > Cheers, > > Mark > I'm happy to help moderating/spambusting for the GCC lists.
Re: Patches submission policy change
Hi, On Fri, 2024-04-05 at 09:17 +0200, Christophe Lyon wrote: > On 4/4/24 23:35, Mark Wielaard wrote: > > wrt to the mailinglists maybe getting larger patches, I think most > > will still be under 400K and I wouldn't raise the limit (because most > > such larger emails are really just spam). But we might want to get > > more mailinglist moderators. > > > > gcc-patches, binutils and gdb-patches all have only one moderator > > (Jeff, Ian and Thiago). It would probably be good if there were > > more. > > > > Any volunteers? It shouldn't be more than 1 to 3 emails a week > > (sadly most of them spam). > > > I'm happy to help with moderation of any/all of these 3 lists. I added Simon for gdb, Marc for gcc and you as admin for binutils. So all three projects now have at least two moderators. Thanks so much for volunteering. And please reach out to overseers if this becomes too much of a burden (it really should not be more than a handful of messages each week). Then we'll see if we can adjust the (spam) settings so only oversized messages reach the moderation queues (or raise the limits if that makes more sense). Thanks, Mark
Re: Patches submission policy change
On Thu, 4 Apr 2024 at 10:12, Jan Beulich wrote: > > On 03.04.2024 15:11, Christophe Lyon wrote: > > On Wed, 3 Apr 2024 at 10:30, Jan Beulich wrote: > >> > >> On 03.04.2024 10:22, Christophe Lyon wrote: > >>> Dear release managers and developers, > >>> > >>> TL;DR: For the sake of improving precommit CI coverage and simplifying > >>> workflows, I’d like to request a patch submission policy change, so > >>> that we now include regenerated files. This was discussed during the > >>> last GNU toolchain office hours meeting [1] (2024-03-28). > >>> > >>> Benefits or this change include: > >>> - Increased compatibility with precommit CI > >>> - No need to manually edit patches before submitting, thus the “git > >>> send-email” workflow is simplified > >>> - Patch reviewers can be confident that the committed patch will be > >>> exactly what they approved > >>> - Precommit CI can test exactly what has been submitted > >>> > >>> Any concerns/objections? > >> > >> Yes: Patch size. And no, not sending patches inline is bad practice. > > Not sure what you mean? Do you mean sending patches as attachments is > > bad practice? > > Yes. It makes it difficult to reply to them (with proper reply context). Agreed. > > >> Even assuming sending patches bi-modal (inline and as attachment) works > >> (please indicate whether that's the case), it would mean extra work on > >> the sending side. > >> > > For the CI perspective, we use what patchwork is able to detect as patches. > > Looking at recent patches submissions, it seems patchwork is able to > > cope with the output of git format-patch/git send-email, as well as > > attachments. > > There are cases where patchwork is not able to detect the patch, but I > > don't know patchwork's exact specifications. > > Question was though: If a patch was sent inline plus attachment, what > would CI use as the patch to apply? IOW would it be an option to > attach the un-stripped patch, while inlining the stripped one? > Sorry, I don't know how patchwork would handle such a case. Thanks, Christophe > Jan >
Re: Patches submission policy change
Hi Mark, On 4/4/24 23:35, Mark Wielaard wrote: Hi Christophe, On Wed, Apr 03, 2024 at 10:22:24AM +0200, Christophe Lyon via Gdb wrote: TL;DR: For the sake of improving precommit CI coverage and simplifying workflows, I’d like to request a patch submission policy change, so that we now include regenerated files. This was discussed during the last GNU toolchain office hours meeting [1] (2024-03-28). Benefits or this change include: - Increased compatibility with precommit CI - No need to manually edit patches before submitting, thus the “git send-email” workflow is simplified - Patch reviewers can be confident that the committed patch will be exactly what they approved - Precommit CI can test exactly what has been submitted Any concerns/objections? I am all for it. It will make testing patches easier for everyone. I do think we still need a better way to make sure all generated files can be regenerated. If only to check that the files were generated correctly with the correct versions. The autoregen buildbots are able to catch some, but not all such mistakes. wrt to the mailinglists maybe getting larger patches, I think most will still be under 400K and I wouldn't raise the limit (because most such larger emails are really just spam). But we might want to get more mailinglist moderators. gcc-patches, binutils and gdb-patches all have only one moderator (Jeff, Ian and Thiago). It would probably be good if there were more. Any volunteers? It shouldn't be more than 1 to 3 emails a week (sadly most of them spam). I'm happy to help with moderation of any/all of these 3 lists. Thanks, Christophe Cheers, Mark
Re: Patches submission policy change
Mark Wielaard writes: Hello Mark! > gcc-patches, binutils and gdb-patches all have only one moderator > (Jeff, Ian and Thiago). It would probably be good if there were > more. > > Any volunteers? It shouldn't be more than 1 to 3 emails a week > (sadly most of them spam). If still needed, I volunteer for the gcc list! Marc
Re: Patches submission policy change
On 2024-04-04 17:35, Mark Wielaard wrote: > Hi Christophe, > > On Wed, Apr 03, 2024 at 10:22:24AM +0200, Christophe Lyon via Gdb wrote: >> TL;DR: For the sake of improving precommit CI coverage and simplifying >> workflows, I’d like to request a patch submission policy change, so >> that we now include regenerated files. This was discussed during the >> last GNU toolchain office hours meeting [1] (2024-03-28). >> >> Benefits or this change include: >> - Increased compatibility with precommit CI >> - No need to manually edit patches before submitting, thus the “git >> send-email” workflow is simplified >> - Patch reviewers can be confident that the committed patch will be >> exactly what they approved >> - Precommit CI can test exactly what has been submitted >> >> Any concerns/objections? > > I am all for it. It will make testing patches easier for everyone. > > I do think we still need a better way to make sure all generated files > can be regenerated. If only to check that the files were generated > correctly with the correct versions. The autoregen buildbots are able > to catch some, but not all such mistakes. > > wrt to the mailinglists maybe getting larger patches, I think most > will still be under 400K and I wouldn't raise the limit (because most > such larger emails are really just spam). But we might want to get > more mailinglist moderators. > > gcc-patches, binutils and gdb-patches all have only one moderator > (Jeff, Ian and Thiago). It would probably be good if there were > more. > > Any volunteers? It shouldn't be more than 1 to 3 emails a week > (sadly most of them spam). I can help with the various gdb mailing lists. Simon
Re: Patches submission policy change
Hi Christophe, On Wed, Apr 03, 2024 at 10:22:24AM +0200, Christophe Lyon via Gdb wrote: > TL;DR: For the sake of improving precommit CI coverage and simplifying > workflows, I’d like to request a patch submission policy change, so > that we now include regenerated files. This was discussed during the > last GNU toolchain office hours meeting [1] (2024-03-28). > > Benefits or this change include: > - Increased compatibility with precommit CI > - No need to manually edit patches before submitting, thus the “git > send-email” workflow is simplified > - Patch reviewers can be confident that the committed patch will be > exactly what they approved > - Precommit CI can test exactly what has been submitted > > Any concerns/objections? I am all for it. It will make testing patches easier for everyone. I do think we still need a better way to make sure all generated files can be regenerated. If only to check that the files were generated correctly with the correct versions. The autoregen buildbots are able to catch some, but not all such mistakes. wrt to the mailinglists maybe getting larger patches, I think most will still be under 400K and I wouldn't raise the limit (because most such larger emails are really just spam). But we might want to get more mailinglist moderators. gcc-patches, binutils and gdb-patches all have only one moderator (Jeff, Ian and Thiago). It would probably be good if there were more. Any volunteers? It shouldn't be more than 1 to 3 emails a week (sadly most of them spam). Cheers, Mark
Re: Patches submission policy change
On 03.04.2024 15:11, Christophe Lyon wrote: > On Wed, 3 Apr 2024 at 10:30, Jan Beulich wrote: >> >> On 03.04.2024 10:22, Christophe Lyon wrote: >>> Dear release managers and developers, >>> >>> TL;DR: For the sake of improving precommit CI coverage and simplifying >>> workflows, I’d like to request a patch submission policy change, so >>> that we now include regenerated files. This was discussed during the >>> last GNU toolchain office hours meeting [1] (2024-03-28). >>> >>> Benefits or this change include: >>> - Increased compatibility with precommit CI >>> - No need to manually edit patches before submitting, thus the “git >>> send-email” workflow is simplified >>> - Patch reviewers can be confident that the committed patch will be >>> exactly what they approved >>> - Precommit CI can test exactly what has been submitted >>> >>> Any concerns/objections? >> >> Yes: Patch size. And no, not sending patches inline is bad practice. > Not sure what you mean? Do you mean sending patches as attachments is > bad practice? Yes. It makes it difficult to reply to them (with proper reply context). >> Even assuming sending patches bi-modal (inline and as attachment) works >> (please indicate whether that's the case), it would mean extra work on >> the sending side. >> > For the CI perspective, we use what patchwork is able to detect as patches. > Looking at recent patches submissions, it seems patchwork is able to > cope with the output of git format-patch/git send-email, as well as > attachments. > There are cases where patchwork is not able to detect the patch, but I > don't know patchwork's exact specifications. Question was though: If a patch was sent inline plus attachment, what would CI use as the patch to apply? IOW would it be an option to attach the un-stripped patch, while inlining the stripped one? Jan
Re: Patches submission policy change
On 4/3/24 4:22 AM, Christophe Lyon via Gdb wrote: > Dear release managers and developers, > > TL;DR: For the sake of improving precommit CI coverage and simplifying > workflows, I’d like to request a patch submission policy change, so > that we now include regenerated files. This was discussed during the > last GNU toolchain office hours meeting [1] (2024-03-28). > > Benefits or this change include: > - Increased compatibility with precommit CI > - No need to manually edit patches before submitting, thus the “git > send-email” workflow is simplified > - Patch reviewers can be confident that the committed patch will be > exactly what they approved > - Precommit CI can test exactly what has been submitted > > Any concerns/objections? > > As discussed on the lists and during the meeting, we have been facing > issues with testing a class of patches: those which imply regenerating > some files. Indeed, for binutils and gcc, the current patch submission > policy is to *not* include the regenerated files (IIUC the policy is > different for gdb [2]). > > This means that precommit CI receives an incomplete patch, leading to > wrong and misleading regression reports, and complaints/frustration. > (our notifications now include a warning, making it clear that we do > not regenerate files ;-) ) > > I thought the solution was as easy as adding –enable-maintainer-mode > to the configure arguments but this has proven to be broken (random > failures with highly parallel builds). I tried to workaround that by > adding new “regenerate” rules in the makefiles, that we could build at > -j1 before running the real build with a higher parallelism level, but > this is not ideal, not to mention that in the case of gcc, configuring > target libraries requires having built all-gcc before, which is quite > slow at -j1. > > Another approach used in binutils and gdb builtbots is a dedicated > script (aka autoregen.py) which calls autotools as appropriate. It > could be extended to update other types of files, but this can be a > bit tricky (eg. some opcodes files require to build a generator first, > some doc fragments also use non-trivial build sequences), and it > creates a maintenance issue: the build recipe is duplicated in the > script and in the makefiles. Such a script has proven to be useful > though in postcommit CI, to catch regeneration errors. > > Having said that, it seems that for the sake of improving usefulness > of precommit CI, the simplest way forward is to change the policy to > include regenerated files. This does not seem to be a burden for > developers, since they have to regenerate the files before pushing > their patches anyway, and it also enables reviewers to make sure the > generated files are correct. > > Said differently, if you want to increase the chances of having your > patches tested by precommit CI, make sure to include all the > regenerated files, otherwise you might receive failure notifications. > > Any concerns/objections? We already do that for GDB (include generated files), and it works fine. I sometimes have a patch that exceeds the mailing list limit (400k?), but it seems like the only consequence is that someone needs to approve it (thanks to whoever does that). Simon
Re: Patches submission policy change
On Wed, 3 Apr 2024 at 14:59, Joel Sherrill wrote: > > Another possible issue which may be better now than in years past > is that the versions of autoconf/automake required often had to be > installed by hand. I think newlib has gotten better but before the > rework on its Makefile/configure, I had a special install of autotools > which precisely matched what it required. > > And that led to very few people being able to successfully regenerate. > > Is that avoidable? > > OTOH the set of people touching these files may be small enough that > pain isn't an issue. > For binutils/gcc/gdb we still have to use specific versions which are generally not the distro's ones. So indeed, the number of people having to update autotools-related files is relatively small, but there are other files which are regenerated during the build process when maintainer-mode is enabled (for instance parts of the gcc documentation, or opcodes tables in binutils, and these are modified by more people. Thanks, Christophe > --joel > > On Wed, Apr 3, 2024 at 5:22 AM Jan Beulich via Gcc wrote: >> >> On 03.04.2024 10:57, Richard Biener wrote: >> > On Wed, 3 Apr 2024, Jan Beulich wrote: >> >> On 03.04.2024 10:45, Jakub Jelinek wrote: >> >>> On Wed, Apr 03, 2024 at 10:22:24AM +0200, Christophe Lyon wrote: >> Any concerns/objections? >> >>> >> >>> I'm all for it, in fact I've been sending it like that myself for years >> >>> even when the policy said not to. In most cases, the diff for the >> >>> regenerated files is very small and it helps even in patch review to >> >>> actually check if the configure.ac/m4 etc. changes result just in the >> >>> expected changes and not some unrelated ones (e.g. caused by user using >> >>> wrong version of autoconf/automake etc.). >> >>> There can be exceptions, e.g. when in GCC we update from a new version >> >>> of Unicode, the regenerated ucnid.h diff can be large and >> >>> uname2c.h can be huge, such that it can trigger the mailing list size >> >>> limits even when the patch is compressed, see e.g. >> >>> https://gcc.gnu.org/pipermail/gcc-patches/2023-November/636427.html >> >>> https://gcc.gnu.org/pipermail/gcc-patches/2023-November/636426.html >> >>> But I think most configure or Makefile changes should be pretty small, >> >>> usual changes shouldn't rewrite everything in those files. >> >> >> >> Which may then call for a policy saying "include generate script diff-s, >> >> but don't include generated data file ones"? At least on the binutils >> >> side, dealing (for CI) with what e.g. opcodes/*-gen produce ought to be >> >> possible by having something along the lines of "maintainer mode light". >> > >> > I'd say we should send generated files when it fits the mailing list >> > limits (and possibly simply lift those limits?). >> >> Well, that would allow patches making it through, but it would still >> severely increase overall size. I'm afraid more people than not also >> fail to cut down reply context, so we'd further see (needlessly) huge >> replies to patches as well. >> >> Additionally - how does one up front determine "fits the mailing list >> limits"? My mail UI (Thunderbird) doesn't show me the size of a message >> until I've actually sent it. >> >> > As a last resort >> > do a series splitting the re-generation out (but I guess this would >> > confuse the CI as well and of course for the push you want to squash >> > again). >> >> Yeah, unless the CI would only ever test full series, this wouldn't help. >> It's also imo even more cumbersome than simply stripping the generated >> file parts from emails. >> >> Jan
Re: Patches submission policy change
On Wed, 3 Apr 2024 at 12:21, Jan Beulich wrote: > > On 03.04.2024 10:57, Richard Biener wrote: > > On Wed, 3 Apr 2024, Jan Beulich wrote: > >> On 03.04.2024 10:45, Jakub Jelinek wrote: > >>> On Wed, Apr 03, 2024 at 10:22:24AM +0200, Christophe Lyon wrote: > Any concerns/objections? > >>> > >>> I'm all for it, in fact I've been sending it like that myself for years > >>> even when the policy said not to. In most cases, the diff for the > >>> regenerated files is very small and it helps even in patch review to > >>> actually check if the configure.ac/m4 etc. changes result just in the > >>> expected changes and not some unrelated ones (e.g. caused by user using > >>> wrong version of autoconf/automake etc.). > >>> There can be exceptions, e.g. when in GCC we update from a new version > >>> of Unicode, the regenerated ucnid.h diff can be large and > >>> uname2c.h can be huge, such that it can trigger the mailing list size > >>> limits even when the patch is compressed, see e.g. > >>> https://gcc.gnu.org/pipermail/gcc-patches/2023-November/636427.html > >>> https://gcc.gnu.org/pipermail/gcc-patches/2023-November/636426.html > >>> But I think most configure or Makefile changes should be pretty small, > >>> usual changes shouldn't rewrite everything in those files. > >> > >> Which may then call for a policy saying "include generate script diff-s, > >> but don't include generated data file ones"? At least on the binutils > >> side, dealing (for CI) with what e.g. opcodes/*-gen produce ought to be > >> possible by having something along the lines of "maintainer mode light". > > > > I'd say we should send generated files when it fits the mailing list > > limits (and possibly simply lift those limits?). > > Well, that would allow patches making it through, but it would still > severely increase overall size. I'm afraid more people than not also > fail to cut down reply context, so we'd further see (needlessly) huge > replies to patches as well. > > Additionally - how does one up front determine "fits the mailing list > limits"? My mail UI (Thunderbird) doesn't show me the size of a message > until I've actually sent it. > > > As a last resort > > do a series splitting the re-generation out (but I guess this would > > confuse the CI as well and of course for the push you want to squash > > again). > > Yeah, unless the CI would only ever test full series, this wouldn't help. > It's also imo even more cumbersome than simply stripping the generated > file parts from emails. > Our CI starts by testing the full series, then iterates by dropping the top-most patches one by one, to make sure no patch breaks something that is fixed in a later patch. This is meant to be additional information for patch reviewers, if they use patchwork to assist them. Other CIs may behave differently though. Thanks, Christophe > Jan
Re: Patches submission policy change
On Wed, 3 Apr 2024 at 10:30, Jan Beulich wrote: > > On 03.04.2024 10:22, Christophe Lyon wrote: > > Dear release managers and developers, > > > > TL;DR: For the sake of improving precommit CI coverage and simplifying > > workflows, I’d like to request a patch submission policy change, so > > that we now include regenerated files. This was discussed during the > > last GNU toolchain office hours meeting [1] (2024-03-28). > > > > Benefits or this change include: > > - Increased compatibility with precommit CI > > - No need to manually edit patches before submitting, thus the “git > > send-email” workflow is simplified > > - Patch reviewers can be confident that the committed patch will be > > exactly what they approved > > - Precommit CI can test exactly what has been submitted > > > > Any concerns/objections? > > Yes: Patch size. And no, not sending patches inline is bad practice. Not sure what you mean? Do you mean sending patches as attachments is bad practice? > Even assuming sending patches bi-modal (inline and as attachment) works > (please indicate whether that's the case), it would mean extra work on > the sending side. > For the CI perspective, we use what patchwork is able to detect as patches. Looking at recent patches submissions, it seems patchwork is able to cope with the output of git format-patch/git send-email, as well as attachments. There are cases where patchwork is not able to detect the patch, but I don't know patchwork's exact specifications. Thanks, Christophe > Jan
Re: Patches submission policy change
On Wed, Apr 3, 2024 at 6:23 AM Jan Beulich via Gcc wrote: > On 03.04.2024 10:57, Richard Biener wrote: > > On Wed, 3 Apr 2024, Jan Beulich wrote: > >> On 03.04.2024 10:45, Jakub Jelinek wrote: > >>> On Wed, Apr 03, 2024 at 10:22:24AM +0200, Christophe Lyon wrote: > Any concerns/objections? > >>> > >>> I'm all for it, in fact I've been sending it like that myself for years > >>> even when the policy said not to. In most cases, the diff for the > >>> regenerated files is very small and it helps even in patch review to > >>> actually check if the configure.ac/m4 etc. changes result just in the > >>> expected changes and not some unrelated ones (e.g. caused by user using > >>> wrong version of autoconf/automake etc.). > >>> There can be exceptions, e.g. when in GCC we update from a new version > >>> of Unicode, the regenerated ucnid.h diff can be large and > >>> uname2c.h can be huge, such that it can trigger the mailing list size > >>> limits even when the patch is compressed, see e.g. > >>> https://gcc.gnu.org/pipermail/gcc-patches/2023-November/636427.html > >>> https://gcc.gnu.org/pipermail/gcc-patches/2023-November/636426.html > >>> But I think most configure or Makefile changes should be pretty small, > >>> usual changes shouldn't rewrite everything in those files. > >> > >> Which may then call for a policy saying "include generate script diff-s, > >> but don't include generated data file ones"? At least on the binutils > >> side, dealing (for CI) with what e.g. opcodes/*-gen produce ought to be > >> possible by having something along the lines of "maintainer mode light". > > > > I'd say we should send generated files when it fits the mailing list > > limits (and possibly simply lift those limits?). > > Well, that would allow patches making it through, but it would still > severely increase overall size. I'm afraid more people than not also > fail to cut down reply context, so we'd further see (needlessly) huge > replies to patches as well. > > Additionally - how does one up front determine "fits the mailing list > limits"? My mail UI (Thunderbird) doesn't show me the size of a message > until I've actually sent it. > > > As a last resort > > do a series splitting the re-generation out (but I guess this would > > confuse the CI as well and of course for the push you want to squash > > again). > > Yeah, unless the CI would only ever test full series, this wouldn't help. > It's also imo even more cumbersome than simply stripping the generated > file parts from emails. > Massive patches need special handling either way; I wouldn't lift the mailing list limits because of this change. I'm in favor of this change for typical patches. Jason
Re: Patches submission policy change
Another possible issue which may be better now than in years past is that the versions of autoconf/automake required often had to be installed by hand. I think newlib has gotten better but before the rework on its Makefile/configure, I had a special install of autotools which precisely matched what it required. And that led to very few people being able to successfully regenerate. Is that avoidable? OTOH the set of people touching these files may be small enough that pain isn't an issue. --joel On Wed, Apr 3, 2024 at 5:22 AM Jan Beulich via Gcc wrote: > On 03.04.2024 10:57, Richard Biener wrote: > > On Wed, 3 Apr 2024, Jan Beulich wrote: > >> On 03.04.2024 10:45, Jakub Jelinek wrote: > >>> On Wed, Apr 03, 2024 at 10:22:24AM +0200, Christophe Lyon wrote: > Any concerns/objections? > >>> > >>> I'm all for it, in fact I've been sending it like that myself for years > >>> even when the policy said not to. In most cases, the diff for the > >>> regenerated files is very small and it helps even in patch review to > >>> actually check if the configure.ac/m4 etc. changes result just in the > >>> expected changes and not some unrelated ones (e.g. caused by user using > >>> wrong version of autoconf/automake etc.). > >>> There can be exceptions, e.g. when in GCC we update from a new version > >>> of Unicode, the regenerated ucnid.h diff can be large and > >>> uname2c.h can be huge, such that it can trigger the mailing list size > >>> limits even when the patch is compressed, see e.g. > >>> https://gcc.gnu.org/pipermail/gcc-patches/2023-November/636427.html > >>> https://gcc.gnu.org/pipermail/gcc-patches/2023-November/636426.html > >>> But I think most configure or Makefile changes should be pretty small, > >>> usual changes shouldn't rewrite everything in those files. > >> > >> Which may then call for a policy saying "include generate script diff-s, > >> but don't include generated data file ones"? At least on the binutils > >> side, dealing (for CI) with what e.g. opcodes/*-gen produce ought to be > >> possible by having something along the lines of "maintainer mode light". > > > > I'd say we should send generated files when it fits the mailing list > > limits (and possibly simply lift those limits?). > > Well, that would allow patches making it through, but it would still > severely increase overall size. I'm afraid more people than not also > fail to cut down reply context, so we'd further see (needlessly) huge > replies to patches as well. > > Additionally - how does one up front determine "fits the mailing list > limits"? My mail UI (Thunderbird) doesn't show me the size of a message > until I've actually sent it. > > > As a last resort > > do a series splitting the re-generation out (but I guess this would > > confuse the CI as well and of course for the push you want to squash > > again). > > Yeah, unless the CI would only ever test full series, this wouldn't help. > It's also imo even more cumbersome than simply stripping the generated > file parts from emails. > > Jan >
Re: Patches submission policy change
On 03.04.2024 10:57, Richard Biener wrote: > On Wed, 3 Apr 2024, Jan Beulich wrote: >> On 03.04.2024 10:45, Jakub Jelinek wrote: >>> On Wed, Apr 03, 2024 at 10:22:24AM +0200, Christophe Lyon wrote: Any concerns/objections? >>> >>> I'm all for it, in fact I've been sending it like that myself for years >>> even when the policy said not to. In most cases, the diff for the >>> regenerated files is very small and it helps even in patch review to >>> actually check if the configure.ac/m4 etc. changes result just in the >>> expected changes and not some unrelated ones (e.g. caused by user using >>> wrong version of autoconf/automake etc.). >>> There can be exceptions, e.g. when in GCC we update from a new version >>> of Unicode, the regenerated ucnid.h diff can be large and >>> uname2c.h can be huge, such that it can trigger the mailing list size >>> limits even when the patch is compressed, see e.g. >>> https://gcc.gnu.org/pipermail/gcc-patches/2023-November/636427.html >>> https://gcc.gnu.org/pipermail/gcc-patches/2023-November/636426.html >>> But I think most configure or Makefile changes should be pretty small, >>> usual changes shouldn't rewrite everything in those files. >> >> Which may then call for a policy saying "include generate script diff-s, >> but don't include generated data file ones"? At least on the binutils >> side, dealing (for CI) with what e.g. opcodes/*-gen produce ought to be >> possible by having something along the lines of "maintainer mode light". > > I'd say we should send generated files when it fits the mailing list > limits (and possibly simply lift those limits?). Well, that would allow patches making it through, but it would still severely increase overall size. I'm afraid more people than not also fail to cut down reply context, so we'd further see (needlessly) huge replies to patches as well. Additionally - how does one up front determine "fits the mailing list limits"? My mail UI (Thunderbird) doesn't show me the size of a message until I've actually sent it. > As a last resort > do a series splitting the re-generation out (but I guess this would > confuse the CI as well and of course for the push you want to squash > again). Yeah, unless the CI would only ever test full series, this wouldn't help. It's also imo even more cumbersome than simply stripping the generated file parts from emails. Jan
Re: Patches submission policy change
On Wed, 3 Apr 2024 at 09:46, Jakub Jelinek via Gcc wrote: > > On Wed, Apr 03, 2024 at 10:22:24AM +0200, Christophe Lyon wrote: > > Any concerns/objections? > > I'm all for it, in fact I've been sending it like that myself for years > even when the policy said not to. In most cases, the diff for the > regenerated files is very small and it helps even in patch review to > actually check if the configure.ac/m4 etc. changes result just in the > expected changes and not some unrelated ones (e.g. caused by user using > wrong version of autoconf/automake etc.). > There can be exceptions, e.g. when in GCC we update from a new version > of Unicode, the regenerated ucnid.h diff can be large and > uname2c.h can be huge, such that it can trigger the mailing list size > limits even when the patch is compressed, see e.g. > https://gcc.gnu.org/pipermail/gcc-patches/2023-November/636427.html > https://gcc.gnu.org/pipermail/gcc-patches/2023-November/636426.html > But I think most configure or Makefile changes should be pretty small, > usual changes shouldn't rewrite everything in those files. For libstdc++ we've had two "large" changes to regenerated files in the past year, but they're not common: https://gcc.gnu.org/r14-5424-gdb50aea6259545 https://gcc.gnu.org/r14-5342-g898fd81b831c10 We were getting large, useless diffs for libstdc++-v3/include/bits/version.h too (e.g. r14-7220-gac1a399bf61b04) but I've fixed that now. In ye olde days I used filterdiff to strip the generated files from patch submissions, but with git send-email I no longer use filterdiff, so as Christophe said, the suggested policy would avoid manually editing emails before sending. I don't feel strongly either way, but I have no objection to the change.
Re: Patches submission policy change
On Wed, 3 Apr 2024, Jan Beulich wrote: > On 03.04.2024 10:45, Jakub Jelinek wrote: > > On Wed, Apr 03, 2024 at 10:22:24AM +0200, Christophe Lyon wrote: > >> Any concerns/objections? > > > > I'm all for it, in fact I've been sending it like that myself for years > > even when the policy said not to. In most cases, the diff for the > > regenerated files is very small and it helps even in patch review to > > actually check if the configure.ac/m4 etc. changes result just in the > > expected changes and not some unrelated ones (e.g. caused by user using > > wrong version of autoconf/automake etc.). > > There can be exceptions, e.g. when in GCC we update from a new version > > of Unicode, the regenerated ucnid.h diff can be large and > > uname2c.h can be huge, such that it can trigger the mailing list size > > limits even when the patch is compressed, see e.g. > > https://gcc.gnu.org/pipermail/gcc-patches/2023-November/636427.html > > https://gcc.gnu.org/pipermail/gcc-patches/2023-November/636426.html > > But I think most configure or Makefile changes should be pretty small, > > usual changes shouldn't rewrite everything in those files. > > Which may then call for a policy saying "include generate script diff-s, > but don't include generated data file ones"? At least on the binutils > side, dealing (for CI) with what e.g. opcodes/*-gen produce ought to be > possible by having something along the lines of "maintainer mode light". I'd say we should send generated files when it fits the mailing list limits (and possibly simply lift those limits?). As a last resort do a series splitting the re-generation out (but I guess this would confuse the CI as well and of course for the push you want to squash again). Richard. > Jan >
Re: Patches submission policy change
On 03.04.2024 10:45, Jakub Jelinek wrote: > On Wed, Apr 03, 2024 at 10:22:24AM +0200, Christophe Lyon wrote: >> Any concerns/objections? > > I'm all for it, in fact I've been sending it like that myself for years > even when the policy said not to. In most cases, the diff for the > regenerated files is very small and it helps even in patch review to > actually check if the configure.ac/m4 etc. changes result just in the > expected changes and not some unrelated ones (e.g. caused by user using > wrong version of autoconf/automake etc.). > There can be exceptions, e.g. when in GCC we update from a new version > of Unicode, the regenerated ucnid.h diff can be large and > uname2c.h can be huge, such that it can trigger the mailing list size > limits even when the patch is compressed, see e.g. > https://gcc.gnu.org/pipermail/gcc-patches/2023-November/636427.html > https://gcc.gnu.org/pipermail/gcc-patches/2023-November/636426.html > But I think most configure or Makefile changes should be pretty small, > usual changes shouldn't rewrite everything in those files. Which may then call for a policy saying "include generate script diff-s, but don't include generated data file ones"? At least on the binutils side, dealing (for CI) with what e.g. opcodes/*-gen produce ought to be possible by having something along the lines of "maintainer mode light". Jan
Re: Patches submission policy change
On Wed, Apr 03, 2024 at 10:22:24AM +0200, Christophe Lyon wrote: > Any concerns/objections? I'm all for it, in fact I've been sending it like that myself for years even when the policy said not to. In most cases, the diff for the regenerated files is very small and it helps even in patch review to actually check if the configure.ac/m4 etc. changes result just in the expected changes and not some unrelated ones (e.g. caused by user using wrong version of autoconf/automake etc.). There can be exceptions, e.g. when in GCC we update from a new version of Unicode, the regenerated ucnid.h diff can be large and uname2c.h can be huge, such that it can trigger the mailing list size limits even when the patch is compressed, see e.g. https://gcc.gnu.org/pipermail/gcc-patches/2023-November/636427.html https://gcc.gnu.org/pipermail/gcc-patches/2023-November/636426.html But I think most configure or Makefile changes should be pretty small, usual changes shouldn't rewrite everything in those files. Jakub
Re: Patches submission policy change
On 03.04.2024 10:22, Christophe Lyon wrote: > Dear release managers and developers, > > TL;DR: For the sake of improving precommit CI coverage and simplifying > workflows, I’d like to request a patch submission policy change, so > that we now include regenerated files. This was discussed during the > last GNU toolchain office hours meeting [1] (2024-03-28). > > Benefits or this change include: > - Increased compatibility with precommit CI > - No need to manually edit patches before submitting, thus the “git > send-email” workflow is simplified > - Patch reviewers can be confident that the committed patch will be > exactly what they approved > - Precommit CI can test exactly what has been submitted > > Any concerns/objections? Yes: Patch size. And no, not sending patches inline is bad practice. Even assuming sending patches bi-modal (inline and as attachment) works (please indicate whether that's the case), it would mean extra work on the sending side. Jan
Patches submission policy change
Dear release managers and developers, TL;DR: For the sake of improving precommit CI coverage and simplifying workflows, I’d like to request a patch submission policy change, so that we now include regenerated files. This was discussed during the last GNU toolchain office hours meeting [1] (2024-03-28). Benefits or this change include: - Increased compatibility with precommit CI - No need to manually edit patches before submitting, thus the “git send-email” workflow is simplified - Patch reviewers can be confident that the committed patch will be exactly what they approved - Precommit CI can test exactly what has been submitted Any concerns/objections? As discussed on the lists and during the meeting, we have been facing issues with testing a class of patches: those which imply regenerating some files. Indeed, for binutils and gcc, the current patch submission policy is to *not* include the regenerated files (IIUC the policy is different for gdb [2]). This means that precommit CI receives an incomplete patch, leading to wrong and misleading regression reports, and complaints/frustration. (our notifications now include a warning, making it clear that we do not regenerate files ;-) ) I thought the solution was as easy as adding –enable-maintainer-mode to the configure arguments but this has proven to be broken (random failures with highly parallel builds). I tried to workaround that by adding new “regenerate” rules in the makefiles, that we could build at -j1 before running the real build with a higher parallelism level, but this is not ideal, not to mention that in the case of gcc, configuring target libraries requires having built all-gcc before, which is quite slow at -j1. Another approach used in binutils and gdb builtbots is a dedicated script (aka autoregen.py) which calls autotools as appropriate. It could be extended to update other types of files, but this can be a bit tricky (eg. some opcodes files require to build a generator first, some doc fragments also use non-trivial build sequences), and it creates a maintenance issue: the build recipe is duplicated in the script and in the makefiles. Such a script has proven to be useful though in postcommit CI, to catch regeneration errors. Having said that, it seems that for the sake of improving usefulness of precommit CI, the simplest way forward is to change the policy to include regenerated files. This does not seem to be a burden for developers, since they have to regenerate the files before pushing their patches anyway, and it also enables reviewers to make sure the generated files are correct. Said differently, if you want to increase the chances of having your patches tested by precommit CI, make sure to include all the regenerated files, otherwise you might receive failure notifications. Any concerns/objections? Thanks, Christophe [1] https://gcc.gnu.org/wiki/OfficeHours#Meeting:_2024-03-28_.40_1100h_EST5EDT [2] https://inbox.sourceware.org/gdb/cc0a5c86-a041-429a-9890-efd393760...@simark.ca/