pip: combine chdir and env only when env is set

[tyll]

SUMMARY

This fixes an AttributeError when chdir without virtualenv is specified:

File "/tmp/ansible_2UAFsZ/ansible_module_pip.py", line 387, in main
env = os.path.join(chdir, env)
File "/usr/lib64/python2.7/posixpath.py", line 75, in join
if b.startswith('/'):
AttributeError: 'NoneType' object has no attribute 'startswith'

ISSUE TYPE

• Bugfix Pull Request

COMPONENT NAME

pip

ANSIBLE VERSION

ansible 2.5.2
  config file = /etc/ansible/ansible.cfg
  configured module search path = [u'/home/till/.ansible/plugins/modules', u'/usr/share/ansible/plugins/modules']
  ansible python module location = /usr/lib/python2.7/site-packages/ansible
  executable location = /usr/bin/ansible
  python version = 2.7.15 (default, May 16 2018, 17:50:09) [GCC 8.1.1 20180502 (Red Hat 8.1.1-1)]

ADDITIONAL INFORMATION

---
cat > 'bug_pip_chdir.yml' <<EOF
- name: Show pip chdir bug
  hosts: all
  tasks:
    - pip:
        name: pip
        extra_args: --user --upgrade
        chdir: /tmp
EOF

ansible-playbook -i rhel7, bug_pip_chdir.yml 

PLAY [Run tox tests and nmctl show] *********************************************************************

TASK [Gathering Facts] **********************************************************************************
ok: [rhel7]

TASK [pip] **********************************************************************************************
fatal: [rhel7]: FAILED! => {"changed": false, "module_stderr": "Shared connection to 192.168.122.177 closed.\r\n", "module_stdout": "Traceback (most recent call last):\r\n  File \"/tmp/ansible_Q3ae1r/ansible_module_pip.py\", line 562, in <module>\r\n    main()\r\n  File \"/tmp/ansible_Q3ae1r/ansible_module_pip.py\", line 387, in main\r\n    env = os.path.join(chdir, env)\r\n  File \"/usr/lib64/python2.7/posixpath.py\", line 75, in join\r\n    if b.startswith('/'):\r\nAttributeError: 'NoneType' object has no attribute 'startswith'\r\n", "msg": "MODULE FAILURE", "rc": 1}

PLAY RECAP **********************************************************************************************
rhel7                      : ok=1    changed=0    unreachable=0    failed=1   

[resmo]

LGTM

[ansibot]

cc @Lujeni @mattupstate @robinro
click here for bot help

[robinro]

Thanks @tyll for this PR.
This issue is already known as #37912 and there is a fix for it under discussion in #40129. The fix given here is not complete.

[tyll]

@robinro thank you for the pointers. The fix here is complete for the error that is reported here. There seem to be other issues with the pip module discussed in #40129 that can be addressed with extra changes, but the AttributeError in discussion here could be easily fixed. IMHO it would be better to fix this actual error first and then finish the discussion about other problems. The fix could have been in several releases already and it fixes this playbook:
https://github.com/tyll/nmstate/blob/integration_tests/tests/ci_tests_basic.yml#L36

Therefore it would be great to merge at least this fix and then continue the discussion about the other problems.

[robinro]

Since #40129 is not updated
@tyll Can you add a test for your change? I'd merge the simple fix then and ignore the executable situation for now.

[tyll]

@robinro I added an integration test

[ansibot]

The test ansible-test sanity --test pep8 [explain] failed with 2 errors:

test/integration/targets/pip/files/ansible_test_pip_chdir/__init__.py:4:1: E302 expected 2 blank lines, found 1
test/integration/targets/pip/files/setup.py:11:7: E126 continuation line over-indented for hanging indent

The test ansible-test sanity --test shebang [explain] failed with 2 errors:

test/integration/targets/pip/files/ansible_test_pip_chdir/__init__.py:1:1: unexpected non-module shebang: b'#!/usr/bin/python3 -tt'
test/integration/targets/pip/files/setup.py:1:1: unexpected non-module shebang: b'#!/usr/bin/python -tt'

click here for bot help

[robinro]

@tyll Thanks for the test. To me it's a bit over-the-top and also the CI complains. I wouldn't use a custom package for this.
Maybe we can have a test that "only" checks for the original issue: Not failing when chdir and env are both passed. Maybe use a relative path to a trivial requirements.txt to check if the chdir has effect, but even that is imo not necessary.

[tyll]

On Thu, Jun 21, 2018 at 11:50:23AM -0700, Robin Roth wrote: @tyll Thanks for the test. To me it's a bit over-the-top and also the CI complains. I wouldn't use a custom package for this. Maybe we can have a test that "only" checks for the original issue: Not failing when `chdir` and `env` are both passed. Maybe use a relative path to a trivial requirements.txt to check if the chdir has effect, but even that is imo not necessary.

This is a test for the use case I have, therefore it would be great if it could be tested. The CI results are outdated AFAICS, I addressed them back then, I believe. The original issue happens when only chdir is set but env is not and this seems to make only sense when running "pip install ." but I might be missing something.

[ansibot]

The test ansible-test sanity --test shebang [explain] failed with 1 error:

test/integration/targets/pip/files/setup.py:1:1: unexpected non-module shebang: b'#!/usr/bin/python'

click here for bot help

[tyll]

@mattclay The failure on OS X should be fixed now, Shippable seems to take some time, so we will know later.

[robinro]

shipit

[robinro]

bot_status

[ansibot]

Components

lib/ansible/modules/packaging/language/pip.py
support: core
maintainers: lujeni mattupstate robinro

test/integration/targets/pip/files/ansible_test_pip_chdir/init.py
support: core
maintainers: lujeni mattupstate robinro

test/integration/targets/pip/files/setup.py
support: core
maintainers: lujeni mattupstate robinro

test/integration/targets/pip/tasks/pip.yml
support: core
maintainers: lujeni mattupstate robinro

Metadata

waiting_on: ansible
changes_requested_by: null
needs_info: False
needs_revision: False
needs_rebase: False
merge_commits: []
too many files or commits: False
mergeable_state: clean
shippable_status: success
maintainer_shipits (module maintainers): 1
community_shipits (namespace maintainers): 0
ansible_shipits (core team members): 0
shipit_actors (maintainers or core team members): robinro
shipit_actors_other: []
automerge: automerge shipit test failed

click here for bot help

[resmo]

shipit

[orenwolf]

any chance of getting this merged?

[GaboFDC]

Will this fix only be avaiable in 2.7, or is there any chance of it being back ported to 2.5 or 2.6?

[resmo]

it is in 2.7