Upgrade pip after virtualenv is created #11149

campaul · 2016-05-12T04:35:54Z

./mach build does not report any errors
./mach test-tidy does not report any errors
These changes fix pip should be updated when the virtualenv is first created #11074 (github issue number if applicable).

Either:

There are tests for these changes OR
These changes do not require tests because they execute before the test runner is installed. I would actually like to test them but I didn't find any system in place for testing the bootstraping code.

This change is

highfive · 2016-05-12T04:35:59Z

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @larsbergstrom (or someone else) soon.

highfive · 2016-05-12T04:35:59Z

Heads up! This PR modifies the following files:

@wafflespeanut: python/mach_bootstrap.py

larsbergstrom · 2016-05-27T19:20:08Z

@campaul What problem is this addressing? Does an out of date pip cause problems? I'm a little worried about what will happen here if we're building and not connected to the network or even just that it might cause extra latency on a no-op build as it goes out to check for a new version of pip.

metajack · 2016-05-27T19:24:08Z

One alternative is add instructions to upgrade pip to the README.md.

larsbergstrom · 2016-05-27T19:33:20Z

@metajack Yeah, I think I'd prefer that.

@campaul Could you please make that change?

campaul · 2016-06-09T13:54:47Z

@larsbergstrom the build crashes if pip is out of date because pip fails to install mach. Manually updating pip right now is really cumbersome because you have to run mach, let it crash, then activate the virtualenv it created and then run the pip upgrade.

What I would like to do instead of simply adding those instructions to the README is add a build target like mach upgrade-pip or something similar so that manually upgrading is easy. Then, in the place I'm currently upgrading pip, add a check for the version and warn the user they might need to run mach upgrade-pip if we suspect the build is going to crash. Does that sound reasonable?

larsbergstrom · 2016-06-09T13:55:56Z

@campaul I like that plan - sounds perfect!

larsbergstrom · 2016-06-29T00:21:36Z

@campaul Are you still planning on doing this? Sorry if we're asking for too much work here - let me know if there's anything we can do to clarify or help!

campaul · 2016-06-29T14:13:11Z

My proposed solution doesn't work as well as I imagined. The crash occurs during bootstraping, which is before args are parsed, so adding an upgrade-pip arg would involve manually parsing the args during bootstrap for that one special case.

At this point I want to go back to the strategy of asking the user to upgrade manually, but instead of asking them to upgrade pip ask them to upgrade virtualenv and exit before the virtualenv is even created. This completely avoids the problem of having a partially initialized virtualenv. I just need to go find what version of virtualenv started bundling a new enough pip so I know what version to check for.

campaul · 2016-08-04T02:56:09Z

By trial and error I found that virtualenv 14.0.0 was the oldest version that works fine on Mac OS and Linux, so I changed the branch to check for older versions and exit with a message to upgrade. This happens before the the virtualenv is created so you're never left in a broken state.

It would be more robust to use the parse_version function from setuptools, but that isn't guaranteed to be installed.

campaul · 2016-08-04T03:03:42Z

And now I'm really confused. CI fails because my version check detects that virtualenv is too old, but clearly Travis is able to build master without issue so it must not be too old.

larsbergstrom · 2016-08-19T21:03:19Z

@campaul Can you please change this to print out the version that was found?

Sorry for the horrible delay in review, btw! Let me know if you want me to carry this through to landing!

campaul · 2016-08-23T19:21:54Z

I'm fine continuing work on this. I'll make it print the version it found, but I also need to figure out how Travis CI manages to build with a version that's too old to work on my machine.

bors-servo · 2016-09-01T12:51:45Z

