978211 - Establish a proxy script for performing automation work

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Gregory Szorc [:gps]]

I just had a meeting with :rail, :armenzg, and :jlund to talk about the interaction between automation and the build system / tree. Full notes are available at https://etherpad.mozilla.org/build-releng-integration

The major action item from that meeting is that we're going to establish a mach-like proxy script in the tree for performing automation tasks. Essentially, all interaction between automation and the tree will be marshaled through a common script.

There are major benefits to this approach:

* It is easy to audit what automation can do (just look at what the script can do). (Currently we need to look at buildbot configs, mozharness, and logs.)
* The policy around what automation is allowed to do is clearly defined (if it's defined in the script, it's likely allowed). This will establish clear ownership of failures when things go wrong (currently things like l10n repacks have nebulous ownership).
* Introduces a level of indirection between automation and the tree. This allows refactoring to occur in the tree with minimal regard for how automation works (just preserve the existing/simple interface). This should allow us to move faster. It also allows automation to focus on automation and the tree on tree stuff. (Yay separation of concerns.)

The initial goal is to find every command that buildbot/mozharness invokes in the tree and make that command go through this proxy script. For example, to perform the core build step, automation currently runs:

  make -f client.mk build MOZ_BUILD_DATE=20140228054330

With this script in place, automation will do something like:

  ./run-automation build --build-date=20140228054330

That will allow the tree to do things like use an appropriate build backend (currently this is coded in automation and we can't e.g. switch from GNU make to pymake to mozmake to Tup without coordination from automation).

To package, automation currently runs:

  make package MOZ_PKG_PRETTYNAMES=1

With this script in place, automation will do something like:

  ./run-automation package

An eventual goal is to reduce the interaction between automation and the tree into as few steps as possible. e.g. instead of running compile + package + symbol generation, automation will do something like:

  ./run-automation build --config=win32opt

Like mozharness, this should help local developers reproduce things done in automation outside of automation. But the major goals here are to a) simplify the relationship between automation and the tree thus reducing the surface area for bugs and incompatibilities and b) move more logic into single steps so it can be optimized easier (e.g. the build system could perform symbol generation concurrently with packaging).

Currently, only things performed in build and related jobs (like l10n repacks) are in scope for moving to this script. Test jobs are *not* in scope because they don't run /directly/ against the tree.

Initially, we're going to focus on moving all automation invocations of "make" to use this proxy. In that set, we're going to focus on removing calls to client.mk first (because we want to kill client.mk - it's been effectively replaced by mach).

As for technical details, the in-tree script will likely be Python. I'm open to the idea of using mach. But I don't want to pollute mach with automation targets (not yet anyway). Later, we can have automation switch to use mach and/or we can hook mach up to this proxy script.

I'm very excited about the potential for this new approach!

Comment 1

[(not currently active) Ted Mielczarek]

+1

We started moving in this direction a few years ago when we established targets like "make buildsymbols" and "make upload" as simple top-level targets.

I understand the desire to not pollute mach with automation-specific targets, but is there any reason not to reuse the mach framework and simply load a different set of commands? It feels like under the covers we'll wind up wanting to execute a lot of the same things in the same ways.

Comment 2

[Gregory Szorc [:gps]]

Since mach is a generic library for doing command dispatch to Python functions, I'd say there is very highly chance we'll leverage the mach core for dispatch :)

Comment 3

[Chris AtLee [:catlee]]

+1e6

hopefully we can backport this to older release branches and get rid of a bunch of legacy code at the same time

Comment 4

[Steve Fink [:sfink] [:s:]]

I probably should have read this bug before filing bug 978865, especially since I was triggered by the dev-platform discussion thread for this bug, but it's still relevant (and related.)

Comment 5

[Michael Shal [:mshal]]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #1)
> I understand the desire to not pollute mach with automation-specific
> targets, but is there any reason not to reuse the mach framework and simply
> load a different set of commands? It feels like under the covers we'll wind
> up wanting to execute a lot of the same things in the same ways.

Can you or gps elaborate on why we want a separate script instead of just mach? I think there would be a lot of duplication between the two otherwise - ie: do we really want do have a 'run-automation build' and a 'mach build' that presumably do the same thing? (Or worse, do different things?) I realize under the hood they can call the same function, but I think having multiple entry points for it is confusing.

Additionally, if developers are used to mach being the "interface for everything" in the build system, why do we want to separate out the automation-y commands? Especially if we want to make it easier for devs to reproduce automation steps locally, I think it makes sense to run it from the same interface (mach).

I'm still getting caught up on the discussion surrounding this, so if I missed some details that are obvious to you guys feel free to point them out to me :)

Comment 6

[Gregory Szorc [:gps]]

Initially and possibly indefinitely I would like separate entry points so we:

a) clearly identify a supported API for automation
b) don't pollute mach

I don't want automation calling into any possible mach command because not all mach commands are created equal. Some are more fit for consumption from automation than others.

Having an explicit automation entry point clearly defines the supported API and establishes a higher bar for adding automation-runnable code. (The bar for adding mach commands is much lower.) This ensures that automation continues to run only run things that it should.

In the beginning of this effort, we'll likely be creating many "targets" for automation. Many of these will likely be eventually collapsed. I'd rather not pollute mach with these next-to-useless commands.

That being said, we could probably add a backdoor to build/mach_bootstrap.py where if a certain environment variable (MOZ_AUOTMATION=1 or some such) is defined we'll load extra commands, etc. But I like keeping code paths clean and consistent. mach_bootstrap.py is already a horrible hack, especially for things like virtualenv interaction. I'd like to maintain flexibility for changing mach_bootstrap.py without significant automation implications for the time-being. Long term, we can always merge the entry points. Until then, I like maintaining flexibility.

Comment 7

[Steve Fink [:sfink] [:s:]]

You could call it "automach". Then a connection is clearly suggested, yet it's still different.

Please don't hate me.

Comment 8

[Michael Shal [:mshal]]

(In reply to Gregory Szorc [:gps] from comment #6)
> That being said, we could probably add a backdoor to build/mach_bootstrap.py
> where if a certain environment variable (MOZ_AUOTMATION=1 or some such) is
> defined we'll load extra commands, etc. But I like keeping code paths clean
> and consistent.

Yeah, I'd definitely favor a separate command instead of triggering off of an environment variable. I was more just curious what the reasons were since I didn't find them spelled out anywhere. Thanks for clearing it up!

Comment 9

[Mike Hommey [:glandium]]

(In reply to Gregory Szorc [:gps] from comment #6)
> That being said, we could probably add a backdoor to build/mach_bootstrap.py
> where if a certain environment variable (MOZ_AUOTMATION=1 or some such) is
> defined we'll load extra commands, etc.

I think it should be the opposite, and I'd rather do it now. By the opposite, I mean having the extra commands handled by mach, but MOZ_AUTOMATION would makes mach reject those that are not automation commands.

Comment 10

[Nick Alexander :nalexander [he/him]]

(In reply to Mike Hommey [:glandium] from comment #9)
> (In reply to Gregory Szorc [:gps] from comment #6)
> > That being said, we could probably add a backdoor to build/mach_bootstrap.py
> > where if a certain environment variable (MOZ_AUOTMATION=1 or some such) is
> > defined we'll load extra commands, etc.
> 
> I think it should be the opposite, and I'd rather do it now. By the
> opposite, I mean having the extra commands handled by mach, but
> MOZ_AUTOMATION would makes mach reject those that are not automation
> commands.

I don't feel too strongly one way or the other -- I think the code will get inter-twined very quickly -- but this sounds like a mach condition (is_automation?).

Comment 11

[Gregory Szorc [:gps]]

If we really want automation going through mach (with a backdoor to trigger additional functionality), we can do that. I just fear we're needlessly painting ourselves into a corner, especially for things like sys.path and virtualenv interaction.

Meh. Worse case ./mach and build/mach_bootstrap.py have an if statement at the beginning of execution. It's a bit annoying to maintain and test. But such is the life of build peers.

Let's just use ./mach </bikeshed>

Comment 12

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

So who's going to own this?

Comment 13

[Gregory Szorc [:gps]]

I believe all the build peers are allocated to other work now and for the foreseeable future. I'd put this near the top of our priorities list. But there is still a resourcing problem.

I don't think the initial implementation would be too bad. I'd just add a branch to build/mach_bootstrap.py that has separate behavior in automation. It can consist of a single command initially (anyone have any good ideas - "build" might be too complicated). We can consolidate things over time.

Comment 14

[(not currently active) Ted Mielczarek]

I don't think build would be all that bad, really, considering the current setup just calls "make -f client.mk", and that's what mach build does under the covers anyway.

Other simple targets that would be trivial to convert to the new way of doing things:
package
package-tests
upload
buildsymbols
uploadsymbols

We could mostly swap those out wholesale, and then if we wanted to consolidate later (i.e., have "build" do all of the building + packaging work) we could simply make the other targets no-ops and then remove them. (Ideally in the future the mozharness commands to run would be in-tree so we could make these sorts of changes easily.)

Comment 15

[Jordan Lund (:jlund)]

==== status update

mshal, glandium, and myself met today on this. An implementation strategy was formed and this bug will be actively be worked on from today.

Comment 16

[Jordan Lund (:jlund)]

I have created an implementation strategy and roll-out plan: https://docs.google.com/document/d/1N3xi3drA49_5zYiJ8J3VdYfQ05Je5CNnF5r3XLO_D4E/edit?usp=sharing

This is based off a discussion I had with mshal and glandium. The doc also includes knowledge transfer information to help get the build team up to speed with what and how things are done in automation.

Comment 17

[Jordan Lund (:jlund)]

as per: doc in https://bugzilla.mozilla.org/show_bug.cgi?id=978211#c16,

Rollout Plan:

Overview:
1) on a dev buildbot setup, get all Linux variants working


Detailed:

To complete step 1)

from releng side:

1) give mshal and glandium a ec2 loan instance
    * done: bug 1006178
2) connect that instance to a dev buildbot master to test runs in automation
    * done
