provision: Give a better error message; ensure the correct version of pip. #128

alenavolk · 2017-09-27T22:00:22Z

Right now when the virtualenv package is not installed the related subprocess call results in an error with the following traceback:

Traceback (most recent call last):
  File "./tools/provision", line 68, in <module>
    main()
  File "./tools/provision", line 25, in main
    if subprocess.call(['virtualenv', '-p', python_interpreter, venv_dir]):
  File "/usr/lib/python2.7/subprocess.py", line 522, in call
    return Popen(*popenargs, **kwargs).wait()
  File "/usr/lib/python2.7/subprocess.py", line 710, in __init__
    errread, errwrite)
  File "/usr/lib/python2.7/subprocess.py", line 1327, in _execute_child
    raise child_exception
OSError: [Errno 2] No such file or directory

I think it would be great to have a more beginner-friendly error message for this easy-to-fix (and potentially frequent) case.

Without this tweak, running the script results in a vague "No such file or directory" error if the virtualenv package is not installed.

rht · 2017-09-28T02:12:32Z

tools/provision

-        if subprocess.call(['virtualenv', '-p', python_interpreter, venv_dir]):
-            raise OSError("The command `virtualenv -p {} {}` failed. Virtualenv not created!"
-                          .format(python_interpreter, venv_dir))
+        try:


Rather than try-except, there is a way to detect specifically whether virtualenv has been installed. Ref: https://stackoverflow.com/questions/1871549/python-determine-if-running-inside-virtualenv/1883251#1883251.

Sounds great, I'll look into that. Thanks for the feedback!

Unfortunately, I don't see how this can be applied here. This is a good way to check whether the code is running inside a virtual environment. What would be helpful in this case is a way to check whether the virtualenv package is installed (before using it to create an environment).

Here is how it is checked in one of the tool in the Zulip webapp/server:
https://github.com/zulip/zulip/blob/master/tools/update-locked-requirements#L4-L7
If the venv_dir is known, a check along this line can be done.

This is already being done one line above:

if not os.path.isdir(venv_dir): ...

But what if the environment doesn't exist? We run the following code to create it:

subprocess.call(['virtualenv', '-p', python_interpreter, venv_dir])

To do it successfully we need to have the virtualenv package installed. On Ubuntu it can be done by running this command in the terminal:

sudo apt-get install python-virtualenv

If we make the subprocess call before the virtualenv package is installed the script will exit with an error and print the traceback I included above.

What I'm trying to do here is to replace this traceback with an error message that tells people exactly what's going on: they need to install the package to run the script.

I agree with the idea of checking if virtualenv is installed as the first step, makes the logic easier. Instead of which virtualenv, I suggest checking the exit code of command -v virtualenv, since its a shell built-in and part of posix.

@rht @derAnfaenger Thanks for the suggestions! I agree that checking if the virtualenv package is installed before using it makes more sense than a try-except block. I'll make use of command -v virtualenv too.

Hm, the subprocess call fails when I use command -v virtualenv:

Traceback (most recent call last): File "./tools/provision", line 76, in <module> main() File "./tools/provision", line 26, in main if subprocess.call(['command', '-v', 'virtualenv']): File "/usr/lib/python2.7/subprocess.py", line 522, in call return Popen(*popenargs, **kwargs).wait() File "/usr/lib/python2.7/subprocess.py", line 710, in __init__ errread, errwrite) File "/usr/lib/python2.7/subprocess.py", line 1327, in _execute_child raise child_exception OSError: [Errno 2] No such file or directory

Should I keep using which virtualenv or wrap it in a try-except block, perhaps?

try: subprocess.call(['command', '-v', 'virtualenv']) except OSError: print("{red}Please install the virtualenv package and try again.{end_format}" .format(red='\033[91m', end_format='\033[0m')) sys.exit(1) if subprocess.call(['virtualenv', '-p', python_interpreter, venv_dir]): ...

This discussion had a lot of suggestions that didn't actually work. @rht next time, I'd encourage you to read the code more closely before commenting; e.g. https://stackoverflow.com/questions/1871549/python-determine-if-running-inside-virtualenv/1883251#1883251 was totally irrelevant to the problem at hand :)

