Replace os.system() with subprocess.check_call() #53210

New Issue

Lawrence D'Oliveiro · 2017-10-31T09:34:02+01:00

2017-10-31 09:34:02 +01:00

Change all instances of os.system() that do not need to go through the shell to use subprocess.check_call() instead. Not only does this save a process creation, it is also safer, with less chance of being tripped up by filenames with odd characters in them.

ldo-use-subprocess-20171031.patch

Change all instances of `os.system()` that do not need to go through the shell to use `subprocess.check_call()` instead. Not only does this save a process creation, it is also safer, with less chance of being tripped up by filenames with odd characters in them. [ldo-use-subprocess-20171031.patch](https://archive.blender.org/developer/F1079096/ldo-use-subprocess-20171031.patch)

Lawrence D'Oliveiro commented

2017-10-31 09:34:02 +01:00

Changed status to: 'Open'

Lawrence D'Oliveiro commented

2017-10-31 09:34:02 +01:00

Added subscriber: @ldo

Bastien Montagne commented

2017-10-31 11:35:04 +01:00

Added subscriber: @mont29

Bastien Montagne commented

2017-10-31 11:35:04 +01:00

Changed status from 'Open' to: 'Archived'

Bastien Montagne closed this issue

2017-10-31 11:35:04 +01:00

Bastien Montagne self-assigned this 2017-10-31 11:35:04 +01:00

Bastien Montagne commented

2017-10-31 11:35:04 +01:00

Thanks for the patch, but there are two problems here:

First, cleanup patches are not usually that easy to handle, because they tend to touch several, totally unrelated areas (like this one, even affecting buildbot scripts), so it's complicated to get several maintainers to review and test it. Further more, as 'cleanup' says, it does not really adds any new feature, nor fix any real issue, it’s just 'nice improvement to have', but… finding time to handle this is a bit difficult tbh.
Secondly, subprocess.check_call() & co are not latest recommended stuff, since py3.5 you have subprocess.run() which is new recommended high-level API to be used.

Also, please create a differential, it’s correct way to submit patches now. :)

We appreciate the effort, but am going to close that task, if you really think getting rid of os.system() calls is that important please resubmit to correct place with latest API usage, and avoid mixing totally different areas of code base in the same diff. ;)

Thanks for the patch, but there are two problems here: * First, cleanup patches are not usually that easy to handle, because they tend to touch several, totally unrelated areas (like this one, even affecting buildbot scripts), so it's complicated to get several maintainers to review and test it. Further more, as 'cleanup' says, it does not really adds any new feature, nor fix any real issue, it’s just 'nice improvement to have', but… finding time to handle this is a bit difficult tbh. * Secondly, `subprocess.check_call()` & co are not latest recommended stuff, since py3.5 you have `subprocess.run()` which is new recommended high-level API to be used. Also, please [create a differential](https://developer.blender.org/differential/diff/create/), it’s correct way to submit patches now. :) We appreciate the effort, but am going to close that task, if you really think getting rid of `os.system()` calls is that important please resubmit to correct place with latest API usage, and avoid mixing totally different areas of code base in the same diff. ;)

Lawrence D'Oliveiro commented

2017-11-01 02:48:54 +01:00

subprocess.run() was not appropriate for any of them. It returns an object for separate extraction of output, status etc, which is not needed in any of the cases I replaced.

`subprocess.run()` was not appropriate for any of them. It returns an object for separate extraction of output, status etc, which is not needed in any of the cases I replaced.

Lawrence D'Oliveiro commented

2017-12-17 03:38:13 +01:00

Updated patch:

ldo-use-subprocess-20171217.patch

Updated patch: [ldo-use-subprocess-20171217.patch](https://archive.blender.org/developer/F1427442/ldo-use-subprocess-20171217.patch)

Ray molenkamp commented

2017-12-17 04:11:55 +01:00

Added subscriber: @LazyDodo