Ensure virtualenv activates #10618

frewsxcv · 2016-04-16T04:49:02Z

Sign in to view

Someone should check to see if this breaks Linux/OSX paths with spaces

When I get rid of the explicit check for paths with spaces that prevents building, removing the quote does in fact give me:

Running virtualenv with interpreter /usr/local/bin/python2.7 New python executable in /Users/jdm/servo test spaces/servo/python/_virtualenv/bin/python2.7 Also creating executable in /Users/jdm/servo test spaces/servo/python/_virtualenv/bin/python Installing setuptools, pip...done. Pip failed to execute properly.

on OS X.

@zwn What happens if you keep the quoting here? How is it "counterproductive"?

I tried to explain the problem in #10595. The __file__ is used for os.paht.abspath. It expects unquoted path. I do not see how it could have ever worked. Let me know if I should add anything. I tested it on linux before the commit like this:

$ mkdir "test spaces" $ virtualenv test\ spaces/_virtualenv New python executable in test spaces/_virtualenv/bin/python Installing setuptools, pip...done. $ virtualenv --version 1.11.6 $ python --version Python 2.7.9

The problem with spaces seems to be more deeply ingrained. See http://lists.gnu.org/archive/html/bug-bash/2008-05/msg00053.html. The pip script contains shebang with full path to the python interpreter. This is interpreted directly by the kernel and apparently there is no way to escape the whitespace.

Python itself does not quote the path it sets to the __file__ variable so it should be ok not to quote it in execfile.

When putting the quote call there, the environment fails to activate properly. That means the following call to pip will not execute the pip from the virtualenv but the global one (for me that was /usr/bin/pip2). That is how you can end up with the requirements installed globally (for more discussion see #8968).

To support building in a directory with a spaces we would need to stop relying on shebang scripts.

@zwn I'm not super exited to take a fix that trades working with spaces in paths on OSX+Linux for working with spaces in paths on Windows. Can you do something per-platform in your changes?

In general, I would have preferred that we not support paths with spaces in them for the build environment on any platform (which is why I requested the addition of an error message to mach). We can get them somewhat working for Servo's Rust code proper, but there are a lot of pieces of code - especially native libraries we rely on from others - where building with spaces in the path is explicitly a non-goal and may never work out for us.

@larsbergstrom If that is the only concern, that is great! 😃 Because by merging this change we are not trading anything. The original code had one of two possible effects:

did nothing when no quoting was required

caused virtualenv not to activate when quoting was deemed necessary (on any platform)

Especially it did not help with the "space in the path" problem in any way. I've went into great lengths to better explain it in the issue #10595 and in the commit message.

Because by merging this change we are not trading anything.

Yes, we are. Previously:

✅ Works on OSX

❌ Doesn't work on Windows

With your changes:

❌ Doesn't work on OSX

✅ Works on Windows

As jdm demonstrated earlier, your changes break OSX. And to put the nail in the coffin, I tried this out myself:

diff --git a/python/mach_bootstrap.py b/python/mach_bootstrap.py index e77c78e..d143a1b 100644 --- a/python/mach_bootstrap.py +++ b/python/mach_bootstrap.py @@ -112,7 +112,7 @@ def _activate_virtualenv(topdir): except (subprocess.CalledProcessError, OSError): sys.exit("Python virtualenv failed to execute properly.") - execfile(activate_path, dict(__file__=quote(activate_path))) + execfile(activate_path, dict(__file__=activate_path)) # TODO: Right now, we iteratively install all the requirements by invoking # `pip install` each time. If it were the case that there were conflicting @@ -161,10 +161,10 @@ def bootstrap(topdir): # We don't support paths with spaces for now # https://github.com/servo/servo/issues/9442 - if ' ' in topdir: - print('Cannot run mach in a path with spaces.') - print('Current path:', topdir) - sys.exit(1) + #if ' ' in topdir: + # print('Cannot run mach in a path with spaces.') + # print('Current path:', topdir) + # sys.exit(1) # Ensure we are running Python 2.7+. We put this check here so we generate a # user-friendly error message rather than a cryptic stack trace on module

coreyf@frewbook-pro ~/D/r/f/servo (master)> ./mach build -d Pip failed to execute properly.

@zwn Do you have this library installed? https://virtualenv.pypa.io/en/latest/userguide.html#windows-notes

What you are seeing is the correct behavior. It was already broken however the problem was hidden. It appeared that it works because it called the globally installed pip and not the one from the virtualenv.

With the change I am proposing the virtualenv successfully activates but the pip script fails to execute due to the way exec call is defined. To quote myself

The problem with spaces seems to be more deeply ingrained. See http://lists.gnu.org/archive/html/bug-bash/2008-05/msg00053.html. The pip script contains shebang with full path to the python interpreter. This is interpreted directly by the kernel and apparently there is no way to escape the whitespace.

You can test it on a linux box too. This is what happens when you create virtualenv in a directory with a space in the name:

$ virtualenv 'env spaces' Running virtualenv with interpreter /usr/bin/python2 New python executable in env spaces/bin/python2 Also creating executable in env spaces/bin/python Installing setuptools, pip...done. $ . ./env\ spaces/bin/activate $ echo $PATH /tmp/env spaces/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/games:/usr/local/games $ which pip /tmp/env spaces/bin/pip $ pip bash: /tmp/env spaces/bin/pip: "/tmp/env: bad interpreter: No such file or directory $ cat env\ spaces/bin/pip #!"/tmp/env spaces/bin/python2" # -*- coding: utf-8 -*- import re import sys from pip import main if __name__ == '__main__': sys.argv[0] = re.sub(r'(-script\.pyw|\.exe)?$', '', sys.argv[0]) sys.exit(main())

Activating it differently just breaks the PATH so the wrong pip (=global) is found. I am not sure how to explain it differently 😞 It is not windows problem, it is quoting problem.

zwn · 2016-04-23T18:46:14Z

Sign in to view

@frewsxcv I appreciate you taking the time to test this. Could you please include these 3 lines in your test as well? It might shine some light on the problem.

@@ @@ -9,7 +9,6 @@ @@
     import subprocess
     import sys
     from distutils.spawn import find_executable
-    from pipes import quote
     SEARCH_PATHS = [
         os.path.join("python", "mach"),
@@ @@ -111,7 +110,11 @@ def _activate_virtualenv(topdir): @@
             except (subprocess.CalledProcessError, OSError):
                 sys.exit("Python virtualenv failed to execute properly.")
-        execfile(activate_path, dict(__file__=quote(activate_path)))
+        execfile(activate_path, dict(__file__=activate_path))
+        python = find_executable("python")
+        if python is None or not python.startswith(virtualenv_path):
+            sys.exit("Python virtualenv failed to activate.")
         # TODO: Right now, we iteratively install all the requirements by invoking
         # `pip install` each time. If it were the case that there were conflicting

servo / servo

Ensure virtualenv activates #10618

Always

Just for now

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.