Gregory Szorc [:gps] Assignee
	Description • 12 years ago

The mach driver currently has its own hacky sys.path manipulation that essentially reproduces the virtualenv. It needs to integrate better. There should only be one canonical source for the virtulenv.

	Gregory Szorc [:gps] Assignee
	Comment 1 • 12 years ago

I'm giving this to a build peer to review because it touches packages.txt and I want a build peer to comment on the usefulness and future applicability.

There are two parts to this patch:

1) mach now automatically creates the virtualenv upon running
2) we have "activate" functionality similar to virtualenv's 'activate' script which modifies the current shell environment to enter a build environment.

Essentially, mach can now be used to bootstrap the current shell environment.

From a fresh tree:

  $ eval `mach activate`
  # This will both populate the virtualenv and set up PATH in the current shell

  $ python
  $ import Preprocessor
  # This works because PATH contains objdir/_virtualenv/...
  
  $ cd toolkit
  $ mach build
  # This works because PATH contains topsrcdir

In theory this will allow us to do cool stuff like:

  $ cd services/common/tests/unit
  $ mach xpcshell-test test_load_modules.js

This example doesn't work today because the xpcshell-test command kinda-sorta assumes PWD == topsrcdir. But, that can be changed with a follow-up bug.

	Gregory Szorc [:gps] Assignee
	Comment 2 • 12 years ago

Comment on attachment 675895 [details] [diff] [review]
Integration mach with virtualenv, v1

After thinking about this some more, I'd like to clean this up a bit before landing. Specifically, I want to ensure it works on Windows and have better support for other shells.

If you want, I can land just the automatic virtualenv population land and do the shell activation as a follow-up (it is a related but separate feature).

	Josh Matthews [:jdm]
	Comment 3 • 12 years ago

Greg, what's the status here? If we could get the virtualenv stuff landed without waiting for shell activation support, that might allow bug 597064 to be fixed quicker.

	Gregory Szorc [:gps] Assignee
	Comment 4 • 12 years ago

What does this have to do with bug 597064? The xpcshell runner itself will need to gain the timeout support because mach is just a frontend and isn't used on build infra.

As for this bug, activation and virtualenv integration go hand in hand. I have patches in various states of completion. But, FHR and moz.build refactorings are high priorities for me. I likely won't get around to this bug until the 2nd week of January at the earliest.

	Josh Matthews [:jdm]
	Comment 5 • 12 years ago

I'm assuming this bug is required for bug 597064 because the patch there makes various xpcshell make targets import a postprocessed python file in the build directory. I don't see any way to make the mach xpcshell command gain access to that path right now without this patch.

	Ed Morley [:emorley]
	Comment 6 • 11 years ago