3) branch mozharness, remove all actions that are being ported to build configs, and add ‘mach build’ step
    * done: https://github.com/lundjordan/mozharness/compare/fx-desktop-builds...build-cfg-integration
4) point new mozharness branch to dev buildbot master
    * done
5) point a forked user m-c branch that contains new mozconfig and build config logic to dev buildbot master
    * not done.

Things should be set up now. Once there is a branch or something landed on m-c to test 'mach build' against https://github.com/lundjordan/mozharness/blob/build-cfg-integration/mozharness/mozilla/building/buildbase.py#L876, we can resume progress.

mshal, glandium, I can meet anytime to sync up on this and I'm available to continue making iterations from mozharness/buildbot end to suit your changes :)

Comment 18

[Jordan Lund (:jlund)]

mshal, glandium,

ping - want to meet, say, Monday? I just want to make sure that I am not blocking anything as I have moved on to other projects for the moment. My zimbra calendar is fairly accurate for mtg schedule :)

Comment 19

[Jordan Lund (:jlund)]

mshal touched base via irc and is available at 1630PT.

to be clear I do not *need* to meet. As https://bugzilla.mozilla.org/show_bug.cgi?id=978211#c17 states, I have set up everything from my end. I am only suggesting to meet to address any follow up questions the build config team may have.

mshal, glandium if you find https://docs.google.com/document/d/1N3xi3drA49_5zYiJ8J3VdYfQ05Je5CNnF5r3XLO_D4E/edit?usp=sharing sufficient and you're already working on this, we can skip touching base; I'll make myself available as you need :)

Comment 20

[Jordan Lund (:jlund)]

I touched base with glandium and mshal.

They figured out some details on build config side. I will continue to be available for anything needed on releng side.

Comment 21

[Michael Shal [:mshal]]

Attachment: 0002-Bug-978211-add-an-automationbuild-target-for-post-bu.patch

I'm just looking for feedback for the general approach so far, since this patch still doesn't account for mar generation, and hasn't been fully tested yet (we've just managed a successful linux m-c build).

Rather than move everything currently in the Makefiles into various mach targets, I've just added an "automationbuild" target that depends on various auto-* targets. The auto-* targets just call the actual underlying make targets used by build automation, but they also have dependencies among each other.

We can still move things into mach later if we want, but for now this gets us a single 'MOZ_AUTOMATION=1 ./mach build' step from automation to do all of the build+packaging+etc, and all post-build steps can be done in parallel according to the DAG.

Comment 22

[Mike Hommey [:glandium]]

Comment on attachment 8424080 [details] [diff] [review]
0002-Bug-978211-add-an-automationbuild-target-for-post-bu.patch

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

::: Makefile.in
@@ +134,5 @@
>  
> +ifdef NIGHTLY_BUILD
> +automationbuild: auto-uploadsymbols
> +endif
> +automationbuild: auto-package-tests auto-package auto-upload auto-l10n-check auto-check

For easier future changes, one dep per line might be better.

@@ +135,5 @@
> +ifdef NIGHTLY_BUILD
> +automationbuild: auto-uploadsymbols
> +endif
> +automationbuild: auto-package-tests auto-package auto-upload auto-l10n-check auto-check
> +auto-uploadsymbols: auto-buildsymbols

bikeshedding: automation/%

::: client.mk
@@ +181,5 @@
>  # Rules
>  
>  # The default rule is build
>  build::
> +	$(MAKE) -f $(TOPSRCDIR)/client.mk $(if $(MOZ_PGO),profiledbuild,realbuild) $(if $(MOZ_AUTOMATION),automationbuild)

Maybe better to put automationbuild on a separate line, in case we ever want to remove .NOTPARALLEL from client.mk.

Comment 23

[Jordan Lund (:jlund)]

Lot's of progress here.

We re-added mozharness logic for things like:

1) reading app.ini for sourcestamp, appversion, etc and sets props accordingly
2) set stats for ctors, vsize and graphserverpost
3) setting the post_upload_cmd

mshal has all make targets for linux working and saves properties to a mach_build_properties.json in the objdir. This gets picked up by mozharness during postflight_build and uplifted to buildbot/tbpl along with other props.

Finally mozharness uses BalrogMixin and can successfully submit updates for nightly builds.

Seems like both sides are coming together. Next steps:

1) differentiate between linux branch and build variants.
** The subtle differences between say a PGO, asan, debug, etc build and the branch running against are largely complete from mozharness end. But now that build configs do post build make targets (package, upload, etc), either mozharn will need to set an env var to tell the build configs which targets to run or the build config logic will have to learn all the edge cases itself.

2) land linux changes on Cedar

3) implement windows (all variant/branch builds)

3) land windows on Cedar

4) implement mac osx (all variant/branch builds)

5) land osx on Cedar

6) land across all m-c based branches including try

7) ride the trains

Comment 24

[Jordan Lund (:jlund)]

example of a linux nightly build using mozharness+buildconfigs: http://people.mozilla.org/~jlund/nigthly_build_config

Comment 25

[Michael Shal [:mshal]]

Attachment: 0001-Bug-978211-add-an-automationbuild-target-for-post-bu.patch

This should be a good start, though it likely will need some touching up as we test out all the various platforms & configurations. In the meantime, it shouldn't break any existing buildbot or user workflows.

Comment 26

[Mike Hommey [:glandium]]

Comment on attachment 8431780 [details] [diff] [review]
0001-Bug-978211-add-an-automationbuild-target-for-post-bu.patch

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

I'm really not convinced by the whole gen_mach_buildprops.py file. See inline comments.

::: build/gen_mach_buildprops.py
@@ +32,5 @@
> +    json_data.update({
> +        'completeMarFilename': os.path.basename(filename),
> +        'completeMarSize': complete_mar_size,
> +        'completeMarHash': complete_mar_hash,
> +    })

Better to just return that dict, and have the caller do the update. Likewise in setUrlProperties.

@@ +50,5 @@
> +        ('completeMarUrl', "m.endswith('.complete.mar')"),
> +    ]
> +
> +    with open(filename) as f:
> +        for line in f.readlines():

for line in f:

@@ +53,5 @@
> +    with open(filename) as f:
> +        for line in f.readlines():
> +            prop_assigned = False
> +            pat = r'''^(https?://.*?\.(?:tar\.bz2|dmg|zip|apk|rpm|mar|tar\.gz))$'''
> +            m = re.compile(pat).match(line)

You're compiling the regexp for each line.

@@ +57,5 @@
> +            m = re.compile(pat).match(line)
> +            if m:
> +                m = m.group(1)
> +                for prop, condition in property_conditions:
> +                    if eval(condition):

Rule #1 of scripting languages: stay away from eval as much as you can. Just make condition a function and do "if condition(m):"
And since defining a function for each property condition can be verbose, you can use lambdas. e.g. property_conditions = [('symbolsUrl', lambda m: m.endswith('crashreporter-symbols.zip') or m.endswith('crashreporter-symbols-full.zip')), ...]

@@ +64,5 @@
> +                        break
> +                if not prop_assigned:
> +                    # if we found a match but haven't identified the prop then this
> +                    # is the packageURL. Let's consider this the else block
> +                    json_data.update({'packageUrl': m})

Since property_conditions is a list, iterating over it is deterministic. If you add ('packageUrl': lambda m: true) as last element of the list, you can remove this branch, and remove prop_assigned.

You can also compact the whole loop to something like (untested):
regexp = re.compile(pat)
matches = regexp.match(line) for line in f
matches = m.group(1) for m in matches if m
return {prop: m for m in matches for prop, m in any((prop, m) for prop, condition in property_conditions if condition(m))}

That said, what is it you're replacing with this? It seems to me you just want the build system to emit the individual variables that end up in UPLOAD_FILES, and I'm not convinced parsing the upload output log is the best way to do that. In fact, it's probably already not working on try, because we also upload sccache.log.gz, which is likely to become the packageUrl by the current logic in this script.

@@ +67,5 @@
> +                    # is the packageURL. Let's consider this the else block
> +                    json_data.update({'packageUrl': m})
> +
> +if __name__ == '__main__':
> +    parser = OptionParser(usage="usage: %prog [options]")

Use ArgumentParser.

@@ +70,5 @@
> +if __name__ == '__main__':
> +    parser = OptionParser(usage="usage: %prog [options]")
> +    parser.add_option("--complete-mar-file",
> +                      action="store", dest="complete_mar_file",
> +                      help="Path to the complete MAR file, relative to the objdir.")

