Skip to content

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

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.

Sign up for GitHub

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

Closed
wants to merge 3 commits into from
Closed

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

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0ccd827
provision: Give a clear error message when virtualenv is missing.
alenavolk Sep 27, 2017
cdb27f4
provision: Make sure the correct version of pip is installed.
alenavolk Sep 28, 2017
ec1ccd9
provision: Remove unnecessary imports.
alenavolk Sep 28, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 15 additions & 7 deletions tools/provision
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,6 @@
import os
import sys
import argparse
import pip
import six
import subprocess
import warnings

Expand All @@ -24,10 +22,18 @@ the Python version this command is executed with."""

venv_dir = os.path.join(base_dir, venv_name)
if not os.path.isdir(venv_dir):
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:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor

@rht rht Sep 28, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

@alenavolk alenavolk Sep 29, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

@alenavolk alenavolk Sep 29, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Collaborator Author

@alenavolk alenavolk Sep 29, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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]):
    ...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(my mistake :( )

return_code = subprocess.call(['virtualenv', '-p', python_interpreter, venv_dir])
except OSError:
if subprocess.call(['which', 'virtualenv']):
print("{red}Please install the virtualenv package and try again.{end_format}"
.format(red='\033[91m', end_format='\033[0m'))
sys.exit(1)
raise
else:
if return_code:
raise OSError("The command `virtualenv -p {} {}` failed. Virtualenv not created!"
.format(python_interpreter, venv_dir))
print("New virtualenv created.")
else:
print("Virtualenv already exists.")
Expand All @@ -51,8 +57,10 @@ the Python version this command is executed with."""
# the venv's Python interpreter. `--prefix venv_dir` ensures that all modules are installed
# in the right place.
def install_dependencies(requirements_filename):
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'])
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@rht rht Sep 29, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if subprocess.call([pip_path, 'install', '--prefix', venv_dir, '-r',
os.path.join(base_dir, requirements_filename)]):
raise OSError("The command `pip install -r {}` failed. Dependencies not installed!"
.format(os.path.join(base_dir, requirements_filename)))

Expand Down