Re: Patches submission policy change

[Simon Marchi <simark@simark.ca>]

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