898554 - Enable static rooting analysis on b2g

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Chris Peterson [:cpeterson]]

The static rooting analysis is not currently checking B2G's device-dependent code, e.g. bluetooth. B2G needs gcc -mandroid and the static analyzer needs gcc 4.7 with plugins.

Comment 1

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

Status update on this:

The latest Android NDK has gcc 4.8, apparently with the plugin support compiled in. Sadly, it does not ship with the corresponding include files.

I've come up with a crazy scheme to punt over compiler invocations to the sixgill wrapper and then to the NDK compiler, and it seems to work. It successfully compiles all the code.

Unfortunately, the sixgill plugin itself needs to be compiled in a way that is compatible with the prebuilt Android toolchain. The linkage of cc1 and cc1plus is different from my host gcc's, so I had to hack around that. Now it's failing to find some GMP symbols in cc1/cc1plus. Maybe the prebuilt compiler was compiled against a different version of GMP? It's a bit of a puzzle, because one of the symbols (__gmpz_init_set_si) is invoked directly by the sixgill code, and cc1 only contains gmpz_init_set_ui (unsigned vs signed). It seems very odd to have one but not the other. So I don't know if it's enough to just dig up the version of GMP that the prebuilt gcc was compiled with, or if I need to do something more drastic.

I'm pulling down the NDK source right now. Lucky me.

Comment 3

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

Attachment: Ignore emacs backup files in mozharness repo

Ok, I have a patch stack that gets me closer to having a b2g analysis, though it's not totally done yet. It starts out with a bunch of cleanup/refactoring patches, and it'd be good to just get the darn things in while I'm polishing up the last pieces.

Comment 4

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

Attachment: Use dict.update in base/config.py

Totally minor, but I couldn't see any difference between the explicit loop and dict.update.

Comment 5

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

Attachment: Expand out the --list of actions

I use the list of actions quite frequently, but they're too densely packed to be readable. And it's nice to have a unified list of all actions annoted with which ones are run by default.

Comment 6

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

Attachment: Automatic mock-or-not commands in b2g build

Clear up some repetition in b2g_build.py due to the the mock config not being in the main config.

Comment 7

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

Attachment: Extract logic for generating build commands

I ended up not using this after all, but I still think it reads a little better. The function name probably isn't quite right, though. generate_build_command?

Comment 8

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

Attachment: Define an explicit gecko_src directory for testing

I probably shouldn't dump all the b2g_build.py-specific patches on aki.

This change makes it much easier to build b2g against a local gecko checkout.

Comment 9

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

Attachment: Ensure script_obj always exists, and for BaseScript set it in __init__. Then use script_obj.query_abs_dirs in query_exe so exes can use the dirs keys in config files

The b2g build checks out all of its stuff in the same place that the build-tools would normally get checked out, which makes it annoying to point to the right hgtool.py and gittool.py. This patch enables the next patch up, which will do

-        'hgtool.py': 'tools/buildfarm/utils/hgtool.py',
+        'hgtool.py': '%(abs_tools_dir)s/buildfarm/utils/hgtool.py',

In general, it seems very convenient to be able to use dirs['foo'] keys in these exe paths, since some build script chdir to different directories so relative paths are a pain.

Comment 10

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

Attachment: Move actions list out of __init__ call

I originally did this so I could subclass b2g_build.py and generate a new class list as a modification of B2GBuild's, but I ended up not using this. Still, I think this is better from readability because you see the list of actions that the class is going to support early on. It's your call; I don't depend on it at all.

Comment 11

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

Attachment: Move default config settings out of __init__ call

Very similar to the previous patch, but this is even closer to being useful -- it allows initializing a B2GBuild with a custom config, and uses it to override the default config options.

And yet, I don't make use of this either. It just seems a little cleaner overall to me, so I'm submitting it anyway.

Comment 12

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

Attachment: Find hgtool, gittool at an absolute path

Oops, I lied, I said this patch would come next after the script_obj patch.