(my mistake :( )

alenavolk · 2017-09-28T19:17:25Z

Another little tweak I suggest is to ensure that pip inside the environment has a version 8.0.0 or higher before using it. The older versions of pip don't have the prefix option, which causes the script to exit with an error.

I used a clean Ubuntu 14.04 (a pretty common setup I think) and tried installing the virtualenv package several different ways, but the version of pip inside the environment it created was always as low as 1.5.4 by default.

rht · 2017-09-28T22:48:57Z

Hmm, according to http://www.python3statement.org/practicalities/, the safe recommended version of pip SHOULD be pip>=9.0.

alenavolk · 2017-09-29T01:52:37Z

@rht This is a good point, thanks! I'll update the commit.

pip 8.0+ is required to successfully run the script (otherwise, the prefix option doesn't work). pip 9.0+ is installed because of the safety features.

roberthoenig

Thanks for working on this @alenavolk ! Left two comments - the rest looks good to me.

roberthoenig · 2017-09-29T07:39:41Z

tools/provision

-        if subprocess.call([os.path.join(venv_dir, venv_exec_dir, 'pip'),
-                           'install', '--prefix', venv_dir, '-r', os.path.join(base_dir, requirements_filename)]):
+        pip_path = os.path.join(venv_dir, venv_exec_dir, 'pip')
+        subprocess.call([pip_path, 'install', 'pip>=9.0'])


Instead of making another call, could we add pip>=9.0 to requirements.txt?

The pip must be updated first so that the >=9.0 version of it can take effect when installing the packages.

roberthoenig · 2017-09-29T07:53:14Z

tools/provision

-        if subprocess.call(['virtualenv', '-p', python_interpreter, venv_dir]):
-            raise OSError("The command `virtualenv -p {} {}` failed. Virtualenv not created!"
-                          .format(python_interpreter, venv_dir))
+        try:


I agree with the idea of checking if virtualenv is installed as the first step, makes the logic easier. Instead of which virtualenv, I suggest checking the exit code of command -v virtualenv, since its a shell built-in and part of posix.

zulipbot · 2017-09-29T23:01:14Z

Heads up @alenavolk, we just merged some commits that conflict with the changes your made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the upstream/master branch and resolve your pull request's merge conflicts accordingly.

timabbott · 2017-09-29T23:01:28Z

Awesome, thanks for doing this @alenavolk! This looks great, I merged this, with one tweak to add a comment explaining why we do the pip upgrade. Thanks again for doing this!

timabbott · 2017-09-29T23:01:40Z

Let us know if you're looking for a next project! There's lots to do.

alenavolk · 2017-09-29T23:44:40Z

@timabbott Awesome! I was planning to get busy with these tasks in the main Zulip repo, then work on this issue. If there is something more urgent I could do, let me know :)

timabbott · 2017-09-30T00:31:49Z

Sounds great!

provision: Give a clear error message when virtualenv is missing.

0ccd827

Without this tweak, running the script results in a vague "No such file or directory" error if the virtualenv package is not installed.

zulipbot added needs review reviewed and removed needs review labels Sep 27, 2017

rht reviewed Sep 28, 2017

View reviewed changes

zulipbot added needs review and removed reviewed labels Sep 28, 2017

alenavolk changed the title ~~provision: Give a clear error message when virtualenv is missing.~~ provision: Give a better error message; ensure the correct version of pip. Sep 28, 2017

zulipbot added reviewed and removed needs review labels Sep 28, 2017

alenavolk force-pushed the provision-script branch from d0d8c24 to ce3ff50 Compare September 29, 2017 02:45

zulipbot added needs review and removed reviewed labels Sep 29, 2017

alenavolk added 2 commits September 28, 2017 22:57

provision: Make sure the correct version of pip is installed.

cdb27f4

pip 8.0+ is required to successfully run the script (otherwise, the prefix option doesn't work). pip 9.0+ is installed because of the safety features.

provision: Remove unnecessary imports.

ec1ccd9

alenavolk force-pushed the provision-script branch from ce3ff50 to ec1ccd9 Compare September 29, 2017 03:01

zulipbot added reviewed and removed needs review labels Sep 29, 2017

roberthoenig reviewed Sep 29, 2017

View reviewed changes

timabbott closed this Sep 29, 2017

zulipbot removed the reviewed label Sep 29, 2017

alenavolk deleted the provision-script branch September 29, 2017 23:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

provision: Give a better error message; ensure the correct version of pip. #128

provision: Give a better error message; ensure the correct version of pip. #128

alenavolk commented Sep 27, 2017 •

edited

Loading

rht Sep 28, 2017

alenavolk Sep 28, 2017

alenavolk Sep 28, 2017

rht Sep 28, 2017 •

edited

Loading

alenavolk Sep 29, 2017 •

edited

Loading

roberthoenig Sep 29, 2017

alenavolk Sep 29, 2017 •

edited

Loading

alenavolk Sep 29, 2017 •

edited

Loading

timabbott Sep 29, 2017

rht Sep 29, 2017

alenavolk commented Sep 28, 2017 •

edited

Loading

rht commented Sep 28, 2017

alenavolk commented Sep 29, 2017 •

edited

Loading

roberthoenig left a comment

roberthoenig Sep 29, 2017

rht Sep 29, 2017 •

edited

Loading

roberthoenig Sep 29, 2017

zulipbot commented Sep 29, 2017

timabbott commented Sep 29, 2017

timabbott commented Sep 29, 2017

alenavolk commented Sep 29, 2017

timabbott commented Sep 30, 2017

provision: Give a better error message; ensure the correct version of pip. #128

provision: Give a better error message; ensure the correct version of pip. #128

Conversation

alenavolk commented Sep 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rht Sep 28, 2017 • edited Loading

Choose a reason for hiding this comment

alenavolk Sep 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alenavolk Sep 29, 2017 • edited Loading

Choose a reason for hiding this comment

alenavolk Sep 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alenavolk commented Sep 28, 2017 • edited Loading

rht commented Sep 28, 2017

alenavolk commented Sep 29, 2017 • edited Loading

roberthoenig left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rht Sep 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zulipbot commented Sep 29, 2017

timabbott commented Sep 29, 2017

timabbott commented Sep 29, 2017

alenavolk commented Sep 29, 2017

timabbott commented Sep 30, 2017

alenavolk commented Sep 27, 2017 •

edited

Loading

rht Sep 28, 2017 •

edited

Loading

alenavolk Sep 29, 2017 •

edited

Loading

alenavolk Sep 29, 2017 •

edited

Loading

alenavolk Sep 29, 2017 •

edited

Loading

alenavolk commented Sep 28, 2017 •

edited

Loading

alenavolk commented Sep 29, 2017 •

edited

Loading

rht Sep 29, 2017 •

edited

Loading