Last Comment Bug 774106 - Change how virtualenv is populated
: Change how virtualenv is populated
Product: Core
Classification: Components
Component: Build Config (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla17
Assigned To: Gregory Szorc [:gps]
: Gregory Szorc [:gps]
Depends on: 661908 776537
Blocks: 774049 776046 777465 1195264
  Show dependency treegraph
Reported: 2012-07-15 13:17 PDT by Gregory Szorc [:gps]
Modified: 2015-08-17 05:02 PDT (History)
5 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

refactor how virtualenv is populated (5.82 KB, patch)
2012-07-18 19:02 PDT, Gregory Szorc [:gps]
no flags Details | Diff | Splinter Review
refactor how virtualenv is populated, v2 (7.42 KB, patch)
2012-07-18 19:59 PDT, Gregory Szorc [:gps]
no flags Details | Diff | Splinter Review
refactor how virtualenv is populated, v3 (8.41 KB, patch)
2012-07-20 13:48 PDT, Gregory Szorc [:gps]
mh+mozilla: review+
Details | Diff | Splinter Review

Description Gregory Szorc [:gps] 2012-07-15 13:17:16 PDT
In order to support alternate build backends, we can't rely on's because Makefiles may not be generated by alternate build backends. Since the virtualenv today is populated in configure immediately after aclocal-fast creates build/virtualenv/Makefile, we are in a bit of a bind.

We need to change how the virtualenv is populated such that it doesn't rely on the mass Makefile generation as it is currently implemented in configure.

We could still use a make file if we really wanted to. However, the existing make file logic is dead simple and it could easily be ported to a Python module/script. Having it exist as an importable module would also likely benefit mach. Since mach will run before configure, it needs a way of knowing where all the Python source lives so it can import its own modules. This likely involves some kind of self bootstrapping. It's entirely possible mach will want to create and populate the virtualenv itself, before configure is executed. At the very least, mach will need to adjust sys.path on first run in order to be able to import its own modules.

Anyway, the mach work will be in a follow-up bug. Just know that its usage needs to be considered when changing this.

Our options depend on what :glandium is doing for refactoring autoconf output file generation. If we lump virtualenv's make file into the always-generated set, we should be cool. That being said, I'm advocating for an importable Python module (which can also be executed as a script from configure) so mach can do magic without having to fork a new process.
Comment 1 Ted Mielczarek [:ted.mielczarek] 2012-07-16 09:45:18 PDT
I don't really care, honestly. I just wanted to have an easy place for people to rebuild if they wanted to re-populate the virtualenv. It doesn't have to be a Makefile.
Comment 2 Gregory Szorc [:gps] 2012-07-18 19:02:44 PDT
Created attachment 643701 [details] [diff] [review]
refactor how virtualenv is populated

Replaced with packages.json manifest and a Python file. Population of virtualenv in configure now happens before Makefile generation.

This works fine for me on Python 2.7 on OS X. I don't doubt the first revision will cause burnage. That's why we have
Comment 3 Gregory Szorc [:gps] 2012-07-18 19:10:27 PDT
Comment on attachment 643701 [details] [diff] [review]
refactor how virtualenv is populated

Review of attachment 643701 [details] [diff] [review]:

This will need a hook-up somewhere in or elsewhere so virtualenv population is retriggered if the manifest file changes. I'm not sure where all I need to hook that up.
Comment 4 Gregory Szorc [:gps] 2012-07-18 19:29:37 PDT
Bit by Python 2.5 once again. I completely forgot the json module isn't present in 2.5. I guess I'll go with a flat file...
Comment 5 Gregory Szorc [:gps] 2012-07-18 19:59:13 PDT
Created attachment 643711 [details] [diff] [review]
refactor how virtualenv is populated, v2

Now with text file and dependencies in I also may need to ensure something gets written to config.status - I don't know how all this magic is hooked up.
Comment 6 :Ms2ger (⌚ UTC+1/+2) 2012-07-19 02:55:20 PDT
Comment on attachment 643711 [details] [diff] [review]
refactor how virtualenv is populated, v2

Review of attachment 643711 [details] [diff] [review]:

::: build/virtualenv/
@@ +14,5 @@
> +
> +    The manifest file is opened and read. For each package defined in the
> +    manifest, we install it into the current environment.
> +
> +    Note that the Python interpretter running this function should be the one


@@ +32,5 @@
> +    setup = os.path.join(directory, '')
> +    result =[sys.executable, setup, 'develop'], cwd=directory)
> +
> +    if result != 0:
> +        raise Exception('Error installing package: %s' % directory)

I guess using check_call would give a worse error message
Comment 7 Gregory Szorc [:gps] 2012-07-19 13:17:16 PDT
Current patch has green builds for all arches on Try.
Comment 8 Gregory Szorc [:gps] 2012-07-20 13:48:00 PDT
Created attachment 644453 [details] [diff] [review]
refactor how virtualenv is populated, v3

I think Mike is interested in using this (due to dependent bugs), so I'll put him in the review path.

I changed the manifest format to facilitate multiple install actions. Currently, we just support <arg0> [<arg1>[...<argN>]]. But, we'll need to support raw .pth in the near future. And, who knows what the distant future holds (maybe even symlinks to individual files?). Anyway, we now have a plan for tackling that when it becomes a problem.

Try at
Comment 9 Gregory Szorc [:gps] 2012-07-20 17:02:19 PDT
Try build was all green.
Comment 10 Mike Hommey [:glandium] 2012-07-23 00:00:59 PDT
Comment on attachment 644453 [details] [diff] [review]
refactor how virtualenv is populated, v3

Review of attachment 644453 [details] [diff] [review]:

@@ +8782,5 @@
> +# Populate the virtualenv
> +AC_MSG_RESULT([Populating Python virtualenv])

While at it, you should also set CC and CXX.
Comment 12 Ed Morley [:emorley] 2012-07-24 03:03:32 PDT

Note You need to log in before you can comment on or make changes to this bug.