Closed
Bug 1410773
Opened 7 years ago
Closed 7 years ago
Optionally include python packages from comm-central in python environment.
Categories
(Firefox Build System :: General, enhancement)
Firefox Build System
General
Tracking
(firefox58 fixed)
RESOLVED
FIXED
mozilla58
Tracking | Status | |
---|---|---|
firefox58 | --- | fixed |
People
(Reporter: tomprince, Assigned: tomprince)
References
Details
Attachments
(1 file)
As Thunderbird re-aligns its build system with Firefox, we'll want to extend parts of the build system, without needing to land code in mozilla-central. So far, I've wanted to be able to include custom taskcluster transforms (such as the experimental https://hg.mozilla.org/try-comm-central/diff/73bcc95bd6d9/taskcluster/comm_taskgraph/__init__.py) without needing the code to land in mozilla-central.
Comment hidden (mozreview-request) |
Comment 2•7 years ago
|
||
mozreview-review |
Comment on attachment 8920943 [details] Bug 1410773: Add optional comm-central virtualenv manifest; https://reviewboard.mozilla.org/r/191896/#review197286 I'm not expert in this area, but there's already an "optional:" in the given `virtual_packages.txt`. It was being ignored, correct? Can you explain why there's such duplication between this code and http://searchfox.org/mozilla-central/source/python/mozbuild/mozbuild/virtualenv.py#308? r- only to loop back onto these questions; some other peer might know more and r+ without me.
Attachment #8920943 -
Flags: review-
Assignee | ||
Comment 3•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8920943 [details] Bug 1410773: Add optional comm-central virtualenv manifest; https://reviewboard.mozilla.org/r/191896/#review197286 > I'm not expert in this area, but there's already an "optional:" in the given virtual_packages.txt. It was being ignored, correct? It was, and still is, since this code doesn't handle entries of type `setup.py`. > Can you explain why there's such duplication between this code and [`python/mozbuild/mozbuild/virtualenv.py`](http://searchfox.org/mozilla-central/source/python/mozbuild/mozbuild/virtualenv.py#308?) I'm not sure, but I think the main reason is that this code is what gets that code into the python path. This code gets run when running `mach` and modifies the python path of the current process. The code in mozbuild is used to create a virtualenv, which gets used when a python subprocess is run.
Comment 4•7 years ago
|
||
mozreview-review |
Comment on attachment 8920943 [details] Bug 1410773: Add optional comm-central virtualenv manifest; https://reviewboard.mozilla.org/r/191896/#review197320 ::: build/mach_bootstrap.py:126 (Diff revision 1) > def search_path(mozilla_dir, packages_txt): > with open(os.path.join(mozilla_dir, packages_txt)) as f: > packages = [line.rstrip().split(':') for line in f] > > - for package in packages: > + def handle_package(package): > + if package[0] == 'optional': Is `optional` just an annotation here? I don't see anything that makes it conditional, so that it's included for thunderbird but not firefox builds.
Assignee | ||
Comment 5•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8920943 [details] Bug 1410773: Add optional comm-central virtualenv manifest; https://reviewboard.mozilla.org/r/191896/#review197320 > Is `optional` just an annotation here? I don't see anything that makes it conditional, so that it's included for thunderbird but not firefox builds. `optional` just means that it isn't an error if the file doesn't exist. Without the annotation (and the associated `try`...`except`, it would try to open `comm/build/virtualenv_packages.txt` (which doesn't exist in m-c) and the exception would cause mach to fail ([here](https://treeherder.mozilla.org/logviewer.html#?job_id=138797291&repo=try&lineNumber=54) is a try build from before I added the `try`...`except` block.
Comment 6•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8920943 [details] Bug 1410773: Add optional comm-central virtualenv manifest; https://reviewboard.mozilla.org/r/191896/#review197320 > `optional` just means that it isn't an error if the file doesn't exist. Without the annotation (and the associated `try`...`except`, it would try to open `comm/build/virtualenv_packages.txt` (which doesn't exist in m-c) and the exception would cause mach to fail ([here](https://treeherder.mozilla.org/logviewer.html#?job_id=138797291&repo=try&lineNumber=54) is a try build from before I added the `try`...`except` block. I see, thanks. It would be good to put that explanation in the commit message.
Comment hidden (mozreview-request) |
Comment 8•7 years ago
|
||
mozreview-review |
Comment on attachment 8920943 [details] Bug 1410773: Add optional comm-central virtualenv manifest; https://reviewboard.mozilla.org/r/191896/#review198706 OK, I'm fine with this. If it's green on try, it's fine by me. (Make sure to run a linux64 build so that the Python checks are run.)
Attachment #8920943 -
Flags: review+
Assignee | ||
Updated•7 years ago
|
Attachment #8920943 -
Flags: review?(core-build-config-reviews)
Pushed by mozilla@hocat.ca: https://hg.mozilla.org/integration/autoland/rev/3ace75745163 Add optional comm-central virtualenv manifest; r=nalexander
Comment 10•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/3ace75745163
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox58:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
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
•