with ArgumentParser, you can add required=True here, and avoid doing the check yourself.

::: build/moz-automation.mk
@@ +6,5 @@
> +include $(topsrcdir)/toolkit/mozapps/installer/package-name.mk
> +
> +# Setup post-build dependencies. The automationbuild target is the main target
> +# called by client.mk if MOZ_AUTOMATION is set.
> +AUTOMATION_UPLOAD_OUTPUT = automation-upload.txt

I'd rather put this file in $(DIST)

@@ +10,5 @@
> +AUTOMATION_UPLOAD_OUTPUT = automation-upload.txt
> +
> +automationbuild:
> +	$(PYTHON) $(topsrcdir)/build/gen_mach_buildprops.py --complete-mar-file $(DIST)/$(COMPLETE_MAR) --upload-output $(AUTOMATION_UPLOAD_OUTPUT)
> +

# Automation build steps

@@ +12,5 @@
> +automationbuild:
> +	$(PYTHON) $(topsrcdir)/build/gen_mach_buildprops.py --complete-mar-file $(DIST)/$(COMPLETE_MAR) --upload-output $(AUTOMATION_UPLOAD_OUTPUT)
> +
> +ifdef NIGHTLY_BUILD
> +automationbuild: automation-uploadsymbols

automation/build: automation/uploadsymbols (likewise for the others)

@@ +27,5 @@
> +automationbuild: automation-package-tests
> +automationbuild: automation-package
> +automationbuild: automation-upload
> +automationbuild: automation-check
> +

# Dependencies between automation build steps

@@ +37,5 @@
> +automation-upload: automation-update-packaging
> +
> +automation-l10n-check: automation-package
> +
> +automation-update-packaging: automation-package

Something tells me the comment about buildsymbols changing the binaries apply here too.

@@ +50,5 @@
> +AUTOMATION_FLAGS-check = -k
> +
> +# We need the log from make upload to grep it for urls in order to set
> +# properties.
> +AUTOMATION_FLAGS-upload = 2>&1 | tee $(AUTOMATION_UPLOAD_OUTPUT)

Technically not really flags. I don't have a better suggestion for a name, though.

@@ +53,5 @@
> +# properties.
> +AUTOMATION_FLAGS-upload = 2>&1 | tee $(AUTOMATION_UPLOAD_OUTPUT)
> +
> +automation-%:
> +	@echo "Started $@: $(MAKE) $* $(AUTOMATION_FLAGS-$*)"

Please use something like $(call BUILDSTATUS, TIER_START/TIER_FINISH, $*), and augment TIERS in Makefile.in. Bonus points if you automatically check that $* here is in TIERS there.

Comment 27

[Michael Shal [:mshal]]

