Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upCommit that fixes the issue #11074 by upgrading pip whenever virtuale… #14452
Conversation
…nv is created.
highfive
commented
Dec 3, 2016
|
Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @jdm (or someone else) soon. |
highfive
commented
Dec 3, 2016
|
Heads up! This PR modifies the following files:
|
|
Feel free to reassign if this isn't your jam, @frewsxcv. |
| # https://github.com/servo/servo/issues/11074 | ||
| pip = _get_exec_path(PIP_NAMES, is_valid_path=check_exec_path) | ||
| if not pip: | ||
| sys.exit("Python pip is either not installed or not found in virtualenv.") |
This comment has been minimized.
This comment has been minimized.
frewsxcv
Dec 4, 2016
Member
At this point, we're already in a virtualenv right? Shouldn't this always have pip?
This comment has been minimized.
This comment has been minimized.
PeterZhizhin
Dec 4, 2016
Author
Contributor
At this point we have pip inside virtualenv. But the problem could be that it's version is too old.
To solve the issue, we update it.
This comment has been minimized.
This comment has been minimized.
frewsxcv
Dec 4, 2016
Member
Right, that part makes sense. I'm wondering why there is this if not pip: check on line 153 if pip is always included in a virtualenv? Not a big deal, just curious if I'm overlooking something.
This comment has been minimized.
This comment has been minimized.
PeterZhizhin
Dec 4, 2016
Author
Contributor
I don't really know. @broadcrawford added such check on Sep 18, 2015 for all package installations (you can check them in the code below). I just copy-pasted them.
|
@bors-servo r+ Thanks @PeterZhizhin! |
|
|
…r=frewsxcv Commit that fixes the issue #11074 by upgrading pip whenever virtuale… <!-- Please describe your changes on the following line: --> I have kind of resolved the issue #11074 by adding bool variable which is set to `True` if we had created the virtualenv and `False` otherwise. Then it updates pip by executing `pip install --upgrade pip` in the same way as packages are updated. I am a little bit worried that I have almost duplicated the installation routine from the `for` loop but I am not sure whether I should add a function or not. I think it is the best way of doing this because it does not need any Internet access for regular work (only for the first time you execute mach) as @larsbergstrom worried [here](#11149). It also doesn't add any extra latency on a no-op build. I have checked the solution inside a docker container based on debian wheezy. Before the patch `./mach` failed to run because it wasn't able to install some packages. Now it runs successfully. --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: --> - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #11074 (github issue number if applicable). <!-- Either: --> - [ ] There are tests for these changes OR - [X] These changes do not require tests because it changes only mach_bootstrap.py <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. --> <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14452) <!-- Reviewable:end -->
|
|
PeterZhizhin commentedDec 3, 2016
•
edited by larsbergstrom
I have kind of resolved the issue #11074 by adding bool variable which is set to
Trueif we had created the virtualenv andFalseotherwise.Then it updates pip by executing
pip install --upgrade pipin the same way as packages are updated. I am a little bit worried that I have almost duplicated the installation routine from theforloop but I am not sure whether I should add a function or not.I think it is the best way of doing this because it does not need any Internet access for regular work (only for the first time you execute mach) as @larsbergstrom worried here. It also doesn't add any extra latency on a no-op build.
I have checked the solution inside a docker container based on debian wheezy. Before the patch
./machfailed to run because it wasn't able to install some packages. Now it runs successfully../mach build -ddoes not report any errors./mach test-tidydoes not report any errorsThis change is