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

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

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