Anyway, different build scripts can chdir to different places, and checkout colliding versions of a directory named 'tools'. So I needed to find hgtool/gittool via an absolute path.

Comment 13

[Aki Sasaki (not active)]

Comment on attachment 8429768 [details] [diff] [review]
Automatic mock-or-not commands in b2g build

>diff --git a/scripts/b2g_build.py b/scripts/b2g_build.py
>--- a/scripts/b2g_build.py
>+++ b/scripts/b2g_build.py
>@@ -888,99 +888,91 @@ class B2GBuild(LocalesMixin, MockMixin, 
>             env['LOCALES_FILE'] = os.path.join(dirs['abs_work_dir'], 'gaia', self.config['gaia_languages_file'])
>         if self.config.get('locales_file'):
>             env['L10NBASEDIR'] = dirs['abs_l10n_dir']
>             env['MOZ_CHROME_MULTILOCALE'] = " ".join(self.query_locales())
>             if 'PATH' not in env:
>                 env['PATH'] = os.environ.get('PATH')
>             env['PATH'] += ':%s' % os.path.join(dirs['compare_locales_dir'], 'scripts')
>             env['PYTHONPATH'] = os.environ.get('PYTHONPATH', '')
>             env['PYTHONPATH'] += ':%s' % os.path.join(dirs['compare_locales_dir'], 'lib')
> 
>-        if 'mock_target' in gecko_config:
>-            # initialize mock
>+        self.enable_mock(config=gecko_config)
>+        if self.mock_enabled:
>             self.setup_mock(gecko_config['mock_target'], gecko_config['mock_packages'], gecko_config.get('mock_files'))
>-            if self.config['ccache']:
>-                self.run_mock_command(gecko_config['mock_target'], 'ccache -z', cwd=dirs['work_dir'], env=env)
> 
>-            for cmd in cmds:
>-                retval = self.run_mock_command(gecko_config['mock_target'], cmd, cwd=dirs['work_dir'], env=env, error_list=B2GMakefileErrorList)
>-                if retval != 0:
>-                    break

Ok, we stop running cmds here, and...

>-            if self.config['ccache']:
>-                self.run_mock_command(gecko_config['mock_target'], 'ccache -s', cwd=dirs['work_dir'], env=env)
>-        else:
>-            if self.config['ccache']:
>-                self.run_command('ccache -z', cwd=dirs['work_dir'], env=env)
>+        if self.config['ccache']:
>+            self.run_command('ccache -z', cwd=dirs['work_dir'], env=env)
>             for cmd in cmds:
>                 retval = self.run_command(cmd, cwd=dirs['work_dir'], env=env, error_list=B2GMakefileErrorList)
>                 if retval != 0:
>                     break

We keep it here, but only if ccache is enabled.

>-            if self.config['ccache']:
>-                self.run_command('ccache -s', cwd=dirs['work_dir'], env=env)
>+        if self.config['ccache']:
>+            self.run_command('ccache -s', cwd=dirs['work_dir'], env=env)
>         if retval != 0:
>             self.fatal("failed to build", exit_code=2)
> 
>+        self.disable_mock(config=gecko_config)
>+
>         buildid = self.query_buildid()
>         self.set_buildbot_property('buildid', buildid, write_to_file=True)

So if ccache isn't enabled, we stop running ./build.sh?

I think it otherwise looks ok.

Comment 14

[Aki Sasaki (not active)]

Comment on attachment 8429769 [details] [diff] [review]
Extract logic for generating build commands

Sure, generate_ or query_ .

Comment 15

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

Attachment: Automatic mock-or-not commands in b2g build

Good catch, thanks!

Comment 16

[Aki Sasaki (not active)]

Comment on attachment 8430980 [details] [diff] [review]
Automatic mock-or-not commands in b2g build

This looks good, but will conflict with the patch in bug 1017448.
Can you work with :lightsofapollo to come up with a solution that works for both of you?

Comment 17

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

Attachment: Extract out common portions of b2g build and b2g hazard analysis into a base class

This is just an FYI upload. I will be adding a new b2g hazard analysis build, and this patch extracts out the shared portions of b2g_build.py into a base class.

I am not requesting review on this yet because I haven't tested to see whether I broke anything with this split. Also, I don't have my new build working yet under mock, and my fixes so far have been somewhat intrusive, so I'm worried that this might still change.

Comment 18

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

Attachment: Add a --gecko-checkout option to point to a local gecko source tree

Another FYI patch, mostly including in case somebody wants to apply these patches. The next patch is the interesting one.

Comment 19

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

Attachment: Add a new b2g hazard build script

This is the big one. It adds in the new b2g hazard build script.

It will definitely still change (it's not working under mock). At some point, I'll also want to share code with (the misnamed) spidermonkey_build.py.

Comment 20

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

Attachment: Automatic mock-or-not commands in b2g build,

Ok, there was a serious problem with that last attempt at the automatic mock stuff: when calling run_command, it dispatches to run_command_m correctly, but then it uses self.config['mock_target'], which is None. I want the mock_target from gecko_config.

This may be too automagical for your taste, but I changed things so that enable_mock() remembers what mock_target it is enabled for. In fact, I replaced the self.mock_enabled flag with self.active_mock_target.

Other mock commands still require the mock_target to be passed in. I think this is ok, since many of these other calls are intermixed with commands that are run in the host environment. Though I'm tempted to make the mock_target parameter be optional and default to self.active_mock_target, but I didn't want to change that much code.

Let me know if there's a better approach.

Comment 21

[Aki Sasaki (not active)]

Comment on attachment 8432845 [details] [diff] [review]
Automatic mock-or-not commands in b2g build,

This looks good.
a) I'd like to also see --disable-mock (bug 1017448), though that doesn't strictly block
b) this changes enough that I'd love to see a green pass on Cypress after this lands on mozharness default.

