1027146 - all android single locale repacks broken

Status: RESOLVED FIXED

(Release Engineering :: General, defect)

Description

[[e-mail] (:bhearsum)]

They don't seem to be install mock packages anymore:
Before:
02:16:27     INFO - #####
02:16:27     INFO - ##### Running setup step.
02:16:27     INFO - #####
02:16:27     INFO - Running main action method: setup
02:16:27     INFO - Copying /builds/slave/m-aurora-and-l10n_1-0000000000/build/mozilla-aurora/mobile/android/config/mozconfigs/android/l10n-nightly to /builds/slave/m-aurora-and-l10n_1-0000000000/build/mozilla-aurora/.mozconfig
02:16:27     INFO - Getting output from command: ['mock_mozilla', '-r', 'mozilla-centos6-x86_64', '--print-root-path']
02:16:27     INFO - Copy/paste: mock_mozilla -r mozilla-centos6-x86_64 --print-root-path
02:16:28     INFO - Reading from file tmpfile_stdout
02:16:28     INFO - Output received:
02:16:28     INFO -  /builds/mock_mozilla/mozilla-centos6-x86_64/root/
02:16:28     INFO - retry: Calling <built-in function remove> with args: ('tmpfile_stderr',), kwargs: {}, attempt #1
02:16:28     INFO - retry: Calling <built-in function remove> with args: ('tmpfile_stdout',), kwargs: {}, attempt #1
02:16:28     INFO - no previous package list found
02:16:28     INFO - Running command: ['mock_mozilla', '-r', 'mozilla-centos6-x86_64', '--init']
02:16:28     INFO - Copy/paste: mock_mozilla -r mozilla-centos6-x86_64 --init

After:
02:24:48     INFO - #####
02:24:48     INFO - ##### Running setup step.
02:24:48     INFO - #####
02:24:48     INFO - Running main action method: setup
02:24:48     INFO - Copying /builds/slave/m-aurora-and-l10n_3-0000000000/build/mozilla-aurora/mobile/android/config/mozconfigs/android/l10n-nightly to /builds/slave/m-aurora-and-l10n_3-0000000000/build/mozilla-aurora/.mozconfig
02:24:48     INFO - Running command: ['cat', '/builds/slave/m-aurora-and-l10n_3-0000000000/build/mozilla-aurora/.mozconfig']
02:24:48     INFO - Copy/paste: cat /builds/slave/m-aurora-and-l10n_3-0000000000/build/mozilla-aurora/.mozconfig
02:24:48     INFO -  . "$topsrcdir/mobile/android/config/mozconfigs/common"
02:24:48     INFO -  # L10n

The last successful jobs were the morning of June 16th.

Comment 1

[[e-mail] (:bhearsum)]

I can't find any changes that in the regression range that look remotely relevant. Maybe something to do with reimaged slaves after they moved?

Comment 2

[Justin Wood (:Callek)]

Relatedly (possibly) comm-central Thunderbird repacks are broken too: Bug 1026491  (reported on June 17)

Comment 3

[Aki Sasaki (not active)]

Guessing bug 898554 broke them.

Comment 4

[Jordan Lund (:jlund)]

yes it is due to http://hg.mozilla.org/build/mozharness/rev/b2069bb5736e

l10n and mozharness builds were using MockMixin differently. (I think desktop l10n too but that might not be live).

Anyway, we need to call enable_mock() and disable_mock() explicitly or else back out the patch.

my proposed solution for desktop builds that l10n should be able to take: https://bugzilla.mozilla.org/attachment.cgi?id=8441857&action=edit

Comment 5

[[e-mail] (:bhearsum)]

Steve, it looks like you broke this. Can you have a look?

Comment 6

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

Attachment: Implement a mock_enabled context manager and use it in place of explicit run_command_m calls

This is the heavyweight solution, if we want to preserve that other patch. Alternatively, we could back it out. Or make explicit invocation of run_command_m do what is currently expected, and use something else as self.run_command when mock is enabled.

I haven't tried running any builds with this patch yet.

Comment 7

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

Comment on attachment 8442311 [details] [diff] [review]
Implement a mock_enabled context manager and use it in place of explicit run_command_m calls

jlund, I'm wondering what you think about this approach as compared to your patch in bug 1026468. Or maybe it would be better to call this

  with self.maybe_mock_enabled():

