[WIP-FIX] Remove the shell=True globally from Windows Python commands (pip bind, version check) #681
Conversation
lambdaclan
changed the title
|
Hooray! I don't have the time to properly look through this just now but
will do post-siggraph. Cheers!
A
…On Sun, Jul 28, 2019 at 9:26 PM Ira ***@***.***> wrote:
Description
[ Relates to merged PRs #659 <#659>
and #662 <#662> ]
Virtualenv used to install rez now correctly picks up the correct pip
version
Looking at the releases of rez <https:/nerdvegas/rez/releases>,
the changes in #659 <#659> came
almost at the same time as the rez installer updates from #662
<#662>. Some changes in the
installer seem to have fixed the issue where an improper pip was being
picked up during rez-bind of pip and pip version check (>=19 for wheels) -
more specifically
virtualenv has been updated to latest
The old virtualenv version
<https://virtualenv.pypa.io/en/latest/changes/#v1-11-6-2014-05-16> seems
to have been the root of the issue. rez installer on Windows seems to have
been using the old bundled pip version from the initialized virtualenv
(which defaulted to 1.5.6) then binding pip will cause rez-pip to use
version 1.5.6 instead of the pip version of the Python used to install rez
leading to all kinds of issues.
The new virtualenv_support_dirs
<a3d53ea#diff-a2a787942fb384ce1eac5c4ab769d8e2R471>
function seems to be the key according to my research. A quick look at this
thread <pypa/virtualenv#656> up on virtualenv's
repo states that
When you create a bootstrap script, it is essentially just a customised
version of virtualenv.py. But as noted in
https://virtualenv.pypa.io/en/latest/virtualenv.html (under
"Installation", at the end of that section, the blue note), any
virtualenv.py script (vanilla or customised via the bootstrap process) must
" include a virtualenv_support directory alongside virtualenv.py which
contains a complete set of pip and setuptools distributions".
So, wherever your bootstrap script is located, you need to create a
directory called virtualenv_support next to it, and put wheels for pip and
setuptools in there. These can either be from PyPI, or you can get the ones
from your virtualenv installation.
Since both the virtualenv bundled pip and the provided wheel of pip have
been updated with that PR now the old pip version 1.5.6 is no longer an
issue.
Breakdown
- Remove shell=True from the subprocess commands related to pip bind
and pip version check on Windows. This in turn improves the overall
security of shell=True is not recommended due to shell injection that can
lead to arbitrary command execution. See relevant section up on subprocess
documention
<https://docs.python.org/2/library/subprocess.html#frequently-used-arguments>
.
Type of change
- Bug fix (non-breaking change which fixes an issue)
How Has This Been Tested?
I have tried installing rez on Windows multiple times without admin
privileges and the right pip version gets detected. Right version implies
the same pip version as the one included in the Python
used to install rez.
Sample install (non admin)
installing rez to C:\Users\Joe\.rez...
# note the correct bundled pip version
['C:\\Users\\Joe\\Downloads\\rez\\src\\build_utils\\virtualenv\\virtualenv_support\\setuptools-41.0.1-py2.py3-none-any.whl', 'C:\\Users\\Joe\\Downloads\\rez\\src\\build_utils\\virtualenv\\virtualenv_support\\pip-19.1.1-py2.py3-none-any.whl']
running in C:\Users\Joe\Downloads\rez: c:\users\joe\.rez\scripts\python.exe -m pip -V
# note the correct pip version picked up from Python used to install rez
pip 19.1.1 from c:\users\joe\.rez\lib\site-packages\pip (python 2.7)
running in C:\Users\Joe\Downloads\rez: where pip
C:\Python27\Scripts\pip.exe
running in C:\Users\Joe\Downloads\rez: c:\users\joe\.rez\scripts\python.exe -m pip install .
DEPRECATION: Python 2.7 will reach the end of its life on January 1st, 2020. Please upgrade your Python as Python 2.7 won't be maintained after that date. A future version of pip will drop support for Python 2.7.
Processing c:\users\joe\downloads\rez
Building wheels for collected packages: rez
Building wheel for rez (setup.py) ... done
Stored in directory: c:\users\joe\appdata\local\temp\pip-ephem-wheel-cache-ok7pfx\wheels\fe\67\20\54f9154d310485e796588661c36912ff82dfac1784482210d7
Successfully built rez
Installing collected packages: rez
Successfully installed rez-2.38.2
WARNING: You are using pip version 19.1.1, however version 19.2.1 is available.
You should consider upgrading via the 'python -m pip install --upgrade pip' command.
SUCCESS! To activate Rez, add the following path to $PATH:
C:\Users\Joe\.rez\Scripts\rez
You may also want to source the relevant completion script from:
C:\Users\Joe\.rez\completion
The process cannot access the file because it is being used by another process.
Binding platform into C:\Users\Joe\packages...
Binding arch into C:\Users\Joe\packages...
Binding os into C:\Users\Joe\packages...
Binding python into C:\Users\Joe\packages...
Binding rez into C:\Users\Joe\packages...
Binding rezgui into C:\Users\Joe\packages...
Binding setuptools into C:\Users\Joe\packages...
Binding pip into C:\Users\Joe\packages...
Successfully converted the following software found on the current system into Rez packages:
PACKAGE URI
------- ---
arch C:\Users\Joe\packages\arch\AMD64\package.py
os C:\Users\Joe\packages\os\windows-10.0.17763\package.py
pip C:\Users\Joe\packages\pip\19.1.1\package.py # OK!!!
platform C:\Users\Joe\packages\platform\windows\package.py
python C:\Users\Joe\packages\python\2.7.16\package.py
rez C:\Users\Joe\packages\rez\2.38.2\package.py
rezgui C:\Users\Joe\packages\rezgui\2.38.2\package.py
setuptools C:\Users\Joe\packages\setuptools\41.0.1\package.py
Sample rez-pip install (non admin)
C:\Users\Joe
λ rez-pip -i lxml
20:25:27 INFO Using pip-19.1.1 (C:\Users\Joe\packages\pip\19.1.1\package.py[0])
DEPRECATION: Python 2.7 will reach the end of its life on January 1st, 2020. Please upgrade your Python as Python 2.7 won't be maintained after that date. A future version of pip will drop support for Python 2.7.
Collecting lxml
Downloading https://files.pythonhosted.org/packages/ad/55/c08f56e64b6e66349984029a38f3d9948eca90834ae0a195a2d5d7e6e0a0/lxml-4.4.0-cp27-cp27m-win_amd64.whl (3.6MB)
|UUUUUUUUUUUUUUUUUUUUUUUUUUUUUUUU| 3.6MB 819kB/s
Installing collected packages: lxml
Successfully installed lxml-4.4.0
WARNING: You are using pip version 19.1.1, however version 19.2.1 is available.
You should consider upgrading via the 'python -m pip install --upgrade pip' command.
20:25:39 DEBUG Removed 1.0 due to Maintainer
20:25:39 DEBUG Removed 1.1 due to Maintainer
20:25:39 DEBUG Removed 1.2 due to Provides-Extra
20:25:39 DEBUG Found c:\users\joe\appdata\local\temp\pip-iodwyh-rez\rez_staging\python\lxml-4.4.0.dist-info
1 packages were installed:
lxml-4.4.0: C:\Users\Joe\packages\lxml\4.4.0\package.py (platform-windows\arch-AMD64\python-2.7)
C:\Users\Joe
λ pip -V
pip 19.1.1 from c:\python27\lib\site-packages\pip (python 2.7)
C:\Users\Joe
*Test Configuration*:
- Python version: 2.7.16
- OS: Windows 10
- Toolchain: rez 2.38.2, pip 18.1, 19.1.1, 19.2.1
Changelog Point release
Source <https:/nerdvegas/rez/tree/2.38.1> | Diff
*Notes*
Add native not operator (!) support to the vendored VersionRange class.
Version requirements
can now be negated without prior conversion.
*Merged pull requests:*
- [Feature] Add native not operator (!) support to version ranges #XXX
<#670> (lambdaclan
<https:/lambdaclan>)
*Closed issues:*
- Schema - VersionRange does not support ! (not) operator #666
<#666>
Edit
I have detected what I believe to be an issue with descending order ranges
(both normal and negated does not work) therefore I will try to add the fix
to this PR.
>>> VersionRange("<=2.0.0,1.0.0+")
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "/packages/rez/2.38.1/platform-linux/arch-x86_64/os-Arch-rolling/rez/vendor/version/version.py", line 822, in __init__
% (range_str, str(e)))
rez.vendor.version.util.VersionError: Syntax error in version range '<=2.0.0,1.0.0+': Syntax error in version range '<=2.0.0,1.0.0+'
>>>
------------------------------
You can view, comment on, or merge this pull request online at:
#681
Commit Summary
- fix(rez-pip) remove the shell=True globally from Windows Python
commands
File Changes
- *M* src/rez/bind/_utils.py
<https:/nerdvegas/rez/pull/681/files#diff-0> (4)
- *M* src/rez/pip.py
<https:/nerdvegas/rez/pull/681/files#diff-1> (16)
Patch Links:
- https:/nerdvegas/rez/pull/681.patch
- https:/nerdvegas/rez/pull/681.diff
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#681>,
or mute the thread
<https:/notifications/unsubscribe-auth/AAMOUSRVNQBAI3RL7B7D5ITQBZWPDANCNFSM4IHOSTKA>
.
|
|
Hello Allan, happy to see you back from Siggraph and the repo picking up some traction! In terms of this PR after some research as you can see above, I came to the conclusion that for Windows we either go for security (Shell=False) but non pip consistency or vice versa. Without shell=True Windows fails to pick up the right path of Python pip and ends up using the bundled one, now that this >=19 is not a problem but might cause other issues with future updates of pip so we might need to downgrade/upgrade the bundled version which is a chore. The issue is the command Another thing we might consider doing is using the latest pip by default by modifying the installer ensuring we did a pip upgrade command during rez install. Of course the security issue might not be as big as we make it sounds so keeping shell=True might be better than modifying the installer. Thoughts? |
|
So does this mean that with shell=False, the rez-pip command could then end up ignoring |
|
So another thing you could do is test this with a proper rez pip package, rather than one created with rez-bind. Fortunately this works: I also just discovered a bug - it seems that |
|
I think I'm seeing the same issue on Linux now, and I think I can see why. Will look into this shortly, stay tuned. |
|
Hello Allan, thanks for looking into it. I am running the tests you mentioned above to see if I can find something as well. |
|
Hi @lambdaclan, do you mind testing on #667? There have been some changes that may address this issue. Thx |
|
Hello Allan, apologies for the delay. I have tried the changes in this branch on top of your pip improvements #667 branch and by the looks of it the issue still persists. I am adding a few screenshots for reference.
|
|
Hey @lambdaclan, I hate to ask, but could you revisit this after #667 has been merged? Things have changed a bit - specifically, the pip executable is no longer used - and I think this PR will have to be rewritten to take that into account. But from what you've said here, there may still be an issue on Windows that needs to be addressed even after that merge. There's also another small PR to follow #667 that will have to be done first, but I'll get on that ASAP. Newer python versions include pip now, so searching for a rezified pip should be done only for backwards compatibility (ie, this PR will test that pip is already provided by python, before attempting to find a rezified pip). Cheers! |
|
Hello Allan, Alright understood, that would be no problem at all. I will keep an eye on the repo and once #667 and the other small PR are merged I will try once more. Looking forward to trying out the positive changes that the PRs will bring since pip management has been a pain. I have a few things to keep me busy for now so once the PRs are complete I will get back into it. In the meantime If you need me to test anything out feel free to reach out. Thank you for the update 👍 |
|
@nerdvegas Hello Allan can you please re-open the issue, not sure what happened. I will try to set the shell to false again and see how it behaves now that many changes took place. |
|
Hmm I don't seem able to reopen it. GH says "There are no new commits on this branch" |
|
Testing on the latest master shows that regardless of the shell=True or False flag the bundled pip is being used. Edit: Same thing occurs on Linux, the bundled pip seems to always take precedence over the local pip of the Python used to install rez. Shell = True and Shell = False First of all a warning during install appears saying that our pip (19.1.1) is older than the currently available one (19.2.3). In this case pip 19.1.1 is the bundled pip, the python which was used to install rez actually has 19.2.1. The warning should be shown but the correct pip should be used for comparison.
Secondly, the pip package is set to the bundled pip regardless of the one being available being a newer version
Before the various mergred PRs, setting the shell=True would always pick the right pip version but now always the bundled pip seems to be selected. |
|
@nerdvegas I see maybe is because at current state is the same as master. I will push shell=False just so we are able to re-open it. Damn it, by the looks of it now the pushed commits are not detected since it is asking for a new PR... We can continue the discussion here and push relevant changes to another PR. |
Description
TLDR: The updated installer now uses a newer version of virtualenv that bundles newer versions of pip this means that on Windows shell=True is not needed since the bundled pip is >=19 and no longer the legacy 1.5.6. Saying that the installer on Windows always skips the system pip and uses
the bundled pip currently set at 19.1.1.
[ Relates to merged PRs #659 and #662 ]
Virtualenv used to install rez now uses a valid pip version (>=19)
Looking at the releases of rez, the changes in #659 came almost at the same time as the rez installer updates from #662. Some changes in the installer seem to have fixed the issue where an improper pip was being picked up during rez-bind of pip and pip version check (>=19 for wheels) - more specifically
The old virtualenv version seems to have been the root of the issue. rez installer on Windows seems to have been using the old bundled pip version from the initialized virtualenv (which defaulted to 1.5.6) then binding pip will cause rez-pip to use version 1.5.6 instead of the pip version of the Python used to install rez leading to all kinds of issues.
The new virtualenv_support_dirs function seems to be the key according to my research. A quick look at this thread up on virtualenv's repo states that
Since both the virtualenv bundled pip and the provided wheel of pip have been updated with that PR now the old pip version 1.5.6 is no longer an issue.
Important Remark - WIP part
On Linux the installer seems to always favour the pip version included in the Python used to install rez whereas on Windows it always seems to use the one in the support dir completely ignoring
the system one (see examples below). Since the bundled one is now >=19 it is not a problem in terms of using wheels but it might create confusion as system and rez-pip version start to diverge.
Using shell=True seems to emulate the same behaviour as in Linux so now we need to decide which one is more important, security or pip version consistency. I will look a bit further into this
by debugging the install on Linux and see if there are any differences in the support dir contents and add more info later. Hopefully there is a way to keep shell=False and pick the Python pip version.
Breakdown
Type of change
How Has This Been Tested?
I have tried installing rez on Windows multiple times without admin privileges and the right pip version gets detected. Right version normally implies the same pip version as the one included in the Python used to install rez but there are complications on Windows.
Sample install (non admin)
Sample rez-pip install (non admin)
Using an older version of pip still uses the one in the support dir >=19
Using a newer version of pip still uses the one in the support dir >=19
Keeping shell=True picks up pip from Python used to install rez rather than the bundled wheel
Test Configuration:
Update
During the package creation, the correct pip executable (matching the version of Python pip used to install rez) is detected but the one being binded ends up being the bundled one.
The issue seems to be more closely related with the binding process rather than the virtualenv generation
On Windows (Python pip is 19.2.1) - pip picked up from rez installation directory
On Linux (Python pip is 19.2.1) - pip picked up from Python install
Changelog
Point release
Source | Diff
Notes`