Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

pip should be updated when the virtualenv is first created #11074

Closed
autrilla opened this issue May 7, 2016 · 10 comments · Fixed by #14452
Closed

pip should be updated when the virtualenv is first created #11074

autrilla opened this issue May 7, 2016 · 10 comments · Fixed by #14452
Labels
A-build Related to or part of the build process C-has-open-pr There is a PR open that resolves the issue E-less-complex Straightforward. Recommended for a new contributor. L-python Python is required

Comments

@autrilla
Copy link
Contributor

autrilla commented May 7, 2016

Judging by several reports on IRC, people who have old versions of pip on their systems (OS X ships with 1.4.1, apparently) can't install mach 0.6. To solve this, we could simply update pip whenever we create the virtualenv.

@fitzgen fitzgen added E-less-complex Straightforward. Recommended for a new contributor. L-python Python is required A-build Related to or part of the build process labels May 8, 2016
@metajack
Copy link
Contributor

metajack commented May 8, 2016

Does that just replace the one in the virtualenv or hte system one? If the former, +1.

@autrilla
Copy link
Contributor Author

autrilla commented May 8, 2016

It updates the one in the virtualenv, as long as the virtualenv is active.

@campaul
Copy link
Contributor

campaul commented May 11, 2016

I'm interested in working on this. I think I've already got a working solution (on my system it upgrades pip from 7.1.2 to 8.1.2), but I want to verify what happens on a system that's old enough to fail installing mach before I submit a PR.

@campaul
Copy link
Contributor

campaul commented May 11, 2016

I spoke too soon. I tried on my mac and the upgrade command failed. The only difference I noticed between systems was on my mac_get_exec(*PIP_NAMES) is returning the system pip instead of the one in the virtualenv. I'm going to continue investigating.

@campaul
Copy link
Contributor

campaul commented May 11, 2016

I'm going to assume the system pip issue was just a problem with configuration on my machine. I forced it to use the correct path and was able to reproduce the failure with old pip and fix it with an upgrade. On that note though, would changing pip = _get_exec(*PIP_NAMES) to just pip = find_executable("pip") be a good idea? Since the virtualenv is already active by then it should always point to the correct pip executable and be more robust to strange system configurations like mine.

@autrilla
Copy link
Contributor Author

@campaul I'm not sure whether mach should be doing that, but it doesn't seem right to me. mach should always use the virtualenv and never use the system Python, especially considering it's installing packages, which it won't be able to do if it uses the system Python unless ran with root privileges, which will in turn break your system Python.

As long as the virtualenv is active, it should be fine to use find_executable, yes. I'm on the fence regarding using activate_this.py vs just calling pip, considering we control how the virtualenv is created and we know what names the pip executables will have. Considering most existing code relies on the virtualenv being active, I suggest we keep it that way for now.

@campaul
Copy link
Contributor

campaul commented May 11, 2016

The system pip issue I was having is actually #8968. I'll follow up in that ticket and make a PR that just upgrades pip for now.

@webmaven
Copy link

webmaven commented Jul 2, 2016

Is this issue now just a tracking issue? That is, is all the work that would close this bug already covered by other issues like #11149 and #8968?

@campaul
Copy link
Contributor

campaul commented Jul 3, 2016

#11149 is my PR that fixes this (sort of, it's still being tweaked). #8968 is a different but related issue.

@jdm jdm added the C-has-open-pr There is a PR open that resolves the issue label Jul 4, 2016
@KiChjang KiChjang added the C-assigned There is someone working on resolving the issue label Jul 5, 2016
@wafflespeanut wafflespeanut removed the C-assigned There is someone working on resolving the issue label Nov 5, 2016
@wafflespeanut
Copy link
Contributor

wafflespeanut commented Nov 5, 2016

No one's working on this right now. But, there's an open PR (#14253).

bors-servo pushed a commit that referenced this issue Dec 4, 2016
…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 -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-build Related to or part of the build process C-has-open-pr There is a PR open that resolves the issue E-less-complex Straightforward. Recommended for a new contributor. L-python Python is required
Projects
None yet
8 participants