Patches submission policy change

Patches submission policy change

[Christophe Lyon]

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-efd3937606e5@simark.ca/

Re: Patches submission policy change

[Jan Beulich]

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

Re: Patches submission policy change

[Jakub Jelinek]

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

[Jan Beulich]

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

[Richard Biener]

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

[Jonathan Wakely]

On Wed, 3 Apr 2024 at 09:46, Jakub Jelinek via Gcc <gcc@gcc.gnu.org> 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

[Jan Beulich]

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

[Joel Sherrill]

[-- Attachment #1: Type: text/plain, Size: 3122 bytes --]

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 <gcc@gcc.gnu.org> 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

[Jason Merrill]

[-- Attachment #1: Type: text/plain, Size: 2771 bytes --]

On Wed, Apr 3, 2024 at 6:23 AM Jan Beulich via Gcc <gcc@gcc.gnu.org> 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

[Christophe Lyon]

On Wed, 3 Apr 2024 at 10:30, Jan Beulich <jbeulich@suse.com> 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

[Christophe Lyon]

On Wed, 3 Apr 2024 at 12:21, Jan Beulich <jbeulich@suse.com> 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

[Christophe Lyon]

On Wed, 3 Apr 2024 at 14:59, Joel Sherrill <joel@rtems.org> 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 <gcc@gcc.gnu.org> 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

[Simon Marchi]

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

[Jan Beulich]

On 03.04.2024 15:11, Christophe Lyon wrote:
> On Wed, 3 Apr 2024 at 10:30, Jan Beulich <jbeulich@suse.com> 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

[Mark Wielaard]

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

[Simon Marchi]

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

[Marc]

Mark Wielaard <mark@klomp.org> 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

[Christophe Lyon]

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

[Christophe Lyon]

On Thu, 4 Apr 2024 at 10:12, Jan Beulich <jbeulich@suse.com> wrote:
>
> On 03.04.2024 15:11, Christophe Lyon wrote:
> > On Wed, 3 Apr 2024 at 10:30, Jan Beulich <jbeulich@suse.com> 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

[Mark Wielaard]

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

[Jonathan Wakely]

[-- Attachment #1: Type: text/plain, Size: 631 bytes --]

On Thu, 4 Apr 2024, 22:36 Mark Wielaard, <mark@klomp.org> 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

[Mark Wielaard]

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, <mark@klomp.org> 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

[Jonathan Wakely]

[-- Attachment #1: Type: text/plain, Size: 1735 bytes --]

On Sun, 7 Apr 2024, 15:02 Mark Wielaard, <mark@klomp.org> 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, <mark@klomp.org> 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

[Mark Wielaard]

Hi Jonathan,

On Sun, Apr 07, 2024 at 03:20:42PM +0100, Jonathan Wakely wrote:
> On Sun, 7 Apr 2024, 15:02 Mark Wielaard, <mark@klomp.org> 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

[Richard Earnshaw (lists)]

On 03/04/2024 14:23, Christophe Lyon via Gcc wrote:
> On Wed, 3 Apr 2024 at 14:59, Joel Sherrill <joel@rtems.org> 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.