Enable static rooting analysis on b2g

RESOLVED FIXED in mozilla33

Status

()

defect
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: cpeterson, Assigned: sfink)

Tracking

unspecified
mozilla33
ARM
Gonk (Firefox OS)
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(b2g-v2.0 affected, b2g-v2.1 affected)

Details

Attachments

(21 attachments, 6 obsolete attachments)

458 bytes, patch
aki
: review+
sfink
: checkin+
Details | Diff | Splinter Review
1.23 KB, patch
aki
: review+
sfink
: checkin+
Details | Diff | Splinter Review
1.67 KB, patch
aki
: review+
sfink
: checkin+
Details | Diff | Splinter Review
2.61 KB, patch
aki
: review+
sfink
: checkin+
Details | Diff | Splinter Review
11.43 KB, patch
catlee
: review+
sfink
: checkin+
Details | Diff | Splinter Review
3.38 KB, patch
aki
: review+
sfink
: checkin+
Details | Diff | Splinter Review
4.81 KB, patch
catlee
: review+
sfink
: checkin+
Details | Diff | Splinter Review
4.54 KB, patch
catlee
: review+
sfink
: checkin+
Details | Diff | Splinter Review
2.12 KB, patch
catlee
: review+
sfink
: checkin+
Details | Diff | Splinter Review
11.77 KB, patch
aki
: review+
Details | Diff | Splinter Review
51.96 KB, patch
catlee
: review+
sfink
: checkin+
Details | Diff | Splinter Review
43.78 KB, patch
catlee
: review+
sfink
: checkin+
Details | Diff | Splinter Review
8.70 KB, patch
aki
: review+
Details | Diff | Splinter Review
12.40 KB, patch
aki
: review+
sfink
: checkin+
Details | Diff | Splinter Review
8.53 KB, patch
aki
: review+
sfink
: checkin+
Details | Diff | Splinter Review
28.12 KB, text/plain
Details
6.46 KB, patch
jlund
: review+
sfink
: checkin+
Details | Diff | Splinter Review
1.73 KB, patch
sfink
: checkin+
Details | Diff | Splinter Review
4.46 KB, patch
Details | Diff | Splinter Review
4.31 KB, patch
emorley
: review+
sfink
: checkin+
Details | Diff | Splinter Review
2.10 KB, patch
aki
: review+
sfink
: checkin+
Details | Diff | Splinter Review
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.
No longer blocks: 834909
Blocks: 834909
No longer blocks: 831409
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.
Duplicate of this bug: 941797
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.
Attachment #8429762 - Flags: review?(aki)
Totally minor, but I couldn't see any difference between the explicit loop and dict.update.
Attachment #8429764 - Flags: review?(aki)
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.
Attachment #8429765 - Flags: review?(aki)
Clear up some repetition in b2g_build.py due to the the mock config not being in the main config.
Attachment #8429768 - Flags: review?(aki)
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?
Attachment #8429769 - Flags: review?(aki)
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.
Attachment #8429770 - Flags: review?(catlee)
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.
Attachment #8429773 - Flags: review?(aki)
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.
Attachment #8429776 - Flags: review?(catlee)
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.
Attachment #8429777 - Flags: review?(catlee)
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.
Attachment #8429778 - Flags: review?(catlee)
Attachment #8429762 - Flags: review?(aki) → review+
Attachment #8429764 - Flags: review?(aki) → review+
Attachment #8429765 - Flags: review?(aki) → review+
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 on attachment 8429769 [details] [diff] [review]
Extract logic for generating build commands

