Closed
Bug 898554
Opened 11 years ago
Closed 10 years ago
Enable static rooting analysis on b2g
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
mozilla33
People
(Reporter: cpeterson, Assigned: sfink)
References
Details
Attachments
(21 files, 6 obsolete files)
(deleted),
patch
|
mozilla
:
review+
sfink
:
checkin+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
mozilla
:
review+
sfink
:
checkin+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
mozilla
:
review+
sfink
:
checkin+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
mozilla
:
review+
sfink
:
checkin+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
catlee
:
review+
sfink
:
checkin+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
mozilla
:
review+
sfink
:
checkin+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
catlee
:
review+
sfink
:
checkin+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
catlee
:
review+
sfink
:
checkin+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
catlee
:
review+
sfink
:
checkin+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
mozilla
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
catlee
:
review+
sfink
:
checkin+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
catlee
:
review+
sfink
:
checkin+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
mozilla
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
mozilla
:
review+
sfink
:
checkin+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
mozilla
:
review+
sfink
:
checkin+
|
Details | Diff | Splinter Review |
(deleted),
text/plain
|
Details | |
(deleted),
patch
|
jlund
:
review+
sfink
:
checkin+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
sfink
:
checkin+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
emorley
:
review+
sfink
:
checkin+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
mozilla
:
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.
Updated•11 years ago
|
Assignee | ||
Comment 1•11 years ago
|
||
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.
Updated•11 years ago
|
Blocks: ExactRootingB2G
Assignee | ||
Comment 3•11 years ago
|
||
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)
Assignee | ||
Comment 4•11 years ago
|
||
Totally minor, but I couldn't see any difference between the explicit loop and dict.update.
Attachment #8429764 -
Flags: review?(aki)
Assignee | ||
Comment 5•11 years ago
|
||
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)
Assignee | ||
Comment 6•11 years ago
|
||
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)
Assignee | ||
Comment 7•11 years ago
|
||
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)
Assignee | ||
Comment 8•11 years ago
|
||
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)
Assignee | ||
Comment 9•11 years ago
|
||
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)
Assignee | ||
Comment 10•11 years ago
|
||
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)
Assignee | ||
Comment 11•11 years ago
|
||
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)
Assignee | ||
Comment 12•11 years ago
|
||
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)
Updated•11 years ago
|
Attachment #8429762 -
Flags: review?(aki) → review+
Updated•11 years ago
|
Attachment #8429764 -
Flags: review?(aki) → review+
Updated•11 years ago
|
Attachment #8429765 -
Flags: review?(aki) → review+
Comment 13•11 years ago
|
||
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•11 years ago
|
||
Comment on attachment 8429769 [details] [diff] [review]
Extract logic for generating build commands
Sure, generate_ or query_ .
Attachment #8429769 -
Flags: review?(aki) → review+
Updated•11 years ago
|
Attachment #8429773 -
Flags: review?(aki) → review+
Updated•11 years ago
|
Attachment #8429768 -
Flags: review?(aki) → review-
Assignee | ||
Updated•11 years ago
|
Attachment #8429768 -
Attachment is obsolete: true
Comment 16•11 years ago
|
||
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+
Assignee | ||
Comment 17•11 years ago
|
||
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.
Assignee | ||
Comment 18•11 years ago
|
||
Another FYI patch, mostly including in case somebody wants to apply these patches. The next patch is the interesting one.
Assignee | ||
Comment 19•11 years ago
|
||
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.
Updated•11 years ago
|
Attachment #8429770 -
Flags: review?(catlee) → review+
Updated•11 years ago
|
Attachment #8429776 -
Flags: review?(catlee) → review+
Updated•11 years ago
|
Attachment #8429777 -
Flags: review?(catlee) → review+
Updated•11 years ago
|
Attachment #8429778 -
Flags: review?(catlee) → review+
Assignee | ||
Comment 20•10 years ago
|
||
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)
Assignee | ||
Updated•10 years ago
|
Attachment #8430980 -
Attachment is obsolete: true
Comment 21•10 years ago
|
||
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+
Assignee | ||
Comment 22•10 years ago
|
||
(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•10 years ago
|
||
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.
status-firefox32:
--- → affected
tracking-firefox32:
--- → ?
Assignee | ||
Comment 24•10 years ago
|
||
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)
Assignee | ||
Updated•10 years ago
|
Attachment #8431018 -
Attachment is obsolete: true
Assignee | ||
Comment 25•10 years ago
|
||
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)
Assignee | ||
Updated•10 years ago
|
Attachment #8431024 -
Attachment is obsolete: true
Assignee | ||
Comment 26•10 years ago
|
||
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)
Assignee | ||
Comment 27•10 years ago
|
||
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 28•10 years ago
|
||
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+
Assignee | ||
Comment 29•10 years ago
|
||
(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•10 years ago
|
||
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 31•10 years ago
|
||
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+
Assignee | ||
Updated•10 years ago
|
Attachment #8429762 -
Flags: checkin+
Assignee | ||
Updated•10 years ago
|
Attachment #8429764 -
Flags: checkin+
Assignee | ||
Updated•10 years ago
|
Attachment #8429765 -
Flags: checkin+
Assignee | ||
Updated•10 years ago
|
Attachment #8429769 -
Flags: checkin+
Assignee | ||
Updated•10 years ago
|
Attachment #8429770 -
Flags: checkin+
Assignee | ||
Updated•10 years ago
|
Attachment #8429773 -
Flags: checkin+
Assignee | ||
Updated•10 years ago
|
Attachment #8429776 -
Flags: checkin+
Assignee | ||
Updated•10 years ago
|
Attachment #8429777 -
Flags: checkin+
Assignee | ||
Updated•10 years ago
|
Attachment #8432845 -
Flags: checkin+
Assignee | ||
Comment 32•10 years ago
|
||
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
Assignee | ||
Comment 33•10 years ago
|
||
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+
Assignee | ||
Comment 34•10 years ago
|
||
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)
Assignee | ||
Comment 35•10 years ago
|
||
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 36•10 years ago
|
||
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+
Assignee | ||
Comment 37•10 years ago
|
||
(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.)
Assignee | ||
Comment 38•10 years ago
|
||
(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.
Assignee | ||
Updated•10 years ago
|
Attachment #8429778 -
Flags: checkin+
Assignee | ||
Updated•10 years ago
|
Attachment #8434669 -
Flags: checkin+
Assignee | ||
Updated•10 years ago
|
Attachment #8443752 -
Flags: checkin+
Assignee | ||
Comment 39•10 years ago
|
||
Comment 40•10 years ago
|
||
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Assignee | ||
Comment 41•10 years ago
|
||
Oops, sorry. This is very much a [leave-open].
Comment 42•10 years ago
|
||
something here is in production
Assignee | ||
Comment 43•10 years ago
|
||
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)
Assignee | ||
Updated•10 years ago
|
Attachment #8443815 -
Attachment is obsolete: true
Attachment #8443815 -
Flags: review?(bhearsum)
Assignee | ||
Comment 44•10 years ago
|
||
Updated•10 years ago
|
Attachment #8447316 -
Flags: review?(bugspam.Callek) → review?(aki)
Comment 45•10 years ago
|
||
Comment on attachment 8447316 [details] [diff] [review]
Add b2g hazard builds
lgtm
Attachment #8447316 -
Flags: review?(aki) → review+
Assignee | ||
Comment 46•10 years ago
|
||
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
Assignee | ||
Comment 47•10 years ago
|
||
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+
Assignee | ||
Comment 48•10 years ago
|
||
Part of the manifest fallback patch snuck into the previously landed one. Spot fix:
https://hg.mozilla.org/build/mozharness/rev/930858624d92
Assignee | ||
Comment 49•10 years ago
|
||
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)
Assignee | ||
Comment 50•10 years ago
|
||
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 51•10 years ago
|
||
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+
Assignee | ||
Comment 52•10 years ago
|
||
(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.
Assignee | ||
Comment 53•10 years ago
|
||
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.)
Assignee | ||
Comment 54•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8449835 -
Flags: checkin+
Assignee | ||
Updated•10 years ago
|
Attachment #8450379 -
Flags: checkin+
Comment 55•10 years ago
|
||
> 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.
Assignee | ||
Comment 56•10 years ago
|
||
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•10 years ago
|
||
Merged to production, and deployed.
Comment 58•10 years ago
|
||
Clearing the desktop tracking flags and setting the b2g status flags as this is B2G specific.
status-b2g-v2.0:
--- → affected
status-b2g-v2.1:
--- → affected
status-firefox32:
affected → ---
tracking-firefox32:
? → ---
Assignee | ||
Comment 59•10 years ago
|
||
Threw the big switch (enabling the build on all main trees, at the next reconfig):
https://hg.mozilla.org/build/buildbot-configs/rev/88b9ef301baf
Assignee | ||
Comment 60•10 years ago
|
||
Comment on attachment 8447316 [details] [diff] [review]
Add b2g hazard builds
Both halves of this patch have now landed.
Attachment #8447316 -
Flags: checkin+
Assignee | ||
Comment 61•10 years ago
|
||
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)
Assignee | ||
Comment 62•10 years ago
|
||
(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•10 years ago
|
||
About to roll this out to production...
Updated•10 years ago
|
Attachment #8451450 -
Flags: review?(emorley) → review+
Comment 64•10 years ago
|
||
In production
Comment 65•10 years ago
|
||
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)
Assignee | ||
Updated•10 years ago
|
Attachment #8451450 -
Flags: checkin+
Comment 67•10 years ago
|
||
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.
Fun, retriggers of previously green runs are failing with that failure...
Comment 70•10 years ago
|
||
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•10 years ago
|
||
And hidden on Try, since people are just blindly retriggering it over and over and over.
Assignee | ||
Comment 72•10 years ago
|
||
(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.
Assignee | ||
Comment 73•10 years ago
|
||
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.
Assignee | ||
Comment 74•10 years ago
|
||
Attachment #8453246 -
Flags: review?(aki)
Updated•10 years ago
|
Attachment #8453246 -
Flags: review?(aki) → review+
Assignee | ||
Comment 75•10 years ago
|
||
Comment 76•10 years ago
|
||
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! :)
Assignee | ||
Updated•10 years ago
|
Attachment #8453246 -
Flags: checkin+
Assignee | ||
Comment 77•10 years ago
|
||
(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: 10 years ago → 10 years ago
Keywords: leave-open
Resolution: --- → FIXED
Comment 78•10 years ago
|
||
TBPL patch in production :)
Assignee | ||
Comment 79•10 years ago
|
||
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.
Description
•