Use pure python code in place of os.system #743
Conversation
glehmann
force-pushed
the
gln/srpm-error-handling-wlnl
branch
from
a1f7b08 to
3782f97
Compare
glehmann
force-pushed
the
gln/srpm-error-handling-wlnl
branch
from
3782f97 to
5149284
Compare
glehmann
force-pushed
the
gln/srpm-error-handling-wlnl
branch
from
5149284 to
2c59a22
Compare
scripts/import_srpm.py
Outdated
| raise ValueError("All commands in the list must be non-empty.") | ||
|
|
||
| processes: list[subprocess.Popen[bytes]] = [] | ||
| last_process_stdout = None |
There was a problem hiding this comment.
naming it next_process_stdin would make it more accurate
There was a problem hiding this comment.
I suppose it depends on which side of the pipe you're looking at 🙂
There was a problem hiding this comment.
in one way yes, but I'm rather seeing it as "in one case the name is always valid, while in the other there is no previous process when launching the first one"
There was a problem hiding this comment.
That's why it's a IO[bytes] | None.
But I'm also ok with the other name, I'll use it 👍
scripts/import_srpm.py
Outdated
| # to prevent deadlocks if the current process finishes before the previous one. | ||
| if last_process_stdout: | ||
| last_process_stdout.close() |
There was a problem hiding this comment.
I'm not 100% sure of the nature of the deadlock, but avoiding keeping 2 readers on the pipe is definitely a good idea. Maybe describe it like "the new process does the reading from this pipe, drop our ref to it, notably preventing deadlocks"?
There was a problem hiding this comment.
I wrote that based on an example I found, but I can't produce any deadlock, and I find plenty of examples that don't use it. I'll remove it.
There was a problem hiding this comment.
It should be kept, as explained (much better) in the API doc
scripts/import_srpm.py
Outdated
| for i, p in enumerate(processes): | ||
| p.wait() | ||
| if p.returncode != 0: | ||
| raise subprocess.CalledProcessError(returncode=p.returncode, cmd=commands[i]) |
There was a problem hiding this comment.
Maybe more clear with for cmd, p in zip(commands, processes)?
os.system calls the user's shell, and is not directly checking for errors. This should raise an error when cpio or rpm2cpio fail, or if they can't be found. Signed-off-by: Gaëtan Lehmann <gaetan.lehmann@vates.tech>
glehmann
force-pushed
the
gln/srpm-error-handling-wlnl
branch
from
2c59a22 to
fa8bab3
Compare
os.system calls the user's shell, and is not directly checking for errors. This should raise an error when cpio or rpm2cpio fail, or if they can't be found.