Change how virtualenv is created, populated

RESOLVED FIXED in mozilla19

Status

defect
RESOLVED FIXED
7 years ago
Last year

People

(Reporter: gps, Assigned: gps)

Tracking

Trunk
mozilla19
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 5 obsolete attachments)

Assignee

Description

7 years ago
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.
Assignee

Comment 1

7 years ago
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).
Attachment #671905 - Flags: review?(ted.mielczarek)
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.
Attachment #671905 - Flags: review?(ted.mielczarek) → review-
Assignee

Comment 3

7 years ago
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?
(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.
Assignee

Comment 5

7 years ago
Got it. So, you are telling me I need to launch the processes with a clean, untainted environment.
(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...
Assignee

Comment 7

7 years ago
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?.
Attachment #671905 - Attachment is obsolete: true
Attachment #672670 - Flags: review?(ted.mielczarek)
Attachment #672670 - Flags: review?(mh+mozilla)
Assignee

Comment 8

7 years ago
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.
Attachment #672670 - Flags: review?(ted.mielczarek)
Attachment #672670 - Flags: review?(mh+mozilla)
Assignee

Comment 9

7 years ago
OK. Now we spin up a new process to perform virtualenv population. Try at https://tbpl.mozilla.org/?tree=Try&rev=3c977ba6d9dd
Attachment #672670 - Attachment is obsolete: true
Attachment #672681 - Flags: review?(ted.mielczarek)
Attachment #672681 - Flags: review?(mh+mozilla)
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
Attachment #672681 - Flags: review?(mh+mozilla) → review-
Assignee

Comment 11

7 years ago
(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.
(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...
Assignee

Comment 13

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

Need to incorporate Mike's review feedback.
Attachment #672681 - Flags: review?(ted)
Assignee

Comment 14

7 years ago
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
Attachment #672681 - Attachment is obsolete: true
Attachment #673081 - Flags: review?(mh+mozilla)
Assignee

Comment 15

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

https://tbpl.mozilla.org/?tree=Try&rev=c4c6b4602734
Attachment #673081 - Attachment is obsolete: true
Attachment #673081 - Flags: review?(mh+mozilla)
Attachment #673095 - Flags: review?(mh+mozilla)
Assignee

Comment 16

7 years ago
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).
Assignee

Comment 17

7 years ago
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?
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
/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.
(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.
Assignee

Comment 21

7 years ago
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...
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.
(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.
Assignee

Comment 24

7 years ago
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.
Attachment #673095 - Attachment is obsolete: true
Attachment #673095 - Flags: review?(mh+mozilla)
Attachment #673301 - Flags: review?(mh+mozilla)
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 :)
Attachment #673301 - Flags: review?(mh+mozilla) → review+
Assignee

Comment 26

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

With all feedback addressed.
Target Milestone: --- → mozilla19
Assignee

Comment 27

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

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

Comment 28

7 years ago
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.
Depends on: 804536

Updated

7 years ago
Blocks: 803834

Updated

7 years ago
Blocks: 806784

Updated

Last year
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.