Sure, generate_ or query_ .
Attachment #8429769 - Flags: review?(aki) → review+
Attachment #8429773 - Flags: review?(aki) → review+
Attachment #8429768 - Flags: review?(aki) → review-
Good catch, thanks!
Attachment #8430980 - Flags: review?(aki)
Attachment #8429768 - Attachment is obsolete: true
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?
Attachment #8430980 - Flags: review?(aki) → review+
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.
Another FYI patch, mostly including in case somebody wants to apply these patches. The next patch is the interesting one.
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.
Attachment #8429770 - Flags: review?(catlee) → review+
Attachment #8429776 - Flags: review?(catlee) → review+
Attachment #8429777 - Flags: review?(catlee) → review+
Attachment #8429778 - Flags: review?(catlee) → review+
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.
Attachment #8432845 - Flags: review?(aki)
Attachment #8430980 - Attachment is obsolete: true
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.
Attachment #8432845 - Flags: review?(aki) → review+
(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.
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.
Depends on: 1019914
Depends on: 1019920
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.
Attachment #8434669 - Flags: review?(catlee)
Attachment #8431018 - Attachment is obsolete: true
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.
Attachment #8434675 - Flags: review?(catlee)
Attachment #8431024 - Attachment is obsolete: true
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.
Attachment #8434677 - Flags: review?(aki)
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.
Attachment #8431021 - Attachment is obsolete: true
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.
Attachment #8434677 - Flags: review?(aki) → review+
(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 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
Attachment #8434669 - Flags: review?(catlee) → review+
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
Attachment #8434675 - Flags: review?(catlee) → review+
Attachment #8429762 - Flags: checkin+
Attachment #8429764 - Flags: checkin+
Attachment #8429765 - Flags: checkin+
Attachment #8429769 - Flags: checkin+
Attachment #8429770 - Flags: checkin+
Attachment #8429773 - Flags: checkin+
Attachment #8429776 - Flags: checkin+
Attachment #8429777 - Flags: checkin+
Attachment #8432845 - Flags: checkin+
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
Attachment #8432845 - Flags: checkin+
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.
Attachment #8443752 - Flags: review?(aki)
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.
Attachment #8443815 - Flags: review?(bhearsum)
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.
Attachment #8443752 - Flags: review?(aki) → review+
(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.)
(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.
Attachment #8429778 - Flags: checkin+
Attachment #8434669 - Flags: checkin+
Attachment #8443752 - Flags: checkin+
https://hg.mozilla.org/mozilla-central/rev/bdb1af2c18fd
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Oops, sorry. This is very much a [leave-open].
Status: RESOLVED → REOPENED
Keywords: leave-open
Resolution: FIXED → ---
something here is in production
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.
Attachment #8447316 - Flags: review?(bugspam.Callek)
Attachment #8443815 - Attachment is obsolete: true
Attachment #8443815 - Flags: review?(bhearsum)
Attachment #8447316 - Flags: review?(bugspam.Callek) → review?(aki)
Comment on attachment 8447316 [details] [diff] [review]
Add b2g hazard builds

lgtm
Attachment #8447316 - Flags: review?(aki) → review+
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 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
Attachment #8434675 - Flags: checkin+
Part of the manifest fallback patch snuck into the previously landed one. Spot fix:

https://hg.mozilla.org/build/mozharness/rev/930858624d92
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.
Attachment #8449835 - Flags: review?(aki)
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.
Attachment #8449835 - Flags: review?(aki) → review?(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. :)
Attachment #8449835 - Flags: review?(jlund) → review+
(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.
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.)
Attachment #8449835 - Flags: checkin+
Attachment #8450379 - Flags: checkin+
> 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.
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.
Merged to production, and deployed.
Clearing the desktop tracking flags and setting the b2g status flags as this is B2G specific.
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 on attachment 8447316 [details] [diff] [review]
Add b2g hazard builds

Both halves of this patch have now landed.
Attachment #8447316 - Flags: checkin+
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.
Attachment #8451450 - Flags: review?(emorley)
(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
About to roll this out to production...
Attachment #8451450 - Flags: review?(emorley) → review+
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)
Attachment #8451450 - Flags: checkin+
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.
Duplicate of this bug: 1036211
Fun, retriggers of previously green runs are failing with that failure...
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.
And hidden on Try, since people are just blindly retriggering it over and over and over.
(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.
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.
Attachment #8453246 - Flags: review?(aki) → review+
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! :)
Attachment #8453246 - Flags: checkin+
(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/
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Keywords: leave-open
Resolution: --- → FIXED
Depends on: 1037175
TBPL patch in production :)
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
You need to log in before you can comment on or make changes to this bug.