Skip to content

Add Popen_with_deplayed_expansion to process_utils #188

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
uvdn7 merged 1 commit into from
Jun 24, 2015
Merged

Add Popen_with_deplayed_expansion to process_utils #188

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 25 additions & 0 deletions app/util/process_utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
from contextlib import suppress
import os
import subprocess
from subprocess import TimeoutExpired


Expand All @@ -23,3 +25,26 @@ def kill_gracefully(process, timeout=2):
stdout, stderr = process.communicate()

return process.returncode, stdout, stderr


def Popen_with_delayed_expansion(cmd, *args, **kwargs):
Copy link
Copy Markdown
Contributor

@josephharrington josephharrington Jun 23, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think we would want to use this wrapper everywhere throughout the app? If so, we might want to just name it Popen and maybe enforce that this wrapper must be used instead of subprocess.Popen directly.

Copy link
Copy Markdown
Contributor Author

@uvdn7 uvdn7 Jun 23, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. I can't think of a reason not to use it everywhere throughout the app
  2. You mean monkey patch subprocess.Popen at the very beginning during import? Can you elaborate how exactly you want to enforce it?

Copy link
Copy Markdown
Contributor

@josephharrington josephharrington Jun 23, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably not patching. We'd probably enforce via convention (lightest weight), custom lint rule, or unit test blacklist.

I'm not too concerned about the enforcing since we very rarely add Popen calls. The main thing I was wondering is if we should just name this Popen.

Copy link
Copy Markdown
Contributor Author

@uvdn7 uvdn7 Jun 24, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@josephharrington I don't like to name it Popen as it's confusing to people. Why I should use this Popen instead of subprocess.Popen? Naming it this way, IMO, is explicit so hopefully more readable and maintainable. I am going to merge this as is but I am more than happy to change it later. 😃

Copy link
Copy Markdown
Contributor

@josephharrington josephharrington Jun 24, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I totally agree that naming it something more descriptive would be helpful to people encountering it for the first time.

My main reasoning about why I'd name it something other than Popen_with_delayed_expansion specifically is that I can totally see us adding more functionality to this method in the future, especially if, as you said, we'd probably want to use it everywhere throughout the app. (Just as an example of what I mean: if we add some sort of output redirection, then Popen_with_delayed_expansion would either become Popen_with_delayed_expansion_and_output_redirection or more likely just become a not-quite-so-descriptive name.) I suppose something still descriptive but a bit more general such as "Popen_cross_platform" would avoid that.

I also think the naming discussion definitely depends on how this will be used. More specifically, I think it depends on if it's more of a special case when we do use this instead of standard Popen or when we don't. (And actually I think the answer to that is pretty tightly linked to the likelihood that we'll add more functionality to this method in the future, so this is pretty close to my previous point.)

Answering the question of "Why I should use this Popen instead of subprocess.Popen?" was also what I was thinking about when suggesting something like a lint rule. Chances are that even with the name Popen_with_delayed_expansion someone will still end up going to read the method's docstring to find out what delayed expansion is anyway. :) We'd probably want a lint rule if we decided there would never be a reason someone would not want to use this -- but I'm not yet sure that's true.

I'm fine with leaving this for now -- we can always come back to it if we want to. I just mostly wanted to explain my line of reasoning in excruciating detail. If you've read this far then you've suffered enough. 😪

Copy link
Copy Markdown
Contributor Author

@uvdn7 uvdn7 Jun 24, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lol. It's all good thoughts!

"""
A thin wrapper around subprocess.Popen which ensures that all environment variables in the cmd are expanded at
execution time. By default, Windows CMD *disables* delayed expansion which means it will expand the command first
before execution. E.g. run 'set FOO=1 && echo %FOO%' won't actually echo 1 because %FOO% gets expanded before the
execution.

:param cmd: The command to execute
:type cmd: str | iterable

:return: Popen object, just like the Popen object returned by subprocess.Popen
:rtype: :class:`Popen`
"""
if os.name == 'nt':
cmd_with_deplayed_expansion = ['cmd', '/V', '/C']
if isinstance(cmd, str):
cmd_with_deplayed_expansion.append(cmd)
else:
cmd_with_deplayed_expansion.extend(cmd)
cmd = cmd_with_deplayed_expansion
return subprocess.Popen(cmd, *args, **kwargs)
43 changes: 43 additions & 0 deletions test/unit/util/test_process_utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
from genty import genty, genty_dataset

from app.util.process_utils import Popen_with_delayed_expansion

from test.framework.base_unit_test_case import BaseUnitTestCase


@genty
class TestProcessUtils(BaseUnitTestCase):

@genty_dataset(
str_cmd_on_windows=(
'set FOO=1 && echo !FOO!',
'nt',
['cmd', '/V', '/C', 'set FOO=1 && echo !FOO!'],
),
list_cmd_on_windows=(
['set', 'FOO=1', '&&', 'echo', '!FOO!'],
'nt',
['cmd', '/V', '/C', 'set', 'FOO=1', '&&', 'echo', '!FOO!'],
),
str_cmd_on_posix=(
'export FOO=1; echo $FOO',
'posix',
'export FOO=1; echo $FOO',
),
list_cmd_on_posix=(
['export', 'FOO=1;', 'echo', '$FOO'],
'posix',
['export', 'FOO=1;', 'echo', '$FOO'],
),
)
def test_Popen_with_deplayed_expansion(self, input_cmd, os_name, expected_final_cmd):
# Arrange
mock_os = self.patch('app.util.process_utils.os')
mock_os.name = os_name
mock_subprocess_popen = self.patch('subprocess.Popen')

# Act
Popen_with_delayed_expansion(input_cmd)

# Assert
mock_subprocess_popen.assert_called_once_with(expected_final_cmd)