(In reply to Mike Hommey [:glandium] from comment #26)
> You can also compact the whole loop to something like (untested):
> regexp = re.compile(pat)
> matches = regexp.match(line) for line in f
> matches = m.group(1) for m in matches if m
> return {prop: m for m in matches for prop, m in any((prop, m) for prop,
> condition in property_conditions if condition(m))}

I feel the loop is a little more explicit & easy to understand. I've made the other requested changes, though.

> That said, what is it you're replacing with this? It seems to me you just
> want the build system to emit the individual variables that end up in
> UPLOAD_FILES, and I'm not convinced parsing the upload output log is the
> best way to do that. In fact, it's probably already not working on try,
> because we also upload sccache.log.gz, which is likely to become the
> packageUrl by the current logic in this script.

Perhaps jlund knows more of the history here, but this is meant to replace the buildbotcustom/mozharness code that sets some of these properties. See:

http://mxr.mozilla.org/build/source/buildbotcustom/process/factory.py#202
http://mxr.mozilla.org/build/source/mozharness/mozharness/mozilla/building/buildbase.py#89

(The mozharness code is what I used as a starting point for gen_mach_buildprops.py)

Some of the properties are already known or easily knowable by m-c (eg: the mar filename, as well as its hash and size). The *Url properties are unfortunately not easily determined by m-c (or buildbot or mozharness), since it relies on the post_upload.py tools script and values in post_upload.ini on the upload servers. In other words, if we want to generate the properties without grepping for 'http://...', we would need to duplicate some of post_upload.ini and post_upload.py logic in m-c (as far as I know - I'd love to be wrong about this!)

We could leave the grepping logic in mozharness for this bug, but since we're now combining the output of the entire build/package/tests/check/etc into one big log, we thought it would make sense if m-c just grepped the upload output rather than have mozharness grep the entire output.

> @@ +37,5 @@
> > +automation-upload: automation-update-packaging
> > +
> > +automation-l10n-check: automation-package
> > +
> > +automation-update-packaging: automation-package
> 
> Something tells me the comment about buildsymbols changing the binaries
> apply here too.

Can you elaborate here? I think these steps all depend transitively on buildsymbols. Which two targets do you think will conflict?

> 
> @@ +50,5 @@
> > +AUTOMATION_FLAGS-check = -k
> > +
> > +# We need the log from make upload to grep it for urls in order to set
> > +# properties.
> > +AUTOMATION_FLAGS-upload = 2>&1 | tee $(AUTOMATION_UPLOAD_OUTPUT)
> 
> Technically not really flags. I don't have a better suggestion for a name,
> though.

AUTOMATION_EXTRA-upload ?
AUTOMATION_EXTRA_CMDLINE-upload ?

I was also considering just making each step tee out to a log, and cat the log when the step finishes. Though that would combine stdout/stderr for all steps if that's a concern. It would make it easier to track down errors in some cases since we can be running ~3 of these steps in parallel. Thoughts?

> 
> @@ +53,5 @@
> > +# properties.
> > +AUTOMATION_FLAGS-upload = 2>&1 | tee $(AUTOMATION_UPLOAD_OUTPUT)
> > +
> > +automation-%:
> > +	@echo "Started $@: $(MAKE) $* $(AUTOMATION_FLAGS-$*)"
> 
> Please use something like $(call BUILDSTATUS, TIER_START/TIER_FINISH, $*),
> and augment TIERS in Makefile.in. Bonus points if you automatically check
> that $* here is in TIERS there.

It looks like we already get an error from mozbuild if the value isn't in TIERS:

Traceback (most recent call last):
  File "/usr/lib/python2.7/threading.py", line 810, in __bootstrap_inner
    self.run()
  File "/usr/lib/python2.7/threading.py", line 763, in run
    self.__target(*self.__args, **self.__kwargs)
  File "/home/marf/mozilla-central-git/testing/mozbase/mozprocess/mozprocess/processhandler.py", line 774, in _processOutput
    self.processOutputLine(line.rstrip())
  File "/home/marf/mozilla-central-git/testing/mozbase/mozprocess/mozprocess/processhandler.py", line 719, in processOutputLine
    handler(line)
  File "/home/marf/mozilla-central-git/python/mach/mach/mixin/process.py", line 86, in handleLine
    line_handler(line)
  File "/home/marf/mozilla-central-git/python/mozbuild/mozbuild/mach_commands.py", line 243, in on_line
    warning, state_changed, relevant = self.monitor.on_line(line)
  File "/home/marf/mozilla-central-git/python/mozbuild/mozbuild/controller/building.py", line 223, in on_line
    self.tiers.begin_tier(tier)
  File "/home/marf/mozilla-central-git/python/mozbuild/mozbuild/controller/building.py", line 82, in begin_tier
    t = self.tiers[tier]
KeyError: u'foo'

Did you still want to add an extra check in the Makefile?

I had to modify mozbuild slightly to handle multiple active tiers, but it's a small change.

Comment 28

[Michael Shal [:mshal]]

Attachment: 0001-Bug-978211-add-an-automation-build-target-for-post-b.patch

Comment 29

[Mike Hommey [:glandium]]

Comment on attachment 8433686 [details] [diff] [review]
0001-Bug-978211-add-an-automation-build-target-for-post-b.patch

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

Will look at this later, but just adding a comment for now:

::: build/moz-automation.mk
@@ +23,5 @@
> +
> +ifdef MOZ_UPDATE_PACKAGING
> +MOZ_AUTOMATION_TIERS += update-packaging
> +automation/upload: automation/update-packaging
> +automation/update-packaging: automation/package

The point I was making in previous review is that update-packaging presumably needs the updates that buildsymbols does to have been done.

Comment 30

[Jordan Lund (:jlund)]

> > That said, what is it you're replacing with this? It seems to me you just
> > want the build system to emit the individual variables that end up in
> > UPLOAD_FILES, and I'm not convinced parsing the upload output log is the
> > best way to do that. In fact, it's probably already not working on try,
> > because we also upload sccache.log.gz, which is likely to become the
> > packageUrl by the current logic in this script.
> 
> Perhaps jlund knows more of the history here, but this is meant to replace
> the buildbotcustom/mozharness code that sets some of these properties. See:
> 
> http://mxr.mozilla.org/build/source/buildbotcustom/process/factory.py#202
> http://mxr.mozilla.org/build/source/mozharness/mozharness/mozilla/building/
> buildbase.py#89
> 
> (The mozharness code is what I used as a starting point for
> gen_mach_buildprops.py)

ya, as mentioned this code is based off mozharness code. I was experimenting with trying a switch/case to do this logic but it should mirror the behavior found in buildbot.

There is probably a better way than parsing the output of make upload to set props but this is how it is currently done everywhere in production (not just in mozharn builds). As far as my version in mozharness not working on Try, I am not sure as they are not enabled on Try yet.

However, the actual regex pattern in buildbot(http://mxr.mozilla.org/build/source/buildbotcustom/process/factory.py#208) is the same as in mozharness (http://mxr.mozilla.org/build/source/mozharness/mozharness/mozilla/building/buildbase.py#113) and both fall back on to else block that assigns packageUrl (http://mxr.mozilla.org/build/source/buildbotcustom/process/factory.py#235)

would 'sccache.log.gz' actually match against '^(https?://.*?\.(?:tar\.bz2|dmg|zip|apk|rpm|mar|tar\.gz))$'?

the '.gz' extension also requires 'tar' IIUC.

Comment 31

[Mike Hommey [:glandium]]

(In reply to Jordan Lund (:jlund) from comment #30)
> would 'sccache.log.gz' actually match against
> '^(https?://.*?\.(?:tar\.bz2|dmg|zip|apk|rpm|mar|tar\.gz))$'?
> 
> the '.gz' extension also requires 'tar' IIUC.

Ah, i forgot this is filtered already. That seems generally fragile, but if it's already what's done by buildbot, there's not much point arguing. That can be fixed later, when the in-tree version.

Comment 32

[Mike Hommey [:glandium]]

(and since the code comes from mozharness, the mozharness version should be fixed wrt comment 26.

Comment 33

[Mike Hommey [:glandium]]

Comment on attachment 8433686 [details] [diff] [review]
0001-Bug-978211-add-an-automation-build-target-for-post-b.patch

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

::: build/gen_mach_buildprops.py
@@ +72,5 @@
> +                        help="Path to the text output of 'make upload'")
> +    args = parser.parse_args()
> +
> +    json_data = {}
> +    json_data.update(getMarProperties(args.complete_mar_file))

You could just do
json_data = getMarProperties(args.complete_mar_file)

::: build/moz-automation.mk
@@ +23,5 @@
> +
> +ifdef MOZ_UPDATE_PACKAGING
> +MOZ_AUTOMATION_TIERS += update-packaging
> +automation/upload: automation/update-packaging
> +automation/update-packaging: automation/package

IOW, it seems to me this should really be
automation/update-packaging: automation/buildsymbols

Comment 34

[Mike Hommey [:glandium]]

(In reply to Michael Shal [:mshal] from comment #27)
> AUTOMATION_EXTRA-upload ?
> AUTOMATION_EXTRA_CMDLINE-upload ?

Maybe the latter?

> I was also considering just making each step tee out to a log, and cat the
> log when the step finishes. Though that would combine stdout/stderr for all
> steps if that's a concern. It would make it easier to track down errors in
> some cases since we can be running ~3 of these steps in parallel. Thoughts?

Do you mean the output for each step would be printed out twice overall (once by the tee and then by the cat) or was that a mistake? On one hand, that can seem weird, on the other hand, there's the opposite concern that there could be no output for a while, and that could trigger timeouts at the buildbot level. Although i think those timeouts are in the order of hours, not minutes, if we even hit them, buildbot would kill us and any log from the step would be lost.

With that being said, I think we shouldn't work around this inefficiency of make < 4 and work towards upgrading the slaves to use make 4 instead. (and make mach enable make 4 output-sync by default)

I don't mind automation/build being called with -j1 until we sort that out.

Comment 35

[Michael Shal [:mshal]]

(In reply to Mike Hommey [:glandium] from comment #33)
> ::: build/moz-automation.mk
> @@ +23,5 @@
> > +
> > +ifdef MOZ_UPDATE_PACKAGING
> > +MOZ_AUTOMATION_TIERS += update-packaging
> > +automation/upload: automation/update-packaging
> > +automation/update-packaging: automation/package
> 
> IOW, it seems to me this should really be
> automation/update-packaging: automation/buildsymbols

The update-packaging step uses files in dist/firefox, so we do require a dependency on automation/package (ie: we can't just replace it with a dependency on automation/buildsymbols instead). Since automation/package already depends on automation/buildsymbols, the update-packaging step transitively depends on it as well.

Comment 36

[Michael Shal [:mshal]]

(In reply to Mike Hommey [:glandium] from comment #34)
> (In reply to Michael Shal [:mshal] from comment #27)
> > AUTOMATION_EXTRA-upload ?
> > AUTOMATION_EXTRA_CMDLINE-upload ?
> 
> Maybe the latter?

WFM.

> > I was also considering just making each step tee out to a log, and cat the
> > log when the step finishes. Though that would combine stdout/stderr for all
> > steps if that's a concern. It would make it easier to track down errors in
> > some cases since we can be running ~3 of these steps in parallel. Thoughts?
> 
> Do you mean the output for each step would be printed out twice overall
> (once by the tee and then by the cat) or was that a mistake? On one hand,
> that can seem weird, on the other hand, there's the opposite concern that
> there could be no output for a while, and that could trigger timeouts at the
> buildbot level. Although i think those timeouts are in the order of hours,
> not minutes, if we even hit them, buildbot would kill us and any log from
> the step would be lost.

I meant just once by the cat - essentially a poor-mans output-sync (so redirect the output to a file, then cat it out afterward while handling the returncode of $(MAKE) properly).

> 
> With that being said, I think we shouldn't work around this inefficiency of
> make < 4 and work towards upgrading the slaves to use make 4 instead. (and
> make mach enable make 4 output-sync by default)

Sounds good to me - do you want to followup with getting make-4 onto slaves?

> 
> I don't mind automation/build being called with -j1 until we sort that out.

Ok, I'll stick with -j1 for now.

Comment 37

[Nick Thomas [:nthomas] (UTC+12)]

(In reply to Michael Shal [:mshal] from comment #35)
> The update-packaging step uses files in dist/firefox, so we do require a
> dependency on automation/package (ie: we can't just replace it with a
> dependency on automation/buildsymbols instead). Since automation/package
> already depends on automation/buildsymbols, the update-packaging step
> transitively depends on it as well.

Just by way of clarification - package strips the symbols out of binaries dist/firefox, so buildsymbols must run first. However buildsymbols should be modifying dist/firefox at all.

Comment 38

[Nick Thomas [:nthomas] (UTC+12)]

er, ... should *not* be modifying dist/firefox at all.

Comment 39

[Jordan Lund (:jlund)]

mshal and I will be meeting on monday[1] for figuring out what is left to do here. Anyone is welcome.

[1] June 9th 10am PT @ Jordan Lund's Vidyo room

Comment 40

[Michael Shal [:mshal]]

So, this can't land quite yet because MOZ_AUTOMATION is already set in buildbot as of bug 1011352, which means buildbot would be trying to kick off the automation targets during a normal build (which it's not setup to do). glandium suggested just moving the test for MOZ_AUTOMATION (and the call to 'make automation/build') inside mach, since buildbot calls 'make -f client.mk' and mozharness will call './mach build' once we're using jlund's work. This should work in theory, but unfortunately the Hf build type uses mozharness/scripts/spidermonkey/build.browser, which ends up calling 'mach build', so it ends up triggering the automation targets.

I think we either need to temporarily back out 1011352 if it's not being used by anything else, or we need to trigger the automation/build target off of a separate environment variable -- jlund suggested MACH_AUTOMATION. glandium, thoughts?

Comment 41

[Mike Hommey [:glandium]]

Why not just remove MOZ_AUTOMATION for that one build type? (note 1011352 did also some cleanup, I'd rather remove MOZ_AUTOMATION as a new patch than backing that out)

Comment 42

[(not currently active) Ted Mielczarek]

(In reply to Nick Thomas [:nthomas] from comment #37)
> Just by way of clarification - package strips the symbols out of binaries
> dist/firefox, so buildsymbols must run first. However buildsymbols should be
> modifying dist/firefox at all.

This is not true on Linux. `buildsymbols` strips the binaries and inserts debug link sections into the ELF headers so you can debug the packaged binaries with the debug symbols we've stripped off.

Comment 43

[Michael Shal [:mshal]]

(In reply to Mike Hommey [:glandium] from comment #41)
> Why not just remove MOZ_AUTOMATION for that one build type? (note 1011352
> did also some cleanup, I'd rather remove MOZ_AUTOMATION as a new patch than
> backing that out)

I think we could do something like 'unset MOZ_AUTOMATION' in the build.browser mozharness script, but that feels very hacky to me. I was thinking of just deleting these lines from buildbot (not a full backout):

./buildbot-configs/mozilla/config.py:    'MOZ_AUTOMATION': '1',
./buildbot-configs/mozilla/thunderbird_config.py:    'MOZ_AUTOMATION': '1',
./buildbot-configs/mozilla/b2g_config.py:    'MOZ_AUTOMATION': '1',

Comment 44

[Jordan Lund (:jlund)]

(In reply to Michael Shal [:mshal] from comment #43)
> (In reply to Mike Hommey [:glandium] from comment #41)
> > Why not just remove MOZ_AUTOMATION for that one build type? (note 1011352
> > did also some cleanup, I'd rather remove MOZ_AUTOMATION as a new patch than
> > backing that out)
> 
> I think we could do something like 'unset MOZ_AUTOMATION' in the
> build.browser mozharness script, but that feels very hacky to me. I was
> thinking of just deleting these lines from buildbot (not a full backout):
> 
> ./buildbot-configs/mozilla/config.py:    'MOZ_AUTOMATION': '1',
> ./buildbot-configs/mozilla/thunderbird_config.py:    'MOZ_AUTOMATION': '1',
> ./buildbot-configs/mozilla/b2g_config.py:    'MOZ_AUTOMATION': '1',

ya, my 2c is that we may be overloading MOZ_AUTOMATION (for now). I think we should be setting MOZ_AUTOMATION exactly where we need it; opposed to setting everywhere and unsetting it on an individual basis. Ultimately, I think after we port every ff build variant to mozharness/scripts/fx_desktop_build.py, we set MOZ_AUTOMATION at that mozharness level instead of at Buildbot level and then everything will just 'work'

But for today, maybe going with one of these two options: (1) have a temp env that just signifies 'desktop ff automation builds using mozharness + mach', e.g., MACH_AUTOMATION (2) remove existing 'MOZ_AUTOMATION' env vars like suggested in comment 43 and only setting it for builds concerned with this bug.

(1) assumes we need MOZ_AUTOMATION *today* for things outside this bug, (2) we don't

I don't want to block with 2c so happy to go with alternative if others feel strongly against 1 and 2

Comment 45

[Mike Hommey [:glandium]]

That seems like an awful lot of complication when you could just add:

del PLATFORM_VARS["linux64-sh-haz"]["env"]["MOZ_AUTOMATION"]
del PLATFORM_VARS["linux64-br-haz"]["env"]["MOZ_AUTOMATION"]

before apply_localconfig.

Comment 46

[Jordan Lund (:jlund)]

(In reply to Mike Hommey [:glandium] from comment #45)
> That seems like an awful lot of complication when you could just add:
> 
> del PLATFORM_VARS["linux64-sh-haz"]["env"]["MOZ_AUTOMATION"]
> del PLATFORM_VARS["linux64-br-haz"]["env"]["MOZ_AUTOMATION"]
> 
> before apply_localconfig.

k. I see pros and cons with the various solutions. but for speed reasons, choosing one is more important than which one. so glandium's solution WFM as long as mshal's tests shows it works and he's happy with it too :)

Comment 47

[Jordan Lund (:jlund)]

Hey mshal,

I thought it might be useful to spell out how our Linux variants differ so we don't have to cross reference buildbot or mozharness: https://docs.google.com/spreadsheets/d/1EcRvYDUi8XdBb4RcEDf2M63aeVo9hLPSVcmnsSYiKZQ/edit?usp=sharing

Good news is I think there will be only a few edge cases for certain branches but we can worry about that later: e.g. for win/mac on the b2g-inbound branch, we don't do 'make -k check'. It turns out many of the branch differences don't really pertain to the make targets or the stuff we ported to build configs.

So really, we just need to add the appropriate env flags to the respective builder's mozconfig IIUC.

Comment 48

[Michael Shal [:mshal]]

Attachment: 0001-Bug-978211-add-an-automation-build-target-for-post-b.patch

Requesting re-review since I moved the MOZ_AUTOMATION check from client.mk into mach. Pulling that out of the environment seems wonky - is there a better way to do that?

Comment 49

[Michael Shal [:mshal]]

inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/c0efa7f1e54c

Comment 50

[Michael Shal [:mshal]]

Leaving open until we get the rest of the pieces landed.

Comment 51

[Mike Hommey [:glandium]]

Attachment: Seemlessly support make 4 output buffering for automation/% steps

Comment 52

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/c0efa7f1e54c

Comment 53

[Michael Shal [:mshal]]

Comment on attachment 8440174 [details] [diff] [review]
Seemlessly support make 4 output buffering for automation/% steps

># HG changeset patch
># User Mike Hommey <[e-mail]>
># Date 1402701622 -32400
>#      Sat Jun 14 08:20:22 2014 +0900
># Node ID e8895ae4323c52c939c7ccf67771d3fe437b47a7
># Parent  95bd46f8f4c221e9d7fa4a9f487a8459ab2aaf2b
>Bug 978211 - Seemlessly support make 4 output buffering for automation/% steps
>
>diff --git a/Makefile.in b/Makefile.in
>--- a/Makefile.in
>+++ b/Makefile.in
>@@ -127,17 +127,19 @@ endif
> $(addprefix install-dist-,$(install_manifests)): install-dist-%: $(install_manifest_depends)
> 	$(call py_action,process_install_manifest,$(if $(NO_REMOVE),--no-remove )$(DIST)/$* _build_manifests/install/dist_$*)
> 
> .PHONY: install-tests
> install-manifests: install-tests
> install-tests: $(install_manifest_depends)
> 	$(call py_action,process_install_manifest,$(if $(NO_REMOVE),--no-remove )_tests _build_manifests/install/tests)
> 
>+ifneq(,$(filter automation/%,$(MAKECMDGOALS)))
> include $(topsrcdir)/build/moz-automation.mk
>+endif

We might need to move this conditional inside moz-automation.mk (I assume you need it to avoid the .NOTPARALLEL during the regular compile phase?). If you skip all of moz-automation.mk, then MOZ_AUTOMATION_TIERS won't be set, so mach will barf when it gets to one of the automation steps.

Also you'll need a space between 'ifneq' and '('

Comment 54

[Michael Shal [:mshal]]

Attachment: 0001-Bug-978211-enable-MOZ_AUTOMATION_-flags-in-linux-moz.patch

This changes moz-automation.mk slightly to use a specific environment variable for each stage of the automation work. Each mozconfig then overrides the defaults for its specific configuration, so we can explicitly set what stages run for which build types using the in-tree configs. So far only the Linux configs have been modified.

The funky eval/call macros are used to ensure that dependent build steps are set so that mach is happy when tiers are started/finished.

The final variable definitions are subject to change slightly as I'm still going through the process of checking all the builds.

Comment 55

[Mike Hommey [:glandium]]

Attachment: Seemlessly support make 4 output buffering for automation/% steps

Comment 56

[Mike Hommey [:glandium]]

Comment on attachment 8441702 [details] [diff] [review]
0001-Bug-978211-enable-MOZ_AUTOMATION_-flags-in-linux-moz.patch

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

::: build/gen_mach_buildprops.py
@@ +69,5 @@
> +    except IOError as e:
> +        if e.errno != errno.ENOENT:
> +            raise
> +        for prop, condition in property_conditions:
> +            properties.update({prop: 'UNKNOWN'})

The loop could just be replaced with properties = {prop: 'UNKNOWN' for prop, condition in property_conditions}

::: build/moz-automation.mk
@@ +36,5 @@
> +define optional_automation_dependency
> +ifdef MOZ_AUTOMATION_$(tier_$1)
> +automation/$1: automation/$2
> +endif
> +endef

If you do:
define foo_
...
endef

foo = $(eval $(call foo, $1, $2))

You can avoid the evals later on.

@@ +48,2 @@
>  MOZ_AUTOMATION_TIERS += package-tests
> +endif

It seems to me you're not going far enough with automation :)
That is, MOZ_AUTOMATION_TIERS could be automatically derived from the MOZ_AUTOMATION_* variables. And the dependency declarations don't really need to be under ifdefs.

::: build/mozconfig.automation
@@ +8,5 @@
> +# automation builds.  For example, if MOZ_AUTOMATION_PACKAGE is set, then the
> +# package step will run.  This file contains the default settings, which can be
> +# overridden by setting them earlier in the appropriate mozconfig.
> +
> +mk_add_options "export MOZ_AUTOMATION_BUILD_SYMBOLS=${MOZ_AUTOMATION_BUILD_SYMBOLS:-1}"

should be ${foo-1} (no colon)

Comment 57

[Michael Shal [:mshal]]

Comment on attachment 8441737 [details] [diff] [review]
Seemlessly support make 4 output buffering for automation/% steps

>diff --git a/python/mozbuild/mozbuild/mach_commands.py b/python/mozbuild/mozbuild/mach_commands.py
>--- a/python/mozbuild/mozbuild/mach_commands.py
>+++ b/python/mozbuild/mozbuild/mach_commands.py
>@@ -367,21 +367,19 @@ class Build(MachCommandBase):
>                     allow_parallel=False, ensure_exit_code=False, num_jobs=jobs,
>                     silent=not verbose, force_pymake=pymake)
> 
>                 make_extra = self.mozconfig['make_extra'] or []
>                 make_extra = dict(m.split('=', 1) for m in make_extra)
> 
>                 moz_automation = os.getenv('MOZ_AUTOMATION') or make_extra.get('export MOZ_AUTOMATION', None)
>                 if moz_automation and status == 0:
>-                    # Default to 1 job until we have make-4.0 in for output
>-                    # buffering.
>                     status = self._run_make(target='automation/build',
>                         line_handler=output.on_line, log=False, print_directory=False,
>-                        allow_parallel=False, ensure_exit_code=False, num_jobs=1,
>+                        allow_parallel=False, ensure_exit_code=False, num_jobs=jobs,

Turns out we'll need allow_parallel=True here (which is the default, so the arg can just be removed). Otherwise mach uses MOZ_PARALLEL_BUILD instead of -j, which is only used by client.mk. That's my fault.

Also probably worth mentioning that because make-4 is slurping output, mach won't actually see that the automation tier has started until it actually finished. This means you don't see which tiers are active - they just turn green after they are finished. Maybe the $(call BUILDSTATUS,TIER_START $*) message could be in a different target that automation/% depends on? In any case, we don't need to hold up landing the patch for this since it's a minor display issue.

Comment 58

[Mike Hommey [:glandium]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cdb542f9cfc7

Comment 59

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/cdb542f9cfc7

Comment 60

[Michael Shal [:mshal]]

(In reply to Mike Hommey [:glandium] from comment #56)
> It seems to me you're not going far enough with automation :)
> That is, MOZ_AUTOMATION_TIERS could be automatically derived from the
> MOZ_AUTOMATION_* variables. And the dependency declarations don't really
> need to be under ifdefs.

Yeah, looks like I was overthinking about how to prevent mach from getting a bad TIER_START, while also getting the ordering right for the optional build steps. It turns out it's much easier to always run the automation/% target, and just do nothing if the step isn't enabled. That should get rid of a lot of the garbage :)

Comment 61

[Michael Shal [:mshal]]

Attachment: 0001-Bug-978211-enable-MOZ_AUTOMATION_-flags-in-linux-moz.patch

Comment 62

[Mike Hommey [:glandium]]

Comment on attachment 8443038 [details] [diff] [review]
0001-Bug-978211-enable-MOZ_AUTOMATION_-flags-in-linux-moz.patch

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

::: browser/config/mozconfigs/linux32/debug
@@ +3,5 @@
>  ac_add_options --enable-signmar
>  
>  ac_add_options --disable-unified-compilation
>  
> +export MOZ_AUTOMATION_TALOS_SENDCHANGE=0

You shouldn't need to export those, as they only matter in mozconfig.automation.

::: build/moz-automation.mk
@@ +69,5 @@
> +# However, the target automation/buildsymbols will still be executed in this
> +# case because it is a prerequisite of automation/upload.
> +define automation_commands
> +$(call BUILDSTATUS,TIER_START $1)
> +@$(MAKE) $* $(AUTOMATION_EXTRA_CMDLINE-$*)

@$(MAKE) $1 $(AUTOMATION_EXTRA_CMDLINE-$1)

Comment 63

[Michael Shal [:mshal]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/26b95471c5f8

Comment 64

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/26b95471c5f8

Comment 65

[Michael Shal [:mshal]]

Attachment: 0001-Bug-978211-add-pretty-targets-for-automation.patch

This adds the duplicate steps that have MOZ_PKG_PRETTYNAMES=1 (package, package-tests, l10n-check, and update-packaging). I also fixed linux64/debug-nonunified, since I missed that before.

Comment 66

[Mike Hommey [:glandium]]

Comment on attachment 8449944 [details] [diff] [review]
0001-Bug-978211-add-pretty-targets-for-automation.patch

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

::: build/moz-automation.mk
@@ +35,5 @@
>  # Automation build steps. Everything in MOZ_AUTOMATION_TIERS also gets used in
>  # TIERS for mach display. As such, the MOZ_AUTOMATION_TIERS are roughly sorted
>  # here in the order that they will be executed (since mach doesn't know of the
>  # dependencies between them).
> +moz_automation_symbols = PACKAGE_TESTS PRETTY_PACKAGE_TESTS BUILD_SYMBOLS UPLOAD_SYMBOLS PACKAGE PRETTY_PACKAGE UPDATE_PACKAGING PRETTY_UPDATE_PACKAGING CHECK L10N_CHECK PRETTY_L10N_CHECK UPLOAD

Could you make that a diff friendly list for the future?

@@ +61,5 @@
> +# conflicts in writing to the same files.
> +automation/pretty-package: automation/package
> +automation/pretty-package-tests: automation/package-tests
> +automation/pretty-l10n-check: automation/l10n-check
> +automation/pretty-update-packaging: automation/update-packaging

Could you put those dependencies in the opposite direction?

Comment 67

[Jordan Lund (:jlund)]

it looks like for windows builds we are setting --with-ccache

taking a look at mozconfigs, I think this is because $platform is getting non in mozconfig.cache so the 'win*' condition fails to block.

I think the reason platform might not have any value is because the mozconfigs can't find buildprops.json: http://mxr.mozilla.org/mozilla-central/source/build/mozconfig.cache#11

when this build is through mozharness, buildprops.json is expected to be at:

 - '$topsrcdir"'/../../buildprops.json'

we may need to add this search path in this file and look for both until one is found.

Comment 68

[Jordan Lund (:jlund)]

windows builds are also hitting this error: Bug 1013730 - mach build: RuntimeError: couldn't find any physical disk

Comment 69

[Jordan Lund (:jlund)]

oops, didn't mean to finish comment.

info about the fail:
    - log: https://pastebin.mozilla.org/5523363
    - slave: b-2008-ix-0097 (6.1.7601 Service Pack 1 Build 7601)
    - c:\\mozilla-build\\python27\\python.exe --version == Python 2.7.3

Comment 70

[Jordan Lund (:jlund)]

after switching to a new windows box I could build ff but make -k check failed. I think this is to the point mshal can get to locally. definitely progress.:

windows error:

19:04:08     INFO -  37:42.25 c:/builds/moz2_slave/m-cen-w32-00000000000000000000/build/source/config/rules.mk:600: *** SHARED_LIBRARY_LIBS must contain .lib files only.  Stop.
19:04:08     INFO -  37:42.25
19:04:08     INFO -  37:42.25 c:/builds/moz2_slave/m-cen-w32-00000000000000000000/build/source/config/rules.mk:1683: recipe for target 'check' failed
19:04:08     INFO -  37:42.25
19:04:08     INFO -  37:42.26 mozmake.EXE[1]: *** [check] Error 2
19:04:08     INFO -  37:42.26
19:04:08     INFO -  37:42.26 c:/builds/moz2_slave/m-cen-w32-00000000000000000000/build/source/build/moz-automation.mk:91: recipe for target 'automation/check' failed
19:04:08     INFO -  37:42.26 mozmake.EXE: *** [automation/check] Error 2
19:04:08     INFO -  37:42.29 690 compiler warnings present.
19:04:08     INFO -  2
19:04:08    ERROR - Return code: 1
19:04:08     INFO - setting properties set by mach build. Looking in path: c:\builds\moz2_slave\m-cen-w32-00000000000000000000\build\source\obj-firefox\mach_build_properties.json
19:04:08    FATAL - Could not find any properties set from mach build. Path does not exist: c:\builds\moz2_slave\m-cen-w32-00000000000000000000\build\source\obj-firefox\mach_build_properties.json
19:04:08    FATAL - Running post_fatal callback...
19:04:08    FATAL - Exiting -1
19:04:08     INFO - Running post-run listener: _summarize
19:04:08     INFO - #####


mac is also close. runs for 55 min but fails near the end of a build. I think it's just a reference error.

mac log:

02:37:32     INFO -  47:38.22 tar: Error exit delayed from previous errors.
02:37:32     INFO -  47:38.47 tar: manifestparser.py: Cannot stat: No such file or directory
02:37:32     INFO -  47:38.47 tar: Error exit delayed from previous errors.
02:38:51     INFO -  48:57.34 /builds/slave/m-cen-osx64-000000000000000000/build/source/build/macosx/universal/unify: warning: makeUniversalDirectory: only in x86 /builds/slave/m-cen-osx64-000000000000000000/build/source/obj-firefox/x86_64/dist/test-stage/cppunittests:
02:38:51     INFO -  48:57.34   TestJemalloc
02:40:28     INFO -  50:34.90 /builds/slave/m-cen-osx64-000000000000000000/build/source/build/macosx/universal/unify: warning: makeUniversalDirectory: only in ppc /builds/slave/m-cen-osx64-000000000000000000/build/source/obj-firefox/i386/dist/test-stage/xpcshell/tests/dom/plugins/test/unit:
02:40:28     INFO -  50:34.91   testaddon_x86-gcc3.xpi
02:40:28     INFO -  50:34.91 /builds/slave/m-cen-osx64-000000000000000000/build/source/build/macosx/universal/unify: warning: makeUniversalDirectory: only in x86 /builds/slave/m-cen-osx64-000000000000000000/build/source/obj-firefox/x86_64/dist/test-stage/xpcshell/tests/dom/plugins/test/unit:
02:40:28     INFO -  50:34.91   testaddon_x86_64-gcc3.xpi
02:40:41     INFO -  50:47.35 /builds/slave/m-cen-osx64-000000000000000000/build/source/build/macosx/universal/unify: warning: makeUniversalDirectory: only in x86 /builds/slave/m-cen-osx64-000000000000000000/build/source/obj-firefox/x86_64/dist/test-stage/xpcshell/tests/xpcom/tests/unit:
02:40:41     INFO -  50:47.35   TestJemalloc
02:40:41     INFO -  50:47.42 /usr/bin/make -j8 -s automation/build
02:40:41     INFO -  50:47.43 make: *** No rule to make target `automation/build'.  Stop.
02:40:41     INFO -  50:47.43 166 compiler warnings present.
02:40:41    ERROR - Return code: 2
02:40:41     INFO - setting properties set by mach build. Looking in path: /builds/slave/m-cen-osx64-000000000000000000/build/source/obj-firefox/i386/mach_build_properties.json
02:40:41    FATAL - Could not find any properties set from mach build. Path does not exist: /builds/slave/m-cen-osx64-000000000000000000/build/source/obj-firefox/i386/mach_build_properties.json
02:40:41    FATAL - Running post_fatal callback...
02:40:41    FATAL - Exiting -1
02:40:41     INFO - Running post-run listener: _summarize
02:40:41    ERROR - # TBPL FAILURE #

Comment 71

[Michael Shal [:mshal]]

Updated with review feedback.

try: https://tbpl.mozilla.org/?tree=Try&rev=4b9f17b590cd
inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/a95aada9a565

Comment 72

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I backed this out (alongside bug 1030985's patch) under suspicion of breaking Android xpcshell tests: https://tbpl.mozilla.org/php/getParsedLog.php?id=43281523&full=1&branch=mozilla-inbound#error0


https://hg.mozilla.org/integration/mozilla-inbound/rev/99116baeec50


I'm not sure which bug is at fault because the test got coalesced away, but I don't want to wait the 75 minutes it'll take to get the test retriggered and finished.

Comment 73

[Mike Hommey [:glandium]]

Very unlikely to be causing trouble on android xpcshell tests.

Comment 74

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Yeah, the retrigger came back green on this, so I relanded it as  https://hg.mozilla.org/integration/mozilla-inbound/rev/19176ab7d700

Comment 75

[Michael Shal [:mshal]]

> mac is also close. runs for 55 min but fails near the end of a build. I
> think it's just a reference error.

I can reproduce the mac issue locally - it is definitely because of universal builds. Mach is trying to run make in the objdir, which is 'obj-firefox', but we really have two objdirs in 'obj-firefox/i386' and 'obj-firefox/x86_64'. In buildbot-configs, the automation steps are hard-coded to run in the i386 objdir:

'platform_objdir': "%s/i386" % OBJDIR,

Is there a good way to do this automatically in mach when we're doing a universal build? Or should we pass something in from mozharness?

Comment 76

[Mike Hommey [:glandium]]

Run the command on objdir/$(firstword $(MOZ_BUILD_PROJECTS)), MOZ_BUILD_PROJECTS being defined in mozconfig.

Comment 77

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/19176ab7d700

Comment 78

[Michael Shal [:mshal]]

Attachment: 0001-Bug-978211-Windows-configs.patch

We're starting to have some success with Windows & OSX builds, so mind as well get these configs & tweaks reviewed. There may still be some followups as we continue to test all the variations.

Comment 79

[Michael Shal [:mshal]]

Attachment: 0002-Bug-978211-OSX-automation-configs.patch

OSX configs & fix for universal builds. Note OSX builds still require a fix for bug 1036093 as well.

Comment 80

[Michael Shal [:mshal]]

Attachment: 0003-Bug-978211-various-asan-mar-pretty-fixes.patch

Comment 81

[Michael Shal [:mshal]]

Attachment: 0001-Bug-978211-Windows-configs.patch

win32/beta and release configs also need to include mozconfig.win-common, otherwise 'make check' complains.

Comment 82

[Michael Shal [:mshal]]

Attachment: 0001-Bug-978211-Windows-configs.patch

Updating Windows patch to include proper dependencies for pretty-update-packaging, l10n-check, and pretty-l10n-check.

Comment 83

[Michael Shal [:mshal]]

Attachment: 0001-Bug-978211-various-asan-mar-pretty-upload-fixes.patch

Updated to include some of the environment variables required for 'upload'. This works around the MSYS corruption issue by setting the paths inside make.

Comment 84

[Mike Hommey [:glandium]]

Comment on attachment 8453375 [details] [diff] [review]
0002-Bug-978211-OSX-automation-configs.patch

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

::: browser/config/mozconfigs/macosx64/debug-nonunified
@@ +1,1 @@
> +MOZ_AUTOMATION_UPLOAD=0

MOZ_AUTOMATION_PRETTY=1 ?

::: build/macosx/mozconfig.common
@@ +1,5 @@
>  # This Source Code Form is subject to the terms of the Mozilla Public
>  # License, v. 2.0. If a copy of the MPL was not distributed with this
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
> +MOZ_AUTOMATION_L10N_CHECK=0

No l10n check on mac at all? really?

::: python/mozbuild/mozbuild/mach_commands.py
@@ +372,5 @@
> +                # For universal builds, we need to run the automation steps in
> +                # the first architecture from MOZ_BUILD_PROJECTS
> +                projects = make_extra.get('MOZ_BUILD_PROJECTS')
> +                if projects:
> +                    subdir = os.path.join(self.topobjdir, projects.split(' ')[0])

projects.split()[0]

Comment 85

[Mike Hommey [:glandium]]

Comment on attachment 8454659 [details] [diff] [review]
0001-Bug-978211-Windows-configs.patch

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

::: build/moz-automation.mk
@@ +107,3 @@
>  define automation_commands
>  $(call BUILDSTATUS,TIER_START $1)
> +@$(MAKE) -j1 $1 $(AUTOMATION_EXTRA_CMDLINE-$1)

Do all targets need -j1, or only installer? In that case, you could add -j1 to AUTOMATION_EXTRA_CMDLINE-installer instead.

Comment 86

[Mike Hommey [:glandium]]

Comment on attachment 8456295 [details] [diff] [review]
0001-Bug-978211-various-asan-mar-pretty-upload-fixes.patch

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

::: build/gen_mach_buildprops.py
@@ +29,5 @@
>  
>      return (sha1Hash, size)
>  
>  def getMarProperties(filename):
> +    if os.path.exists(filename):

if not os.path.exists(filename):
    return {}

and then you don't have to add indentation :)

::: build/mozconfig.automation
@@ +19,5 @@
>  mk_add_options "export MOZ_AUTOMATION_UNITTEST_SENDCHANGE=${MOZ_AUTOMATION_UNITTEST_SENDCHANGE-1}"
>  mk_add_options "export MOZ_AUTOMATION_UPDATE_PACKAGING=${MOZ_AUTOMATION_UPDATE_PACKAGING-0}"
>  mk_add_options "export MOZ_AUTOMATION_UPLOAD=${MOZ_AUTOMATION_UPLOAD-1}"
>  mk_add_options "export MOZ_AUTOMATION_UPLOAD_SYMBOLS=${MOZ_AUTOMATION_UPLOAD_SYMBOLS-0}"
> +mk_add_options "export SYMBOL_SERVER_PATH=/mnt/netapp/breakpad/symbols_ffx/"

In the end, how do you get msys not to screw it up?

Comment 87

[Mike Hommey [:glandium]]

(In reply to Mike Hommey [:glandium] from comment #86)
> > +mk_add_options "export SYMBOL_SERVER_PATH=/mnt/netapp/breakpad/symbols_ffx/"
> 
> In the end, how do you get msys not to screw it up?

Ah, comment 83.

Comment 88

[Mike Hommey [:glandium]]

Comment on attachment 8456295 [details] [diff] [review]
0001-Bug-978211-various-asan-mar-pretty-upload-fixes.patch

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

::: build/mozconfig.automation
@@ +19,5 @@
>  mk_add_options "export MOZ_AUTOMATION_UNITTEST_SENDCHANGE=${MOZ_AUTOMATION_UNITTEST_SENDCHANGE-1}"
>  mk_add_options "export MOZ_AUTOMATION_UPDATE_PACKAGING=${MOZ_AUTOMATION_UPDATE_PACKAGING-0}"
>  mk_add_options "export MOZ_AUTOMATION_UPLOAD=${MOZ_AUTOMATION_UPLOAD-1}"
>  mk_add_options "export MOZ_AUTOMATION_UPLOAD_SYMBOLS=${MOZ_AUTOMATION_UPLOAD_SYMBOLS-0}"
> +mk_add_options "export SYMBOL_SERVER_PATH=/mnt/netapp/breakpad/symbols_ffx/"

The problem I can see with doing this in-tree is that if you ever need to change it because something changes on the symbol server change, you need to do it cross branches.

Relatedly, something I just thought about that is likely to be problematic, is try. You don't want upload symbols to happen on try. There are possibly other things that should be excluded from try builds.

Comment 89

[Michael Shal [:mshal]]

> ::: browser/config/mozconfigs/macosx64/debug-nonunified
> @@ +1,1 @@
> > +MOZ_AUTOMATION_UPLOAD=0
> 
> MOZ_AUTOMATION_PRETTY=1 ?

The pretty check is currently only enabled in non-debug nonunified builds. The relevant buildbot logic is here: http://mxr.mozilla.org/build/source/buildbotcustom/misc.py#1889

> 
> ::: build/macosx/mozconfig.common
> @@ +1,5 @@
> >  # This Source Code Form is subject to the terms of the Mozilla Public
> >  # License, v. 2.0. If a copy of the MPL was not distributed with this
> >  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
> >  
> > +MOZ_AUTOMATION_L10N_CHECK=0
> 
> No l10n check on mac at all? really?

Apparently not, according to bug 700997.

Comment 90

[Michael Shal [:mshal]]

> Do all targets need -j1, or only installer? In that case, you could add -j1
> to AUTOMATION_EXTRA_CMDLINE-installer instead.

I don't believe installer needs it, but l10n-check currently does (bug 1036563). I'll try to just add it to l10n-check.

Comment 91

[Michael Shal [:mshal]]

> The problem I can see with doing this in-tree is that if you ever need to
> change it because something changes on the symbol server change, you need to
> do it cross branches.

If there's a better solution here, I'm all for it. From Procmon, it looks like we have a good environment through mach, but make is invoked as:

C:/mozilla-build/msys/bin/sh.exe -c "c:/builds/moz2_slave/.../build/mozmake.EXE -f client.mk -s"

The sh process has a good environment, but mozmake's gets hosed. By putting the variables in the mozconfig, we work around that by setting them in mozmake itself.

I'm not sure if changing how mozmake invoked is all we need to do or not - we may also have to contend with the fact that buildbot is invoked with an msys shell as well:

http://mxr.mozilla.org/build/source/opsi-package-sources/buildbot-startup/CLIENT_DATA/start-buildbot.bat#80

> Relatedly, something I just thought about that is likely to be problematic,
> is try. You don't want upload symbols to happen on try. There are possibly
> other things that should be excluded from try builds.

Yeah, there will be followup work for branch specific configs - so far we've just managed the platform variances. jlund has already started to look into it, and from what we can tell so far it should be pretty manageable in the mozconfigs.

Comment 92

[Mike Hommey [:glandium]]

(In reply to Michael Shal [:mshal] from comment #91)
> If there's a better solution here, I'm all for it. From Procmon, it looks
> like we have a good environment through mach, but make is invoked as:
> 
> C:/mozilla-build/msys/bin/sh.exe -c
> "c:/builds/moz2_slave/.../build/mozmake.EXE -f client.mk -s"

It shouldn't. Wherever it's invoked from, we should add shell=False or remove shell=True.

> The sh process has a good environment, but mozmake's gets hosed. By putting
> the variables in the mozconfig, we work around that by setting them in
> mozmake itself.
> 
> I'm not sure if changing how mozmake invoked is all we need to do or not -
> we may also have to contend with the fact that buildbot is invoked with an
> msys shell as well:

Those variables are set by buildbot, that buildbot is invoked by msys doesn't matter.

Comment 93

[Mike Hommey [:glandium]]

(In reply to Mike Hommey [:glandium] from comment #92)
> (In reply to Michael Shal [:mshal] from comment #91)
> > If there's a better solution here, I'm all for it. From Procmon, it looks
> > like we have a good environment through mach, but make is invoked as:
> > 
> > C:/mozilla-build/msys/bin/sh.exe -c
> > "c:/builds/moz2_slave/.../build/mozmake.EXE -f client.mk -s"
> 
> It shouldn't. Wherever it's invoked from, we should add shell=False or
> remove shell=True.

Try this:

diff --git a/python/mozbuild/mozbuild/base.py b/python/mozbuild/mozbuild/base.py
--- a/python/mozbuild/mozbuild/base.py
+++ b/python/mozbuild/mozbuild/base.py
@@ -454,17 +454,17 @@ class MozbuildObject(ProcessExecutionMix
         append_env[b'MACH'] = '1'
 
         params = {
             'args': args,
             'line_handler': line_handler,
             'append_env': append_env,
             'explicit_env': explicit_env,
             'log_level': logging.INFO,
-            'require_unix_environment': True,
+            'require_unix_environment': False,
             'ensure_exit_code': ensure_exit_code,
             'pass_thru': pass_thru,
 
             # Make manages its children, so mozprocess doesn't need to bother.
             # Having mozprocess manage children can also have side-effects when
             # building on Windows. See bug 796840.
             'ignore_children': True,
         }

Comment 94

[Michael Shal [:mshal]]

Attachment: 0001-Bug-978211-various-asan-mar-pretty-mach-fixes.patch

Thanks! That seemed to do the trick.

Comment 95

[Mike Hommey [:glandium]]

Comment on attachment 8458308 [details] [diff] [review]
0001-Bug-978211-various-asan-mar-pretty-mach-fixes.patch

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

::: python/mozbuild/mozbuild/base.py
@@ +458,5 @@
>              'line_handler': line_handler,
>              'append_env': append_env,
>              'explicit_env': explicit_env,
>              'log_level': logging.INFO,
> +            'require_unix_environment': False,

Can you split this out in a separate patch?

Comment 96

[Mike Hommey [:glandium]]

(In reply to Mike Hommey [:glandium] from comment #95)
> Comment on attachment 8458308 [details] [diff] [review]
> 0001-Bug-978211-various-asan-mar-pretty-mach-fixes.patch
> 
> Review of attachment 8458308 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: python/mozbuild/mozbuild/base.py
> @@ +458,5 @@
> >              'line_handler': line_handler,
> >              'append_env': append_env,
> >              'explicit_env': explicit_env,
> >              'log_level': logging.INFO,
> > +            'require_unix_environment': False,
> 
> Can you split this out in a separate patch?

(Better to have it separate imho, and r? gps, since I wrote that part)

Comment 97

[Michael Shal [:mshal]]

Attachment: 0001-Bug-978211-run-make-from-mach-without-a-shell.patch

See #c91 - #c93 for some context. Basically when mach invokes make, it does so with an MSYS shell. This ends up corrupting environment variables like SYMBOL_SERVER_PATH from '/mnt/netapp/breakpad/symbols_ffx/' to 'C:/mozilla-build/msys/mnt/netapp/breakpad/symbols_ffx/', which isn't a valid path on the symbol server. So we need to run make directly, without MSYS in the middle.

Comment 98

[Michael Shal [:mshal]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cf02c8f760b1
https://hg.mozilla.org/integration/mozilla-inbound/rev/fa90c963385b
https://hg.mozilla.org/integration/mozilla-inbound/rev/1e4bf244a89c

Comment 99

[Gregory Szorc [:gps]]

Comment on attachment 8458691 [details] [diff] [review]
0001-Bug-978211-run-make-from-mach-without-a-shell.patch

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

I'm pretty sure this was required for building with pymake. Since we don't support pymake anymore, this should be safe.

*crosses fingers*

Comment 100

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/cf02c8f760b1
https://hg.mozilla.org/mozilla-central/rev/fa90c963385b
https://hg.mozilla.org/mozilla-central/rev/1e4bf244a89c

Comment 101

[Michael Shal [:mshal]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/beb02af9af1c

Comment 102

[Michael Shal [:mshal]]

try for #c101: https://tbpl.mozilla.org/?tree=Try&rev=fd2066f8ff33

Comment 103

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/beb02af9af1c

Comment 104

[(not currently active) Ted Mielczarek]

(In reply to Mike Hommey [:glandium] from comment #88)
> The problem I can see with doing this in-tree is that if you ever need to
> change it because something changes on the symbol server change, you need to
> do it cross branches.

This is not the end of the world, certainly. We've changed the symbol upload information all of once in 7 years. Having the ability to make a change and have it ride the trains would actually be nicer.

Comment 105

[Jordan Lund (:jlund)]

mshal: how do you feel about resolving this and any follow up work can be tracked in 'branch rollout' bugs? I am going to close my other bugs that revolved around the implementation of mozharness + mach builds. I think it be nice to have just one bug from here on out. what do you think?

Comment 106

[Jordan Lund (:jlund)]

FTR - I filed 'Bug 1055912 - [tracking] rollout desktop mozharness mach builders across all branches'. we can use that or this for any mach stuff.

Comment 107

[Michael Shal [:mshal]]

(In reply to Jordan Lund (:jlund) from comment #105)
> mshal: how do you feel about resolving this and any follow up work can be
> tracked in 'branch rollout' bugs? I am going to close my other bugs that
> revolved around the implementation of mozharness + mach builds. I think it
> be nice to have just one bug from here on out. what do you think?

Makes sense to me since we have the initial implementation landed. This bug is already too long anyway :)

Comment 108

[Phil Ringnalda (:philor)]

Attachment: unbreak win64 compare-mozconfig

You'd think, with all the people claiming that we're starting to care about Win64 again, someone would have done the two lines of copy-paste to unbreak it sometime in the last 40 days.

Comment 109

[Phil Ringnalda (:philor)]

Comment on attachment 8482023 [details] [diff] [review]
unbreak win64 compare-mozconfig

https://hg.mozilla.org/integration/mozilla-inbound/rev/7bd4662c6d8f

Comment 110

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7bd4662c6d8f

Comment 111

[Michael Shal [:mshal]]

beta uplift for bug 1093897: https://hg.mozilla.org/releases/mozilla-beta/rev/8201c42832ef

Comment 112

[Michael Shal [:mshal]]

I think we're done here for now. Any other work can be done in follow-ups.