802210 - Change how virtualenv is created, populated

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Gregory Szorc [:gps]]

Per IRC discussions, we want to change how the virtualenv is created and populated to make it easier to use from other tools (like mach) and so it can be used *before* configure is executed.

Essentially:

* Remove the CC/CFLAGS, etc variables from virtualenv population. The reasoning is Python does a good job on its own detecting these things. We don't run the virtualenv outside of the compiling host, so the toolchain it uses should be irrelevant.

* Refactor creation and population into a standalone script. You should be able to run |python create_virtualenv.py /path/to/virtualenv| or similar and it just works. No configure necessary.

Patches forthcoming.

Comment 1

[Gregory Szorc [:gps]]

Attachment: Part 1: Remove toolchain env vars, v1

Try at https://tbpl.mozilla.org/?tree=Try&rev=049787148c78. I will dance with joy if this just works on buildbot. Note that "just works" implies that psutil builds where it previously used to (it is optional, so if it doesn't build things are still green).

Comment 2

[Mike Hommey [:glandium]]

Comment on attachment 671905 [details] [diff] [review]
Part 1: Remove toolchain env vars, v1

I'm pretty sure this will break when using export CFLAGS in a mozconfig for cross compilation.

Comment 3

[Gregory Szorc [:gps]]

We don't care about cross-compilation for binaries in the virtualenv because said binaries only need to run on the host machine. Is there ever a time where binaries produced by the virtualenv are not used on the machine that created them?

Comment 4

[Mike Hommey [:glandium]]

