Ensure virtualenv activates

[zwn]

Fixes #10595. For more info see #10595 or the commit message.

---

This change is

[highfive]

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

[highfive]

Heads up! This PR modifies the following files:

• @wafflespeanut: python/mach_bootstrap.py
• @KiChjang: components/script/dom/bindings/codegen/Configuration.py

[highfive]

Warning

• These commits modify script code, but no tests are modified. Please consider adding a test!

[Manishearth]

r? @larsbergstrom

[larsbergstrom]

Reviewed 2 of 3 files at r1.
Review status: 2 of 3 files reviewed at latest revision, 1 unresolved discussion.

---

README.md, line 99 [r1] (raw file):
If you are going to remove the steps that rename the existing binaries, please provide explicit instructions on how to set up the user's PATH so that it will always be correct - either in the form of a shell profile file modification that appends the correct Python directory to the front of the path or explicit shortcut modifications.

I'm also really, really surprised that you were able to get Servo to build at all from a normal MSYS shell window. The various python and shell scripts do not all detect environment variables correctly in that configuration. I will need to test this change myself on a clean machine to ensure that it really does work correctly, as there were a huge number of virtualenv-related problems with using an MSYS environment in the past, even with the correct MINGW64 python.

cc @vvuk

---

Comments from Reviewable

[larsbergstrom]

Reviewed 1 of 3 files at r1.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

---

Comments from Reviewable

[zwn]

The native windows python can be installed in a user selected directory. I am using simple modification of the PATH variable using my python install directory:

$ echo $PATH
/mingw64/bin:/usr/local/bin:/usr/bin:/bin:/c/Windows/System32:/c/Windows:/c/Windows/System32/Wbem:/c/Windows/System32/WindowsPowerShell/v1.0/:/usr/bin/site_perl:/usr/bin/vendor_perl:/usr/bin/core_perl
$ PATH=/c/software/Python27/:/c/software/Python27/Scripts/:$PATH
$ which python
/c/software/Python27/python
$ which virtualenv
/c/software/Python27/Scripts/virtualenv

It works for mach and mozjs as well. I am not sure how to write a script to detect the python installation directory and I also do not know what "explicit shortcut modifications" is. 😞

I have found no reason to rename the /mingw64/bin/python. If anything /usr/bin/python is in the PATH before. But I agree, it is a total mess.

One more thing - I call mach as python mach to have a bit of control.

I'd be really glad if someone else have tried it as it is all funky.

[zwn]

I can take the commit 7621d65 out of this PR and make separate one just for the python version issue. Would it make it easier to review?

[larsbergstrom]

Yes, definitely!

Sorry for the delay - I'm trying to get a Windows buildbot instance online, and that's taking up a lot of my testing time right now.

[frewsxcv]

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

[jdm]

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.

[frewsxcv]

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

[zwn]

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

[zwn]

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

[zwn]

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

[larsbergstrom]

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

[zwn]

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

[frewsxcv]

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

[zwn]

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]

I have started to split the commits to separate PRs. I am going to keep only 8fbd416 in this PR and force push. I'll do separate PR for @larsbergstrom and the README stuff. If this does not seem ok, please, feel free to stop me 😃

[zwn]

What else is needed to merge this? I have updated the issue #10595 to better reflect my understanding of the problem (I can copy it to the commit message if that is the custom here).

[zwn]

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

[larsbergstrom]

@bors-servo try

• Trying this, now that we have a windows buildbot

[bors-servo]

⌛ Trying commit 7002fbe with merge 306dd55...

[frewsxcv]

I don't think 'try'ing will do much since the issue at hand is related to spaces in path names, which I'm assuming isn't a factor on the builders? (at least on Linux)

[larsbergstrom]

@frewsxcv Based on the conversation thread above, it was not clear to me that this worked even on non-space-containing pathnames on all platforms.

We should perhaps have another conversation about this problem in general. I'm not really excited about ensuring our build environment supports spaces in the path name, as it feels like it will either be an endless bug farm or we'll have to spawn up extra per-platform build tests with spaces in the path to ensure it keeps working.

[bors-servo]

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt, windows

[jdm]

We need to make a decision here.

[metajack]

It sounds like Mac was working by accident, and spaces-based builds are disabled anyway. If this makes them work on windows and doesn't break non-spaces builds on OS X, we can toggle the check for spaces to be a check for spaces and sys.platform not in ('msys', 'win32').

I share Lars' concern that will will just uncover more weird bugs, but it seems harmless to try this and if it doesn't work we can go back to immediate failure in paths with spaces.

Did I miss anything? Are there any objections to landing this as I have described?

[larsbergstrom]

@bors-servo r+

[bors-servo]

📌 Commit 7002fbe has been approved by larsbergstrom

[frewsxcv]

Did @bors-servo decide not to run this? Is this still good to go?

[KiChjang]

@bors-servo try- clean r=larsbergstrom

[bors-servo]

💡 This pull request was already approved, no need to approve it again.

[bors-servo]

📌 Commit 7002fbe has been approved by larsbergstrom

[KiChjang]

@bors-servo r- try- clean force

[KiChjang]

@bors-servo r=larsbergstrom

[bors-servo]

📌 Commit 7002fbe has been approved by larsbergstrom

[KiChjang]

@zwn It looks like we'll need you to push a different commit hash onto your branch. Switch to your branch and do git commit --amend -C HEAD and then git push -f to push a new commit hash. This should reset the CI status on github.

[cbrewster]

@bors-servo retry

[bors-servo]

⌛ Testing commit 7002fbe with merge e7a55ae...

[bors-servo]

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt, windows