Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Actually activate the venv when building the wheelhouse #282
Conversation
| + def _pip(self, *args): | ||
| + # have to use bash to call pip to activate the venv properly first | ||
| + utils.Process(('bash', '-c', ' '.join( | ||
| + ('.', self._venv / 'bin' / 'activate', ';', 'pip3') + args |
mbruzek
Nov 14, 2016
Contributor
You initialize _venv to None. Do you need to check for none before doing this path creation?
johnsca
Nov 14, 2016
Member
It's an internal method that should only be called from call. I can add an assertion; it will blow up either way but maybe the error will be more useful.
| + ('virtualenv', '--python', 'python3', self._venv) | ||
| + ).exit_on_error()() | ||
| + self._pip('install', '-U', | ||
| + '"pip>=9.0.0,<10.0.0"', |
mbruzek
Nov 14, 2016
Contributor
Is version 9.x valid in all cases? I saw a PR come through recently where version 1.x was used to fix a bug on trusty. Do we need to carry that through here as well? Series determination I mean.
johnsca
Nov 14, 2016
•
Edited 1 time
-
johnsca
Nov 14, 2016
Member
-
Nov 14, 2016
Version 1.x is required in the install_requires because that has to match the pre-installed system pip in order for the command to launch. This is installing a newer version of pip into the venv, which is needed to be able to build the wheelhouse.
Edit: To clarify, the system-level pip (1.x) is required to bootstrap the process and is used to create the venv and install into the venv the newer version of pip which supports the download and wheel commands.
| - create_venv = venv is None | ||
| - venv = venv or path(tempfile.mkdtemp()) | ||
| - pip = venv / 'bin' / 'pip3' | ||
| + create_venv = self._venv is None |
mbruzek
Nov 14, 2016
Contributor
This method does not use the "venv" parameter in this version of the code. I would recommend removing or keeping the logic as it was previously.
|
I have reviewed this code and the logic looks good to me. +1 Thanks for addressing my comments! |
marcoceppi
merged commit bf840f5
into
master
Nov 14, 2016
marcoceppi
deleted the
issue-276
branch
Nov 14, 2016
added a commit
that referenced
this pull request
Nov 14, 2016
johnsca commentedNov 14, 2016
Description of change
Checklist
make test?Fixes #276
Also removes a deprecation warning by switching to
pip downloadandpins the pip version used in the venv to prevent unexpected major
version changes in the future.