Comment 22

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

(In reply to Aki Sasaki [:aki] from comment #21)
> Comment on attachment 8432845 [details] [diff] [review]
> Automatic mock-or-not commands in b2g build,
> 
> This looks good.
> a) I'd like to also see --disable-mock (bug 1017448), though that doesn't
> strictly block

That kind of needs to go into the new base class I'm carving out because I don't want the MockMixin to be mucking with config_options. It'll be in a later patch on this bug; I'm still testing it.

> b) this changes enough that I'd love to see a green pass on Cypress after
> this lands on mozharness default.

I haven't decided whether to land any of this before I land all of it. I probably should. I have green runs on ash already, though I didn't retrigger everything. And I'm still worried that I might still go back and change things before I get all 3 of these related builds working.

Getting closer, though. Thanks.

Comment 23

[Nicolas B. Pierron [:nbp]]

I am flagging Gecko 32 as affected, as Bug 941796 landed in Gecko 32.
We should ensure that we enable the static rooting analysis on this branch.

Comment 24

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

Attachment: Extract out common portions of b2g build and b2g hazard analysis into a base class

Ok, I guess I'll stick catlee with this one, since I've been giving him the b2g_build.py ones.

This extracts out a base class B2GBuildBaseScript that is shared by b2g_build.py and hazard_build.py. It just moves up the parts of the b2g build that are needed by the hazard build, which perhaps makes it a little odd what goes in the base class and what remains in b2g_build.py.

Comment 25

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

Attachment: Add a new b2g hazard build script