(In reply to Gregory Szorc [:gps] from comment #4)
> I likely won't get around to this
> bug until the 2nd week of January at the earliest.

This bug is blocking bug 597064, which is in turn blocking bug 824022, which is a top 5 orange. 

I'd really like to avoid having to disable the tests in bug 824022 as there are so many of them -- any idea when you might be able to work on this some more? :-)

	Gregory Szorc [:gps] Assignee
	Comment 7 • 11 years ago

Again, I'm not sure how this bug is blocking anything. The virtualenv can contain paths in the object directory. mach isn't used by any production system.

	Ed Morley [:emorley]
	Comment 8 • 11 years ago

So you're fine with bug 597064 breaking mach with xpcshell short term?

	Jonathan Griffin (:jgriffin)
	Comment 9 • 11 years ago

This breaks bug 799308 (mach for Marionette).  Marionette tests use:

from marionette import MarionetteTestCase

which works when the Marionette package is installed, but doesn't work with the sys.path manipulation done by mach.  In order for that to work, the tests would need to:

from marionette_test import MarionetteTestCase

	Jonathan Griffin (:jgriffin)
	Comment 10 • 11 years ago

That should be *blocks* not *breaks*, since the command hasn't been landed yet.

	Jonathan Griffin (:jgriffin)
	Comment 11 • 11 years ago

(In reply to Jonathan Griffin (:jgriffin) from comment #9)
> This breaks bug 799308 (mach for Marionette).  Marionette tests use:
> 
> from marionette import MarionetteTestCase
> 
> which works when the Marionette package is installed, but doesn't work with
> the sys.path manipulation done by mach.  In order for that to work, the
> tests would need to:
> 
> from marionette_test import MarionetteTestCase

Nm, I'm way too tired, this should be fixable in another way.

	Gregory Szorc [:gps] Assignee
	Comment 12 • 11 years ago

This bug/feature is in a weird state. Interaction between the virtualenv and mach is going to be funky for the short to medium term. IMO we should continue to add sys.path hacks to mach until a more unified robust solution is available. Please don't block on this bug.

	Jeff Hammel
	Comment 13 • 11 years ago

Status?  Viewing http://mxr.mozilla.org/mozilla-central/search?string=mozbase depresses me in the number of ways we import things.  It'd be nice to know where/how to add things.  

This came up with bug 688667 ; I was surprised to find that the example given on https://developer.mozilla.org/en-US/docs/Mochitest did not work with the virtualenv(!):

"""
│./mach mochitest-plain content/base/test/test_CrossSiteXHR.html
Error running mach:

    ['mochitest-plain', 'content/base/test/test_CrossSiteXHR.html']

The error occurred in code that was called by the mach command. This is either
a bug in the called code itself or in the way that mach is calling it.

You should consider filing a bug for this issue.

If filing a bug, please include the full output of mach, including this error
message.

The details of the failure are as follows:

ImportError: No module named manifestparser

  File "/home/jhammel/mozilla/src/mozilla-central/testing/mochitest/mach_commands.py", line 268, in run_mochitest_plain
    return self.run_mochitest(test_file, 'plain', **kwargs)
  File "/home/jhammel/mozilla/src/mozilla-central/testing/mochitest/mach_commands.py", line 299, in run_mochitest
    **kwargs)
  File "/home/jhammel/mozilla/src/mozilla-central/testing/mochitest/mach_commands.py", line 85, in run_mochitest_test
    from automation import Automation
  File "/home/jhammel/mozilla/src/mozilla-central/../obj-browser/build/automation.py", line 34, in <module>
    from mozprofile import Profile, Preferences
  File "/home/jhammel/mozilla/src/mozilla-central/testing/mozbase/mozprofile/mozprofile/__init__.py", line 13, in <module>
    from profile import *
  File "/home/jhammel/mozilla/src/mozilla-central/testing/mozbase/mozprofile/mozprofile/profile.py", line 12, in <module>
    from addons import AddonManager
  File "/home/jhammel/mozilla/src/mozilla-central/testing/mozbase/mozprofile/mozprofile/addons.py", line 11, in <module>
    from manifestparser import ManifestParser
│./mach mochitest-plain content/base/test/test_CrossSiteXHR.html
"""

	Gregory Szorc [:gps] Assignee
	Comment 14 • 11 years ago

This is a largely mechanical move. I want the VirtualenvManager class
importable from mozbuild so we can use it as part of mach commands and
other parts of the build. As future patches will show, I want to hang
off some additional APIs, such as install_pip_package().

It might be worth waiting for the followup parts before you perform
review.

	Gregory Szorc [:gps] Assignee
	Comment 15 • 11 years ago

The path to the virtualenv_packages.txt file was hardcoded in
populate_virtualenv.py. Now that virtualenv.py is a standalone module,
this doesn't make much sense.

I'm not sure if this patch is strictly required. But it doesn't hurt
IMO.

	Gregory Szorc [:gps] Assignee
	Comment 16 • 11 years ago

This patch adds some useful virtualenv APIs.

I have added an activate_virtualenv() API to MozbuildObject(). When this
function is called, the virtualenv is activated in the current Python
environment. If the virtualenv doesn't exist, it is created. If it isn't
up-to-date, it is updated. It "just works." It's nice.

I've also added an API to virtualenv.py to install packages via pip. I
will be using this API in bug 917988 to install sphinx in the local
virtualenv. I figured I might as well make that change here.

This patch should make your spine tingle a little bit. We're talking
directly to the pip APIs. I don't believe there are any stability
guarantees for the pip APIs. But, we always use the in-tree pip via
virtualenv, so as long as we pay attention during virtualenv upgrades,
we should be fine.

	Gregory Szorc [:gps] Assignee
	Comment 17 • 11 years ago

Oops - forgot to remove the setting of sys.executable, which was stupid
in the first place.

	Gregory Szorc [:gps] Assignee
	Comment 18 • 11 years ago

Comment on attachment 806931 [details] [diff] [review]
Part 2: Add virtualenv APIs

Erik Rose dabbles in witchcraft^Wpython packaging and might be able to stamp approval on the pip bits.

	Gregory Szorc [:gps] Assignee
	Comment 19 • 11 years ago

OK. I think I'm done with this bug. I stopped short of fixing the sys.path munging in build/mach_bootstrap.py. There is a serious chicken-and-egg problem there and I'm going to punt that to a followup bug.

If you look at the patch from bug 917988, you can see that this patch set introduces potential awesomeness into the tree. mach commands can now install packages from pip into a virtualenv with one line of code. Want a mach command that relies on a package not in the tree? Just install it inside the mach command! There's a lot that can be done with this power.

	Erik Rose [:erik][:erikrose]
	Comment 20 • 11 years ago

One nice thing is that you don't have to implement [as much of] bin_path() or python_path() yourself. virtualenv.path_locations gives you almost everything you need. I had to do something very similar in shiva: https://github.com/erikrose/shiva/blob/fast-burn/shiva_deployer/commandline.py#L107.

	Erik Rose [:erik][:erikrose]
	Comment 21 • 11 years ago

As for install_pip_package(), did you know that InstallRequirement has a check_if_exists() method? That saves you comparing a bunch of package names yourself, and it takes version into consideration, which your code doesn't yet. Also, it's quite easy to call pip without spawning a new process, so you can get normal exception percolation: https://github.com/erikrose/peep/blob/master/peep/__init__.py#L86.

	Erik Rose [:erik][:erikrose]
	Comment 22 • 11 years ago

Btw, w.r.t. comparing package names: I believe they are case-sensitive. At least PyPI treats them as such. And that's all the feedback I have. :-) Cheers!

	Gregory Szorc [:gps] Assignee
	Comment 23 • 11 years ago

Erik: That's awesome feedback that we could have only gotten from someone who has survived the bowels of Python's packaging code. Thank you so much!

	Gregory Szorc [:gps] Assignee
	Comment 24 • 11 years ago

Incorporated Erik's comments. Some of them were rejected for technical
reasons now explained as code comments.

	Erik Rose [:erik][:erikrose]
	Comment 25 • 11 years ago

Wow, we are writing the *same thing* and almost at the same time. Maybe we should get them together at some point, though I'm not sure how hard dependencies are in your situation. Here's my "run it in a virtualenv" method: https://github.com/erikrose/shiva/blob/fast-burn/shiva_deployer/commandline.py#L110.

Your code looks great to me now, btw.

	(not currently active) Ted Mielczarek
	Comment 26 • 11 years ago

Comment on attachment 806837 [details] [diff] [review]
Part 1b: Make virtualenv.py standalone

Review of attachment 806837 [details] [diff] [review]:
-----------------------------------------------------------------

::: python/mozbuild/mozbuild/virtualenv.py
@@ +413,5 @@
>  
>      topsrcdir = sys.argv[1]
>      topobjdir = sys.argv[2]
>      virtualenv_path = sys.argv[3]
> +    manifest_path = sys.argv[4]

Maybe just smash these into a destructuring assignment on args[:4]?

	Gregory Szorc [:gps] Assignee
	Comment 27 • 11 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/c678ea1db9c9
https://hg.mozilla.org/integration/mozilla-inbound/rev/392068d519fb

	Phil Ringnalda (:philor)
	Comment 28 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/c678ea1db9c9
https://hg.mozilla.org/mozilla-central/rev/392068d519fb

	Dustin J. Mitchell [:dustin] (he/him)
	Comment 29 • 8 years ago

This bug is still mentioned in the source tree:

 55 # TODO Bug 794506 Integrate with the in-tree virtualenv configuration.
 56 SEARCH_PATHS = [

and it's worth noting that the given SEARCH PATHS are not, in fact, available in the in-tree virtualenv.