instead? Or |with self.mock_enabled_if_needed|?

Comment 8

[Jordan Lund (:jlund)]

backout live in production: https://hg.mozilla.org/build/mozharness/rev/3347b848256c :)

not clearing needinfo. Will respond after trees reopen.

Comment 9

[Aki Sasaki (not active)]

Comment on attachment 8442311 [details] [diff] [review]
Implement a mock_enabled context manager and use it in place of explicit run_command_m calls

I'm sadly a bit of a luddite here, and don't actually grok what's going on in this patch.

I'm of the mind that if we directly call run_command_m() we must actually want to run something in mock, and we should be able to do so without setting anything up.  If we want to conditionally run mock, we can run run_command().  And yes, sometimes we always want to run something in mock in production and want to stop running it in mock locally, so calling run_command_m() directly should be frowned upon.

I think I grok _possibly_run_command_with_mock() more than this context stuff, but by the look of it that also hardcodes self.config['mock_target'], and I think part of your patch allowed for arbitrary config dicts to be passed around, though I don't recall why.

I think the lesson here is that in the long run, we want to kill mock_mozilla dead and use docker for linux containers, so we stop having to deal with it.  In the short term I'd like to keep the tree green.  It's the murky middle term that we're going to have to compromise around, and it sounds like :lightsofapollo, :jlund, and yourself all have opinions about it, and the greenness of the existing scripts is the constraining factor.

Comment 10

[Jordan Lund (:jlund)]

Comment on attachment 8442311 [details] [diff] [review]
Implement a mock_enabled context manager and use it in place of explicit run_command_m calls

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

Bear with me as I'm new to contextmanager and some python idioms.

Overall, wrapping the repetitive enable/disable calls into a generator function is concise and sophisticated. 10x better than my simple helper method :)

One concern I have here is this takes a certain knowledge of python (contextlib, decorators, and generators) that you need to get up to speed with. For e.g., you overrode mock_enabled in b2g_build.py so if we had to do that elsewhere, you'd have to know what's going on. Maybe that's not an issue.

See in-line for questions.

::: mozharness/mozilla/mock.py
@@ +70,5 @@
> +    @contextmanager
> +    def mock_enabled(self, config=None):
> +        self.enable_mock(config=config)
> +        yield self.active_mock_target
> +        self.disable_mock()

so what happens if say in the 'with' block, we throw exception?

IIUC, disable_mock() would never be hit.

Do we need:
    try:
        yield self.active_mock_target
    finally:
        self.disable_mock()

to ensure we always disable after the with block?

::: scripts/b2g_build.py
@@ +536,5 @@
> +    @contextmanager
> +    def mock_enabled(self, config=None):
> +        self.enable_mock(config=config)
> +        if self.active_mock_target:
> +            self.setup_mock(self.active_mock_target, config['mock_packages'], config.get('mock_files'))

Why does b2g_build need this logic? AFAIK enable_mock() only monkey patches run_command and get_output_from_command.

The _m equivalents it replaces it with actually check for active_mock_target and call setup_mock every time no?

@@ +969,5 @@
>              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')
>  
> +        with self.mock_enabled(config=gecko_config) as active_mock_target:

So the 'as' here captures the generators __enter__ return val (yield self.active_mock_target) right? I don't think the 'with' block uses it and it can reach it from 'self' anyway ya?

::: scripts/mobile_l10n.py
@@ +318,5 @@
>          make = self.query_exe("make")
> +        if self.run_command([make, "-f", "client.mk", "configure"],
> +                            cwd=dirs['abs_mozilla_dir'],
> +                            env=env,
> +                            error_list=MakefileErrorList):

Are we missing the with block here?

Comment 11

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

(In reply to Jordan Lund (:jlund) from comment #10)
> Bear with me as I'm new to contextmanager and some python idioms.

So am I. But I wanted some way to call enable_mock and make sure the corresponding disable_mock call happened even if there was an early return or something. When I went to look for how to do that in Python, I came up with either (1) this contextmanager goop, or (2) try...finally blocks. I initially was thinking I'd do the latter since it involves less magic, but it felt a little harder to follow, surprisingly enough. I may have gotten that backwards, though.

> Overall, wrapping the repetitive enable/disable calls into a generator
> function is concise and sophisticated. 10x better than my simple helper
> method :)

But definitely not as simple, which is a mark against.

