Closed Bug 1132123 Opened 9 years ago Closed 9 years ago

Port B2G Desktop and Mulet builds to mozharness+mach

Categories

(Release Engineering :: Applications: MozharnessCore, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: mshal, Assigned: mshal)

References

Details

Attachments

(9 files)

B2G Desktop & Mulet are built with buildbot calling make targets directly. We should port these over to use mozharness + calling 'mach build' instead of the make targets. This will enable us to use the S3 uploader and move away from FTP, and also provide a better way to build these in Taskcluster directly.

jlund, would we just re-use fx_desktop_build.py for these? One new thing I see in the b2g desktop logs is the call to mozharness/scripts/b2g_desktop_multilocale.py, so I guess that's something we'd have to add.

It looks like buildbot-configs is already setup to do linux64-mulet with fx_desktop_build.py, though the build log suggests that it isn't using mozharness. Any idea what's going on there?
> jlund, would we just re-use fx_desktop_build.py for these? One new thing I
> see in the b2g desktop logs is the call to
> mozharness/scripts/b2g_desktop_multilocale.py, so I guess that's something
> we'd have to add.

for the mh side of things, catlee has a patch that stated some of this work: http://hg.mozilla.org/users/catlee_mozilla.com/mozharness/file/0d630cda3de9/scripts/b2g_desktop_build.py (that rev should have the associated mh configs he uses too)

from the buildbot-config/custom side of things, here are some notes of how we can integrate this:

bear with the braindump...

we normally tell buildbot if a pf should be ran via mh with a pf['mozharness_config'] key. But that got overloaded and so I made a pf['mozharness_desktop_build'] to seperate it from b2g *device* builds and spider builds. pf['mozharness_desktop_build'] uses the same scheduling logic as the ff desktop builds that do not use mh: http://mxr.mozilla.org/build/source/buildbotcustom/misc.py#1152 b2g desktop builds use the same logic too.

essentially, there are three condition branches you can go down for builder logic:
1) http://mxr.mozilla.org/build/source/buildbotcustom/misc.py#1499 and in effect: http://mxr.mozilla.org/build/source/buildbotcustom/misc.py#1499
    a) desktop mh builds that replace part of the responsibility of (3)
2) http://mxr.mozilla.org/build/source/buildbotcustom/misc.py#1513
    a) currently only b2g device and spider builds
3) http://mxr.mozilla.org/build/source/buildbotcustom/misc.py#1577 -> end of generateBranchObjects
   a) non trunk ff desktop builds, xulrunner, android builds, and afaik, b2g desktop builds. this one is the beast, the I do all builder logic for almost all platforms

so by that logic, b2g desktop building/scheduling, at least as far as misc and generateBranchObjects() are concerned, is much more like ff desktop builds than b2g device builds.

In which case, I think what you want to do is extend (1) so that it replaces (3)'s responsibility of b2g desktop build builder logic, like (1) does for ff desktop build builder logic.

> 
> It looks like buildbot-configs is already setup to do linux64-mulet with
> fx_desktop_build.py, though the build log suggests that it isn't using
> mozharness. Any idea what's going on there?

because .... configs :)

seriously though, because two things are needed to do a desktop mh build:
1) the platform variant must support it: we use 'mozharness_desktop_build' key http://mxr.mozilla.org/build/source/buildbot-configs/mozilla/b2g_config.py#436
2) the branch must allow it: we use 'desktop_mozharness_builds_enabled' http://mxr.mozilla.org/build/source/buildbot-configs/mozilla/config.py#2976

desktop_mozharness_builds_enabled is not in b2g_config.py (config.py and b2g_config.py are separate and do not share branch values). iow - you need a similar block like http://mxr.mozilla.org/build/source/buildbot-configs/mozilla/config.py#2973

I can meet and discuss things further if you like
Mulet builds look like they should be straightforward - there aren't any new steps that we don't already do for desktop builds. We should be able to port them over as-is by just setting up the right mozconfig variables. However, I'm confused by the following differences between the 3 mulet build types:

1) OSX & Windows Mulet builds both have enable_checktests: True (so they run 'make -k check'), but Linux does not for some reason. Is this intentional? I would figure we either want to run 'make -k check' for all 3 platforms, or for none of them.

2) Linux & Windows Mulet builds both have packageTests: True (so they run 'make buildsymbols' and 'make package-tests'), but OSX builds do not. Similar to above, I'd expect we want to package-tests for all 3 or for none of them. It doesn't look like any tests are scheduled for these builds, so maybe they should all be off?
Flags: needinfo?(poirot.alex)
Also,

3) Is b2g/dev/config/mozconfigs/macosx-universal/mulet actually used anywhere? I only see a reference to the macosx64 one in buildbot-configs. Should we just remove it?
TBH I have no idea what is the value of 'make -k check', nor buildsymbols and package-tests.
I haven't followed how mulet builds have been automated.
It may be some question for jgriffin or whoever automated mulet at first, or sheriffs that may know if some particular checks are know to fail on mulet.

We do want builds for all platforms as mulet is going to be used to build simulators that is meant to work on all platform. Then we want it to work and not regress. But we may not care about all firefox desktop checks. The most important ones are b2g ones. We want mulet to have the same coverage or more than b2g desktop (gaia tests, webapi, DOM mochitests, ...).
Flags: needinfo?(poirot.alex)
jgriffin, can you provide some insight into #c2 and #c3? From #c4 it sounds like maybe we should set enable_checktests and packageTests to false for all three mulet builds.
Flags: needinfo?(jgriffin)
Punting to Armen; I'm definitely not an expert on buildbot/mozharness builds.
Flags: needinfo?(jgriffin) → needinfo?(armenzg)
Go ahead and disable them except package tests for Linux 64.
We can enable them later if people start caring about them.

We're only running some tests on Linux 64 on Cedar/Try:
https://treeherder.mozilla.org/#/jobs?repo=cedar&filter-searchStr=mulet
Flags: needinfo?(armenzg)
Depends on: 1136985
Depends on: 1137243
This is the mozconfig changes to setup mulet builds for mozharness+mach. Note that mozharness isn't configured to actually build them yet, so these are inactive for now.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8d24612635b6

Linux mulet has:
    export MOZ_AUTOMATION_BUILD_SYMBOLS=1
    export MOZ_AUTOMATION_PACKAGE=1
    export MOZ_AUTOMATION_PACKAGE_TESTS=1
    export MOZ_AUTOMATION_UPLOAD=1

OSX and Windows mulet have:
    export MOZ_AUTOMATION_PACKAGE=1
    export MOZ_AUTOMATION_UPLOAD=1

All other automation steps are disabled.
Assignee: nobody → mshal
Attachment #8571364 - Flags: review?(armenzg)
Comment on attachment 8571364 [details] [diff] [review]
0001-Bug-1132123-Port-mulet-to-mozharness-mach.patch

Review of attachment 8571364 [details] [diff] [review]:
-----------------------------------------------------------------

Passing it to mshal as he's more aware of the build system changes.
Attachment #8571364 - Flags: review?(armenzg) → review?(mshal)
Comment on attachment 8571364 [details] [diff] [review]
0001-Bug-1132123-Port-mulet-to-mozharness-mach.patch

Hmm, don't think I can review my own patch :). bhearsum has kindly volunteered.
Attachment #8571364 - Flags: review?(mshal) → review?(bhearsum)
Comment on attachment 8571364 [details] [diff] [review]
0001-Bug-1132123-Port-mulet-to-mozharness-mach.patch

Review of attachment 8571364 [details] [diff] [review]:
-----------------------------------------------------------------

::: b2g/dev/config/mozconfigs/win32/mulet
@@ +1,4 @@
> +MOZ_AUTOMATION_BUILD_SYMBOLS=0
> +MOZ_AUTOMATION_L10N_CHECK=0
> +MOZ_AUTOMATION_PACKAGE_TESTS=0
> +MOZ_AUTOMATION_INSTALLER=0

What's the reason for the differenc in flgas between platforms? Eg, I'm surprised that BUILD_SYMBOLS is disabled on Mac and Windows, but not on Linux.
According to #c7 we want to have package tests enabled only for linux at the moment, so that's why only linux has MOZ_AUTOMATION_PACKAGE_TESTS=1. And in buildbot land, if package-tests is enabled, then we do buildsymbols as well:

http://mxr.mozilla.org/build/source/buildbotcustom/process/factory.py#1054

So that's why only linux has MOZ_AUTOMATION_BUILD_SYMBOLS=1.

The platform defaults are also slightly different - for example, only Windows has a use for MOZ_AUTOMATION_INSTALLER, but we don't need that in mulet, so only the windows config has MOZ_AUTOMATION_INSTALLER=0. And l10n-check is disabled for OSX, so the mulet OSX config doesn't need to re-specify MOZ_AUTOMATION_L10N_CHECK=0, while Linux & Windows do need to explicitly disable it.
Attachment #8571364 - Flags: review?(bhearsum) → review+
Attached patch mulet-mozharnessSplinter Review
This patch has the mozharness changes. A few notes:

- I took out the seemingly unused MOZ_OBJDIR from the linux config
- I set stage_product to 'b2g' in all the mulet configs, since that looks like it affects the upload directory and I wanted it to match the existing builds
- enable_check_test is false for all platforms per #c7
- only linux has enable_unittest_sendchange per #c7

This seems to build correctly on all 3 in staging with m-c patched and buildbot-configs patched.
Attachment #8572162 - Flags: review?(jlund)
Attached patch bb-muletSplinter Review
And the buildbot changes. Is 39 the right gecko_version?
Attachment #8572165 - Flags: review?(jlund)
Comment on attachment 8572162 [details] [diff] [review]
mulet-mozharness

Review of attachment 8572162 [details] [diff] [review]:
-----------------------------------------------------------------

looks good to me :)
Attachment #8572162 - Flags: review?(jlund) → review+
Comment on attachment 8572165 [details] [diff] [review]
bb-mulet

Review of attachment 8572165 [details] [diff] [review]:
-----------------------------------------------------------------

I think this will work. builder list diff seems right: http://people.mozilla.org/~jlund/mh_mulet_trunk.diff

to be safe, we should clobber the builddirs when this lands (reconfig is done) for affected builders. essentially, the above builderlist diff or: $TRUNK osx,win,linux mulet builders

The reason we would want to do this is because mh uses a different build tree structure and we will likely end up with two different src checkouts in the same builddir.

I can assist if you have not used https://api.pub.build.mozilla.org/clobberer/ before.

::: mozilla/b2g_config.py
@@ +2138,5 @@
>              "https://hg.mozilla.org/%(repo_path)s/raw-file/%(revision)s/" + \
>              "testing/mozharness/mozharness.json"
>  
> +# Enable mozharness desktop builds
> +for name, branch in items_at_least(BRANCHES, 'gecko_version', 39):

39 works. essentially this means that ff desktop builds (as opposed to these b2g desktop builds) will be one release branch ahead as they have already ridden the trains to m-a
Attachment #8572165 - Flags: review?(jlund) → review+
Keywords: leave-open
Depends on: 1140544
Depends on: 1141534
Depends on: 1142063
See Also: → 1097752
Attached patch b2g-desktopSplinter Review
Update the MOZ_AUTOMATION_* variables for the b2g desktop mozconfigs. Just a reminder that the OSX ones look different since l10n-check is off by default on that platform. We don't do l10n-check, uploadsymbols, or update-packaging for any of these builds.
Attachment #8578229 - Flags: review?(bhearsum)
Patch for buildbot-configs to enable mozharness b2g-desktop builds on the maple branch. I'd like to test on maple initially so I can more easily get tests and such running.
Attachment #8578231 - Flags: review?(rail)
Comment on attachment 8578231 [details] [diff] [review]
bb-b2g-desktop-maple

stamp
Attachment #8578231 - Flags: review?(rail) → review+
Attachment #8578229 - Flags: review?(bhearsum) → review+
Attached patch maple-testsSplinter Review
I think this is all that's needed to get b2g desktop tests running on maple - I tested by generating allthethings.json and diffing. Though if you could double-check I'd appreciate it, since I haven't changed test configs before :)
Attachment #8579437 - Flags: review?(rail)
Attachment #8579437 - Flags: review?(rail) → review+
We need something in-tree that points to the gaia l10n repo for B2G desktop builds. This essentially takes the place of the branch-specific gaia_l10n_root in buildbot-configs.

Is there anything else we need to do to make sure it gets updated on merge day like the device config.json files?
Attachment #8581606 - Flags: review?(bhearsum)
Attached patch mh-b2g-desktopSplinter Review
This is the everything patch for building B2G desktop with mozharness. The base configs are no doubt a mess - it's hard to know what is really necessary there and what is just fluff. Diffing those against the base files in configs/builds/ may provide some more insight on what was changed specifically for B2G.

I had some issues getting the environments configured correctly for git/hg checkouts. On Windows, SYSTEMROOT needs to be set, and git.exe needs to be in the PATH (but after everything else in the PATH). It seems gittool.py wasn't including the config environment, so only PATH and GIT_SHARE_BASE_DIR would be set there. Finally, I had to remove PROPERTIES_FILE from the environment before checking out gaia sources, otherwise when building on maple it would try to add refs to a maple branch on gaia, which doesn't exist. If there are better solutions here, let me know.

I still haven't been able to verify that tests run properly on maple, so if those are broken there will be a followup before this lands.
Attachment #8581613 - Flags: review?(jlund)
Attachment #8581613 - Flags: review?(bhearsum)
(In reply to Michael Shal [:mshal] from comment #32)
> Is there anything else we need to do to make sure it gets updated on merge
> day like the device config.json files?

Rail is probably the best person to ask about this. I imagine that the merge day scripts will need an update though.
Attachment #8581606 - Flags: review?(bhearsum) → review+
Comment on attachment 8581613 [details] [diff] [review]
mh-b2g-desktop

Review of attachment 8581613 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Michael Shal [:mshal] from comment #33)
> Created attachment 8581613 [details] [diff] [review]
> mh-b2g-desktop
> 
> This is the everything patch for building B2G desktop with mozharness. The
> base configs are no doubt a mess - it's hard to know what is really
> necessary there and what is just fluff. Diffing those against the base files
> in configs/builds/ may provide some more insight on what was changed
> specifically for B2G.

I'm sad to see so much get cargo culted into new configs. One thing you could do to try to avoid it in the future is start off with a bare minimum, and add things as you find issues. That's how I got the Graphene configs so small (https://github.com/mozilla/build-mozharness/blob/master/configs/builds/releng_sub_linux_configs/64_graphene.py). I'm not going to ask you to go back and do that for this though.

> I had some issues getting the environments configured correctly for git/hg
> checkouts. On Windows, SYSTEMROOT needs to be set, and git.exe needs to be
> in the PATH (but after everything else in the PATH). It seems gittool.py
> wasn't including the config environment, so only PATH and GIT_SHARE_BASE_DIR
> would be set there. Finally, I had to remove PROPERTIES_FILE from the
> environment before checking out gaia sources, otherwise when building on
> maple it would try to add refs to a maple branch on gaia, which doesn't
> exist. If there are better solutions here, let me know.

This sounds....broken. It's probably not your fault, but something somewhere is making some bad assumptions if you have to remove PROPERTIES_FILE to make things work. If you understand this well enough, can you file a bug on it?
Attachment #8581613 - Flags: review?(bhearsum) → review+
(In reply to Ben Hearsum [:bhearsum] from comment #34)
> (In reply to Michael Shal [:mshal] from comment #32)
> > Is there anything else we need to do to make sure it gets updated on merge
> > day like the device config.json files?
> 
> Rail is probably the best person to ask about this. I imagine that the merge
> day scripts will need an update though.

If the configs are branch agnostic or updated by gaia bumper, then you don't need to update anything.
Comment on attachment 8581613 [details] [diff] [review]
mh-b2g-desktop

Review of attachment 8581613 [details] [diff] [review]:
-----------------------------------------------------------------

I didn't get to this today. I'll have a detailed look tomorrow
(In reply to Ben Hearsum [:bhearsum] from comment #35)
> I'm sad to see so much get cargo culted into new configs. One thing you
> could do to try to avoid it in the future is start off with a bare minimum,
> and add things as you find issues. That's how I got the Graphene configs so
> small
> (https://github.com/mozilla/build-mozharness/blob/master/configs/builds/
> releng_sub_linux_configs/64_graphene.py). I'm not going to ask you to go
> back and do that for this though.

Filed bug 1146894.

> This sounds....broken. It's probably not your fault, but something somewhere
> is making some bad assumptions if you have to remove PROPERTIES_FILE to make
> things work. If you understand this well enough, can you file a bug on it?

Filed bug 1146892.
Comment on attachment 8581613 [details] [diff] [review]
mh-b2g-desktop

Review of attachment 8581613 [details] [diff] [review]:
-----------------------------------------------------------------

nice one! looks great. I didn't cross reference all the individual config items against the buildbot ones but looks pretty sane.

::: mozharness/base/vcs/gittool.py
@@ +60,5 @@
>          branch = c.get('branch')
>          clean = c.get('clean')
>          share_base = c.get('vcs_share_base', os.environ.get("GIT_SHARE_BASE_DIR", None))
>          env = {'PATH': os.environ.get('PATH')}
> +        env.update(c.get('env', {}))

I *think* this is okay. It would be bad if we were relying on an env PATH being passed to the script call for other jobs and then we blow it away if we overwrite it in the config.

However, we should smoke out those instances if they exist and fix that anyway.

yes, gittool.py and hgtool.py use PROPERTIES_FILE a lot and make some unfortunate assumptions

::: scripts/b2g_desktop_build.py
@@ +34,5 @@
> +
> +            gaia_rev = self.vcs_checkout(**vcs_checkout_kwargs)
> +            self.set_buildbot_property('gaia_revision', gaia_rev, write_to_file=True)
> +        except Exception:
> +            self.exception("failed to get the git revision from gaia.json")

`except Exception` could potentially catch a lot of errors. I wonder if it's is too generic for your exception message?

@@ +53,5 @@
> +            l10n_base_dir = os.path.join(dirs['abs_work_dir'], 'build-gaia-l10n')
> +
> +            env = self.query_env()
> +            env['LOCALE_BASEDIR'] = l10n_base_dir
> +            env['LOCALES_FILE'] = languages_file

I think you're reassigning the `env` var here and it is not being used. Maybe this was old code? I see l10n_base_dir and language_file is just passed outside of the env to pull_gaia_locale_source. Are you expecting LOCALE_BASEDIR or LOCALES_FILE to be in the env too?
Attachment #8581613 - Flags: review?(jlund) → review+
(In reply to Jordan Lund (:jlund) from comment #42)
> ::: mozharness/base/vcs/gittool.py
> @@ +60,5 @@
> >          branch = c.get('branch')
> >          clean = c.get('clean')
> >          share_base = c.get('vcs_share_base', os.environ.get("GIT_SHARE_BASE_DIR", None))
> >          env = {'PATH': os.environ.get('PATH')}
> > +        env.update(c.get('env', {}))
> 
> I *think* this is okay. It would be bad if we were relying on an env PATH
> being passed to the script call for other jobs and then we blow it away if
> we overwrite it in the config.

This is what hgtool does too, so I think it is helpful that they are consistent. I haven't seen other side-effects from this on maple yet.

> ::: scripts/b2g_desktop_build.py
> @@ +34,5 @@
> > +
> > +            gaia_rev = self.vcs_checkout(**vcs_checkout_kwargs)
> > +            self.set_buildbot_property('gaia_revision', gaia_rev, write_to_file=True)
> > +        except Exception:
> > +            self.exception("failed to get the git revision from gaia.json")
> 
> `except Exception` could potentially catch a lot of errors. I wonder if it's
> is too generic for your exception message?

Per IRC, I've changed this to "failed to checkout gaia"

> > +            l10n_base_dir = os.path.join(dirs['abs_work_dir'], 'build-gaia-l10n')
> > +
> > +            env = self.query_env()
> > +            env['LOCALE_BASEDIR'] = l10n_base_dir
> > +            env['LOCALES_FILE'] = languages_file
> 
> I think you're reassigning the `env` var here and it is not being used.
> Maybe this was old code? I see l10n_base_dir and language_file is just
> passed outside of the env to pull_gaia_locale_source. Are you expecting
> LOCALE_BASEDIR or LOCALES_FILE to be in the env too?

This is setting up the environment for the build, since gaia's build system needs LOCALE_BASEDIR and LOCALES_FILE set in order to pull in the gaia l10n sources. I've added a comment clarifying this, since it's pretty confusing.
Attached patch bb-b2g-desktopSplinter Review
I think this is the last bit, aside from a mozharness bump on m-c. Note that this should already be limited to 39+ because the mulet builds added this to b2g_config.py:

# Enable mozharness desktop builds
for name, branch in items_at_least(BRANCHES, 'gecko_version', 39):
    # if true, any platform with mozharness_desktop_build in its config
    # will use mozharness instead of MozillaBuildFactory
    branch['desktop_mozharness_builds_enabled'] = True


I think as long as this lands in the next day or two that should still be valid (ie: we won't have to uplift or delete this key for b2g desktop builds on 39). Does that sound accurate?

Also since linux32 is gone and linux64 is moving to TC, this just switches mac+windows b2g desktop.
Attachment #8583338 - Flags: review?(bhearsum)
Attachment #8583338 - Flags: review?(bhearsum) → review+
(In reply to Michael Shal [:mshal] from comment #44)
> Comment on attachment 8581613 [details] [diff] [review]
> mh-b2g-desktop
> 
> https://hg.mozilla.org/build/mozharness/rev/5c90087fe080

correct me if I am wrong but it still looks like this assignment of 'env' var is not being used anywhere: https://hg.mozilla.org/build/mozharness/rev/5c90087fe080#l12.60
(In reply to Jordan Lund (:jlund) from comment #46)
> correct me if I am wrong but it still looks like this assignment of 'env'
> var is not being used anywhere:
> https://hg.mozilla.org/build/mozharness/rev/5c90087fe080#l12.60

Sorry if I wasn't clear. This is setting the variables in the build environment, so they show up like this when running commands from mozharness:

16:13:01     INFO -  'LOCALES_FILE': '/builds/slave/m-cen-osx64_g-0000000000000000/build/src/gaia/locales/languages_all.json',
16:13:01     INFO -  'LOCALE_BASEDIR': '/builds/slave/m-cen-osx64_g-0000000000000000/build/build-gaia-l10n',

query_env() just returns ScriptMixin.env, and we're updating the keys in there directly. So we don't need to pass 'env' around elsewhere in order for those environment settings to persist.

Is there a better way to do that so it's more clear? I don't mind fixing it as a followup if so.
Flags: needinfo?(jlund)
(In reply to Michael Shal [:mshal] from comment #48)
> (In reply to Jordan Lund (:jlund) from comment #46)
> > correct me if I am wrong but it still looks like this assignment of 'env'
> > var is not being used anywhere:
> > https://hg.mozilla.org/build/mozharness/rev/5c90087fe080#l12.60
> 
> Sorry if I wasn't clear. This is setting the variables in the build
> environment, so they show up like this when running commands from mozharness:
> 
> 16:13:01     INFO -  'LOCALES_FILE':
> '/builds/slave/m-cen-osx64_g-0000000000000000/build/src/gaia/locales/
> languages_all.json',
> 16:13:01     INFO -  'LOCALE_BASEDIR':
> '/builds/slave/m-cen-osx64_g-0000000000000000/build/build-gaia-l10n',
> 
> query_env() just returns ScriptMixin.env, and we're updating the keys in
> there directly. So we don't need to pass 'env' around elsewhere in order for
> those environment settings to persist.
> 
> Is there a better way to do that so it's more clear? I don't mind fixing it
> as a followup if so.

your right, sorry. I forgot query_env() had a side effect. we could do:

self.env = self.query_env()

just to be more explicit but I don't mind. ignore me
Flags: needinfo?(jlund)
Ok :) - I think we can close this one then.
Status: NEW → RESOLVED
Closed: 9 years ago
Keywords: leave-open
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.