1017448 - Add --disable-mock option to b2g_build.py

Status: RESOLVED FIXED

(Release Engineering :: General, defect)

Description

[James Lal [:lightsofapollo]]

Attachment: add --disable-mock

In theory mock could be used in with docker on taskcluster but it does not make much sense... Adding a simple flag to disable this does the trick (while adding onto the large pile of configuration here sorry!)

Comment 1

[Aki Sasaki (not active)]

Comment on attachment 8430590 [details] [diff] [review]
add --disable-mock

This patch works for me as is, but this is going to directly conflict with https://bug898554.bugzilla.mozilla.org/attachment.cgi?id=8429768 .

I think there's a way the two patches can coexist, but not as currently written.  Maybe in the other patch, instead of

    self.enable_mock(config=gecko_config)
    if self.mock_enabled:

we can

    if self._mock_enabled():
        self.enable_mock(config=gecko_config)
    if self.mock_enabled:

which is ugly.  Another way would be to write an _enable_mock() that checks _mock_enabled before calling self.enable_mock(), which is confusing, but will work.  Currently wishing that were in an inherited class rather than a mixin so we could override the method easily.

Can you two figure this out, or do you want me to mediate?

Comment 2

[James Lal [:lightsofapollo]]

Another ugly way to work around this would be to hack inside gecko_config remove mock_target when the --disable-mock flag is set =/

I am earnestly hopeful that we can entirely replace the need for mock with taskcluster's docker setup very very soon... I don't want to get in the way of any changes for short term needs but if the goal is to build something linux only I think we have a better path forward soon.

Comment 3

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

Hah! I have a later patch in my bug 898554 stack (not yet posted) that implements --disable-mock, too.

It turns out that I wanted to override enable_mock for another reason, so I ended up with:

    def enable_mock(self, config=None):
        if self.config.get('disable_mock'):
            return
        super(B2GHazardBuild, self).enable_mock(config=config)
        if self.mock_enabled:
            packages = set(config['mock_packages'])
            packages.update(self.config['mock_packages'])
            self.setup_mock(config['mock_target'], list(packages), config['mock_files'])

(I wanted to call setup_mock from within enable_mock.) But the basic idea of overriding enable_mock and bombing out if self.config['disable_mock'] is set seems reasonable to me. What do you think?

Comment 4

[James Lal [:lightsofapollo]]

This sounds good to me (I am not a reviewer or anything for mozharness though).

Comment 5

[Michael Shal [:mshal]]

Attachment: test1.patch

--disable-mock was added in bug 898554 for buildb2gbase, but it currently isn't used. I think this is all that is necessary to use it.

Comment 6

[Michael Shal [:mshal]]

Attachment: test2.patch

Alternatively, we could try something like this to just not use mock if mock_mozilla isn't executable. Thoughts?

Comment 7

[Jordan Lund (:jlund)]

