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)

enhancement
Not set
normal

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 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-
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 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.
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 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 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+
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
https://hg.mozilla.org/mozilla-central/rev/3ace75745163
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: