979440 - we can't call get_output_from_command() from inside setup_mock()

Status: RESOLVED FIXED

(Release Engineering :: General, defect)

Description

[Jordan Lund (:jlund)]

on feb 19th, http://hg.mozilla.org/build/mozharness/rev/b3e2a3f200f5 introduces a recursive bug where we call get_output_from_command() inside setup_mock()

However, if we enable_mock(), we wrap get_output_from_command() with get_output_from_command_m() and get_output_from_command_m() calls setup_mock()!!

So the only way you can use enable_mock() is if you explicitly call setup_mock() before.

Which means we should either:
   1) remove setup_mock() inside of get_output_from_command_m (and run_command_m)
   2) remove get_output_from_command() from inside setup_mock()
   3) add setup_mock() at the beginning of enable_mock() (before we wrap the run_commands)

My vote is on number 3. I'll make a patch now.

desktop_l10n.py (massimo's dev repo) is hitting this and my guess is mobile_l10n (in production) is in danger of hitting it too. Spider, Servo, and my desktop builds will never hit this bug because we explicitly call setup_mock() before enable_mock()

Comment 1

[Jordan Lund (:jlund)]

Attachment: 979440_enable_mock_setup_mock.diff

catlee, mgerva is gone for the day but I think you wrote a lot of mock.py

Comment 2

[Chris AtLee [:catlee]]

Comment on attachment 8385485 [details] [diff] [review]
979440_enable_mock_setup_mock.diff

I think this is ok....make sure it gets lots of eyes on Cedar

Comment 3

[Jordan Lund (:jlund)]

Comment on attachment 8385485 [details] [diff] [review]
979440_enable_mock_setup_mock.diff

this has been pushed to default: http://hg.mozilla.org/build/mozharness/rev/86027af6a98f

I kicked off a push on cypress (our mozharness default twig): https://tbpl.mozilla.org/?showall=all&tree=Cypress&rev=e5b09585215f

I also asked massimo to try it on his dev instance.

results to be posted. :)

Comment 4

[Massimo Gervasini [:massimo]]

> I also asked massimo to try it on his dev instance.
It works on my instance too, thanks Jordan!

Comment 5

[Jordan Lund (:jlund)]

boo:

this has to be backed out: https://hg.mozilla.org/build/mozharness/graph

It turns out the spider build failed (and servo would too): https://tbpl.mozilla.org/php/getParsedLog.php?id=35645339&full=1&branch=cypress

This was due to different recursion issue where spider override's setup_mock and adds enable_mock() in that.

So by my patch adding setup_mock() to enable_mock().... more infinite loops are generated. :(

I think the solution should be:

1) enable_mock is only used internally in MockMixin (via _enable_mock)
2) setup_mock is only used internally in MockMixin (via _setup_mock)
3) we explicitly require setup-mock to be listed in list of actions before any mock calls and we remove every setup_mock() call inside of MockMixin (leaving just the definition).
4) we replace: http://mxr.mozilla.org/build/source/mozharness/mozharness/mozilla/mock.py#168
   with: super(MockMixin, self).get_output_from_command

I am swaying towards option 3. Makes things uniform and explicit. So rather than having (in mockmixin):

    def run_command_m(self, *args, **kwargs):
        """Executes self.run_mock_command if self.config['mock_target'] is set,
        otherwise executes self.run_command."""
        if 'mock_target' in self.config:
            self.setup_mock()
            return self.run_mock_command(self.config['mock_target'], *args, **kwargs)
        else:
            return super(MockMixin, self).run_command(*args, **kwargs)

we instead do:
    def run_command_m(self, *args, **kwargs):
        """Executes self.run_mock_command if self.config['mock_target'] is set,
        otherwise executes self.run_command."""
        if 'mock_target' in self.config and self.done_mock_setup
            return self.run_mock_command(self.config['mock_target'], *args, **kwargs)
        else:
            self.info("Can't call run_command against mock because 'mock_target' is "
                      "not in config or you havn't called setup-mock from list of actions"
                      ". Running Baseclass run_command() instead")
            return super(MockMixin, self).run_command(*args, **kwargs)

Although the issue with 3 is that we would be coupling one action (setup_mock) to other actions that call methods like the one above. Which is not very mozharness like.

Catlee, do you have any opinions?

Comment 6

[Jordan Lund (:jlund)]

maybe option 4 will work. It will let us override things like setup_mock, monkeypatch run_command via enable_mock, etc all without worrying about recursion (I *think*).

Comment 7

[Jordan Lund (:jlund)]

Attachment: 979440_enable_mock_setup_mock-140305.diff

round 2 ...

ash doesn't seem to do mozharness spider builds. I guess I could force one from a master. Either way, we can land this on default and retry a few tests before merging.

Comment 8

[Jordan Lund (:jlund)]

(In reply to Jordan Lund (:jlund) from comment #7)
> Created attachment 8386454 [details] [diff] [review]
> 979440_enable_mock_setup_mock-140305.diff
> 
> round 2 ...
> 
> ash doesn't seem to do mozharness spider builds. I guess I could force one
> from a master. Either way, we can land this on default and retry a few tests
> before merging.

massimo, can you remove the original patch and try this one? That will let us know if it fixes your issue while me triggering on ash/cypress will show if it disrupts what we currently have.

catlee stated in irc that he prefers options 4

Comment 9

[Jordan Lund (:jlund)]

(In reply to Jordan Lund (:jlund) from comment #7)
> Created attachment 8386454 [details] [diff] [review]
> 979440_enable_mock_setup_mock-140305.diff
> 

catlee, ping for quick r? :)

Comment 10

[Jordan Lund (:jlund)]

Comment on attachment 8386454 [details] [diff] [review]
979440_enable_mock_setup_mock-140305.diff

http://hg.mozilla.org/build/mozharness/rev/8557eb43f5a9

Comment 11

[Jordan Lund (:jlund)]

in production: https://hg.mozilla.org/build/mozharness/rev/c084d4f54ed6