Define a new B2GHazardBuild that shares B2GBuildScriptBase with b2g_build.py. It also shares config files with spidermonkey_build.py. (I plan to extract out the shared stuff in hazard_build.py and spidermonkey_build.py into either a mixin or helper object in the future. For now, I've just updated things to work and made spidermonkey_build.py do things a little more like b2g_build.py does them.)

This also ends up implementing a --disable-mock command for B2GBuildScriptBase subclasses. I probably ought to extract that into a separate patch.

Picking on catlee because... er, I already gave him the base script split patch, and it's weird to see that one without something that uses it. Yeah, that's it.

Comment 26

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

Attachment: Fallback tooltool manifests when in-tree ones are not available

For both old releases, and for being able to land these changes independently from gecko tree changes right now, I'd like to have a fallback copy of the tooltool manifest files in the mozharness tree. I think they may also be useful for 3rd party developers trying to run the analysis on their own code.

Comment 27

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

Comment on attachment 8431021 [details] [diff] [review]
Add a --gecko-checkout option to point to a local gecko source tree

This is more complicated and less as useful than I first thought.

Comment 28

[Aki Sasaki (not active)]

Comment on attachment 8434677 [details] [diff] [review]
Fallback tooltool manifests when in-tree ones are not available

Third party won't have access to tooltool most likely, but I can see this being useful for older releases.

Comment 29

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

(In reply to Aki Sasaki [:aki] from comment #28)
> Comment on attachment 8434677 [details] [diff] [review]
> Fallback tooltool manifests when in-tree ones are not available
> 
> Third party won't have access to tooltool most likely, but I can see this
> being useful for older releases.

The tooltool server? Well, they can see http://people.mozilla.org/~sfink/tooltool ...

Comment 30

[Chris AtLee [:catlee]]

Comment on attachment 8434669 [details] [diff] [review]
Extract out common portions of b2g build and b2g hazard analysis into a base class

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

looks good!

::: mozharness/mozilla/building/buildb2gbase.py
@@ +42,5 @@
> +
> +
> +class B2GBuildBaseScript(BuildbotMixin, MockMixin,
> +                     BaseScript,
> +                     TooltoolMixin, VCSMixin, TransferMixin):

fix up indentation here please

@@ +58,5 @@
> +            'hgurl': 'https://hg.mozilla.org/',
> +        }
> +        default_config.update(config)
> +
> +        config_options = config_options[:] + [

don't think [:] is required. adding lists always creates a new list. this is different than += which modifies the original list.

@@ +88,5 @@
> +            [["--checkout-revision"], {
> +                "dest": "checkout_revision",
> +                "help": "checkout a specific gecko revision.",
> +            }],
> +        ]

can these default config options be put in B2GBuildBaseScript rather than inline here?

::: scripts/b2g_build.py
@@ +30,3 @@
>  from mozharness.mozilla.purge import PurgeMixin
>  from mozharness.mozilla.signing import SigningMixin
> +from mozharness.mozilla.repo_manifest import (add_project)

parentheses here are weird for just one import

Comment 31

[Chris AtLee [:catlee]]

Comment on attachment 8434675 [details] [diff] [review]
Add a new b2g hazard build script

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

looks reasonable. didn't read through it too closely though

Comment 32

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

Landed the "safer" packages. Need to test out a tooltool upload prereq, then will push remaining patches.

http://hg.mozilla.org/build/mozharness/rev/2152308aa091
http://hg.mozilla.org/build/mozharness/rev/71ec242d78af
http://hg.mozilla.org/build/mozharness/rev/5b4401a3ca11
http://hg.mozilla.org/build/mozharness/rev/7c8b30691e3e
http://hg.mozilla.org/build/mozharness/rev/e4d388ff871e
http://hg.mozilla.org/build/mozharness/rev/b2069bb5736e
http://hg.mozilla.org/build/mozharness/rev/bb52596257c5
http://hg.mozilla.org/build/mozharness/rev/129585003b0f
http://hg.mozilla.org/build/mozharness/rev/1b53ec0a48d7

Comment 33

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

Comment on attachment 8432845 [details] [diff] [review]
Automatic mock-or-not commands in b2g build,

The mock change broke other builds, see bug 1027146 and bug 1026468. Backed out.

https://hg.mozilla.org/build/mozharness/rev/4ef9ebab4dc2

Comment 34

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

Attachment: Ensure setup_mock is called early with the correct config

Ok, here's a replacement way to deal with the mock situation that's far
simpler, if slightly more error-prone. This patch is to be applied after the
buildb2gbase.py splitup, though I could have done it the other way around.

With this patch, you just need to call setup_mock() early enough, with the right mock_target, and everything will work. For B2G builders, they'll call setup_mock with values pulled out of gecko_config immediately after loading the gecko config, then MockMixin will remember the mock_target used and reuse it in later run_command_m calls.

For something like mobile_l10n, everything works as before, though behind the scenes run_command_m will call setup_mock which will remember the current mock_target instead of pulling it out of self.config['mock_target'] fresh each time. You could always use run_mock_command to run with a different target (as before).

With this patch in place, rebasing the following patch (creating hazard_build.py) consists only of removing the config keyword option to enable_mock(); it's no longer needed.

Then again, I haven't done a test run of this yet. Running into an error locally that happens without any of my patches.

Comment 35

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

Attachment: Add b2g hazard builds to try and ash branches

The script itself hasn't landed yet, but this will allow me to pre-test on ash.

This patch will make it off by default for try, on for ash.

Comment 36

[Aki Sasaki (not active)]

Comment on attachment 8443752 [details] [diff] [review]
Ensure setup_mock is called early with the correct config

I think this works.  I'd love to see a successful device build on Ash or Cypress before merging to prod.

Comment 37

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

(In reply to Aki Sasaki [:aki] from comment #36)
> I think this works.  I'd love to see a successful device build on Ash or
> Cypress before merging to prod.

Me too! I have green emulator and b2g desktop builds on ash, but I neglected to do a device build. Triggered one now. I'm also almost ready to push the rest of this bug, but I need green runs on ash and/or cypress before those get merged to production, and I'll need the b2g hazard build added to one or both for that. (I have a patch up for review for ash.)

Comment 38

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

(In reply to Steve Fink [:sfink] from comment #37)
> (In reply to Aki Sasaki [:aki] from comment #36)
> > I think this works.  I'd love to see a successful device build on Ash or
> > Cypress before merging to prod.
> 
> Me too! I have green emulator and b2g desktop builds on ash, but I neglected
> to do a device build. Triggered one now.

Oh. My mistake. There's a later merge to ash that picked this up, so this *does* have a green device build already.

Comment 39

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

http://hg.mozilla.org/build/mozharness/rev/b80a78e3e53a
http://hg.mozilla.org/build/mozharness/rev/1527d06e46bd
http://hg.mozilla.org/build/mozharness/rev/244e1b4da23e

Comment 40

[Ryan VanderMeulen [:RyanVM]]

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

Comment 41

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

Oops, sorry. This is very much a [leave-open].

Comment 42

[Justin Wood (:Callek)]

something here is in production

Comment 43

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

Attachment: Add b2g hazard builds

Looks like (1) I put the build in completely the wrong place, and (2) bhearsum's out. Sorry for picking on you, Callek. Redirect if there's someone better who is around right now.

I will also output the diff of the basic config dump with this patch.

Comment 44

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

Attachment: Config difference with and without this patch

Comment 45

[Aki Sasaki (not active)]

Comment on attachment 8447316 [details] [diff] [review]
Add b2g hazard builds

lgtm

Comment 46

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

Split into two parts for landing, so I can test it with a non-default try builder before turning it on anywhere else.

Add a try builder only, and turn it off by default: https://hg.mozilla.org/build/buildbot-configs/rev/8c4ea5f01cb7

Comment 47

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

Comment on attachment 8434675 [details] [diff] [review]
Add a new b2g hazard build script

Landed the b2g hazard script: https://hg.mozilla.org/build/mozharness/rev/383ef298caf0

Comment 48

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

Part of the manifest fallback patch snuck into the previously landed one. Spot fix:

https://hg.mozilla.org/build/mozharness/rev/930858624d92

Comment 49

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

Attachment: Run hazard commands under mock when needed

More fun with mock. The b2g hazard build needs to run more stuff under mock to work, and it still needs to make sure it's the right mock target env.

Comment 50

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

Comment on attachment 8449835 [details] [diff] [review]
Run hazard commands under mock when needed

Looks like Aki's out. Picking a new victim. Note that this should be relatively safe; it can only break a build that doesn't run anywhere yet. ;-) (Well, it runs on try if you explicitly request it.)

And I probably ought to do something at the base class level instead of here.

Comment 51

[Jordan Lund (:jlund)]

Comment on attachment 8449835 [details] [diff] [review]
Run hazard commands under mock when needed

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

lgtm. see in line for proposal. As mentioned below, I'm cool with landing this if you disagree with the idea.

::: scripts/hazard_build.py
@@ +253,5 @@
> +    # The gecko config is required before enabling mock, because it determines
> +    # what mock target to use.
> +    def enable_mock(self):
> +        self.load_gecko_config()
> +        super(B2GHazardBuild, self).enable_mock()

so essentially we are overriding enable_mock by putting setup_mock inside it against a custom mock_target. Then every time we call setup_mock without a custom mock_target from within mock, it doesn't matter because we are done_mock_setup already right?

It sort of feels like maybe we should be overriding setup_mock instead. Or maybe just pulling setup_mock out of load_gecko_config as I had to dive into that method to see as a side effect, it called setup_mock.

where overriding setup_mock would look something like my cheap pseudo code: 

def setup_mock(self, *, **):
    if self.done_mock_setup:
        return
    gecko_config = self.load_gecko_config()
    super(B2GHazardBuild, self).setup_mock(gecko_config['mock_target'],
                                           gecko_config['mock_packages'],
                                           gecko_config.get('mock_files'), *, **)

I don't know, I completely understand your fix here and I don't want to block on the details. r+ if you think your way is better or if mine is not feasible. :)

Comment 52

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

(In reply to Jordan Lund (:jlund) from comment #51)
> Comment on attachment 8449835 [details] [diff] [review]
> Run hazard commands under mock when needed
> 
> Review of attachment 8449835 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> lgtm. see in line for proposal. As mentioned below, I'm cool with landing
> this if you disagree with the idea.
> 
> ::: scripts/hazard_build.py
> @@ +253,5 @@
> > +    # The gecko config is required before enabling mock, because it determines
> > +    # what mock target to use.
> > +    def enable_mock(self):
> > +        self.load_gecko_config()
> > +        super(B2GHazardBuild, self).enable_mock()
> 
> so essentially we are overriding enable_mock by putting setup_mock inside it
> against a custom mock_target. Then every time we call setup_mock without a
> custom mock_target from within mock, it doesn't matter because we are
> done_mock_setup already right?

Yes, exactly. It's the watered-down fix from comment 34 of this bug, extended to cover more actions -- including actions that precede when the gecko config would "normally" be downloaded for the main build step. I needed it earlier, during the JS shell configure/build stuff, which happens before the main build. The other alternative would be to build the shell after the main build, but right now the main build happens from within the analyze.py script which also uses the JS shell built here, so I'd need to disentangle those and break them into separate actions, which is actually pretty reasonable but didn't sound like much fun. (analyze.py is sort of a mozharness-like miniframework that I wrote before ever doing anything with mozharness.)

> It sort of feels like maybe we should be overriding setup_mock instead.

Hrm. I think you may be right. I did it this way mostly because it was the latest in a long string of mock-related failures, and I just wanted to get it out of the way. 

> Or maybe just pulling setup_mock out of load_gecko_config as I had to dive into
> that method to see as a side effect, it called setup_mock.

Right, that was also my doing, after trying it the more straightforward way and running into failures due to people manually calling run_command_m. I figured that if it did setup_mock as soon as possible after loading the gecko_config, then there was less of a window to trigger a setup_mock (directly or indirectly) with the wrong mock_target.

But you're right, given that load_gecko_config can be called with no arguments, it makes more sense for setup_mock to call it rather than the other way around. Seems like I should have done that in the first place.

> I don't know, I completely understand your fix here and I don't want to
> block on the details. r+ if you think your way is better or if mine is not
> feasible. :)

Ok, thanks. Due to the difficulty in testing all affected builds since they're not available everywhere yet, I'm going to take the coward's way out: I'll land this patch as-is, and use it to get the new b2g hazard build green on try. Then I'll turn on the b2g hazard build everywhere, and make a patch in the style you suggested above. That way I can use ash to make sure I can keep all 3 mozharness scripts working with that change. (And then in the middle of that, somebody will switch us off of mock and all of this will be wasted effort...)

I think the fundamental difficulty here is a combination of (1) mock.py is used in two different ways, either implicitly via a run_command that decides which way to run things or explicitly via run_command_m (and a few other entry points); and (2) the b2g build loads needed config goop dynamically. Eliminating either of those would make life much simpler.

Comment 53

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

Attachment: Remove verbose sixgill warning message that makes the log file overflow the size cap

I'm a bad person so I'm going to commit this one without review. It's just turning off a compiler warning flag. (It works, but only will a clobber build, as I eventually figured out.)

Comment 54

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

remote:   https://hg.mozilla.org/build/mozharness/rev/760bdc8e04b6
remote:   https://hg.mozilla.org/build/mozharness/rev/7e109100febe

Comment 55

[Jordan Lund (:jlund)]

> Ok, thanks. Due to the difficulty in testing all affected builds since
> they're not available everywhere yet, I'm going to take the coward's way
> out: I'll land this patch as-is

sounds good. makes sense.

> 
> I think the fundamental difficulty here is a combination of (1) mock.py is
> used in two different ways, either implicitly via a run_command that decides
> which way to run things or explicitly via run_command_m (and a few other
> entry points); and (2) the b2g build loads needed config goop dynamically.
> Eliminating either of those would make life much simpler.

yes I agree.

Comment 56

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

Attachment: Make setup_mock call load_gecko_config instead of the other way around

Hrm. So it turns out there's a problem with making setup_mock call load_gecko_config. b2g_build.py has functions that do:

   self.load_gecko_config()
   ...
   self.enable_mock()
   self.run_command(...)
   self.disable_mock()

With this change, the first load_gecko_config() will no longer affect mock. self.enable_mock will look at self.default_mock_target, which hasn't been set yet, and self.config['mock_target'], which shouldn't need to be set (I don't know if it is or not). So it returns without switching self.run_command to run_command_m. As a result, the above call to self.run_command won't run under mock. setup_mock is never called.

I could switch all such functions to do setup_mock instead of load_gecko_config. I'm not sure what's better at this point, though. Neither option really feels right.

Comment 57

[Chris Cooper [:coop] (he/him)]

Merged to production, and deployed.

Comment 58

[Lawrence Mandel [:lmandel] (use needinfo)]

Clearing the desktop tracking flags and setting the b2g status flags as this is B2G specific.

Comment 59

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

Threw the big switch (enabling the build on all main trees, at the next reconfig):

https://hg.mozilla.org/build/buildbot-configs/rev/88b9ef301baf

Comment 60

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

Comment on attachment 8447316 [details] [diff] [review]
Add b2g hazard builds

Both halves of this patch have now landed.

Comment 61

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

Attachment: Label future hazard builds

I'm enabling a new b2g hazard analysis build, so I need tbpl labeling. Also, the existing SM(Hf) build really really really shouldn't be SM(anything); it's checking the full tree, not spidermonkey.

I may end up renaming all of these to H builds, and imply the code being checked by the platform grouping. But I don't fully understand how those platforms are grouped, so unless someone edumacates me, I think it's safer to leave in the extra letters for now.

Comment 62

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

(In reply to Steve Fink [:sfink] from comment #60)
> Comment on attachment 8447316 [details] [diff] [review]
> Add b2g hazard builds
> 
> Both halves of this patch have now landed.

Actually, not quite. I had also turned off the try builds to do the incremental rollout. Turning them back on:

https://hg.mozilla.org/build/buildbot-configs/rev/1f47a7c4b761

Comment 63

[Pete Moore [:pmoore][:pete]]

About to roll this out to production...

Comment 64

[Pete Moore [:pmoore][:pete]]

In production

Comment 65

[Ed Morley [:emorley]]

Hidden on TBPL until the b2g fix merges around, particularly given that the TBPL symbols are wrong until the TBPL piece lands and is in production. Please can you remind me to unhide when appropriate :-) (I'll try to remember, but may not)

Comment 66

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

https://hg.mozilla.org/mozilla-central/rev/1d82bd7c2c97

Comment 67

[Phil Ringnalda (:philor)]

Seems we never got around to hiding it on mozilla-central, though it still being hidden everywhere else was handy since it's now totally busted, https://tbpl.mozilla.org/php/getParsedLog.php?id=43384643&tree=Mozilla-Central

Hidden on m-c.

Comment 69

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

Fun, retriggers of previously green runs are failing with that failure...

Comment 70

[Phil Ringnalda (:philor)]

Yeah, that's supposed to give you the scent of releng, probably something like the clobber patch causes us to delete a directory which isn't recreated before gittool expects it to exist as the parent directory of the checkout.

Comment 71

[Phil Ringnalda (:philor)]

And hidden on Try, since people are just blindly retriggering it over and over and over.

Comment 72

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

(In reply to Phil Ringnalda (:philor) from comment #70)
> Yeah, that's supposed to give you the scent of releng, probably something
> like the clobber patch causes us to delete a directory which isn't recreated
> before gittool expects it to exist as the parent directory of the checkout.

It appears to be weirder than that. What seems to happen is:

1. mkdir /builds/slave/b2g-haz/build
2. mock --init, since the previous environment had different packages
3. gittool.py from within that directory (/builds/slave/b2g-haz/build/build-tools/.../gittool.py), attempting to check out into that directory

I don't know why this is a problem. I thought /builds/slave would be untouched by mock --init. But that's probably why I don't see it in my testing -- I always have a matching set of packages in my mock environment, so mock --init doesn't do anything. (That's just a hypothesis, though.)

#3 drives me nuts. Why oh why do B2G builds insist on putting the actual bits to be built in the same directory as the build tools and things? I would note that if you get a transient failure during the gittool.py run, it's going to commit suicide when it nukes the .../build directory and thus the retries are guaranteed to fail.

I attempted to fix this during my Grand Unification project of the hazard build and b2g_build.py, but the paths were embedded in too many places and my wife started to complain about the constant screaming.

That mock --init thing still sounds wrong. I'm probably barking up the wrong tree.

Comment 73

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

Yep, was totally off base. I can reproduce at will.

The problem was that when I added the clobber in bug 1035609, I did the clobber after the checkout-tools step, which then clobbered the tools it had just checked out. I mistakenly thought I needed those tools in order to perform the clobber, but it seems all that logic is in the mozharness stuff. And it seems to do the right thing if I swap the order.

Comment 74

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

Attachment: Clobber first, then check out tools

Comment 75

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

https://hg.mozilla.org/build/mozharness/rev/52409e49d0f1

Comment 76

[Pete Moore [:pmoore][:pete]]

All buildbot-config, buildbotcustom and mozharness changes that were already landed on default, have been merged to production branches and deployed to production. There are so many attachments to this bug, I wouldn't like to say which one(s) this affects! :)

Comment 77

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

(In reply to Pete Moore [:pete][:pmoore] from comment #76)
> All buildbot-config, buildbotcustom and mozharness changes that were already
> landed on default, have been merged to production branches and deployed to
> production. There are so many attachments to this bug, I wouldn't like to
> say which one(s) this affects! :)

Everything marked checkin+ is now in production. And it finally seems to all be working, and seemingly not even running machines out of disk space and driving them into the ground anymore. Life is good. And if I want to bring down buildbot again, there's always another day and another patch. And another bug; resolving this one now.

\o/

Comment 78

[Ryan VanderMeulen [:RyanVM]]

TBPL patch in production :)

Comment 79

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/74c542d0d896

Landed these patches in aurora because the b2g build is broken there, and these patches only affect this hazard build, not shipping code.
https://hg.mozilla.org/releases/mozilla-aurora/rev/0ec0b9ac39f0