Closed
Bug 774106
Opened 12 years ago
Closed 12 years ago
Change how virtualenv is populated
Categories
(Firefox Build System :: General, defect)
Firefox Build System
General
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla17
People
(Reporter: gps, Assigned: gps)
References
Details
Attachments
(1 file, 2 obsolete files)
8.41 KB,
patch
|
glandium
:
review+
|
Details | Diff | Splinter Review |
In order to support alternate build backends, we can't rely on Makefile.in'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•12 years ago
|
||
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.
Assignee | ||
Comment 2•12 years ago
|
||
Replaced Makefile.in 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 https://tbpl.mozilla.org/?tree=Try&rev=f3fa3ee4237b
Assignee | ||
Comment 3•12 years ago
|
||
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 client.mk or elsewhere so virtualenv population is retriggered if the manifest file changes. I'm not sure where all I need to hook that up.
Assignee | ||
Comment 4•12 years ago
|
||
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...
Assignee | ||
Comment 5•12 years ago
|
||
Now with text file and dependencies in client.mk. I also may need to ensure something gets written to config.status - I don't know how all this magic is hooked up. http://tbpl.mozilla.org/?tree=Try&rev=6f4e3ae19cef
Attachment #643701 -
Attachment is obsolete: true
Attachment #643701 -
Flags: review?(ted.mielczarek)
Attachment #643711 -
Flags: review?(ted.mielczarek)
Comment 6•12 years ago
|
||
Comment on attachment 643711 [details] [diff] [review] refactor how virtualenv is populated, v2 Review of attachment 643711 [details] [diff] [review]: ----------------------------------------------------------------- ::: build/virtualenv/populate_virtualenv.py @@ +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 interpreter @@ +32,5 @@ > + setup = os.path.join(directory, 'setup.py') > + result = subprocess.call([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
Assignee | ||
Comment 7•12 years ago
|
||
Current patch has green builds for all arches on Try.
Assignee | ||
Comment 8•12 years ago
|
||
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 setup.py <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 https://tbpl.mozilla.org/?tree=Try&rev=6416aec3c12d
Attachment #643711 -
Attachment is obsolete: true
Attachment #643711 -
Flags: review?(ted.mielczarek)
Attachment #644453 -
Flags: review?(mh+mozilla)
Assignee | ||
Comment 9•12 years ago
|
||
Try build was all green.
Comment 10•12 years ago
|
||
Comment on attachment 644453 [details] [diff] [review] refactor how virtualenv is populated, v3 Review of attachment 644453 [details] [diff] [review]: ----------------------------------------------------------------- ::: configure.in @@ +8782,5 @@ > > +# Populate the virtualenv > +AC_MSG_RESULT([Populating Python virtualenv]) > +MACOSX_DEPLOYMENT_TARGET= LDFLAGS="${HOST_LDFLAGS}" \ > + CFLAGS="${HOST_CFLAGS}" CXXFLAGS="${HOST_CXXFLAGS}" \ While at it, you should also set CC and CXX.
Attachment #644453 -
Flags: review?(mh+mozilla) → review+
Assignee | ||
Comment 11•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/fda9279d7031
Target Milestone: --- → mozilla17
Comment 12•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/fda9279d7031
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Updated•6 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•