(In reply to Michael Shal [:mshal] from comment #5)
> Created attachment 8471150 [details] [diff] [review]
> test1.patch
> 
> --disable-mock was added in bug 898554 for buildb2gbase, but it currently
> isn't used. I think this is all that is necessary to use it.

I did not get to this in my queue today. I'll review tomorrow.

Comment 8

[Jordan Lund (:jlund)]

(In reply to Michael Shal [:mshal] from comment #6)
> Created attachment 8471152 [details] [diff] [review]
> test2.patch
> 
> Alternatively, we could try something like this to just not use mock if
> mock_mozilla isn't executable. Thoughts?

ahh MockMixin...

I don't think this will work as, IIUC, we will continue to call setup_mock over and over since we have logic that says: we use self.default_mock_target or self.config('mock_target') so we will fallback on the config item if default_mock_target not set. Aside from this possibly causing loops, it will probably result in a lot of log noise.

I could be wrong though. /me looks at test1.patch. I noticed that sfink did use --disable-mock in his patch that was r+ from catlee: https://bugzilla.mozilla.org/show_bug.cgi?id=898554#c31 but that part seems to never have landed. his approach was to put it in enable_mock() definition body.

Comment 9

[Jordan Lund (:jlund)]

Comment on attachment 8471150 [details] [diff] [review]
test1.patch

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

From briefly looking, I *think* this will cause issues (see in-line comment for reason).

there are too many dependent contradicting parts in MockMixin for this to be reviewed easily. As in, sometimes we skip setup_mock/disable_mock methods and call run_command_m directly. Sometimes we use c['mock_target'], others we use a separate gecko_config (which could be remote). we sometimes override setup_mock by putting enable_mock in its body. We call setup_mock from run_command_m but also sometimes before we call run_command_m like inside this load_gecko_config.

TBH - it's a mess. Depending on how long we want to maintain mock, we should re-evaluate how we use MockMixin and either reduce its abilities or else not let its internals be used in so many permutations

tangent aside, I'll look at the scope of this bug's goal and see if we can do this cleanly. If not, I suspect a lot related parts will need to be touched

::: mozharness/mozilla/building/buildb2gbase.py
@@ +204,5 @@
>          gecko_config = self._load_gecko_config()
>  
>          # Set up mock immediately so any later run_command_m doesn't end up
>          # calling setup_mock with the wrong config.
> +        if not self.config.get('disable_mock'):

so I think this will cause the above comments to happen: we will still call setup_mock from run_command_m and worse, use the wrong config...

Comment 10

[Jordan Lund (:jlund)]

Comment on attachment 8471152 [details] [diff] [review]
test2.patch

feedback explained in https://bugzilla.mozilla.org/show_bug.cgi?id=1017448#c8

Comment 11

[Michael Shal [:mshal]]

I initially tried adding the 'disable_mock' config checks in the enable/disable_mock() functions, but unfortunately we would still call setup_mock() anyway, which would throw an exception because part of the setup involves running mock_mozilla. (We don't have mock_mozilla in the docker container for bug 1016845). So this led me to try just catching that exception and assuming we don't have mock enabled if mock_mozilla isn't available. With that patch, self.default_mock_target stays at the default 'None', so this if statement in enable_mock and disable_mock effectively serve as the "is mock enabled" check:

if not self.default_mock_target and 'mock_target' not in self.config:

Admittedly, I don't really know how the "'mock_target' not in self.config" pieces fits into the puzzle, but it wasn't set in my case. Perhaps we just need to ensure that mock_target isn't in self.config if --disable-mock is specified?

However, I figured there could probably be other cases where mock_mozilla fails to run, and we may not just want to default to turning off mock in those cases. So I tried skipping the setup_mock call entirely if --disable-mock is specified, which resulted in test1.patch. This still relies on the same if-statement as above for enable/disable mock.

Can you clarify on what you think will cause the log noise / loop issue? (Or what function we'll loop into?) I can try to look and see if this introduces that problem.

Comment 12

[Jordan Lund (:jlund)]

 
> there are too many dependent contradicting parts in MockMixin for this to be
> reviewed easily. As in, sometimes we skip setup_mock/disable_mock methods
> and call run_command_m directly. Sometimes we use c['mock_target'], others
> we use a separate gecko_config (which could be remote). we sometimes
> override setup_mock by putting enable_mock in its body. We call setup_mock
> from run_command_m but also sometimes before we call run_command_m like
> inside this load_gecko_config.

one 'fix' here would to not let MockMixin figure out if we want mock to be used or not. As in, if you call setup_mock(), enable_mock(), or disable_mock() from a script, MockMixin will assume you are doing so correctly. IOW - wrap these method calls in a 'if we_should_use_mock:' block from the script that uses them.

This would have the side effect of allowing individual scripts to deal with when and with what config it uses mock with and also let us easily disable_mock like we want to do in this bug.

downside is this a substantial change.

sfink: thoughts?

Comment 13

[Jordan Lund (:jlund)]

> Can you clarify on what you think will cause the log noise / loop issue? (Or
> what function we'll loop into?) I can try to look and see if this introduces
> that problem.

we discussed over vidyo. I explained how hazaard_build.py overwrites setup_mock() and calls load_gecko_config(). By applying the first patch and using c['disable_mock'] in load_gecko_config, we would use the default setup_mock().

The other solution of just bailing out if there is an exception may not actually call loops of setup_mock. At least not in the b2g + taskcluster case. Although, we agreed it wasn't clean allowing setup_mock to be called and not setting default_mock_target if there is any sort of exception.

the solution is mshal is going to add a mockmixin method that will evaluate if we should use mock and apply that method at various parts (eg setup_mock, enable_mock, disable_mock, and run_command_m). The body of it will use c['mock_target'], self.default_mock_target and c['disable_mock'] to decide whether it returns true or false.

This is still not perfect but at least it will focus the question of whether we want to use mock or not into one point.

Comment 14

[Michael Shal [:mshal]]

Attachment: disable-mock.patch

Per what we discussed in vidyo:

 - pull out the logic to get mock_target, and add a check for 'disable_mock' there
 - return immediately from setup_mock if 'disable_mock' is set

This doesn't address the overall structural concerns, but it does simplify the logic a bit and prevents setup_mock from retrying multiple times if it's disabled with a 'mock_target' config option present.

Comment 15

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

Comment on attachment 8471927 [details] [diff] [review]
disable-mock.patch

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

I have mentally blotted out all the permutations I went through on this, but this patch seems like a good direction to me. And I am heartened by the idea of all this going away by switching away from mock. (I still think mock is pretty cool, but I think we've pushed it too far at this point. The poor thing just wants to build RPMs!)

::: mozharness/mozilla/mock.py
@@ +232,5 @@
>  
>      def run_command_m(self, *args, **kwargs):
>          """Executes self.run_mock_command if self.config['mock_target'] is set,
>          otherwise executes self.run_command."""
> +        mock_target = self.get_mock_target()

The comment should be updated, probably to be more vague. "Executes self.run_mock_command if we have a mock target set, otherwise..." maybe?

@@ +243,5 @@
>      def get_output_from_command_m(self, *args, **kwargs):
>          """Executes self.get_mock_output_from_command if
>          self.config['mock_target'] is set, otherwise executes
>          self.get_output_from_command."""
> +        mock_target = self.get_mock_target()

Comment here too.

Comment 16

[Jordan Lund (:jlund)]

Comment on attachment 8471927 [details] [diff] [review]
disable-mock.patch

agreed, I think this should work. r+ with docstring update

thx for doing it this way mshal! I know it was a bit more work for you but this improves its state by reducing moving parts.

As you mentioned, this is still not perfect; we are merely silently doing nothing when we call setup_mock, enable_mock, etc. But rather than continue to work on this, my vote is to put our efforts on a replacement for MockMixin altogether :)

Comment 17

[Jordan Lund (:jlund)]

(In reply to Jordan Lund (:jlund) from comment #16)
> Comment on attachment 8471927 [details] [diff] [review]
> disable-mock.patch
> 
> agreed, I think this should work. r+ with docstring update
> 

this patch will touch a lot of jobs. probably goes without saying but we should test some jobs on cypress while it sits on default: https://wiki.mozilla.org/ReleaseEngineering/SpecialBranches#Cypress

Comment 18

[Michael Shal [:mshal]]

Attachment: disable-mock.patch

Updated comments, r+ carried forward.

Comment 19

[Michael Shal [:mshal]]

Comment on attachment 8472607 [details] [diff] [review]
disable-mock.patch

https://hg.mozilla.org/build/mozharness/rev/fd3a165040d3