Fix test_issue_6833_pip_upgrade_pip test on OS X #36246
Conversation
| @@ -130,6 +131,9 @@ def create(path, | |||
| if venv_bin is None: | |||
| venv_bin = __opts__.get('venv_bin') or __pillar__.get('venv_bin') | |||
|
|
|||
| if not os.path.exists(venv_bin): | |||
| venv_bin = '/'.join([os.path.dirname(sys.executable), venv_bin]) | |||
There was a problem hiding this comment.
Could you clarify the need for this, please? I'm not sure under what circumstances we would have the path to the Python binary set but then be unable to find virtualenv that, according to this, would be in the same directory.
There was a problem hiding this comment.
Salt is unable to find the virtualenv binary on OSX because we don't use the system installation of Python and instead put our own build of python in /opt/salt... so you have to pass the full path to the virtualenv binary... just like with pip. salt-call wouldn't work, for example, without specifying the full path to the virtualenv binary. If you don't specify one, and virtualenv happens to be installed in the system installation of python, it will grab that one instead. The value of venv_bin before this was simply virtualenv with no path. Which it couldn't find on my machine because virtualenv is not installed in the default python installation. I can remove this and we can address the problem at a later date...
rallytime
added
the
Pending-Discussion
twangboy
changed the title
|
@cachedout I removed the virtualenv code. |
twangboy
added
bugfix-bckport
| @@ -361,7 +364,6 @@ def test_issue_6833_pip_upgrade_pip(self): | |||
| # previously installed | |||
| ret = self.run_function( | |||
| 'pip.install', ['pip==6.0'], upgrade=True, | |||
| ignore_installed=True, | |||
There was a problem hiding this comment.
Why was this removed?
There was a problem hiding this comment.
@terminalmage The ignore_installed option was creating a corrupted pip environment on OSX. Pip commands would stack trace. Removing this parameter fixed the problem.
There was a problem hiding this comment.
And was any consideration given to platforms other than OSX?
There was a problem hiding this comment.
I just don't think removing an argument altogether that has been part of this test for a while already on other platforms, just to get it passing on another, is a good move. Let's work in Slack to figure out what was happening and perhaps change this line to something like ignore_installed=True if grains['os'] not in ('MacOS',) else False
There was a problem hiding this comment.
@twangboy and I got to the bottom of this earlier today. When this test had failed in the past, the starting pip version has just been updated. But after taking a closer look, I found that the failure was being caused by stale .pyc files left behind. These were left in place by the ignore_installed=True as it prevents an uninstall from being done on a package if a new version is being installed.
I spoke with @s0undt3ch earlier and he could not recall the reason for using this argument initially. My best guess is that perhaps when the test was first written, pip was not capable of updating itself (i.e. the uninstall which pip performs before it installs the new version could have been breaking the update process). This, however, is only a guess.
TL;DR it's OK to remove the ignore_installed=True.
The test that runs these states is testing for behavior that was obsoleted by virtualenv 13.0. Ensure that we have older virtualenv available, and then create a venv with that older version. Use the 2nd virtualenv to attempt the "weird" install.
Ensure we have a test venv created using virtualenv < 13.0
rallytime
added
ZZZ[Done]-back-ported-bf
What does this PR do?
Fixes a problem with the test_issue_6833_pip_upgrade_pip test failure on OS X. Skips the
test_pip_installed_weird_installtest on OS X... can't duplicate the problem on OS X.What issues does this PR fix or reference?
https://github.com/saltstack/qa/issues/236
Previous Behavior
The
ignore_installedoption was corrupting the pip environment causing the followingpip.listfunction to fail.New Behavior
The
ignore_installedoption is not used (nor needed).Tests written?
NA