Salt: Salt Minion dies on Windows when it can't find pip

[cudneys]

Description of Issue/Question

I'm seeing this exception when trying to install pip modules on Windows:

``` An exception occurred in this state: Traceback (most recent call last):
File "c:saltbinlibsite-packagessaltstate.py", line 1878, in call
*cdata['kwargs'])
File "c:saltbinlibsite-packagessaltloader.py", line 1823, in wrapper
return f(args, **kwargs)
File "c:saltbinlibsite-packagessaltstatespip_state.py", line 685, in installed
upgrade, user, cwd, bin_env)
File "c:saltbinlibsite-packagessaltstatespip_state.py", line 193, in _check_if_installed
user=user, cwd=cwd)
File "c:saltbinlibsite-packagessaltmodulespip.py", line 1074, in list_
for line in freeze(bin_env=bin_env, user=user, cwd=cwd):
File "c:saltbinlibsite-packagessaltmodulespip.py", line 1014, in freeze
pip_bin = _get_pip_bin(bin_env)
File "c:saltbinlibsite-packagessaltmodulespip.py", line 134, in _get_pip_bin
which_result.encode('string-escape')
AttributeError: 'NoneType' object has no attribute 'encode'

### Steps to Reproduce Issue
This is a fresh installation of Windows Server 2016, using the python2 version of the minion.  When trying to apply an SLS that contains only this code, the exception above is thrown.

pip:
pip.installed:
- name: pip >= 9.0.3
- upgrade: True

### Versions Report

c:saltbinScripts>salt-minion --versions-report
Salt Version:
Salt: 2018.3.0

Dependency Versions:
cffi: 1.10.0
cherrypy: 10.2.1
dateutil: 2.6.1
docker-py: Not Installed
gitdb: 2.0.3
gitpython: Not Installed
ioflo: Not Installed
Jinja2: 2.10
libgit2: Not Installed
libnacl: 1.6.1
M2Crypto: Not Installed
Mako: 1.0.7
msgpack-pure: Not Installed
msgpack-python: 0.4.8
mysql-python: Not Installed
pycparser: 2.18
pycrypto: 2.6.1
pycryptodome: Not Installed
pygit2: Not Installed
Python: 2.7.14 (v2.7.14:84471935ed, Sep 16 2017, 20:25:58) [MSC v.1500 64 bit (AMD64)]
python-gnupg: 0.4.1
PyYAML: 3.12
PyZMQ: 16.0.3
RAET: Not Installed
smmap: 2.0.3
timelib: 0.2.4
Tornado: 4.5.2
ZMQ: 4.1.6

System Versions:
dist:
locale: cp1252
machine: AMD64
release: 2016Server
system: Windows
version: 2016Server 10.0.14393 Multiprocessor Free

c:saltbinScripts>

The master is the same version.

### Solution

I updated the pip.py on my windows server, making the following changes:

diff --git a/salt/modules/pip.py b/salt/modules/pip.py
index b560612..16274ea 100644
--- a/salt/modules/pip.py
+++ b/salt/modules/pip.py
@@ -130,10 +130,13 @@ def _get_pip_bin(bin_env):
'pip{0}'.format(sys.version_info[0]),
'pip', 'pip-python']
)

• if salt.utils.platform.is_windows() and six.PY2:
• which_result.encode('string-escape')
  +
  if which_result is None:
  raise CommandNotFoundError('Could not find a pip binary')
  +
• if salt.utils.platform.is_windows() and six.PY2:
• which_result.encode('string-escape')
  +
  return which_result

 # try to get pip bin from virtualenv, bin_env

And it now functions as expected.  By checking to see if which_result is None before encoding I get the following response:

pip_|-pip_|-pip >= 9.0.3_|-installed:
    ----------
    __id__:
        pip
    __run_num__:
        0
    __sls__:
        core_packages.python
    changes:
        ----------
    comment:
        Error installing 'pip >= 9.0.3': Could not find a `pip` binary
    duration:
        15.0
    name:
        pip >= 9.0.3
    result:
        False
    start_time:
        17:27:12.294000

```

Would you like me to create a pull request?

[Ch3LL]

ping @saltstack/team-windows any opinions on the proposed fix?

[lorengordon]

Seems reasonable to me... just moving the check that's already there so it happens before attempting to use a method that may not exist on the value.

Personally I'd probably use try/except AttributeError instead, but that's just me.

[skizunov]

Looking at the code, the failure makes sense because which will fail to find pip.exe since its containing folder isn't in the PATH environment variable. On Windows, since you control the distribution, you can get it relative to where Python lives. An example, but note that this will not work on other platforms:

pip_bin = os.path.join(os.path.dirname(sys.executable), 'Scripts', 'pip.exe')

I have a different suggestion as well. This one is a bit more drastic (it will affect all platforms) but it has a few advantages:
1) Doesn't rely on the PATH environment variable.
2) If multiple versions of Python are installed on a system, it will always use the right corresponding pip.
3) In the Windows case, don't need to worry about correctly modifying the path to Python embedded in pip.exe at the installer level.

This suggestion is rather simple. Have _get_pip_bin return a list instead of a string. Have it return:

return [sys.executable, '-m', 'pip']

Then, prepend that list to the list of command line arguments for each pip command.

[lorengordon]

@skizunov I recently was testing something similar but ended up reverting it because python -m pip ... didn't work with python 2.6 (which I _still_ have to support 😢). If that's not a use case here, then big 👍.

[skizunov]

@lorengordon : This is for Salt 2018.3 which I believe doesn't support Python 2.6 anymore.

[twangboy]

If the above change is made it needs to be made clear that by default the pip module will operate on the salt installation of Python. By forcing the user to pass pip_env and cwd they know exactly what python environment is being modified. That is why we haven't done something similar in the past.

[skizunov]

@twangboy : Perhaps I should clarify things. What I meant was that in _get_pip_bin, for the if not bin_env case, return:

return [sys.executable, '-m', 'pip']

I believe the intent of this use case is to use the pip from the Python currently used. This is the use case that this issue refers to. For the other case (when bin_env is set to something), just leave the implementation as is, except return a list of one element instead of a string.

[twangboy]

I agree, but I got some pushback when I wanted to default to the currently used Python. So, I'm just saying we need to make sure it's well documented so that it's clear what environment is receiving the install... which I'm sure it would be.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

If this issue is closed prematurely, please leave a comment and we will gladly reopen the issue.