☔ The latest upstream changes (presumably #13151) made this pull request unmergeable. Please resolve the merge conflicts.

wafflespeanut · 2016-11-04T07:18:04Z

Is this still being worked on?

campaul · 2016-11-04T23:13:06Z

Not at the moment. I never figured out a solution to the issues I mentioned and haven't had the time to investigate recently.

wafflespeanut · 2016-11-17T09:05:37Z

superseded by #14253

@larsbergstrom

…r=frewsxcv Commit that fixes the issue #11074 by upgrading pip whenever virtuale…  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. ---  - [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).  - [ ] There are tests for these changes OR - [X] These changes do not require tests because it changes only mach_bootstrap.py   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14452)

@larsbergstrom

… pip whenever virtuale… (from PeterZhizhin:upgrade-pip-with-new-virtualenv); r=frewsxcv  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](servo/servo#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. ---  - [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).  - [ ] There are tests for these changes OR - [X] These changes do not require tests because it changes only mach_bootstrap.py  Source-Repo: https://github.com/servo/servo Source-Revision: ea59e7bb68e83ac2631dcdad24f270bff68092eb

… pip whenever virtuale… (from PeterZhizhin:upgrade-pip-with-new-virtualenv); r=frewsxcv  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](servo/servo#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. ---  - [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).  - [ ] There are tests for these changes OR - [X] These changes do not require tests because it changes only mach_bootstrap.py  Source-Repo: https://github.com/servo/servo Source-Revision: ea59e7bb68e83ac2631dcdad24f270bff68092eb UltraBlame original commit: ca239fcfa931c379dfc327af16e37358379f0fda

highfive assigned larsbergstrom May 12, 2016

highfive added the S-awaiting-review There is new code that needs to be reviewed. label May 12, 2016

larsbergstrom added S-awaiting-answer Someone asked a question that requires an answer. and removed S-awaiting-review There is new code that needs to be reviewed. labels May 27, 2016

larsbergstrom added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. and removed S-awaiting-answer Someone asked a question that requires an answer. labels May 27, 2016

webmaven mentioned this pull request Jul 2, 2016

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

Closed

Exit if pip version is detected to be too old.

5aa09e8

campaul force-pushed the upgrade-pip branch from 9dd8e42 to 5aa09e8 Compare August 4, 2016 02:49

highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-needs-code-changes Changes have not yet been made that were requested by a reviewer. labels Aug 4, 2016

larsbergstrom added S-awaiting-answer Someone asked a question that requires an answer. and removed S-awaiting-review There is new code that needs to be reviewed. labels Aug 19, 2016

highfive added the S-needs-rebase There are merge conflict errors. label Sep 1, 2016

metajack mentioned this pull request Nov 16, 2016

[WIP] Exit if pip version is detected to be too old. #14253

Closed

5 tasks

wafflespeanut closed this Nov 17, 2016

PeterZhizhin mentioned this pull request Dec 3, 2016

Commit that fixes the issue #11074 by upgrading pip whenever virtuale… #14452

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade pip after virtualenv is created #11149

Upgrade pip after virtualenv is created #11149

campaul commented May 12, 2016 •

edited by larsbergstrom

Loading

highfive commented May 12, 2016

highfive commented May 12, 2016

larsbergstrom commented May 27, 2016

metajack commented May 27, 2016

larsbergstrom commented May 27, 2016

campaul commented Jun 9, 2016

larsbergstrom commented Jun 9, 2016

larsbergstrom commented Jun 29, 2016

campaul commented Jun 29, 2016

campaul commented Aug 4, 2016

campaul commented Aug 4, 2016

larsbergstrom commented Aug 19, 2016

campaul commented Aug 23, 2016

bors-servo commented Sep 1, 2016

wafflespeanut commented Nov 4, 2016

campaul commented Nov 4, 2016

wafflespeanut commented Nov 17, 2016

Upgrade pip after virtualenv is created #11149

Upgrade pip after virtualenv is created #11149

Conversation

campaul commented May 12, 2016 • edited by larsbergstrom Loading

highfive commented May 12, 2016

highfive commented May 12, 2016

larsbergstrom commented May 27, 2016

metajack commented May 27, 2016

larsbergstrom commented May 27, 2016

campaul commented Jun 9, 2016

larsbergstrom commented Jun 9, 2016

larsbergstrom commented Jun 29, 2016

campaul commented Jun 29, 2016

campaul commented Aug 4, 2016

campaul commented Aug 4, 2016

larsbergstrom commented Aug 19, 2016

campaul commented Aug 23, 2016

bors-servo commented Sep 1, 2016

wafflespeanut commented Nov 4, 2016

campaul commented Nov 4, 2016

wafflespeanut commented Nov 17, 2016

campaul commented May 12, 2016 •

edited by larsbergstrom

Loading