1131876 - Create a separate HazardAnalysis object and use it for both browser and b2g analyses

Status: RESOLVED FIXED

(Release Engineering :: General, defect)

Description

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

spidermonkey_build.py and hazard_build.py duplicate all of the analysis code.

Comment 1

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

Attachment: Create a separate HazardAnalysis object and use it for both browser and b2g analyses

Create a common object that knows how to perform analyses, and use it from the mozharness scripts that want to perform analyses (spidermonkey_build.py is for browser and shell-only analyses; hazard_build.py is for B2G analyses. I will probably rename them in a different patch.)

My apologies for the style of this patch, since this both pulls the logic out of the two files, and then modifies it slightly. Mostly, that's to get it to work, but I'll admit there are other things mixed in that may have been possible to do separately.

I *am* pushing this stuff to try (using my own mozharness repo), and it looks like it's finally coming up all green. So I guess it's up to you how closely you want to examine this patch. My use of a custom exception class may be a little iffy.

Comment 2

[Jordan Lund (:jlund)]

Comment on attachment 8562458 [details] [diff] [review]
Create a separate HazardAnalysis object and use it for both browser and b2g analyses

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

this is neat. TBH, I did not scrutinize line by line as it is mostly about ripping out duplicate code into a base class. I see subtle differences here and there but really what we are looking for is 1) more efficient, manageable code (which this obviously is) and 2) production behavior doesn't change (which I am trusting in your try pushes) :D

A higher level thought is  how you use the pattern of calling an attr's method of self and passing it ... self. It certainly makes things very clean but I wonder how efficient it is to pass all of self every time. I suppose it doesn't really matter since this won't be what slows down build times :) but OOC, why do you prefer that pattern instead of instantiating HazardAnalysis with say some needed things: a log_obj (so we can self.info, warning, etc) and some restrictive amount of a config. Then, whatever else we need we pass it explicitly as arguments in HazardAnalysis (something like https://bugzil.la/1063532)? I suppose that would result in more code. I'm not sure, maybe this is fine.

funny note, HazardAnalysis could basically be a bunch of @classmethods as it doesn't even need its own 'self' at all :)

::: mozharness/mozilla/building/hazards.py
@@ +5,5 @@
> +from mozharness.base.errors import MakefileErrorList
> +from mozharness.mozilla.buildbot import TBPL_WARNING
> +
> +
> +class HazardError(Exception):

I am not sure what the HazardError class really saves us over just interpreting the return code in the hazard/spider classes. however, maybe we will find a need to extend its behaviour later on :)

@@ +12,5 @@
> +
> +    def __str__(self):
> +        return repr(self.value)
> +
> +    # Logging ends up calling splitlines directly on what is being logged, which would fail.

huh, good catch

Comment 3

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

(In reply to Jordan Lund (:jlund) from comment #2)
> Comment on attachment 8562458 [details] [diff] [review]
>
> A higher level thought is  how you use the pattern of calling an attr's
> method of self and passing it ... self. It certainly makes things very clean
> but I wonder how efficient it is to pass all of self every time. I suppose
> it doesn't really matter since this won't be what slows down build times :)

"all of self"? It's a reference, so it's just passing a pointer. I doubt it makes any noticeable difference to performance.

It does look weird, though. I thought about passing it into the constructor instead so you wouldn't need to pass it to the methods. But I found it harder to conceptualize what this HazardAnalysis object was, in that case. It would be something that the main builder would call to do the analysis steps, but then it magically knows how to call back into the builder for actually executing things. Odd. And if you had a builder that managed to call methods on someone else's HazardAnalysis object, things would get completely messed up.

> but OOC, why do you prefer that pattern instead of instantiating
> HazardAnalysis with say some needed things: a log_obj (so we can self.info,
> warning, etc) and some restrictive amount of a config. Then, whatever else
> we need we pass it explicitly as arguments in HazardAnalysis (something like
> https://bugzil.la/1063532)? I suppose that would result in more code. I'm
> not sure, maybe this is fine.

In general, I would prefer something like that too. And in fact, that's how I started out. But I kept finding more and more things that I would have to pass around. All of the query_* methods, for example. But I think where it really bit me was with run_command() in combination with the mock craziness. I decided I needed some object that knew how to properly run things so it could worry about whether we were inside or outside of mock. (Some steps are one, some the other, and some are a mixture.) And do filesystem manipulation, since with mock some paths might be different. And of course then I needed the appropriate directories (hence build.query_abs_dirs). And environment settings.

Because of all that, I ended up having the whole builder available, so I just used it for everything else.

Logging seems like something that could safely be handed off to another object -- but see below.

> funny note, HazardAnalysis could basically be a bunch of @classmethods as it
> doesn't even need its own 'self' at all :)

You're right, it totally could. Hmmmmm... with the code as it stands now, I think that's probably the right thing to do. But I'm still unsure as to whether I'm doing this the right way. For example, maybe even if I don't want to construct a HazardAnalysis with the whole builder, I could at least construct it with the output of query_abs_dirs(). Or perhaps just the subset that it needs -- I'd probably go for that, since it really only needs a half dozen of them, and they're all meaningful to a HazardAnalysis. (Where's the source, where do the analysis scripts live, ...) And some of those make more sense to pass in as parameters (eg the upload directory -- that doesn't feel like it belongs to the hazard analysis.)

> ::: mozharness/mozilla/building/hazards.py
> @@ +5,5 @@
> > +from mozharness.base.errors import MakefileErrorList
> > +from mozharness.mozilla.buildbot import TBPL_WARNING
> > +
> > +
> > +class HazardError(Exception):
> 
> I am not sure what the HazardError class really saves us over just
> interpreting the return code in the hazard/spider classes. however, maybe we
> will find a need to extend its behaviour later on :)

