[PATCH 0/8] Some changes to autoregen.py

[PATCH 0/8] Some changes to autoregen.py

[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

[PATCH 1/8] autoregen.py: re-format with black 24.2.0

[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

[PATCH 2/8] autoregen.py: fix a pyright `reportConstantRedefinition` warning

[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

[PATCH 3/8] autoregen.py: add type annotation for config_folders

[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

[PATCH 4/8] autoregen.py: capitalize some constant names

[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

[PATCH 5/8] autoregen.py: add non-suffixed version of the tools

[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

[PATCH 6/8] autoregen.py: print executed commands on stdout

[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

[PATCH 7/8] autoregen.py: let subprocesses print to stdout/stderr

[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

[PATCH 8/8] autoregen.py: be more verbose

[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

Re: [PATCH 0/8] Some changes to autoregen.py

[Sam James]

[-- 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 --]

Re: [PATCH 0/8] Some changes to autoregen.py

[Mark Wielaard]

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

Re: [PATCH 0/8] Some changes to autoregen.py

[Simon Marchi]

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

Re: [PATCH 0/8] Some changes to autoregen.py

[Mark Wielaard]

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

Re: [PATCH 7/8] autoregen.py: let subprocesses print to stdout/stderr

[Simon Marchi]

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

Re: [PATCH 2/8] autoregen.py: fix a pyright `reportConstantRedefinition` warning

[Simon Marchi]

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

Re: [PATCH 7/8] autoregen.py: let subprocesses print to stdout/stderr

[Mark Wielaard]

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

Re: [PATCH 2/8] autoregen.py: fix a pyright `reportConstantRedefinition` warning

[Mark Wielaard]

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

Re: [PATCH 2/8] autoregen.py: fix a pyright `reportConstantRedefinition` warning

[Arsen Arsenović]

[-- 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 --]