From: Christophe Lyon <christophe.lyon@linaro.org>
To: Martin Jambor <mjambor@suse.cz>
Cc: Maxim Kuvyrkov <maxim.kuvyrkov@linaro.org>,
GCC Mailing List <gcc@gcc.gnu.org>,
Siddhesh Poyarekar <siddhesh@redhat.com>,
"Carlos O'Donell" <carlos@redhat.com>
Subject: Re: Checks that autotools generated files were re-generated correctly
Date: Tue, 7 Nov 2023 16:42:25 +0100 [thread overview]
Message-ID: <CAPS5khZkO9p_dOYBQkpvgbWZqANrj-C1EwAsW3Kijz54BPhf6g@mail.gmail.com> (raw)
In-Reply-To: <654a4b60.2e0a0220.7f3b.6e5aSMTPIN_ADDED_BROKEN@mx.google.com>
On Tue, 7 Nov 2023 at 15:36, Martin Jambor <mjambor@suse.cz> wrote:
>
> Hello,
>
> On Tue, Nov 07 2023, Maxim Kuvyrkov wrote:
> n>> On Nov 6, 2023, at 21:19, Christophe Lyon <christophe.lyon@linaro.org> wrote:
> >>
> >> Hi!
> >>
> >> On Mon, 6 Nov 2023 at 18:05, Martin Jambor <mjambor@suse.cz> wrote:
> >>>
> >>> Hello,
> >>>
> >>> I have inherited Martin Liška's buildbot script that checks that all
> >>> sorts of autotools generated files, mainly configure scripts, were
> >>> re-generated correctly when appropriate. While the checks are hopefully
> >>> useful, they report issues surprisingly often and reporting them feels
> >>> especially unproductive.
> >>>
> >>> Could such checks be added to our server side push hooks so that commits
> >>> introducing these breakages would get refused automatically. While the
> >>> check might be a bit expensive, it only needs to be run on files
> >>> touching the generated files and/or the files these are generated from.
> >>>
> >>> Alternatively, Maxim, you seem to have an infrastructure that is capable
> >>> of sending email. Would you consider adding the check to your buildbot
> >>> instance and report issues automatically? The level of totally
> >>
> >> After the discussions we had during Cauldron, I actually thought we
> >> should add such a bot.
> >>
> >> Initially I was thinking about adding this as a "precommit" check, to
> >> make sure the autogenerated files were submitted correctly, but I
> >> realized that the policy is actually not to send autogenerated files
> >> as part of the patch (thus making pre-commit check impracticable in
> >> such cases, unless we autogenerate those files after applying the
> >> patch)
> >>
> >> I understand you mean to run this as a post-commit bot, meaning we
> >> would continue to "accept" broken commits, but now automatically send
> >> a notification, asking for a prompt fix?
>
> My thinking was that ideally bad commits would get refused early, like
> when you get your ChangeLog completely wrong, but if there are drawbacks
> to that approach, a completely automated notification system would be
> great too.
>
Well, making such checks in a precommit-CI means that authors should
include regenerated files in their patch submissions, so it seems this
would imply a policy change (not impossible, but will likely take some
time to get consensus?)
> >>
> >> We can probably implement that, indeed. Is that the general agreement?
> >
> > [CC: Siddhesh, Carlos]
> >
> > Hi Martin,
> >
> > I agree with Christophe, and we can add various source-level checks
> > and wrap them up as a post-commit job. The job will then send out
> > email reports to developers whose patches failed it.
>
> Thanks, automating this would be a huge improvement.
>
> >
> > Where the current script is located? These checks would be useful for
> > all GNU Toolchain projects -- binutils/GDB, GCC, Glibc and, maybe,
> > Newlib -- so it would be useful to put it in a separate "gnutools"
> > repo.
>
> The test consists of running a python script that I'm pasting below in a
> directory with a current master branch and subsequently checking that
> "git diff" does not actually produce any diff (which currently does).
Great, I was thinking about writing something like that :-)
> You need to have locally built autotools utilities of exactly the right
> version. The script (written by Martin Liška) is:
>
> ------------------------------ 8< ------------------------------
> #!/usr/bin/env python3
>
> import os
> import subprocess
> from pathlib import Path
>
> AUTOCONF_BIN = 'autoconf-2.69'
> AUTOMAKE_BIN = 'automake-1.15.1'
> ACLOCAL_BIN = 'aclocal-1.15.1'
> AUTOHEADER_BIN = 'autoheader-2.69'
>
> ENV = f'AUTOCONF={AUTOCONF_BIN} ACLOCAL={ACLOCAL_BIN} AUTOMAKE={AUTOMAKE_BIN}'
>
> config_folders = []
>
> for root, _, files in os.walk('.'):
> for file in files:
> if file == 'configure':
> config_folders.append(Path(root).resolve())
>
> for folder in sorted(config_folders):
> print(folder, flush=True)
> os.chdir(folder)
> configure_lines = open('configure.ac').read().splitlines()
> if any(True for line in configure_lines if line.startswith('AC_CONFIG_HEADERS')):
> subprocess.check_output(f'{ENV} {AUTOHEADER_BIN} -f', shell=True, encoding='utf8')
> # apparently automake is somehow unstable -> skip it for gotools
> if (any(True for line in configure_lines if line.startswith('AM_INIT_AUTOMAKE'))
> and not str(folder).endswith('gotools')):
> subprocess.check_output(f'{ENV} {AUTOMAKE_BIN} -f',
> shell=True, encoding='utf8')
> subprocess.check_output(f'{ENV} {AUTOCONF_BIN} -f', shell=True, encoding='utf8')
>
> ------------------------------ 8< ------------------------------
Nice, thanks for sharing.
>
> > I think Siddhesh and Carlos are looking into creating such a repo on
> > gitlab?
>
> I guess this particular script may be even put into gcc's contrib
> directory. But it can be put anywhere where it makes most sense and
> suits you best.
Indeed, but as Maxim pointed out to me, we would probably want to run
the same kind of checks for binutils and gdb, so it's better if the CI
can clone a very small repo with such scripts rather than having to
clone the whole GCC when checking if a binutils patch is well-formed.
I'm happy with Carlos' proposal of using a new gitlab repo with such
utilities/checkers, which we can then use from various CI
implementations.
Thanks,
Christophe
>
> Thanks,
>
> Martin
next prev parent reply other threads:[~2023-11-07 15:42 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <65491cda.c80a0220.5fa79.7688SMTPIN_ADDED_BROKEN@mx.google.com>
2023-11-06 17:19 ` Christophe Lyon
2023-11-07 7:38 ` Maxim Kuvyrkov
2023-11-07 13:50 ` Carlos O'Donell
2023-11-07 14:36 ` Martin Jambor
[not found] ` <654a4b60.2e0a0220.7f3b.6e5aSMTPIN_ADDED_BROKEN@mx.google.com>
2023-11-07 15:42 ` Christophe Lyon [this message]
2023-11-06 17:04 Martin Jambor
2023-11-07 22:13 ` Frank Ch. Eigler
2023-11-08 23:30 ` Mark Wielaard
2023-11-15 19:48 ` Mark Wielaard
2023-11-16 18:37 ` Martin Jambor
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAPS5khZkO9p_dOYBQkpvgbWZqANrj-C1EwAsW3Kijz54BPhf6g@mail.gmail.com \
--to=christophe.lyon@linaro.org \
--cc=carlos@redhat.com \
--cc=gcc@gcc.gnu.org \
--cc=maxim.kuvyrkov@linaro.org \
--cc=mjambor@suse.cz \
--cc=siddhesh@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).