I'm not happy about having this. Please talk me into getting rid of it.

It's all because of self.fatal(exit_code=...) and self.return_code. Whenever either of those is set, then you are not just logging; you are communicating with the buildbot environment. I wanted HazardAnalysis to be an object that knows how to perform an analysis, without having any awareness of what it was being used for (eg as a buildbot builder). It turns out that it needs a lot of help from another object that knows how to run commands (because eg you might be inside or outside of mock) and knows where stuff should go, but still, it doesn't need to know that a failure to run the analysis is a fatal error, a failure to upload the results is just a warning, and a failure to match the expected number of hazards is a failure for real.

So my thought was that the analysis's job is just to report that the action you requested failed, and it's up to the (buildbot-aware) calling code to decide what to do about it. In Python it seems like you report failures via exceptions rather than return values, so I made an exception.

I don't like it much. Perhaps I could at least log the problem and then throw the exception.

I may just land as-is and change things around a bit with followups.

Comment 4

[Jordan Lund (:jlund)]

> "all of self"? It's a reference, so it's just passing a pointer. I doubt it
> makes any noticeable difference to performance.

hm, not sure how I thought we were passing by value. At any rate, ignore me.


> > but OOC, why do you prefer that pattern instead of instantiating
> > HazardAnalysis with say some needed things: a log_obj (so we can self.info,
> > warning, etc) and some restrictive amount of a config. Then, whatever else
> > we need we pass it explicitly as arguments in HazardAnalysis (something like
> > https://bugzil.la/1063532)? I suppose that would result in more code. I'm
> > not sure, maybe this is fine.
> 
> In general, I would prefer something like that too. And in fact, that's how
> I started out. But I kept finding more and more things that I would have to
> pass around. All of the query_* methods, for example. But I think where it
> really bit me was with run_command() in combination with the mock craziness.
> I decided I needed some object that knew how to properly run things so it
> could worry about whether we were inside or outside of mock. (Some steps are
> one, some the other, and some are a mixture.) And do filesystem
> manipulation, since with mock some paths might be different. And of course
> then I needed the appropriate directories (hence build.query_abs_dirs). And
> environment settings.

We could have HazardAnalysis inherit from similar needed classes: BaseScript, MockMixin, etc.

if you give HazardAnalysis a 'config' attr that is a dict subset of B2GHazardBuild and SpiderHazardBuild's self.config, things like BaseScript's methods should behave pretty well since this is configuration driven and most items live inside self.config. You may have tried this or you may end up with more blockers if you go this route so feel free to land as is now.

> > funny note, HazardAnalysis could basically be a bunch of @classmethods as it
> > doesn't even need its own 'self' at all :)
> 
> You're right, it totally could. Hmmmmm... with the code as it stands now, I
> think that's probably the right thing to do. 

since this is a dynamic language, I think it's up to dev how explicit we want to be about these things. abc module and things like @classmethod seem to be more like documentation to me rather than a rule enforcer. I'm fine reading code with or without it fwiw.

> 
> > ::: mozharness/mozilla/building/hazards.py
> > @@ +5,5 @@
> > > +from mozharness.base.errors import MakefileErrorList
> > > +from mozharness.mozilla.buildbot import TBPL_WARNING
> > > +
> > > +
> > > +class HazardError(Exception):
> > 
> > I am not sure what the HazardError class really saves us over just
> > interpreting the return code in the hazard/spider classes. however, maybe we
> > will find a need to extend its behaviour later on :)
> 
> I'm not happy about having this. Please talk me into getting rid of it.
> 

:) I can't talk you out of it. It sounds like warts from earlier designs around syntactic sugar and self.fatal. I don't mind this being here.

> I may just land as-is and change things around a bit with followups.

yup sounds good

Comment 5

[Chris Cooper [:coop] (he/him)]

In production: https://hg.mozilla.org/build/mozharness/rev/4ec8ad841383