> One concern I have here is this takes a certain knowledge of python
> (contextlib, decorators, and generators) that you need to get up to speed
> with. For e.g., you overrode mock_enabled in b2g_build.py so if we had to do
> that elsewhere, you'd have to know what's going on. Maybe that's not an
> issue.

I kind of understand what's going on, but mostly I was cargo-culting the first example at https://docs.python.org/2/library/contextlib.html

And it appears that I got it wrong, see below.

> ::: mozharness/mozilla/mock.py
> @@ +70,5 @@
> > +    @contextmanager
> > +    def mock_enabled(self, config=None):
> > +        self.enable_mock(config=config)
> > +        yield self.active_mock_target
> > +        self.disable_mock()
> 
> so what happens if say in the 'with' block, we throw exception?
> 
> IIUC, disable_mock() would never be hit.
> 
> Do we need:
>     try:
>         yield self.active_mock_target
>     finally:
>         self.disable_mock()
> 
> to ensure we always disable after the with block?

Yes, I think you're right. I should have followed |pydoc contextlib|. It's straightforward.

> ::: scripts/b2g_build.py
> @@ +536,5 @@
> > +    @contextmanager
> > +    def mock_enabled(self, config=None):
> > +        self.enable_mock(config=config)
> > +        if self.active_mock_target:
> > +            self.setup_mock(self.active_mock_target, config['mock_packages'], config.get('mock_files'))
> 
> Why does b2g_build need this logic? AFAIK enable_mock() only monkey patches
> run_command and get_output_from_command.
> 
> The _m equivalents it replaces it with actually check for active_mock_target
> and call setup_mock every time no?

run_command_m calls self.setup_mock(), which gets mock_packages and mock_files out of self.config. b2g_build.py needs to use gecko_config for those instead. Perhaps mock.py is not the right place to inject this degree of flexibility, but the dynamically-loaded gecko_config dict seems like it makes it easier to do here than elsewhere (eg by somehow getting the gecko config into self.config.)

I could add an optional config parameter to run_command and run_command_m that would get propagated through to setup_mock from run_command_m, but that seems clunkier. I could also just make sure that setup_mock has been called by something in b2g_build.py before it uses the mock environment, but that felt more error-prone since there are a multiple different actions that use the environment.

It's true that I'm overloading the "enable/enabled" to mean different things (enable_mock does not do setup, mock_enabled does). Perhaps this should be |with self.mock_activated| or something.

> @@ +969,5 @@
> >              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')
> >  
> > +        with self.mock_enabled(config=gecko_config) as active_mock_target:
> 
> So the 'as' here captures the generators __enter__ return val (yield
> self.active_mock_target) right? I don't think the 'with' block uses it and
> it can reach it from 'self' anyway ya?

Yes, the return value is somewhat arbitrary. I'm still on the fence as to whether it's better or worse to return something.

Oh! You're right. It's never used at all. Oh... right, my first version of the patch did not do the mock_enabled override in b2g_build.py, so it used it to decide whether to call setup_mock or not. Ok, so now I never use it. I'm still weakly inclined to yield it, but all of the | as active_mock_target| should die.

> ::: scripts/mobile_l10n.py
> @@ +318,5 @@
> >          make = self.query_exe("make")
> > +        if self.run_command([make, "-f", "client.mk", "configure"],
> > +                            cwd=dirs['abs_mozilla_dir'],
> > +                            env=env,
> > +                            error_list=MakefileErrorList):
> 
> Are we missing the with block here?

Ok, this is where it gets confusing. Enough so that I think I need to improve the contextmanager a little. Or maybe mock.py.

This works because _setup_configure is always invoked within the scope of |with self.mock_enabled()|. So it can just use self.run_command and not think about mock. But you're right that it's a little confusing if you're just reading that method and you're thinking it won't work because it doesn't enable mock anywhere. Personally, I kind of like having the mock stuff lifted up as high as it can so all the internal helper functions can just use self.run_command. But there's a big problem with this -- if you decided to be more explicit and put _setup_configure's body inside of a self.mock_enabled(), then you'll disable mock for the caller. self.mock_enabled doesn't nest, in other words.

I'm torn as to whether to add the nesting level to self.mock_enabled() or directly to self.disable_mock. It would probably be safer to do the former, because I bet there are self.enable_mock callers that forget to call self.disable_mock, but work because the next thing just calls self.enable_mock again. But adding nesting to self.disable_mock would break things because you'd get fewer disables than enables and would stay in mock longer than intended.

