Closed Bug 1195299 Opened 6 years ago Closed 2 years ago

Remove duplicated mozbase modules from mozharness

Categories

(Release Engineering :: Applications: MozharnessCore, defect)

Product:
Release Engineering
Component:
Applications: MozharnessCore
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox68+ fixed)

Status:
RESOLVED FIXED
Tracking Flags:
Tracking Status
firefox68 + fixed

People

(Reporter: ahal, Assigned: ahal)

References

Details

Attachments

(3 files, 1 obsolete file)

include_mozbase.diff
1.87 KB, patch
Details | Diff | Splinter Review
Bug 1195299 - [ci] Use 'mach python' to run mozharness scripts when we have a srcdir
47 bytes, text/x-phabricator-request
Details | Review
Bug 1195299 - [mozharness] Remove copies of mozbase from testing/mozharness
47 bytes, text/x-phabricator-request
Details | Review 
There are copies of mozinfo, mozfile and mozprocess under testing/mozharness. These should be removed and mozharness should just use the version in testing/mozbase.

These versions are likely way out of date and some work may be needed to replace them.
I believe base script should create the venv asap and install the minimum set of mozbase with open ended versions.
We can let the actual harnesses pick the exact version when they call create_virtualenv step.

For instance, for the run_command() [1] we can do the following and it works if there is a venv:
 site_packages_path = self.script_obj.query_python_site_packages_path()
 sys.path.insert(0, site_packages_path)
 from mozprocess import ProcessHandler

We can make this more generic to handle as well the remaining mozbase packages.

[1] http://hg.mozilla.org/mozilla-central/file/default/testing/mozharness/mozharness/base/script.py#l994
This patch shows the alternative approach which we would include each mozbase module before calling it.

Unfortunately this approach won't work unless we also pull mozbase for the test jobs.

I'm going to look again into the virtualenv approach.
jlund also proposed if we did things a bit more close to other python projects.

For instance, we could drop virtualenv.py into the repo and do this in two steps before calling mozharness:
rm -rf build/venv
python external_tools/virtualenv.py build/venv
build/venv/bin/pip install mozinfo mozfile mozprocess --find-links http://pypi.pub.build.mozilla.org/pub --find-links http://pypi.pub.build.mozilla.org/pvt

We would also need to prevent clobber from clearing the venv.

We would need to call the script like this:
build/venv/python scripts/...

Calling that specific python would remove the need to modify sys.path afaik.
Another approach would be to remove using mozbase modules by Mozharness.
We don't use it in that many places.
I believe this would take less work and be cleaner.
(In reply to Armen Zambrano Gasparnian [:armenzg] from comment #4)
> Another approach would be to remove using mozbase modules by Mozharness.
> We don't use it in that many places.
> I believe this would take less work and be cleaner.

I think there was a reason we were using mozprocess, probably for the output timeout capability. Between this option and hacking sys.path, I'd rather just hack sys.path (if virtualenv is too hard).
We could create a small script with bits and pieces from this patch [1]

This script could be called "mozharness_bootstrap.py" under external_tools/
It would be able to set up a minimal venv for mozharness.
It could read from configs/releng_infra_configs to determine information about where to find virtualenv on each host.
As soon as archiver is called, we could call this script and generate a "mh_venv" before calling the mozharness script.

After this minimal venv is generated we call our typical mozharness script like this:
> mh_venv/bin/python scripts/desktop_unittest.py ...

We could also take the approach of making the bootstrap script be a single call (instead of two):
> python mozharness_bootstraph.py scripts/desktop_unittest.py ...

Internally we set up the venv, take sys.argv[1:] and call that.

We could even take this one step further by making the bootstrap script to read a buildbot property which contains the whole command and arguments.
This would allow to use any builder as a generic builder since we could schedule a buildername with a property specifiying which command to execute.
Obviously job definition would live in-tree. In fact, we could teach taskcluster's decision task to schedule everything in such generic builder :)

This concept is slightly based on bug 1078362.

Nevertheless, this is a comple scope creep. Getting rid of the older mozbase modules comes first :)


[1] https://bugzilla.mozilla.org/attachment.cgi?id=8656600
> Nevertheless, this is a comple scope creep. Getting rid of the older mozbase
> modules comes first :)


a bootstrap script or single venv has been brought up a number of times. whether it is scope creep or not, ++ to filing and investigating this further.

