Chore: Burnin script shell execution #646
Conversation
| use_shell = True | ||
| try: | ||
| test_proc = subprocess.Popen( | ||
| f"{FFMPEG_EXE_COMMAND} --help", shell=True | ||
| ) | ||
| test_proc.wait() | ||
| except BaseException: | ||
| use_shell = False | ||
|
|
||
| kwargs = { | ||
| "stdout": subprocess.PIPE, | ||
| "stderr": subprocess.PIPE, | ||
| "shell": True, | ||
| "shell": use_shell, |
There was a problem hiding this comment.
Don't these subprocess calls technically mean they may show up a command prompt dialog on Windows? Shouldn't these have that "hide console window" special flag?
Also, it's really annoying we have to do this because these subprocess calls startup always take longer than I'd like. I mean it's better than failing.
Maybe we could cache the "should we run in shell" result for the running process? (Does that even make sense since this would be a script that would maybe already itself be running in a subprocess maybe?)
There was a problem hiding this comment.
Also, it's really annoying we have to do this because these subprocess calls startup always take longer than I'd like. I mean it's better than failing.
Maybe we could cache the "should we run in shell" result for the running process? (Does that even make sense since this would be a script that would maybe already itself be running in a subprocess maybe?)
This is called only once during the process lifetime (single extract burning process).
Ideal fix is to rewrite the logic so output is list of arguments, in that case we can get rid of shell=True (lack of time and patience).
There was a problem hiding this comment.
Yup - thanks for confirming.
Changelog Description
Try to auto-fix usage of
shell=Truein burnin script.Additional info
In some cases the shell set to
Truemight cause that the subprocess don't have access to the executable. Ideal case would be to not useshell=Trueat all, that would require more modifications. To avoid it we can run "testing" ffmpeg command and change it toFalseif it crashes.