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

[Simon Marchi <simon.marchi@efficios.com>]

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