imagine a world where every slave (regardless of platform) had a single mozharness_venv in a determinable path. though this comes with the cost of mozharness no longer being able to be run with a minimal subset of requirements. Maybe we could keep base/* clean from script specific modules and instead have a mozilla_requirements.txt file
I'd like to point out that mozharness is using a very old version of mozprocess still, one prior to bug 794984. Without that bug, the stdout/stderr pipes can potentially fill up which causes any process that writes to them to block. I don't know if this is happening anywhere in practice or not, but thought it's worth mentioning that fixing this could potentially result in major perf gains (or maybe not).

I'd recommend doing this in two parts:
1) Simply update the mozbase modules under testing/mozharness to current
2) Figure out the bootstrapping problem later
Here's a full try run with mozbase packages in mozharness updated.. I think most of those oranges are known, but the windows "CL" jobs are a bit worrisome:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1cd64de8023c9db17e84325a87ee4e3df7267717
In bug 1362070 I'm upgrading mozprocess in mozharness. I'll create a separate bug for upgrading mozinfo and mozfile, and make that a dependency of this bug too.

After those dependent bugs are resolved, we'll still have work left in this bug to remove the mozharness copies, but getting the copies first in sync should make the second step of removing the duplicate copy a little easier.
Depends on: 1362070
Depends on: 1372239

This makes sure that the mozharness scripts have access to all the packages in
the build system virtualenv (namely mozbase).

Andrew, please make sure to run a try build. I feel this is important given that the mozprocess code has been changed a lot in the last couple months, and which might cause regressions.

Assignee: nobody → ahal
Status: NEW → ASSIGNED
Flags: needinfo?(ahal)

I'm not planning to push this through right now. These patches are up here to demonstrate how we can remove these extra mozbase modules from a technical standpoint.

There are tons of bustages caused by this patch, though so far they are all due to mozbase not being available in a variety of contexts where mozharness is. I might chip away at this over time, but if anyone wants to work on it, please feel free (just ping me first in case I have more changes locally).

Assignee: ahal → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(ahal)

I did a push to Try here:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=46353bca84dc5264dac319d90c462d57cd4cf832

That's based on :ahal's patches, with a small fix for Windows. I don't see any mozharness/mozbase related failures there!

Assignee: nobody → ahal
Status: NEW → ASSIGNED
Attachment #9054579 - Attachment is obsolete: true
Pushed by ahalberstadt@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4de7f94119fd
[ci] Use 'mach python' to run mozharness scripts when we have a srcdir r=catlee
https://hg.mozilla.org/integration/autoland/rev/9645ac1a9851
[mozharness] Remove copies of mozbase from testing/mozharness r=catlee

https://hg.mozilla.org/mozilla-central/rev/4de7f94119fd
https://hg.mozilla.org/mozilla-central/rev/9645ac1a9851

Status: ASSIGNED → RESOLVED
Closed: 2 years ago
status-firefox68: --- → fixed
Resolution: --- → FIXED

[Tracking Requested - why for this release]:

Backed out for l10n bustages e.g. https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=238357055&repo=autoland&lineNumber=364

backout: https://hg.mozilla.org/mozilla-central/rev/93075ec49df3982c26873b822d762bd3d8863fad

[task 2019-04-05T11:09:42.746Z] + python2.7 /builds/worker/workspace/build/src/testing/mozharness/scripts/desktop_l10n.py --clone-locales --list-locales --setup --repack --summary --locale=en-CA:default --locale=he:default --locale=it:default --locale=ja:default --config single_locale/firefox.py --config single_locale/linux32.py --config single_locale/tc_common.py --config single_locale/tc_linux_common.py --log-level=debug --scm-level=3 --work-dir=/builds/worker/workspace/build
[task 2019-04-05T11:09:48.503Z] Traceback (most recent call last):
[task 2019-04-05T11:09:48.503Z] File "/builds/worker/workspace/build/src/testing/mozharness/scripts/desktop_l10n.py", line 20, in <module>
[task 2019-04-05T11:09:48.503Z] from mozharness.base.script import BaseScript
[task 2019-04-05T11:09:48.503Z] File "/builds/worker/workspace/build/src/testing/mozharness/mozharness/base/script.py", line 55, in <module>
[task 2019-04-05T11:09:48.503Z] import mozinfo
[task 2019-04-05T11:09:48.503Z] ImportError: No module named mozinfo

Status: RESOLVED → REOPENED
status-firefox68: fixed → affected
tracking-firefox68: --- → ?
Resolution: FIXED → ---

FYI, the entry point here is taskcluster/scripts/builder/build-l10n.sh, which was still using python2.7 directly.

Thanks, should be a quick fix. This is one of those patches that affects the world and is really hard to catch everything on try, so was expecting a backout tbh.

Should we also try a beta simulation with this patch?

I don't think we need to, unless there are mozharness/taskcluster scripts that exist on beta but not central? I don't think this is something that needs to be uplifted either.

It's not about uplifts but checks that current m-c code would work on beta after the next merge day. If we land as is without running such a merge we could bust the weekly beta-sim jobs sheriffs most likely are still running.

Just waiting on try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1de59939ee5818c8b772402abb06b53d4a191057

I also ended adding the same $GECKO_PATH check to the test-macosx.sh script, even though nothing using that script has a source checkout yet, just to future proof a bit.

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+1] from comment #24)

It's not about uplifts but checks that current m-c code would work on beta after the next merge day. If we land as is without running such a merge we could bust the weekly beta-sim jobs sheriffs most likely are still running.

I'm just not sure how this change could work on central but not beta, it's all self-contained in the same commit. But I'm not very familiar with the merge process so if someone wants to run a beta sim (I'm not sure how), feel free!

Ok, I think I ran one:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a0cdc4a1a2dde46dfb6ebb13cd846a3f32688462

But I realized this is running every task which is definitely overkill. I'll let the builds finish + a couple tests, then cancel the rest.

Pushed by ahalberstadt@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/50495a5da9f6
[ci] Use 'mach python' to run mozharness scripts when we have a srcdir r=catlee
https://hg.mozilla.org/integration/autoland/rev/c28c538fca0a
[mozharness] Remove copies of mozbase from testing/mozharness r=catlee

https://hg.mozilla.org/mozilla-central/rev/50495a5da9f6
https://hg.mozilla.org/mozilla-central/rev/c28c538fca0a

Status: REOPENED → RESOLVED
Closed: 2 years ago → 2 years ago
status-firefox68: affected → fixed
Resolution: --- → FIXED
Regressions: 1542242
See Also: → 1547293
You need to log in before you can comment on or make changes to this bug.