* [PATCH 0/8] Some changes to autoregen.py
@ 2024-03-11 19:01 Simon Marchi
2024-03-11 19:01 ` [PATCH 1/8] autoregen.py: re-format with black 24.2.0 Simon Marchi
` (8 more replies)
0 siblings, 9 replies; 18+ messages in thread
From: Simon Marchi @ 2024-03-11 19:01 UTC (permalink / raw)
To: buildbot; +Cc: Simon Marchi
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 1/8] autoregen.py: re-format with black 24.2.0
2024-03-11 19:01 [PATCH 0/8] Some changes to autoregen.py Simon Marchi
@ 2024-03-11 19:01 ` Simon Marchi
2024-03-11 19:01 ` [PATCH 2/8] autoregen.py: fix a pyright `reportConstantRedefinition` warning Simon Marchi
` (7 subsequent siblings)
8 siblings, 0 replies; 18+ messages in thread
From: Simon Marchi @ 2024-03-11 19:01 UTC (permalink / raw)
To: buildbot; +Cc: Simon Marchi
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 2/8] autoregen.py: fix a pyright `reportConstantRedefinition` warning
2024-03-11 19:01 [PATCH 0/8] Some changes to autoregen.py Simon Marchi
2024-03-11 19:01 ` [PATCH 1/8] autoregen.py: re-format with black 24.2.0 Simon Marchi
@ 2024-03-11 19:01 ` Simon Marchi
[not found] ` <871q8f7jx5.fsf@aarsen.me>
2024-03-11 19:01 ` [PATCH 3/8] autoregen.py: add type annotation for config_folders Simon Marchi
` (6 subsequent siblings)
8 siblings, 1 reply; 18+ messages in thread
From: Simon Marchi @ 2024-03-11 19:01 UTC (permalink / raw)
To: buildbot; +Cc: Simon Marchi
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 3/8] autoregen.py: add type annotation for config_folders
2024-03-11 19:01 [PATCH 0/8] Some changes to autoregen.py Simon Marchi
2024-03-11 19:01 ` [PATCH 1/8] autoregen.py: re-format with black 24.2.0 Simon Marchi
2024-03-11 19:01 ` [PATCH 2/8] autoregen.py: fix a pyright `reportConstantRedefinition` warning Simon Marchi
@ 2024-03-11 19:01 ` Simon Marchi
2024-03-11 19:01 ` [PATCH 4/8] autoregen.py: capitalize some constant names Simon Marchi
` (5 subsequent siblings)
8 siblings, 0 replies; 18+ messages in thread
From: Simon Marchi @ 2024-03-11 19:01 UTC (permalink / raw)
To: buildbot; +Cc: Simon Marchi
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 4/8] autoregen.py: capitalize some constant names
2024-03-11 19:01 [PATCH 0/8] Some changes to autoregen.py Simon Marchi
` (2 preceding siblings ...)
2024-03-11 19:01 ` [PATCH 3/8] autoregen.py: add type annotation for config_folders Simon Marchi
@ 2024-03-11 19:01 ` Simon Marchi
2024-03-11 19:01 ` [PATCH 5/8] autoregen.py: add non-suffixed version of the tools Simon Marchi
` (4 subsequent siblings)
8 siblings, 0 replies; 18+ messages in thread
From: Simon Marchi @ 2024-03-11 19:01 UTC (permalink / raw)
To: buildbot; +Cc: Simon Marchi
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 5/8] autoregen.py: add non-suffixed version of the tools
2024-03-11 19:01 [PATCH 0/8] Some changes to autoregen.py Simon Marchi
` (3 preceding siblings ...)
2024-03-11 19:01 ` [PATCH 4/8] autoregen.py: capitalize some constant names Simon Marchi
@ 2024-03-11 19:01 ` Simon Marchi
2024-03-11 19:01 ` [PATCH 6/8] autoregen.py: print executed commands on stdout Simon Marchi
` (3 subsequent siblings)
8 siblings, 0 replies; 18+ messages in thread
From: Simon Marchi @ 2024-03-11 19:01 UTC (permalink / raw)
To: buildbot; +Cc: Simon Marchi
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 6/8] autoregen.py: print executed commands on stdout
2024-03-11 19:01 [PATCH 0/8] Some changes to autoregen.py Simon Marchi
` (4 preceding siblings ...)
2024-03-11 19:01 ` [PATCH 5/8] autoregen.py: add non-suffixed version of the tools Simon Marchi
@ 2024-03-11 19:01 ` Simon Marchi
2024-03-11 19:01 ` [PATCH 7/8] autoregen.py: let subprocesses print to stdout/stderr Simon Marchi
` (2 subsequent siblings)
8 siblings, 0 replies; 18+ messages in thread
From: Simon Marchi @ 2024-03-11 19:01 UTC (permalink / raw)
To: buildbot; +Cc: Simon Marchi
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 7/8] autoregen.py: let subprocesses print to stdout/stderr
2024-03-11 19:01 [PATCH 0/8] Some changes to autoregen.py Simon Marchi
` (5 preceding siblings ...)
2024-03-11 19:01 ` [PATCH 6/8] autoregen.py: print executed commands on stdout Simon Marchi
@ 2024-03-11 19:01 ` Simon Marchi
[not found] ` <87plvz7k10.fsf@aarsen.me>
2024-03-11 19:01 ` [PATCH 8/8] autoregen.py: be more verbose Simon Marchi
2024-03-11 21:04 ` [PATCH 0/8] Some changes to autoregen.py Sam James
8 siblings, 1 reply; 18+ messages in thread
From: Simon Marchi @ 2024-03-11 19:01 UTC (permalink / raw)
To: buildbot; +Cc: Simon Marchi
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 8/8] autoregen.py: be more verbose
2024-03-11 19:01 [PATCH 0/8] Some changes to autoregen.py Simon Marchi
` (6 preceding siblings ...)
2024-03-11 19:01 ` [PATCH 7/8] autoregen.py: let subprocesses print to stdout/stderr Simon Marchi
@ 2024-03-11 19:01 ` Simon Marchi
2024-03-11 21:04 ` [PATCH 0/8] Some changes to autoregen.py Sam James
8 siblings, 0 replies; 18+ messages in thread
From: Simon Marchi @ 2024-03-11 19:01 UTC (permalink / raw)
To: buildbot; +Cc: Simon Marchi
- 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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 0/8] Some changes to autoregen.py
2024-03-11 19:01 [PATCH 0/8] Some changes to autoregen.py Simon Marchi
` (7 preceding siblings ...)
2024-03-11 19:01 ` [PATCH 8/8] autoregen.py: be more verbose Simon Marchi
@ 2024-03-11 21:04 ` Sam James
2024-03-11 21:52 ` Mark Wielaard
8 siblings, 1 reply; 18+ messages in thread
From: Sam James @ 2024-03-11 21:04 UTC (permalink / raw)
To: Simon Marchi; +Cc: buildbot
[-- 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 --]
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 0/8] Some changes to autoregen.py
2024-03-11 21:04 ` [PATCH 0/8] Some changes to autoregen.py Sam James
@ 2024-03-11 21:52 ` Mark Wielaard
2024-03-12 1:53 ` Simon Marchi
0 siblings, 1 reply; 18+ messages in thread
From: Mark Wielaard @ 2024-03-11 21:52 UTC (permalink / raw)
To: Sam James; +Cc: Simon Marchi, buildbot
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 0/8] Some changes to autoregen.py
2024-03-11 21:52 ` Mark Wielaard
@ 2024-03-12 1:53 ` Simon Marchi
2024-03-12 10:17 ` Mark Wielaard
0 siblings, 1 reply; 18+ messages in thread
From: Simon Marchi @ 2024-03-12 1:53 UTC (permalink / raw)
To: Mark Wielaard, Sam James; +Cc: Simon Marchi, buildbot
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 0/8] Some changes to autoregen.py
2024-03-12 1:53 ` Simon Marchi
@ 2024-03-12 10:17 ` Mark Wielaard
0 siblings, 0 replies; 18+ messages in thread
From: Mark Wielaard @ 2024-03-12 10:17 UTC (permalink / raw)
To: Simon Marchi; +Cc: Sam James, Simon Marchi, buildbot
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 7/8] autoregen.py: let subprocesses print to stdout/stderr
[not found] ` <87plvz7k10.fsf@aarsen.me>
@ 2024-03-12 15:40 ` Simon Marchi
2024-03-13 8:46 ` Mark Wielaard
0 siblings, 1 reply; 18+ messages in thread
From: Simon Marchi @ 2024-03-12 15:40 UTC (permalink / raw)
To: Arsen Arsenović; +Cc: buildbot
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/8] autoregen.py: fix a pyright `reportConstantRedefinition` warning
[not found] ` <871q8f7jx5.fsf@aarsen.me>
@ 2024-03-12 16:00 ` Simon Marchi
2024-03-13 8:58 ` Mark Wielaard
0 siblings, 1 reply; 18+ messages in thread
From: Simon Marchi @ 2024-03-12 16:00 UTC (permalink / raw)
To: Arsen Arsenović; +Cc: buildbot
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 7/8] autoregen.py: let subprocesses print to stdout/stderr
2024-03-12 15:40 ` Simon Marchi
@ 2024-03-13 8:46 ` Mark Wielaard
0 siblings, 0 replies; 18+ messages in thread
From: Mark Wielaard @ 2024-03-13 8:46 UTC (permalink / raw)
To: Simon Marchi; +Cc: Arsen Arsenović, buildbot
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/8] autoregen.py: fix a pyright `reportConstantRedefinition` warning
2024-03-12 16:00 ` Simon Marchi
@ 2024-03-13 8:58 ` Mark Wielaard
2024-03-18 17:36 ` Arsen Arsenović
0 siblings, 1 reply; 18+ messages in thread
From: Mark Wielaard @ 2024-03-13 8:58 UTC (permalink / raw)
To: Simon Marchi; +Cc: Arsen Arsenović, buildbot
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/8] autoregen.py: fix a pyright `reportConstantRedefinition` warning
2024-03-13 8:58 ` Mark Wielaard
@ 2024-03-18 17:36 ` Arsen Arsenović
0 siblings, 0 replies; 18+ messages in thread
From: Arsen Arsenović @ 2024-03-18 17:36 UTC (permalink / raw)
To: Mark Wielaard; +Cc: Simon Marchi, buildbot
[-- 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 --]
^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2024-03-18 17:36 UTC | newest]
Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-03-11 19:01 [PATCH 0/8] Some changes to autoregen.py Simon Marchi
2024-03-11 19:01 ` [PATCH 1/8] autoregen.py: re-format with black 24.2.0 Simon Marchi
2024-03-11 19:01 ` [PATCH 2/8] autoregen.py: fix a pyright `reportConstantRedefinition` warning Simon Marchi
[not found] ` <871q8f7jx5.fsf@aarsen.me>
2024-03-12 16:00 ` Simon Marchi
2024-03-13 8:58 ` Mark Wielaard
2024-03-18 17:36 ` Arsen Arsenović
2024-03-11 19:01 ` [PATCH 3/8] autoregen.py: add type annotation for config_folders Simon Marchi
2024-03-11 19:01 ` [PATCH 4/8] autoregen.py: capitalize some constant names Simon Marchi
2024-03-11 19:01 ` [PATCH 5/8] autoregen.py: add non-suffixed version of the tools Simon Marchi
2024-03-11 19:01 ` [PATCH 6/8] autoregen.py: print executed commands on stdout Simon Marchi
2024-03-11 19:01 ` [PATCH 7/8] autoregen.py: let subprocesses print to stdout/stderr Simon Marchi
[not found] ` <87plvz7k10.fsf@aarsen.me>
2024-03-12 15:40 ` Simon Marchi
2024-03-13 8:46 ` Mark Wielaard
2024-03-11 19:01 ` [PATCH 8/8] autoregen.py: be more verbose Simon Marchi
2024-03-11 21:04 ` [PATCH 0/8] Some changes to autoregen.py Sam James
2024-03-11 21:52 ` Mark Wielaard
2024-03-12 1:53 ` Simon Marchi
2024-03-12 10:17 ` Mark Wielaard
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).