[-- Attachment #1: Type: text/plain, Size: 595 bytes --] Hi, Mark Wielaard <mark@klomp.org> writes: > Hi, > > On Tue, Mar 12, 2024 at 12:00:30PM -0400, Simon Marchi wrote: >> On 3/12/24 07:39, Arsen Arsenović wrote: >> > Though, if I am correct, we could also just use subprocess.runs env= >> > parameter to put up a modified env. >> >> Yeah that makes more sense. See patch below that implements this. > > Looks good. > > Applied, > > Mark Thanks, both. Sorry about the wide-reply fails - my MUA keybinds changed subtly recently and I didn't change them back at that point yet. Have a lovely day. -- Arsen Arsenović [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 381 bytes --]
Hi YunQiang Su, On Fri, Mar 15, 2024 at 03:33:28PM +0800, YunQiang Su wrote: > Great work. The CI works well now: it blames me ;) > https://builder.sourceware.org/buildbot/#/builders/269/builds/3846 > > When I add '-mstrict-align' option to MIPS, > the riscv.opt.urls, sysv4.opt.urls, xtensa.opt.urls are changed also. > (why they are effected? They are effected because they also have a '-mstrict-align' option and each option with the same name gets an unique number. > So what's the best practice for this cases? > Should I push a new commit? Or in fact a single commit is preferred? I don't know if there is a rule for this. But I hope this falls under the obvious rule. What I would do is simply take that diff the CI produced. https://builder.sourceware.org/buildbot/api/v2/logs/7798308/raw Apply it and commit that with: Regenerate opt.urls Fixes: acc38ff59976 ("MIPS: Add -m(no-)strict-align option") gcc/ChangeLog: * config/riscv/riscv.opt.urls: Regenerated. * config/rs6000/sysv4.opt.urls: Likewise. * config/xtensa/xtensa.opt.urls: Likewise. Cheers, Mark
Great work. The CI works well now: it blames me ;) https://builder.sourceware.org/buildbot/#/builders/269/builds/3846 When I add '-mstrict-align' option to MIPS, the riscv.opt.urls, sysv4.opt.urls, xtensa.opt.urls are changed also. (why they are effected? So what's the best practice for this cases? Should I push a new commit? Or in fact a single commit is preferred? -- YunQiang Su
For building valgrind g++ -m32 tests. --- builder/containers/Containerfile-debian-stable | 1 + builder/containers/Containerfile-debian-testing | 1 + 2 files changed, 2 insertions(+) diff --git a/builder/containers/Containerfile-debian-stable b/builder/containers/Containerfile-debian-stable index 84ea0b182e35..61050106416e 100644 --- a/builder/containers/Containerfile-debian-stable +++ b/builder/containers/Containerfile-debian-stable @@ -4,6 +4,7 @@ RUN apt-get update && \ apt-get upgrade -y && \ apt-get install -y \ build-essential gcc-multilib g++ libubsan1 libasan6 libtool ccache valgrind \ + g++-multilib \ libisl-dev libmpc-dev libgmp-dev libmpfr-dev libgmp-dev \ patch util-linux diffutils iproute2 libarchive-tools cpio rpm2cpio procps \ coreutils make git autotools-dev autoconf dejagnu automake gettext bison flex \ diff --git a/builder/containers/Containerfile-debian-testing b/builder/containers/Containerfile-debian-testing index bc410f14f5be..0a79771d849a 100644 --- a/builder/containers/Containerfile-debian-testing +++ b/builder/containers/Containerfile-debian-testing @@ -4,6 +4,7 @@ RUN apt-get update && \ apt-get upgrade -y && \ apt-get install -y \ build-essential gcc-multilib g++ libubsan1 libasan6 libtool ccache valgrind \ + g++-multilib \ libisl-dev libmpc-dev libgmp-dev libmpfr-dev libgmp-dev \ patch util-linux diffutils iproute2 libarchive-tools cpio rpm2cpio procps \ coreutils make git autotools-dev autoconf dejagnu automake gettext bison flex \ -- 2.39.3
Hi Simon,
On Tue, Mar 12, 2024 at 01:53:37PM -0400, Simon Marchi wrote:
> The builder currently mis-generates the config files for the gdb and
> gnulib directories. It passes `-I ../config` to aclocal, causing the
> include path order to be different to what it would be without that -I
> argument, causing the generated aclocal.m4 to be different (same entries
> but different order).
>
> For the gdb, gdbserver, gdbsupport and gnulib directories, the config
> files are usually re-generated without any -I argument. In fact, for
> gnulib, it is baked in gnulib/update-gnulib.sh that aclocal should be
> called without -I arguments:
>
> aclocal &&
> autoconf &&
> autoheader &&
> automake
>
> Moreover, they can be re-generated using autoreconf, removing the need
> to guess which speicific tools need to be invoked. autoreconf takes a
> bit more time to run than the individual tools, but it's worth it for
> the simplicity.
>
> So, add a list of directories for which it is known that they can be
> re-generated using a simple autoreconf without special flags. For these
> directories, call `autoreconf -f`.
Very nice, now running this regenerates the binutils-gdb files exactly
as they are checked in. Also tested on the gcc tree, which also works
fine.
Applied (with small adjustments for the previous two patches).
Thanks,
Mark
Hi,
On Tue, Mar 12, 2024 at 12:00:30PM -0400, Simon Marchi wrote:
> On 3/12/24 07:39, Arsen Arsenović wrote:
> > Though, if I am correct, we could also just use subprocess.runs env=
> > parameter to put up a modified env.
>
> Yeah that makes more sense. See patch below that implements this.
Looks good.
Applied,
Mark
Hi,
On Tue, Mar 12, 2024 at 11:40:45AM -0400, Simon Marchi wrote:
> > The default behaviour of Python Popen (and hence subprocess.run) is not
> > to touch the file descriptors on standard input, output and error.
> > Leaving this as 'None' (the default) is hence better, IMO.
>
> Oh ok, I didn't understand it properly when reading the doc.
>
> Here's a patch that removes them.
Looks good.
Applied,
Mark
The builder currently mis-generates the config files for the gdb and gnulib directories. It passes `-I ../config` to aclocal, causing the include path order to be different to what it would be without that -I argument, causing the generated aclocal.m4 to be different (same entries but different order). For the gdb, gdbserver, gdbsupport and gnulib directories, the config files are usually re-generated without any -I argument. In fact, for gnulib, it is baked in gnulib/update-gnulib.sh that aclocal should be called without -I arguments: aclocal && autoconf && autoheader && automake Moreover, they can be re-generated using autoreconf, removing the need to guess which speicific tools need to be invoked. autoreconf takes a bit more time to run than the individual tools, but it's worth it for the simplicity. So, add a list of directories for which it is known that they can be re-generated using a simple autoreconf without special flags. For these directories, call `autoreconf -f`. --- builder/containers/autoregen.py | 63 ++++++++++++++++++++++----------- 1 file changed, 43 insertions(+), 20 deletions(-) diff --git a/builder/containers/autoregen.py b/builder/containers/autoregen.py index ac16f54f6caa..138b9af2590f 100755 --- a/builder/containers/autoregen.py +++ b/builder/containers/autoregen.py @@ -13,12 +13,14 @@ AUTOCONF_NAMES = ["autoconf-vanilla-2.69", "autoconf-2.69", "autoconf"] AUTOMAKE_NAMES = ["automake-vanilla-1.15", "automake-1.15.1", "automake"] ACLOCAL_NAMES = ["aclocal-vanilla-1.15", "aclocal-1.15.1", "aclocal"] AUTOHEADER_NAMES = ["autoheader-vanilla-2.69", "autoheader-2.69", "autoheader"] +AUTORECONF_NAMES = ["autoreconf-vanilla-2.69", "autoreconf-2.69", "autoreconf"] # Pick the first for each list that exists on this system. AUTOCONF_BIN = next(name for name in AUTOCONF_NAMES if shutil.which(name)) AUTOMAKE_BIN = next(name for name in AUTOMAKE_NAMES if shutil.which(name)) ACLOCAL_BIN = next(name for name in ACLOCAL_NAMES if shutil.which(name)) AUTOHEADER_BIN = next(name for name in AUTOHEADER_NAMES if shutil.which(name)) +AUTORECONF_BIN = next(name for name in AUTORECONF_NAMES if shutil.which(name)) # autoconf-wrapper and automake-wrapper from Gentoo look at this environment variable. # It's harmless to set it on other systems though. @@ -41,6 +43,15 @@ SKIP_DIRS = [ "minizip", ] +# these directories are known to can be re-generatable with a simple autoreconf +# without special -I flags +AUTORECONF_DIRS = [ + "gdb", + "gdbserver", + "gdbsupport", + "gnulib", +] + # Run the shell command CMD. # @@ -57,28 +68,11 @@ def run_shell(cmd: str): res.check_returncode() -run_shell(f"{AUTOCONF_BIN} --version") -run_shell(f"{AUTOMAKE_BIN} --version") -run_shell(f"{ACLOCAL_BIN} --version") -run_shell(f"{AUTOHEADER_BIN} --version") - -print(f"Environment: {ENV}") +def regenerate_with_autoreconf(): + run_shell(f"{AUTORECONF_BIN} -f") -config_folders: list[Path] = [] - -for root, _, files in os.walk("."): - for file in files: - if file == "configure.ac": - config_folders.append(Path(root).resolve()) - -for folder in sorted(config_folders): - if folder.stem in SKIP_DIRS: - print(f"Skipping directory {folder}", flush=True) - continue - - print(f"Entering directory {folder}", flush=True) - os.chdir(folder) +def regenerate_manually(): configure_lines = open("configure.ac").read().splitlines() if any(True for line in configure_lines if line.startswith("AC_CONFIG_MACRO_DIRS")): # aclocal does not support the -f short option for force @@ -107,3 +101,32 @@ for folder in sorted(config_folders): run_shell(f"{AUTOMAKE_BIN} -f") run_shell(f"{AUTOCONF_BIN} -f") + + +run_shell(f"{AUTOCONF_BIN} --version") +run_shell(f"{AUTOMAKE_BIN} --version") +run_shell(f"{ACLOCAL_BIN} --version") +run_shell(f"{AUTOHEADER_BIN} --version") + +print(f"Environment: {ENV}") + +config_folders: list[Path] = [] +repo_root = Path.cwd() + +for root, _, files in os.walk("."): + for file in files: + if file == "configure.ac": + config_folders.append(Path(root).resolve()) + +for folder in sorted(config_folders): + if folder.stem in SKIP_DIRS: + print(f"Skipping directory {folder}", flush=True) + continue + + print(f"Entering directory {folder}", flush=True) + os.chdir(folder) + + if str(folder.relative_to(repo_root)) in AUTORECONF_DIRS: + regenerate_with_autoreconf() + else: + regenerate_manually() base-commit: 3e013842366735c373ef19ec6dfc4a33d6f9c473 -- 2.44.0
On 3/12/24 07:39, Arsen Arsenović wrote: > Hi, > > Simon Marchi <simon.marchi@efficios.com> writes: > >> Not a big deal, and this is certainly opinionated, but pyright tells me: >> >> /home/smarchi/src/builder/builder/containers/autoregen.py >> /home/smarchi/src/builder/builder/containers/autoregen.py:24:1 - error: "ENV" is constant (because it is uppercase) and cannot be redefined (reportConstantRedefinition) >> >> Switch to a syntax that avoids redefinition of the variable. >> --- >> builder/containers/autoregen.py | 15 +++++++++------ >> 1 file changed, 9 insertions(+), 6 deletions(-) >> >> diff --git a/builder/containers/autoregen.py b/builder/containers/autoregen.py >> index 861a2ce79ef5..feee5878cac7 100755 >> --- a/builder/containers/autoregen.py >> +++ b/builder/containers/autoregen.py >> @@ -21,12 +21,15 @@ AUTOHEADER_BIN = next(name for name in autoheader_names if shutil.which(name)) >> >> # autoconf-wrapper and automake-wrapper from Gentoo look at this environment variable. >> # It's harmless to set it on other systems though. >> -ENV = f'WANT_AUTOCONF={AUTOCONF_BIN.split("-", 1)[1]} ' >> -ENV += f'WANT_AUTOMAKE={AUTOMAKE_BIN.split("-", 1)[1]} ' >> - >> -ENV += f" AUTOCONF={AUTOCONF_BIN} " >> -ENV += f" ACLOCAL={ACLOCAL_BIN} " >> -ENV += f" AUTOMAKE={AUTOMAKE_BIN}" >> +ENV = " ".join( >> + ( >> + f'WANT_AUTOCONF={AUTOCONF_BIN.split("-", 1)[1]}', >> + f'WANT_AUTOMAKE={AUTOMAKE_BIN.split("-", 1)[1]}', >> + f"AUTOCONF={AUTOCONF_BIN}", >> + f"ACLOCAL={ACLOCAL_BIN}", >> + f"AUTOMAKE={AUTOMAKE_BIN}", >> + ) >> +) > > This is somewhat unimportant, but the following is also valid Python: > ENV = ( > f'WANT_AUTOCONF={AUTOCONF_BIN.split("-", 1)[1]} ' > f'WANT_AUTOMAKE={AUTOMAKE_BIN.split("-", 1)[1]} ' > f"AUTOCONF={AUTOCONF_BIN} " > f"ACLOCAL={ACLOCAL_BIN} " > f"AUTOMAKE={AUTOMAKE_BIN}" > ) Personal preference, I prefer to use " ".join instead of embedding the space in all but the last element. > More importantly, we should maybe use shlex or such due to potential > spaces in here? (not that whatever is invoked by this script won't > break with those..) For now, the values the _BIN variables can have are known not to contain spaces, they come from the _NAME arrays. > Though, if I am correct, we could also just use subprocess.runs env= > parameter to put up a modified env. Yeah that makes more sense. See patch below that implements this. From bf7ce70b29f662c48281ce031f10adcb16e53898 Mon Sep 17 00:00:00 2001 From: Simon Marchi <simon.marchi@polymtl.ca> Date: Tue, 12 Mar 2024 11:49:35 -0400 Subject: [PATCH] autoregen.py: pass environment through subprocess.run's env parameter Arsen pointed out that a better way to pass the environment to the subprocess (one that is not vulnerable to space splitting, for instance) is to use subprocess.run's env parameter. - Define a new EXTRA_ENV dict with the environment variables we want to add to the current process' environment, when running subprocesses - Copy the current process' environment into ENV - Augment ENV with EXTRA_ENV - Use EXTRA_ENV when logging the environment variables --- builder/containers/autoregen.py | 23 ++++++++++++----------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/builder/containers/autoregen.py b/builder/containers/autoregen.py index ac16f54f6caa..8afc168ea811 100755 --- a/builder/containers/autoregen.py +++ b/builder/containers/autoregen.py @@ -22,15 +22,15 @@ AUTOHEADER_BIN = next(name for name in AUTOHEADER_NAMES if shutil.which(name)) # autoconf-wrapper and automake-wrapper from Gentoo look at this environment variable. # It's harmless to set it on other systems though. -ENV = " ".join( - ( - f'WANT_AUTOCONF={AUTOCONF_BIN.split("-", 1)[1] if "-" in AUTOCONF_BIN else ""}', - f'WANT_AUTOMAKE={AUTOMAKE_BIN.split("-", 1)[1] if "-" in AUTOMAKE_BIN else ""}', - f"AUTOCONF={AUTOCONF_BIN}", - f"ACLOCAL={ACLOCAL_BIN}", - f"AUTOMAKE={AUTOMAKE_BIN}", - ) -) +EXTRA_ENV = { + "WANT_AUTOCONF": AUTOCONF_BIN.split("-", 1)[1] if "-" in AUTOCONF_BIN else "", + "WANT_AUTOMAKE": AUTOMAKE_BIN.split("-", 1)[1] if "-" in AUTOMAKE_BIN else "", + "AUTOCONF": AUTOCONF_BIN, + "ACLOCAL": ACLOCAL_BIN, + "AUTOMAKE": AUTOMAKE_BIN, +} +ENV = os.environ.copy() +ENV.update(EXTRA_ENV) # Directories we should skip entirely because they're vendored or have different @@ -48,11 +48,12 @@ SKIP_DIRS = [ def run_shell(cmd: str): print(f"+ {cmd}") res = subprocess.run( - f"{ENV} {cmd}", + f"{cmd}", shell=True, encoding="utf8", stdout=sys.stdout, stderr=sys.stderr, + env=ENV, ) res.check_returncode() @@ -62,7 +63,7 @@ run_shell(f"{AUTOMAKE_BIN} --version") run_shell(f"{ACLOCAL_BIN} --version") run_shell(f"{AUTOHEADER_BIN} --version") -print(f"Environment: {ENV}") +print(f"Extra environment: {EXTRA_ENV}") config_folders: list[Path] = [] base-commit: 3e013842366735c373ef19ec6dfc4a33d6f9c473 -- 2.44.0
It seems like Arsen didn't reply-all. Adding back the buildbot mailing list. On 3/12/24 07:43, Arsen Arsenović wrote: > Simon Marchi <simon.marchi@efficios.com> writes: > >> Use `subprocess.run` with `stdout=sys.stdout` and `stderr=sys.stderr`, >> such that subprocesses print to the job's stdout and stderr directly. >> >> This is not expected to cause any change, as the current commands don't >> output anything. But if they do, I think it's useful to have that >> output in the buildbot's logs. >> --- >> builder/containers/autoregen.py | 10 +++++++++- >> 1 file changed, 9 insertions(+), 1 deletion(-) >> >> diff --git a/builder/containers/autoregen.py b/builder/containers/autoregen.py >> index ff6939b66105..437f0dabe2de 100755 >> --- a/builder/containers/autoregen.py >> +++ b/builder/containers/autoregen.py >> @@ -4,6 +4,7 @@ import os >> import shutil >> import subprocess >> from pathlib import Path >> +import sys >> >> >> # On Gentoo, vanilla unpatched autotools are packaged separately. >> @@ -47,7 +48,14 @@ SKIP_DIRS = [ >> def run_shell(cmd: str): >> cmd = f"{ENV} {cmd}" >> print(f"+ {cmd}") >> - subprocess.check_output(f"{ENV} {cmd}", shell=True, encoding="utf8") >> + res = subprocess.run( >> + f"{ENV} {cmd}", >> + shell=True, >> + encoding="utf8", >> + stdout=sys.stdout, >> + stderr=sys.stderr, >> + ) >> + res.check_returncode() > > The default behaviour of Python Popen (and hence subprocess.run) is not > to touch the file descriptors on standard input, output and error. > Leaving this as 'None' (the default) is hence better, IMO. Oh ok, I didn't understand it properly when reading the doc. Here's a patch that removes them. From deaad76616d9210d5f53532f9159ce8db7035412 Mon Sep 17 00:00:00 2001 From: Simon Marchi <simon.marchi@efficios.com> Date: Tue, 12 Mar 2024 11:01:14 -0400 Subject: [PATCH] autoregen.py: do not pass sys.stdout/sys.stderr to subprocess.run Arsen pointed out that this is unnecessary. If we don't pass an stdout/stderr, the subprocess will use the same file descriptors as the parent, which is what we want. --- builder/containers/autoregen.py | 3 --- 1 file changed, 3 deletions(-) diff --git a/builder/containers/autoregen.py b/builder/containers/autoregen.py index ac16f54f6caa..20481b142d8a 100755 --- a/builder/containers/autoregen.py +++ b/builder/containers/autoregen.py @@ -4,7 +4,6 @@ import os import shutil import subprocess from pathlib import Path -import sys # On Gentoo, vanilla unpatched autotools are packaged separately. @@ -51,8 +50,6 @@ def run_shell(cmd: str): f"{ENV} {cmd}", shell=True, encoding="utf8", - stdout=sys.stdout, - stderr=sys.stderr, ) res.check_returncode() base-commit: 3e013842366735c373ef19ec6dfc4a33d6f9c473 -- 2.44.0
Hi Simon,
On Mon, Mar 11, 2024 at 09:53:40PM -0400, Simon Marchi wrote:
> On 2024-03-11 17:52, Mark Wielaard wrote:
> > I think it is a little too verbose now. Printing the ENV seems a bit
> > busy. But for being clear what exactly is happening I guess it is nice.
>
> Well, my initial implementation didn't show the env, but I made it show
> it to be accurate. But we could print the env only once at the
> beginning.
>
> Here's a patch that does it.
Nice. Lets do that.
Applied and pushed.
Thanks,
Mark
On 2024-03-11 17:52, Mark Wielaard wrote: > I think it is a little too verbose now. Printing the ENV seems a bit > busy. But for being clear what exactly is happening I guess it is nice. Well, my initial implementation didn't show the env, but I made it show it to be accurate. But we could print the env only once at the beginning. Here's a patch that does it. From 0ea23b4d42ae9c026f9d0dc5030a1bf38b4ccfcc Mon Sep 17 00:00:00 2001 From: Simon Marchi <simon.marchi@polymtl.ca> Date: Mon, 11 Mar 2024 21:47:28 -0400 Subject: [PATCH] auroregen.py: print environment only once at the beginning Reduce the duplication in the output a bit by printing the environment variables passed to all commands only once at the beginning. Note that the previous code ended up putting {ENV} twice in the command, which is harmless but unnecessary. --- builder/containers/autoregen.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/builder/containers/autoregen.py b/builder/containers/autoregen.py index ba77320c7248..ac16f54f6caa 100755 --- a/builder/containers/autoregen.py +++ b/builder/containers/autoregen.py @@ -46,7 +46,6 @@ SKIP_DIRS = [ # # Print the command on stdout prior to running it. def run_shell(cmd: str): - cmd = f"{ENV} {cmd}" print(f"+ {cmd}") res = subprocess.run( f"{ENV} {cmd}", @@ -63,6 +62,8 @@ run_shell(f"{AUTOMAKE_BIN} --version") run_shell(f"{ACLOCAL_BIN} --version") run_shell(f"{AUTOHEADER_BIN} --version") +print(f"Environment: {ENV}") + config_folders: list[Path] = [] for root, _, files in os.walk("."): base-commit: de502b23299f15eeca710034218e35819dec5e38 -- 2.44.0
Hi,
On Mon, Mar 11, 2024 at 09:04:14PM +0000, Sam James wrote:
> Simon Marchi <simon.marchi@efficios.com> writes:
>
> > The first half of the patches are cleanups to make autoregen.py clean in
> > the eyes of some tools I usually use for Python code.
> >
> > The rest of the patches make the script log a bit more. My main wish is
> > for the script to print the shell commands it executes.
>
> The series LGTM and tested it.
Same here.
gcc works as expected. binutils-gdb does now show that ordering
difference in gdb/aclocal.m4.
I think it is a little too verbose now. Printing the ENV seems a bit
busy. But for being clear what exactly is happening I guess it is nice.
Pushed,
Mark
[-- Attachment #1: Type: text/plain, Size: 986 bytes --] Simon Marchi <simon.marchi@efficios.com> writes: > The first half of the patches are cleanups to make autoregen.py clean in > the eyes of some tools I usually use for Python code. > > The rest of the patches make the script log a bit more. My main wish is > for the script to print the shell commands it executes. The series LGTM and tested it. r-b > > Simon Marchi (8): > autoregen.py: re-format with black 24.2.0 > autoregen.py: fix a pyright `reportConstantRedefinition` warning > autoregen.py: add type annotation for config_folders > autoregen.py: capitalize some constant names > autoregen.py: add non-suffixed version of the tools > autoregen.py: print executed commands on stdout > autoregen.py: let subprocesses print to stdout/stderr > autoregen.py: be more verbose > > builder/containers/autoregen.py | 110 ++++++++++++++++++++------------ > 1 file changed, 69 insertions(+), 41 deletions(-) > > > base-commit: 753c1b3e9b56949af4600b9a43424ce5004361c2 [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 377 bytes --]
- Print the version of all the tools used. - Print more clearly when entering or skipping a directory. --- builder/containers/autoregen.py | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/builder/containers/autoregen.py b/builder/containers/autoregen.py index 437f0dabe2de..ba77320c7248 100755 --- a/builder/containers/autoregen.py +++ b/builder/containers/autoregen.py @@ -58,6 +58,11 @@ def run_shell(cmd: str): res.check_returncode() +run_shell(f"{AUTOCONF_BIN} --version") +run_shell(f"{AUTOMAKE_BIN} --version") +run_shell(f"{ACLOCAL_BIN} --version") +run_shell(f"{AUTOHEADER_BIN} --version") + config_folders: list[Path] = [] for root, _, files in os.walk("."): @@ -66,11 +71,11 @@ for root, _, files in os.walk("."): config_folders.append(Path(root).resolve()) for folder in sorted(config_folders): - print(folder, flush=True) - if folder.stem in SKIP_DIRS: + print(f"Skipping directory {folder}", flush=True) continue + print(f"Entering directory {folder}", flush=True) os.chdir(folder) configure_lines = open("configure.ac").read().splitlines() -- 2.44.0
Use `subprocess.run` with `stdout=sys.stdout` and `stderr=sys.stderr`, such that subprocesses print to the job's stdout and stderr directly. This is not expected to cause any change, as the current commands don't output anything. But if they do, I think it's useful to have that output in the buildbot's logs. --- builder/containers/autoregen.py | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/builder/containers/autoregen.py b/builder/containers/autoregen.py index ff6939b66105..437f0dabe2de 100755 --- a/builder/containers/autoregen.py +++ b/builder/containers/autoregen.py @@ -4,6 +4,7 @@ import os import shutil import subprocess from pathlib import Path +import sys # On Gentoo, vanilla unpatched autotools are packaged separately. @@ -47,7 +48,14 @@ SKIP_DIRS = [ def run_shell(cmd: str): cmd = f"{ENV} {cmd}" print(f"+ {cmd}") - subprocess.check_output(f"{ENV} {cmd}", shell=True, encoding="utf8") + res = subprocess.run( + f"{ENV} {cmd}", + shell=True, + encoding="utf8", + stdout=sys.stdout, + stderr=sys.stderr, + ) + res.check_returncode() config_folders: list[Path] = [] -- 2.44.0
My setup is that I have the appropriate versions of the autotools in /opt/autostuff/bin. They are not suffixed with their versions. To allow me to run the script locally, add non-suffixed names at the end of the possible names lists. --- builder/containers/autoregen.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/builder/containers/autoregen.py b/builder/containers/autoregen.py index e63436111246..6560077a406b 100755 --- a/builder/containers/autoregen.py +++ b/builder/containers/autoregen.py @@ -8,10 +8,10 @@ from pathlib import Path # On Gentoo, vanilla unpatched autotools are packaged separately. # We place the vanilla names first as we want to prefer those if both exist. -AUTOCONF_NAMES = ["autoconf-vanilla-2.69", "autoconf-2.69"] -AUTOMAKE_NAMES = ["automake-vanilla-1.15", "automake-1.15.1"] -ACLOCAL_NAMES = ["aclocal-vanilla-1.15", "aclocal-1.15.1"] -AUTOHEADER_NAMES = ["autoheader-vanilla-2.69", "autoheader-2.69"] +AUTOCONF_NAMES = ["autoconf-vanilla-2.69", "autoconf-2.69", "autoconf"] +AUTOMAKE_NAMES = ["automake-vanilla-1.15", "automake-1.15.1", "automake"] +ACLOCAL_NAMES = ["aclocal-vanilla-1.15", "aclocal-1.15.1", "aclocal"] +AUTOHEADER_NAMES = ["autoheader-vanilla-2.69", "autoheader-2.69", "autoheader"] # Pick the first for each list that exists on this system. AUTOCONF_BIN = next(name for name in AUTOCONF_NAMES if shutil.which(name)) @@ -23,8 +23,8 @@ AUTOHEADER_BIN = next(name for name in AUTOHEADER_NAMES if shutil.which(name)) # It's harmless to set it on other systems though. ENV = " ".join( ( - f'WANT_AUTOCONF={AUTOCONF_BIN.split("-", 1)[1]}', - f'WANT_AUTOMAKE={AUTOMAKE_BIN.split("-", 1)[1]}', + f'WANT_AUTOCONF={AUTOCONF_BIN.split("-", 1)[1] if "-" in AUTOCONF_BIN else ""}', + f'WANT_AUTOMAKE={AUTOMAKE_BIN.split("-", 1)[1] if "-" in AUTOMAKE_BIN else ""}', f"AUTOCONF={AUTOCONF_BIN}", f"ACLOCAL={ACLOCAL_BIN}", f"AUTOMAKE={AUTOMAKE_BIN}", -- 2.44.0
This allows seeing what commands the buildbot executes exactly. It will help whenever there is a difference in the generated files between what a developer gets locally and what the buildbot gets. --- builder/containers/autoregen.py | 23 +++++++++++++---------- 1 file changed, 13 insertions(+), 10 deletions(-) diff --git a/builder/containers/autoregen.py b/builder/containers/autoregen.py index 6560077a406b..ff6939b66105 100755 --- a/builder/containers/autoregen.py +++ b/builder/containers/autoregen.py @@ -41,6 +41,15 @@ SKIP_DIRS = [ ] +# Run the shell command CMD. +# +# Print the command on stdout prior to running it. +def run_shell(cmd: str): + cmd = f"{ENV} {cmd}" + print(f"+ {cmd}") + subprocess.check_output(f"{ENV} {cmd}", shell=True, encoding="utf8") + + config_folders: list[Path] = [] for root, _, files in os.walk("."): @@ -70,23 +79,17 @@ for folder in sorted(config_folders): include_arg = "-I../.." include_arg2 = "-I../../config" - subprocess.check_output( - f"{ENV} {ACLOCAL_BIN} --force {include_arg} {include_arg2}", - shell=True, - encoding="utf8", - ) + run_shell(f"{ACLOCAL_BIN} --force {include_arg} {include_arg2}") if (folder / "config.in").is_file() or 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" - ) + run_shell(f"{AUTOHEADER_BIN} -f") # 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") + run_shell(f"{AUTOMAKE_BIN} -f") - subprocess.check_output(f"{ENV} {AUTOCONF_BIN} -f", shell=True, encoding="utf8") + run_shell(f"{AUTOCONF_BIN} -f") -- 2.44.0
This is subjective, but I am completely sold on using black for Python projects, so I propose using it here. It lets us completely stop thinking about formatting whatsoever. --- builder/containers/autoregen.py | 72 ++++++++++++++++++--------------- 1 file changed, 40 insertions(+), 32 deletions(-) diff --git a/builder/containers/autoregen.py b/builder/containers/autoregen.py index 3d307ad24132..861a2ce79ef5 100755 --- a/builder/containers/autoregen.py +++ b/builder/containers/autoregen.py @@ -5,12 +5,13 @@ import shutil import subprocess from pathlib import Path + # On Gentoo, vanilla unpatched autotools are packaged separately. # We place the vanilla names first as we want to prefer those if both exist. -autoconf_names = ['autoconf-vanilla-2.69', 'autoconf-2.69'] -automake_names = ['automake-vanilla-1.15', 'automake-1.15.1'] -aclocal_names = ['aclocal-vanilla-1.15', 'aclocal-1.15.1'] -autoheader_names = ['autoheader-vanilla-2.69', 'autoheader-2.69'] +autoconf_names = ["autoconf-vanilla-2.69", "autoconf-2.69"] +automake_names = ["automake-vanilla-1.15", "automake-1.15.1"] +aclocal_names = ["aclocal-vanilla-1.15", "aclocal-1.15.1"] +autoheader_names = ["autoheader-vanilla-2.69", "autoheader-2.69"] # Pick the first for each list that exists on this system. AUTOCONF_BIN = next(name for name in autoconf_names if shutil.which(name)) @@ -23,23 +24,24 @@ AUTOHEADER_BIN = next(name for name in autoheader_names if shutil.which(name)) ENV = f'WANT_AUTOCONF={AUTOCONF_BIN.split("-", 1)[1]} ' ENV += f'WANT_AUTOMAKE={AUTOMAKE_BIN.split("-", 1)[1]} ' -ENV += f' AUTOCONF={AUTOCONF_BIN} ' -ENV += f' ACLOCAL={ACLOCAL_BIN} ' -ENV += f' AUTOMAKE={AUTOMAKE_BIN}' +ENV += f" AUTOCONF={AUTOCONF_BIN} " +ENV += f" ACLOCAL={ACLOCAL_BIN} " +ENV += f" AUTOMAKE={AUTOMAKE_BIN}" + # Directories we should skip entirely because they're vendored or have different # autotools versions. skip_dirs = [ # readline and minizip are maintained with different autotools versions - 'readline', - 'minizip' + "readline", + "minizip", ] config_folders = [] -for root, _, files in os.walk('.'): +for root, _, files in os.walk("."): for file in files: - if file == 'configure.ac': + if file == "configure.ac": config_folders.append(Path(root).resolve()) for folder in sorted(config_folders): @@ -50,31 +52,37 @@ for folder in sorted(config_folders): os.chdir(folder) - configure_lines = open('configure.ac').read().splitlines() - if any(True for line in configure_lines if line.startswith('AC_CONFIG_MACRO_DIRS')): + configure_lines = open("configure.ac").read().splitlines() + if any(True for line in configure_lines if line.startswith("AC_CONFIG_MACRO_DIRS")): # aclocal does not support the -f short option for force - include_arg = '' - include_arg2 = '' - if (folder / '..' / 'config').is_dir(): - include_arg = '-I../config' + include_arg = "" + include_arg2 = "" + if (folder / ".." / "config").is_dir(): + include_arg = "-I../config" # this is really a hack just for binutils-gdb/gprofng/libcollector # make sure that the order of includes is done as --enable-maintainer-mode - if (folder / '..' / '..' / 'config').is_dir(): - include_arg = '-I../..' - include_arg2 = '-I../../config' - - subprocess.check_output(f'{ENV} {ACLOCAL_BIN} --force {include_arg} {include_arg2}', shell=True, encoding='utf8') - - if ((folder / 'config.in').is_file() - or 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') + if (folder / ".." / ".." / "config").is_dir(): + include_arg = "-I../.." + include_arg2 = "-I../../config" + + subprocess.check_output( + f"{ENV} {ACLOCAL_BIN} --force {include_arg} {include_arg2}", + shell=True, + encoding="utf8", + ) + + if (folder / "config.in").is_file() or 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') + 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") -- 2.44.0
Not a big deal, and this is certainly opinionated, but pyright tells me: /home/smarchi/src/builder/builder/containers/autoregen.py /home/smarchi/src/builder/builder/containers/autoregen.py:24:1 - error: "ENV" is constant (because it is uppercase) and cannot be redefined (reportConstantRedefinition) Switch to a syntax that avoids redefinition of the variable. --- builder/containers/autoregen.py | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/builder/containers/autoregen.py b/builder/containers/autoregen.py index 861a2ce79ef5..feee5878cac7 100755 --- a/builder/containers/autoregen.py +++ b/builder/containers/autoregen.py @@ -21,12 +21,15 @@ AUTOHEADER_BIN = next(name for name in autoheader_names if shutil.which(name)) # autoconf-wrapper and automake-wrapper from Gentoo look at this environment variable. # It's harmless to set it on other systems though. -ENV = f'WANT_AUTOCONF={AUTOCONF_BIN.split("-", 1)[1]} ' -ENV += f'WANT_AUTOMAKE={AUTOMAKE_BIN.split("-", 1)[1]} ' - -ENV += f" AUTOCONF={AUTOCONF_BIN} " -ENV += f" ACLOCAL={ACLOCAL_BIN} " -ENV += f" AUTOMAKE={AUTOMAKE_BIN}" +ENV = " ".join( + ( + f'WANT_AUTOCONF={AUTOCONF_BIN.split("-", 1)[1]}', + f'WANT_AUTOMAKE={AUTOMAKE_BIN.split("-", 1)[1]}', + f"AUTOCONF={AUTOCONF_BIN}", + f"ACLOCAL={ACLOCAL_BIN}", + f"AUTOMAKE={AUTOMAKE_BIN}", + ) +) # Directories we should skip entirely because they're vendored or have different -- 2.44.0
Following the "convention" that constant names are capitalized, capitalize the other constants names set at the beginning of the script. --- builder/containers/autoregen.py | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/builder/containers/autoregen.py b/builder/containers/autoregen.py index 26006ff18e0a..e63436111246 100755 --- a/builder/containers/autoregen.py +++ b/builder/containers/autoregen.py @@ -8,16 +8,16 @@ from pathlib import Path # On Gentoo, vanilla unpatched autotools are packaged separately. # We place the vanilla names first as we want to prefer those if both exist. -autoconf_names = ["autoconf-vanilla-2.69", "autoconf-2.69"] -automake_names = ["automake-vanilla-1.15", "automake-1.15.1"] -aclocal_names = ["aclocal-vanilla-1.15", "aclocal-1.15.1"] -autoheader_names = ["autoheader-vanilla-2.69", "autoheader-2.69"] +AUTOCONF_NAMES = ["autoconf-vanilla-2.69", "autoconf-2.69"] +AUTOMAKE_NAMES = ["automake-vanilla-1.15", "automake-1.15.1"] +ACLOCAL_NAMES = ["aclocal-vanilla-1.15", "aclocal-1.15.1"] +AUTOHEADER_NAMES = ["autoheader-vanilla-2.69", "autoheader-2.69"] # Pick the first for each list that exists on this system. -AUTOCONF_BIN = next(name for name in autoconf_names if shutil.which(name)) -AUTOMAKE_BIN = next(name for name in automake_names if shutil.which(name)) -ACLOCAL_BIN = next(name for name in aclocal_names if shutil.which(name)) -AUTOHEADER_BIN = next(name for name in autoheader_names if shutil.which(name)) +AUTOCONF_BIN = next(name for name in AUTOCONF_NAMES if shutil.which(name)) +AUTOMAKE_BIN = next(name for name in AUTOMAKE_NAMES if shutil.which(name)) +ACLOCAL_BIN = next(name for name in ACLOCAL_NAMES if shutil.which(name)) +AUTOHEADER_BIN = next(name for name in AUTOHEADER_NAMES if shutil.which(name)) # autoconf-wrapper and automake-wrapper from Gentoo look at this environment variable. # It's harmless to set it on other systems though. @@ -34,12 +34,13 @@ ENV = " ".join( # Directories we should skip entirely because they're vendored or have different # autotools versions. -skip_dirs = [ +SKIP_DIRS = [ # readline and minizip are maintained with different autotools versions "readline", "minizip", ] + config_folders: list[Path] = [] for root, _, files in os.walk("."): @@ -50,7 +51,7 @@ for root, _, files in os.walk("."): for folder in sorted(config_folders): print(folder, flush=True) - if folder.stem in skip_dirs: + if folder.stem in SKIP_DIRS: continue os.chdir(folder) -- 2.44.0
pyright tells me: /home/smarchi/src/builder/builder/containers/autoregen.py /home/smarchi/src/builder/builder/containers/autoregen.py:46:13 - error: Type of "append" is partially unknown Type of "append" is "(__object: Unknown, /) -> None" (reportUnknownMemberType) Add a type annotation on `config_folders` to fix it. With that, all the subsequent warnings vanish too. --- builder/containers/autoregen.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/builder/containers/autoregen.py b/builder/containers/autoregen.py index feee5878cac7..26006ff18e0a 100755 --- a/builder/containers/autoregen.py +++ b/builder/containers/autoregen.py @@ -40,7 +40,7 @@ skip_dirs = [ "minizip", ] -config_folders = [] +config_folders: list[Path] = [] for root, _, files in os.walk("."): for file in files: -- 2.44.0
The first half of the patches are cleanups to make autoregen.py clean in the eyes of some tools I usually use for Python code. The rest of the patches make the script log a bit more. My main wish is for the script to print the shell commands it executes. Simon Marchi (8): autoregen.py: re-format with black 24.2.0 autoregen.py: fix a pyright `reportConstantRedefinition` warning autoregen.py: add type annotation for config_folders autoregen.py: capitalize some constant names autoregen.py: add non-suffixed version of the tools autoregen.py: print executed commands on stdout autoregen.py: let subprocesses print to stdout/stderr autoregen.py: be more verbose builder/containers/autoregen.py | 110 ++++++++++++++++++++------------ 1 file changed, 69 insertions(+), 41 deletions(-) base-commit: 753c1b3e9b56949af4600b9a43424ce5004361c2 -- 2.44.0
Without the comma between the strings it acts like string concatenation instead of having two string elements in the array. --- builder/master.cfg | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/builder/master.cfg b/builder/master.cfg index 5ea43cb7af01..1a569e4cd333 100644 --- a/builder/master.cfg +++ b/builder/master.cfg @@ -1202,7 +1202,7 @@ make_single_check_step = steps.Test( command=["make", "-k", "check"], name="make check") make_single_test_suite_step = steps.Test( - command=["make", "-k" "check"], + command=["make", "-k", "check"], name="make check", logfiles={"test-suite.log": "tests/test-suite.log"}) # Same as make_check_step but with tests/test-suite.log recorded -- 2.44.0
On Wed, Mar 06, 2024 at 12:14:31PM -0500, Frank Ch. Eigler wrote: > The following diff gets the annobin and a few other package > make_check* steps to use "make -k check" rather than "make check", so > that early partial failures do not doom the entire run. Many other > package builders already use the equivalent. Yes that makes sense. I do hope most testsuites are normally zero fail, so any failure would indicate an issue. But we would like to get as much test results as possible in the CI builders. And make -k check still makes the command fail if any make target fails. It just doesn't bail out early. Thanks, Mark > diff --git a/builder/master.cfg b/builder/master.cfg > index e4cd0802a051..d0b046f7af19 100644 > --- a/builder/master.cfg > +++ b/builder/master.cfg > @@ -1194,26 +1194,26 @@ make_pdf_step = steps.Compile( > command=["make", "pdf"], > name="make pdf") > make_check_step = steps.Test( > - command=addOutputSync.withArgs(["make", "check", > + command=addOutputSync.withArgs(["make", "-k", "check", > util.Interpolate('-j%(prop:ncpus)s')]), > name="make check", haltOnFailure=False, flunkOnFailure=True) > # make check explicitly not using -j > make_single_check_step = steps.Test( > - command=["make", "check"], > + command=["make", "-k", "check"], > name="make check") > make_single_test_suite_step = steps.Test( > - command=["make", "check"], > + command=["make", "-k" "check"], > name="make check", > logfiles={"test-suite.log": "tests/test-suite.log"}) > # Same as make_check_step but with tests/test-suite.log recorded > make_check_test_suite_step = steps.Test( > - command=addOutputSync.withArgs(["make", "check", > + command=addOutputSync.withArgs(["make", "-k", "check", > util.Interpolate('-j%(prop:ncpus)s')]), > name="make check", haltOnFailure=False, flunkOnFailure=True, > logfiles={"test-suite.log": "tests/test-suite.log"}) > # Same but with tests/testsuite.log (note, no dash) > make_check_testsuite_step = steps.Test( > - command=addOutputSync.withArgs(["make", "check", > + command=addOutputSync.withArgs(["make", "-k", "check", > util.Interpolate('-j%(prop:ncpus)s')]), > name="make check", haltOnFailure=False, flunkOnFailure=True, > logfiles={"testsuite.log": "tests/testsuite.log"}) > > > - FChE