Ok, new patch forthcoming...

Comment 12

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

Attachment: Implement a mock_enabled context manager and use it in place of explicit run_command_m calls

Ok, I smushed together the mock patch I backed out from bug 898554, since it's broken without some sort of fix to the other self.enable_mock() callers. But this is getting a little complex for my taste. At the very least, I probably ought to figure out how to make b2g_build.py's mock_activated() call mock.py's instead of reimplementing the logic.

I have mixed feeling about the whole mess, now. I still want some sort of cleanup in b2g_build.py, but not at the cost of sanity.

Comment 13

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

Attachment: Implement a mock_enabled context manager and use it in place of explicit run_command_m calls

A few comments.

Comment 14

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

(In reply to Aki Sasaki [:aki] from comment #9)
> Comment on attachment 8442311 [details] [diff] [review]
> Implement a mock_enabled context manager and use it in place of explicit
> run_command_m calls
> 
> I'm sadly a bit of a luddite here, and don't actually grok what's going on
> in this patch.

Oops, sorry. I somehow missed this comment entirely. Darn it.

> I'm of the mind that if we directly call run_command_m() we must actually
> want to run something in mock, and we should be able to do so without
> setting anything up.  If we want to conditionally run mock, we can run
> run_command().  And yes, sometimes we always want to run something in mock
> in production and want to stop running it in mock locally, so calling
> run_command_m() directly should be frowned upon.
> 
> I think I grok _possibly_run_command_with_mock() more than this context
> stuff, but by the look of it that also hardcodes self.config['mock_target'],
> and I think part of your patch allowed for arbitrary config dicts to be
> passed around, though I don't recall why.

Because b2g_build.py gets all of its mock-related configuration from a gecko_config thing that it downloads at runtime. I don't know how critical that is; I've just been carrying it around as a requirement.

> I think the lesson here is that in the long run, we want to kill
> mock_mozilla dead and use docker for linux containers, so we stop having to
> deal with it.  In the short term I'd like to keep the tree green.  It's the
> murky middle term that we're going to have to compromise around, and it
> sounds like :lightsofapollo, :jlund, and yourself all have opinions about
> it, and the greenness of the existing scripts is the constraining factor.

Well, we're green now, and this has certainly gotten messy enough that I'm fine rebasing my other stuff in bug 898554 without any mock changes at all. I think this was really just a b2g_build.py cleanup anyway.

That's the main motivation for all of this, btw -- b2g_build.py's use of mock is fugly.

        if 'mock_target' in gecko_config:
            # initialize mock
            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
            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)
             for cmd in cmds:
                 retval = self.run_command(cmd, cwd=dirs['work_dir'], env=env, error_list=B2GMakefileErrorList)
                 if retval != 0:
                     break
             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)

Comment 15

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

Comment on attachment 8443695 [details] [diff] [review]
Implement a mock_enabled context manager and use it in place of explicit run_command_m calls

Sounds like context manager craziness is not making people happy, so maybe I'll take a shot at try...finally. Canceling review for now.

Comment 16

[Aki Sasaki (not active)]

(In reply to Steve Fink [:sfink] from comment #14)
> (In reply to Aki Sasaki [:aki] from comment #9)
> > I think I grok _possibly_run_command_with_mock() more than this context
> > stuff, but by the look of it that also hardcodes self.config['mock_target'],
> > and I think part of your patch allowed for arbitrary config dicts to be
> > passed around, though I don't recall why.
> 
> Because b2g_build.py gets all of its mock-related configuration from a
> gecko_config thing that it downloads at runtime. I don't know how critical
> that is; I've just been carrying it around as a requirement.

Aha.  That makes sense.

> Well, we're green now, and this has certainly gotten messy enough that I'm
> fine rebasing my other stuff in bug 898554 without any mock changes at all.
> I think this was really just a b2g_build.py cleanup anyway.
> 
> That's the main motivation for all of this, btw -- b2g_build.py's use of
> mock is fugly.

I agree, thanks for giving it a shot.
I think the real fix here is moving to taskcluster+docker so we no longer have to do any sort of system package stuff in b2g_build.py.  I'm not sure what the timeframe is there.  Optimistically this calendar year.

Comment 17

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

Fixed differently. Or worked around. Details not important.