Skip to content

/ servo

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

Ensure virtualenv activates #10618

Merged
merged 1 commit into from Jul 18, 2016
+5 −2
Merged

Ensure virtualenv activates #10618

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter...
Filter file types
Jump to…
Jump to file
Failed to load files.

Always

Just for now

Loading status checks…

Ensure virtualenv activates. 

There are two changes:
 * remove quoting which causes virtuaenv not activate
 * check virtualenv actually activated

If the quoting added in the fix in #8394 (4ff8d3a) kicks in, it
causes virtualenv to fail to activate.

For the common case it is no op:
```python
>>> from pipes import quote
>>> print quote('common/case')
common/case
```

When the path actually needs quoting, this is what happens:

```python
>>> print quote('test spaces')
'test spaces'
>>> print quote('windows\\path')
'windows\\path'
```

Note the embedded quotes.

Virtualenv in activate_this.py uses __file__ to build the path that
should be added to PATH:

```python
>>> print os.getcwd()
C:\software\git
>>> print os.path.abspath(quote('windows\\path'))
C:\software\git\'windows\path'
>>>
```

The constructed path is not valid. Adding it at the beginning of PATH
has no effect. This issue affects any case when the call to `quote`
kicks in.
  • Loading branch information
@zwn
zwn committed Apr 21, 2016
commit 7002fbedd73c4c240ea1e1593b1275b0df79c5d0
7 python/mach_bootstrap.py
View file
Open in desktop
@@ -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))

This comment has been minimized.

Sign in to view
@frewsxcv

frewsxcv Apr 16, 2016

Member

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

This comment has been minimized.

Sign in to view
@jdm

jdm Apr 16, 2016

Member

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.

This comment has been minimized.

Sign in to view
@frewsxcv

frewsxcv Apr 16, 2016

Member

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

This comment has been minimized.

Sign in to view
@zwn

zwn Apr 17, 2016

Author Contributor

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

This comment has been minimized.

Sign in to view
@zwn

zwn Apr 17, 2016

Author Contributor

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

This comment has been minimized.

Sign in to view
@zwn

zwn Apr 17, 2016

Author Contributor

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

This comment has been minimized.

Sign in to view
@larsbergstrom

larsbergstrom Apr 23, 2016

Contributor

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

This comment has been minimized.

Sign in to view
@zwn

zwn Apr 23, 2016

Author Contributor

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

This comment has been minimized.

Sign in to view
@frewsxcv

frewsxcv Apr 23, 2016

Member

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

This comment has been minimized.

Sign in to view
@zwn

zwn Apr 23, 2016

Author Contributor

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.

python = find_executable("python")
if python is None or not python.startswith(virtualenv_path):
sys.exit("Python virtualenv failed to activate.")

This comment has been minimized.

Sign in to view
@zwn

zwn Apr 23, 2016

Author Contributor

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

# TODO: Right now, we iteratively install all the requirements by invoking
# `pip install` each time. If it were the case that there were conflicting
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.