(In reply to Gregory Szorc [:gps] from comment #3)
> We don't care about cross-compilation for binaries in the virtualenv because
> said binaries only need to run on the host machine. Is there ever a time
> where binaries produced by the virtualenv are not used on the machine that
> created them?

The point is, when you cross-compile, you'll have stuff in CFLAGS for the cross-compilation environment, and that will be inherited by populate_virtualenv, because CFLAGS are exported from mozconfig. So in all likeliness, python is going to try to build its host stuff with cross-compilation flags, such as -march=armv7a.

Comment 5

[Gregory Szorc [:gps]]

Got it. So, you are telling me I need to launch the processes with a clean, untainted environment.

Comment 6

[Mike Hommey [:glandium]]

(In reply to Gregory Szorc [:gps] from comment #5)
> Got it. So, you are telling me I need to launch the processes with a clean,
> untainted environment.

yes. That's mostly what you are removing was doing...

Comment 7

[Gregory Szorc [:gps]]

Attachment: Refactor virtualenv integration, v1

I decided to consolidate things and to take a slightly different direction. Logic for virtualenv population is now in client.mk and runs before configure. We also do Python version checking as part of virtualenv population because why not (if nothing else it's one less new process for configure to spawn). I also threw in the Python 2.6 bump (bug 800614) because I was all in that code.

Try at https://tbpl.mozilla.org/?tree=Try&rev=f7a098335bee. I'm sure I missed an edge case somewhere not covered by try. I have no doubt Mike or Ted will tell me where.

This patch scares me a bit and has potential for breakage, hence the double r?.

Comment 8

[Gregory Szorc [:gps]]

Comment on attachment 672670 [details] [diff] [review]
Refactor virtualenv integration, v1

I've made a huge mistake. I overlooked the bit where populating the virtualenv from a python executable that's not from the virtualenv doesn't actually populate the right Python environment. This will need some minor changes.

Comment 9

[Gregory Szorc [:gps]]

Attachment: Refactor virtualenv integration, v2

OK. Now we spin up a new process to perform virtualenv population. Try at https://tbpl.mozilla.org/?tree=Try&rev=3c977ba6d9dd

Comment 10

[Mike Hommey [:glandium]]

Comment on attachment 672681 [details] [diff] [review]
Refactor virtualenv integration, v2

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

One thing that bothers me is that this makes client.mk mandatory. A lot of people are not using it and just run configure directly.

::: build/virtualenv/populate_virtualenv.py
@@ +160,5 @@
> +    # virtualenv for paths to be proper.
> +    python_bin = os.path.join(output_path, 'bin', 'python')
> +
> +    if sys.platform in ('win32', 'cygwin'):
> +        python_bin += '.exe'

That path is going to be wrong on windows, as it's in Scripts, not bin.

@@ +169,5 @@
> +
> +    if result != 0:
> +        raise Exception('Error populating virtualenv.')
> +
> +    activate_path = os.path.join(output_path, 'bin', 'activate')

Likewise (iirc there is no bin directory at all on windows)

@@ +184,5 @@
> +
> +    This should be the main API used from this file as it is the highest-level.
> +    """
> +    deps = [
> +        os.path.join(topsrcdir, 'build', 'virtualenv', 'packages.txt'),

It bothers me that paths are declared multiple times. If they ever change, there are several places to change.

@@ +199,5 @@
> +    activate_mtime = os.path.getmtime(activate_path)
> +    dep_mtime = max(os.path.getmtime(p) for p in deps)
> +
> +    if dep_mtime > activate_mtime:
> +        return build_virtualenv(topsrcdir, topobjdir, log_handle)

Candid question: do we actually care rebuilding the virtualenv automatically when populate_virtualenv.py is changed? For instance, this change is going to rebuild a virtualenv, but it actually doesn't need to, since it doesn't change the end result. Other changes to this file are usually to add support for new kind of targets in packages.txt, in which case packages.txt is going to be new, too.

@@ +209,5 @@
> +    """Ensure the current version of Python is sufficient to build the tree."""
> +    major = sys.version_info[0]
> +    minor = sys.version_info[1]
> +
> +    if major != 2 or minor < 6:

Might be better to declare globally, so that it is easy to spot when opening the file.
Note you're changing the python version. Not that I'm against it, but that part has its own bug.

::: client.mk
@@ +264,5 @@
> +#
> +# We always run this command and the Python script figures out if anything needs
> +# to be performed.
> +populate-virtualenv:
> +	CC=$(OLD_CC) CXX=$(OLD_CXX) CFLAGS=$(OLD_CFLAGS) CXXFLAGS=$(OLD_CXXFLAGS) LDFLAGS=$(OLD_LDFLAGS) \

Actually probably not a good idea to give values at all. If you trust python to do the right thing, just reset the variables to an empty value.

::: configure.in
@@ +800,5 @@
>      AC_MSG_RESULT([yes])
>  fi
>  
> +case "$host_os" in
> +mingw*)

Please test with $OS_ARCH

Comment 11

[Gregory Szorc [:gps]]

(In reply to Mike Hommey [:glandium] from comment #10)

Great review. You found a lot of things I missed. Par for the course when I touch the build system...

> One thing that bothers me is that this makes client.mk mandatory. A lot of
> people are not using it and just run configure directly.

As I've said before, I feel this is a habit people will need to break. Bypassing client.mk or some other frontend (like mach) is a recipe for disaster. It's a giant footgun. Frontends provide a level of abstraction that allow us to make radical changes to the build system (which coincidentally is on the roadmap) without breaking workflows. Since client.mk is the officially blessed method to interact with the build system, I personally don't care about people running configure by hand. If they insist on bypassing the blessed procedure, then I think they deserve to be inconvenienced. In the case of this patch, that inconvenience is now having to execute populate_virtualenv.py manually.

That being said, I can add virtualenv population back into configure if there is strong pushback. I'd prefer not to. But, it's not that big of a deal. The reason I want it in client.mk is because long-term (when mach replaces client.mk), virtualenv population will need to occur in mach. And, since mach can be thought of as a Python version of client.mk, the transition is smoother when both mach and client.mk behave similarly.

> ::: build/virtualenv/populate_virtualenv.py
> @@ +160,5 @@
> > +    # virtualenv for paths to be proper.
> > +    python_bin = os.path.join(output_path, 'bin', 'python')
> > +
> > +    if sys.platform in ('win32', 'cygwin'):
> > +        python_bin += '.exe'
> 
> That path is going to be wrong on windows, as it's in Scripts, not bin.

Yup. And the Windows Try builds proved it.
> @@ +199,5 @@
> > +    activate_mtime = os.path.getmtime(activate_path)
> > +    dep_mtime = max(os.path.getmtime(p) for p in deps)
> > +
> > +    if dep_mtime > activate_mtime:
> > +        return build_virtualenv(topsrcdir, topobjdir, log_handle)
> 
> Candid question: do we actually care rebuilding the virtualenv automatically
> when populate_virtualenv.py is changed? For instance, this change is going
> to rebuild a virtualenv, but it actually doesn't need to, since it doesn't
> change the end result. Other changes to this file are usually to add support
> for new kind of targets in packages.txt, in which case packages.txt is going
> to be new, too.

Maybe. Maybe not. I think including populate_virtualenv.py in the dependency list is the more robust solution. Will we ever encounter a scenario when we need this? Hard to tell.

> @@ +209,5 @@
> > +    """Ensure the current version of Python is sufficient to build the tree."""
> > +    major = sys.version_info[0]
> > +    minor = sys.version_info[1]
> > +
> > +    if major != 2 or minor < 6:
> 
> Might be better to declare globally, so that it is easy to spot when opening
> the file.
> Note you're changing the python version. Not that I'm against it, but that
> part has its own bug.

Good idea on global variables.

Ted already r+'d the minimum version bump. I had to change all that code as part of refactoring virtualenv, so I figured I'd go ahead and do it. I can add back in support for 2.5 and update the patch on bug 800614 if you insist.

> ::: client.mk
> @@ +264,5 @@
> > +#
> > +# We always run this command and the Python script figures out if anything needs
> > +# to be performed.
> > +populate-virtualenv:
> > +	CC=$(OLD_CC) CXX=$(OLD_CXX) CFLAGS=$(OLD_CFLAGS) CXXFLAGS=$(OLD_CXXFLAGS) LDFLAGS=$(OLD_LDFLAGS) \
> 
> Actually probably not a good idea to give values at all. If you trust python
> to do the right thing, just reset the variables to an empty value.

I actually had this in an earlier version and thought there might be scenarios where the main environment e.g. defined a custom compiler, so I proxied them through. If you don't think we need it, I'll use empty variables and we'll see where that takes us.

Comment 12

[Mike Hommey [:glandium]]

(In reply to Gregory Szorc [:gps] from comment #11)
> (In reply to Mike Hommey [:glandium] from comment #10)
> 
> Great review. You found a lot of things I missed. Par for the course when I
> touch the build system...
> 
> > One thing that bothers me is that this makes client.mk mandatory. A lot of
> > people are not using it and just run configure directly.
> 
> As I've said before, I feel this is a habit people will need to break.
> Bypassing client.mk or some other frontend (like mach) is a recipe for
> disaster. It's a giant footgun. Frontends provide a level of abstraction
> that allow us to make radical changes to the build system (which
> coincidentally is on the roadmap) without breaking workflows. Since
> client.mk is the officially blessed method to interact with the build
> system, I personally don't care about people running configure by hand. If
> they insist on bypassing the blessed procedure, then I think they deserve to
> be inconvenienced. In the case of this patch, that inconvenience is now
> having to execute populate_virtualenv.py manually.

Why inconvenience them when configure could run populate_virtualenv.py, which, if client.mk already did it, would be a nop?

> > Candid question: do we actually care rebuilding the virtualenv automatically
> > when populate_virtualenv.py is changed? For instance, this change is going
> > to rebuild a virtualenv, but it actually doesn't need to, since it doesn't
> > change the end result. Other changes to this file are usually to add support
> > for new kind of targets in packages.txt, in which case packages.txt is going
> > to be new, too.
> 
> Maybe. Maybe not. I think including populate_virtualenv.py in the dependency
> list is the more robust solution. Will we ever encounter a scenario when we
> need this? Hard to tell.

If you keep this, maybe you should use __file__ instead of hand-crafted path to populate_virtualenv.py.
 
> > @@ +209,5 @@
> > > +    """Ensure the current version of Python is sufficient to build the tree."""
> > > +    major = sys.version_info[0]
> > > +    minor = sys.version_info[1]
> > > +
> > > +    if major != 2 or minor < 6:
> > 
> > Might be better to declare globally, so that it is easy to spot when opening
> > the file.
> > Note you're changing the python version. Not that I'm against it, but that
> > part has its own bug.
> 
> Good idea on global variables.
> 
> Ted already r+'d the minimum version bump. I had to change all that code as
> part of refactoring virtualenv, so I figured I'd go ahead and do it. I can
> add back in support for 2.5 and update the patch on bug 800614 if you insist.

It feels like an unrelated change sneaking in. When doing hg blame, it's better if the we can easily find that the version bump comes from bug 800614 and is unrelated to the code changing at the same time.

> I actually had this in an earlier version and thought there might be
> scenarios where the main environment e.g. defined a custom compiler, so I
> proxied them through. If you don't think we need it, I'll use empty
> variables and we'll see where that takes us.

I would tend to trust python more than the user, here. And that custom compiler could very much be unsuitable to build host programs...

Comment 13

[Gregory Szorc [:gps]]

Comment on attachment 672681 [details] [diff] [review]
Refactor virtualenv integration, v2

Need to incorporate Mike's review feedback.

Comment 14

[Gregory Szorc [:gps]]

Attachment: Refactor virtualenv integration, v3

I incorporated all of Mike's feedback. client.mk is untouched now. Instead, I moved virtualenv population to the top of configure.in (at least pretty close to the top).

We still require Python 2.5+, not 2.6.

I refactored populate_virtualenv.py to contain a class with all the logic. This makes the code a little easier to read, IMO. Although, it did mean I touched nearly every line in the file.

Try at https://tbpl.mozilla.org/?tree=Try&rev=68b3c8d0edda

Comment 15

[Gregory Szorc [:gps]]

Attachment: Refactor virtualenv integration, v4

Previous patch didn't work on Windows. I'm optimistic this one will do the trick.

https://tbpl.mozilla.org/?tree=Try&rev=c4c6b4602734

Comment 16

[Gregory Szorc [:gps]]

https://tbpl.mozilla.org/?tree=Try&rev=13f7c8794195 is the new build

Trying $host_os instead of $OS_ARCH for determining the path to the Python executable (I think I moved the Python population foo to before the definition of $OS_ARCH).

Comment 17

[Gregory Szorc [:gps]]

So, uh Windows builds are red on Try and I have no clue why. e.g. https://tbpl.mozilla.org/php/getParsedLog.php?id=16257848&tree=Try&full=1

Virtualenv is populated fully. And, it appears to build just fine. What's going on?

Comment 18

[Mike Hommey [:glandium]]

e:\builds\moz2_slave\try-w64\build\obj-firefox\browser\base\content\test\Makefile:310:0: command '/e/builds/moz2_slave/try-w64/build/obj-firefox/_virtualenv/Scripts/python.exe e:/builds/moz2_slave/try-w64/build/config/nsinstall.py -t "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/head.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_typeAheadFind.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_keywordSearch.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_allTabsPanel.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_alltabslistener.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_bug304198.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/title_test.svg" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_bug329212.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_bug356571.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_bug380960.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_bug386835.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_bug405137.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_bug406216.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_bug409481.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_bug413915.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_bug416661.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_bug417483.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_bug419612.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_identity_UI.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_bug422590.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_bug424101.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_bug427559.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_bug432599.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_bug435035.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_bug435325.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_bug441778.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_popupNotification.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_bug455852.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_bug460146.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_bug462673.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_bug477014.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_bug479408.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_bug479408_sample.html" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_bug481560.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_bug484315.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_bug491431.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_bug495058.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_bug517902.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_bug519216.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_bug520538.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_bug521216.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_bug533232.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_bug537474.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_bug550565.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_bug553455.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_bug555224.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_bug555767.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_bug556061.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_bug559991.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_bug561623.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_bug561636.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_bug562649.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_bug563588.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_bug565575.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_bug567306.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_zbug569342.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_bug575561.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_bug575830.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_bug577121.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_bug578534.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_bug579872.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_bug580638.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_bug580956.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_bug581242.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_bug581253.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_bug581947.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_bug585785.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_bug585830.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_bug590206.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_bug592338.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_bug594131.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_bug595507.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_bug596687.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_bug597218.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_bug598923.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_bug599325.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_bug609700.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_bug616836.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_bug623155.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_bug623893.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_bug624734.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_bug647886.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_bug655584.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_bug664672.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_bug710878.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_bug719271.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_bug724239.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_bug735471.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_bug743421.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_bug749738.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_bug763468.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_bug767836.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_bug783614.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_bug797677.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_canonizeURL.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_customize.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_findbarClose.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_homeDrop.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_keywordBookmarklets.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_contextSearchTabPosition.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_ctrlTab.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_customize_popupNotification.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_disablechrome.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_discovery.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_duplicateIDs.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_gestureSupport.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_getshortcutoruri.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_hide_removing.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_overflowScroll.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_locationBarCommand.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_locationBarExternalLoad.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_page_style_menu.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_pinnedTabs.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_plainTextLinks.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_pluginnotification.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_pluginplaypreview.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_relatedTabs.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_sanitize-passwordDisabledHosts.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_sanitize-sitepermissions.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_sanitize-timespans.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_clearplugindata.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_clearplugindata.html" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_clearplugindata_noage.html" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_popupUI.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_sanitizeDialog.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_save_video.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/bug564387.html" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/bug564387_video1.ogv" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/bug564387_video1.ogv^headers^" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_save_link.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_save_private_link.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/bug792517.html" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/bug792517-2.html" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/bug792517.sjs" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_scope.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_selectTabAtIndex.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_tab_dragdrop.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_tab_dragdrop2.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_tab_dragdrop2_frame1.xul" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_tabfocus.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_tabs_isActive.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_tabs_owner.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_unloaddialogs.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_urlbarAutoFillTrimURLs.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_urlbarCopying.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_urlbarEnter.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_urlbarRevert.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_urlbarStop.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_urlbarTrimURLs.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_urlHighlight.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_visibleFindSelection.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_visibleTabs.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_visibleTabs_contextMenu.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_visibleTabs_bookmarkAllPages.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_visibleTabs_bookmarkAllTabs.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_visibleTabs_tabPreview.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/bug592338.html" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/disablechrome.html" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/discovery.html" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/domplate_test.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/moz.png" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/video.ogg" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/test_bug435035.html" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/test_bug462673.html" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/page_style_sample.html" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/plugin_unknown.html" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/plugin_test.html" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/plugin_test2.html" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/plugin_test3.html" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/plugin_alternate_content.html" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/plugin_both.html" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/plugin_both2.html" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/plugin_bug743421.html" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/plugin_clickToPlayAllow.html" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/plugin_clickToPlayDeny.html" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/plugin_bug749455.html" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/plugin_bug797677.html" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/plugin_hidden_to_visible.html" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/plugin_two_types.html" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/alltabslistener.html" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/zoom_test.html" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/dummy_page.html" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_tabMatchesInAwesomebar.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/file_bug550565_popup.html" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/file_bug550565_favicon.ico" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_aboutHome.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/app_bug575561.html" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/app_subframe_bug575561.html" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_contentAreaClick.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_addon_bar_close_button.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_addon_bar_shortcut.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_addon_bar_aomlistener.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/test_bug628179.html" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_wyciwyg_urlbarCopying.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/test_wyciwyg_copying.html" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/authenticate.sjs" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_minimize.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_aboutSyncProgress.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_middleMouse_inherit.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/redirect_bug623155.sjs" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_tabDrop.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_lastAccessedTab.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_bug734076.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_social_toolbar.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_social_shareButton.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_social_sidebar.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_social_flyout.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_social_mozSocial_API.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_social_isVisible.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_social_chatwindow.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/social_panel.html" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/social_share_image.png" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/social_sidebar.html" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/social_chat.html" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/social_flyout.html" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/social_window.html" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/social_worker.js" "e:/builds/moz2_slave/try-w64/build/browser/base/content/test/browser_bug462289.js" ../../../../_tests/testing/mochitest/browser/browser/base/content/test' failed, return code 101120

Comment 19

[Mike Hommey [:glandium]]

/e/builds/moz2_slave/try-w64/build/obj-firefox/_virtualenv/Scripts/python.exe that's a msys path, and that probably doesn't work because pymake runs the command without a shell.

Comment 20

[Mike Hommey [:glandium]]

(In reply to Mike Hommey [:glandium] from comment #19)
> /e/builds/moz2_slave/try-w64/build/obj-firefox/_virtualenv/Scripts/python.
> exe that's a msys path, and that probably doesn't work because pymake runs
> the command without a shell.

And it worked before because the PYTHON=$MOZ_BUILD_ROOT/..... was after MOZ_BUILD_ROOT=`cd $MOZ_BUILD_ROOT && pwd -W`, while now, it's before.

Comment 21

[Gregory Szorc [:gps]]

Interesting. Another reason why I want to enter a Python environment ASAP and have Python do all the process spawning...

New try at https://tbpl.mozilla.org/?tree=Try&rev=0bea4cbfd55e. On mingw I now:

  PYTHON=`cd $MOZ_BUILD_ROOT && pwd -W`/_virtualenv/Scripts/python.exe

We'll see what happens...

Comment 22

[Mike Hommey [:glandium]]

FWIW, I filed bug 803466 about the logs being not really human-friendly.

(In reply to Gregory Szorc [:gps] from comment #21)
> Interesting. Another reason why I want to enter a Python environment ASAP
> and have Python do all the process spawning...
> 
> New try at https://tbpl.mozilla.org/?tree=Try&rev=0bea4cbfd55e. On mingw I
> now:
> 
>   PYTHON=`cd $MOZ_BUILD_ROOT && pwd -W`/_virtualenv/Scripts/python.exe
> 
> We'll see what happens...

An alternative way would be to have populate_virtualenv.py print the python path and get it from there.

Comment 23

[Mike Hommey [:glandium]]

(In reply to Mike Hommey [:glandium] from comment #22)
> An alternative way would be to have populate_virtualenv.py print the python
> path and get it from there.

Although, if you do that, you'll need to replace os.sep with '/' beforehand.

Comment 24

[Gregory Szorc [:gps]]

Attachment: Refactor virtualenv integration, v5

This patch passed try. I'm not passing the path from populate_virtualenv.py because it is more complicated and I don't think it's worth the effort at this juncture.

Comment 25

[Mike Hommey [:glandium]]

Comment on attachment 673301 [details] [diff] [review]
Refactor virtualenv integration, v5

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

> It's default action now (...)

its

::: build/virtualenv/populate_virtualenv.py
@@ +34,5 @@
>  
> +    @property
> +    def virtualenv_populate_path(self):
> +        return os.path.join(self.topsrcdir, 'python', 'virtualenv',
> +            'virtualenv.py')

The function name suggests it's the path to the populate_virtualenv script, which is not what is returned.

@@ +62,2 @@
>  
> +        If the virtualenv is up to date, this does nothing. Else, it creates

I tend to prefer otherwise to else in such phrasing.

@@ +233,3 @@
>  
> +        # We probably could call the contents of this file inside the context
> +        # of # this interpreter using execfile() or similar. However, if global

Extra # here.

@@ +272,5 @@
> +
> +def verify_python_version(log_handle):
> +    """Ensure the current version of Python is sufficient."""
> +    major = sys.version_info[0]
> +    minor = sys.version_info[1]

major, minor = sys.version_info[:2]

@@ +275,5 @@
> +    major = sys.version_info[0]
> +    minor = sys.version_info[1]
> +
> +    if major != MINIMUM_PYTHON_MAJOR or minor < MINIMUM_PYTHON_MINOR:
> +        log_handle.write('Python %d.%d os greater is required to build.' %

The error message is going to be confusion if running with python 3.

::: configure.in
@@ +18,5 @@
>  
>  MOZ_DEB_TIMESTAMP=`date +"%a, %d  %b %Y %T %z"   2>&1`
>  AC_SUBST(MOZ_DEB_TIMESTAMP)
>  
> +

You don't have to make configure.in bigger by adding empty lines :)

Comment 26

[Gregory Szorc [:gps]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1423fc67d73e

With all feedback addressed.

Comment 27

[Gregory Szorc [:gps]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/74b38f9c3cb0

Follow-up was IRC r+ by glandium to hopefully unbust B2G panda builds.

Comment 28

[Gregory Szorc [:gps]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/82fd81257b82

We need to unset PYTHONDONTWRITEBYTECODE on virtualenv creation in addition to population. Doh.

IRC r+ by ted.

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1423fc67d73e
https://hg.mozilla.org/mozilla-central/rev/74b38f9c3cb0
https://hg.mozilla.org/mozilla-central/rev/82fd81257b82

Comment 30

[Anthony Ramine [:nox]]

(In reply to Gregory Szorc [:gps] from comment #28)

https://hg.mozilla.org/integration/mozilla-inbound/rev/82fd81257b82

We need to unset PYTHONDONTWRITEBYTECODE on virtualenv creation in addition
to population. Doh.

IRC r+ by ted.

I'm late to the party, does anyone remember why this was needed? It's kinda a bother when embedding the SpiderMonkey sources in a Rust crate.

Comment 31

[Anthony Ramine [:nox]]

So I found why, in the past virtualenv wouldn't work if that environment variable was set, but that was fixed in 2013, so a year after the env variable was explicitly unset in mozbuild. I'll make a patch later to remove the removal from mozbuild.

https://github.com/pypa/virtualenv/issues/430