487496 - custom BuildStep's need to explicitly name arguments and call self.addFactoryArguments

Status: RESOLVED FIXED

(Release Engineering :: General, defect, P2)

Description

[[e-mail] (:bhearsum)]

Particularly, 0.7.10p1 includes http://github.com/djmitche/buildbot/commit/071564761f46ea7089bbd6301589eeb466c3ec31, which removes **kwargs from RemoteShellCommand. This has caused all of our ShellCommand subclasses which do not explicitly name arguments to throw exceptions such as:
<type 'exceptions.TypeError'>: __init__() got an unexpected keyword argument 'clobber'

We've got some of these to fix in at least unittest/steps.py and steps/test.py. Maybe elsewhere, too.

Comment 1

[Chris AtLee [:catlee]]

We should also make sure we're setting the factory arguments properly

Comment 2

[[e-mail] (:bhearsum)]

(In reply to comment #1)
> We should also make sure we're setting the factory arguments properly

Good point. I'll fix the summary to include that

Comment 3

[[e-mail] (:bhearsum)]

Attachment: [WIP] fix parameter passing in buildbotcustom BuildStep subclasses

I've tested this pretty extensively on staging-master and staging-try-master and this should get them working with 0.7.10p1. We might need to test patch perfmaster.py, but I'll be testing that next week. I haven't explicitly tested any release things, but there's only one BuildStep that's only used there AFAIK. Given that the rest of these changes fixed up leak tests/unittests/etc. I'm pretty confident in us having no issues there. I haven't explicitly run an en-US nightly either, but that doesn't use any different steps, just a different code path in the BuildFactory.

Here's a summary of what I've actually changed:
* Stop passing the non-existent argument 'clobber' to CreateProfile.
* Move all addFactoryArguments statements to directly below superclass for consistency
* Use addFactoryArguments in all BuildStep subclasses
* Explicitly name all arguments in the constructors, don't let them end up in **kwargs anymore
* Remove unnecessary explicit passing of 'repourl' in some places
* Fix indentation in unittest/steps.py (bad \t's!)

No rush on this patch - I won't be ready to land it until at least mid next week.

Comment 4

[Chris AtLee [:catlee]]

Comment on attachment 371947 [details] [diff] [review]
[WIP] fix parameter passing in buildbotcustom BuildStep subclasses

There are a few instances of kwargs['command'] left in here that I'd love to see go away.

Comment 5

[[e-mail] (:bhearsum)]

Attachment: get rid of some more kwargs

Basically the same as before, but I got rid of the kwargs usage in test.py, tryserver/steps.py, and unittest/steps.py. I didn't touch any of the clobber file stuff in unittest/steps.py - it's only used by the 1.9 master and didn't feel it was worth the effort.

Comment 6

[[e-mail] (:bhearsum)]

Attachment: explicit parameters and removal of kwargs manipulation for talos

This is the same thing as the other patch, but for Talos. I've tested the talos-staging/ code and other than me missing a class on the first pass, I've had no issues with it. It's currently running on http://qm-buildbot01.mozilla.org:2008/waterfall, if you'd like to see for yourself.

I don't think we have a try talos staging area...so I'm not sure I can test those. After all of my testing of the first patch, plus how smoothly testing of this one went, I'm pretty confident in it, though.

Comment 7

[Chris AtLee [:catlee]]

Comment on attachment 372642 [details] [diff] [review]
explicit parameters and removal of kwargs manipulation for talos

>+    def __init__(self, branch="HEAD", command=["wget"], **kwargs):

Don't use lists as the default value for function arguments.  This will probably come back to bite
us later.

Comment 8

[[e-mail] (:bhearsum)]

Attachment: talos patch - same as before w/out lists as defaults parameters

The metadiff for this is big....but there's not much going on.

Comment 9

[[e-mail] (:bhearsum)]

Comment on attachment 372613 [details] [diff] [review]
get rid of some more kwargs

changeset:   251:153cc725f37c

Comment 10

[[e-mail] (:bhearsum)]

(In reply to comment #9)
> (From update of attachment 372613 [details] [diff] [review])
> changeset:   251:153cc725f37c

I reconfig'ed production-master and try-master for this.

Comment 11

[Serge Gautherie (:sgautherie)]

Comment on attachment 372613 [details] [diff] [review]
get rid of some more kwargs


>diff -r 6a71c76fd77a unittest/steps.py

(I checked this file only...)

>@@ -237,8 +235,8 @@
>-	self.super_class = ShellCommandReportTimeout
>-	ShellCommandReportTimeout.__init__(self, **kwargs)
>+        self.super_class = ShellCommandReportTimeout
>+        ShellCommandReportTimeout.__init__(self, **kwargs)

Doesn't MozillaCheck.__init__() miss |self.addFactoryArguments(test_name=test_name)|?

>@@ -284,12 +282,14 @@
>     def __init__(self, test_name, **kwargs):
>+        ShellCommandReportTimeout.__init__(self, **kwargs)
>+        self.addFactoryArguments(test_name=test_name)
>+
>         self.name = test_name
>         self.command = ["make", test_name]
>         self.description = [test_name + " test"]
>         self.descriptionDone = [self.description[0] + " complete"]
>-	self.super_class = ShellCommandReportTimeout
>-	ShellCommandReportTimeout.__init__(self, **kwargs)
>+        self.super_class = ShellCommandReportTimeout

Why did you move ShellCommandReportTimeout.__init__() up (for Reftest and Mochitest only)?
I'm not sure whether it should be at the start or the end,
but it feels odd not to be the same everywhere.

>@@ -348,18 +348,17 @@
> class MozillaMochitest(ShellCommandReportTimeout):
>     warnOnFailure = True
> 
>-    def __init__(self, test_name, leakThreshold=None, **kwargs):
>+    def __init__(self, test_name, leakThreshold=None, env={}, **kwargs):
>+        ShellCommandReportTimeout.__init__(self, env=env, **kwargs)
>+        self.super_class = ShellCommandReportTimeout
>+        self.addFactoryArguments(test_name=test_name,
>+                                 leakThreshold=leakThreshold)

Shouldn't addFactoryArguments() have |env=env| too?

Comment 12

[Serge Gautherie (:sgautherie)]

Comment on attachment 372613 [details] [diff] [review]
get rid of some more kwargs


>diff -r 6a71c76fd77a unittest/steps.py

>@@ -115,22 +117,18 @@
> class MozillaClientMkPull(ShellCommandReportTimeout):
>+        env = env.copy()
>+        env["MOZ_CO_PROJECT"] = self.project
>+        ShellCommandReportTimeout.__init__(self, command=command, env=env,
>+                                           **kwargs)
>+        self.addFactoryArguments(project=project, workdir=workdir, env=env,
>+                                 command=command)

>@@ -348,18 +348,17 @@
> class MozillaMochitest(ShellCommandReportTimeout):
>+        ShellCommandReportTimeout.__init__(self, env=env, **kwargs)

Isn't is broken to call before setting env["EXTRA_TEST_ARGS"]?

>+            self.env["EXTRA_TEST_ARGS"] = "--leak-threshold=%d" % leakThreshold

I'm not sure why you use |env| for MozillaClientMkPull and |self.env| for MozillaMochitest?
I'm not sure which one is "better", but it feels odd to be different...

Comment 13

[[e-mail] (:bhearsum)]

(In reply to comment #11)
> (From update of attachment 372613 [details] [diff] [review])
> Doesn't MozillaCheck.__init__() miss
> |self.addFactoryArguments(test_name=test_name)|?
> 

Yeah, looks like that patch landed after I went through this file.

> Why did you move ShellCommandReportTimeout.__init__() up (for Reftest and
> Mochitest only)?
> I'm not sure whether it should be at the start or the end,
> but it feels odd not to be the same everywhere.
> 

I've moved it to the start wherever possible. In functions where the __init__ method does things that require the parent class __init__ not to have run yet I've moved or left it at the bottom. In both cases I've kept the parent class __init__ call and addFactoryArguments together.

> >@@ -348,18 +348,17 @@
> > class MozillaMochitest(ShellCommandReportTimeout):
> Shouldn't addFactoryArguments() have |env=env| too?

Yes, you're absolutely right.

> Isn't is broken to call before setting env["EXTRA_TEST_ARGS"]?
> 

No, in this scenario self.env is guaranteed to exist. However, you just made me realize that the same bug I spoke of in the reftest leak threshold bug exists here.

Comment 14

[[e-mail] (:bhearsum)]

(thank you for unassigning me, bugzilla)

Comment 15

[[e-mail] (:bhearsum)]

Attachment: followup to addFactoryArguments in MozillaCheck, fix MozillaMochitest bug

This fixes the missing test_name parameter in MozillaCheck, and fixes MozillaMochitest so that the passed in environment is kept. The only reason didn't break (AFAICT) is because I forgot to pass env in addFactoryArguments, so when the Build object was created it ended up getting the env from buildbotcustom anyways...

Comment 16

[Serge Gautherie (:sgautherie)]

Comment on attachment 373694 [details] [diff] [review]
followup to addFactoryArguments in MozillaCheck, fix MozillaMochitest bug


>+    def __init__(self, test_name, leakThreshold=None, **kwargs):

If you do want to s/None/0/, now is the time...

Comment 17

[Serge Gautherie (:sgautherie)]

(In reply to comment #13)

> > Why did you move ShellCommandReportTimeout.__init__() up (for Reftest and
> > Mochitest only)?

> I've moved it to the start wherever possible.

Well, only Reftest and Mochitest have it at the start (now).

> In functions where the __init__
> method does things that require the parent class __init__ not to have run yet
> I've moved or left it at the bottom.

Well, I understand what you write, but I don't understand what these two classes have different than the others (wrt what their __init__() do), MozillaCheck for example.

> In both cases I've kept the parent class
> __init__ call and addFactoryArguments together.

(No problem with that.)

> > Isn't is broken to call before setting env["EXTRA_TEST_ARGS"]?
> 
> No, in this scenario self.env is guaranteed to exist. However, you just made me
> realize that the same bug I spoke of in the reftest leak threshold bug exists
> here.

Maybe I'm just confused by the other issues: let's fix them first...

Comment 18

[Serge Gautherie (:sgautherie)]

Comment on attachment 372613 [details] [diff] [review]
get rid of some more kwargs


>diff -r 6a71c76fd77a steps/release.py

>@@ -25,6 +25,9 @@
>+        self.addFactoryArguments(currentProduct=currentProduct,
>+                                 previousProduct=previousProduct)

Please double check, as not all the code is visible in the patch.


>diff -r 6a71c76fd77a tryserver/steps.py

>@@ -184,7 +183,8 @@
>+    def __init__(self, isOptional=False, patchDir=".", workdir="build",
>+                 **kwargs):
>@@ -196,13 +196,12 @@
>+        self.addFactoryArguments(isOptional=isOptional, patchDir=patchDir)

Misses workdir?

>@@ -278,10 +278,7 @@

>     def __init__(self, baseURL="http://hg.mozilla.org/",
>+                 defaultBranch='mozilla-central', timeout=3600, **kwargs):

Please double check, as not all the code is visible in the patch.

>@@ -303,13 +300,13 @@
>+    def __init__(self, cvsroot, workdir=".", **kwargs):
>+        self.addFactoryArguments(cvsroot=cvsroot)

Misses workdir?

Comment 19

[Serge Gautherie (:sgautherie)]

Comment on attachment 373694 [details] [diff] [review]
followup to addFactoryArguments in MozillaCheck, fix MozillaMochitest bug

See my 2 remaining "nits" in bug 469518 comment 45.

Comment 20

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

Comment on attachment 373694 [details] [diff] [review]
followup to addFactoryArguments in MozillaCheck, fix MozillaMochitest bug

(In reply to comment #16)
> If you do want to s/None/0/, now is the time...

Let's not mingle our fixes unduly, just in case. I don't want to untangle back-outs for unrelated issues.

Comment 21

[[e-mail] (:bhearsum)]

(In reply to comment #18)
> >@@ -184,7 +183,8 @@
> >+    def __init__(self, isOptional=False, patchDir=".", workdir="build",
> >+                 **kwargs):
> >@@ -196,13 +196,12 @@
> >+        self.addFactoryArguments(isOptional=isOptional, patchDir=patchDir)
> 
> Misses workdir?
> 

OK, I just did some good testing of this. Because workdir is a ShellCommand attribute, it will end up in an addFactoryArguments call when we reach ShellCommand.__init__. Same thing goes for 'env' in my latest patch.

Given that, I'm pretty certain my latest patch is fine as is.

Comment 22

[[e-mail] (:bhearsum)]

Comment on attachment 373694 [details] [diff] [review]
followup to addFactoryArguments in MozillaCheck, fix MozillaMochitest bug

changeset:   257:9301fd18edfd

Comment 23

[[e-mail] (:bhearsum)]

Comment on attachment 373694 [details] [diff] [review]
followup to addFactoryArguments in MozillaCheck, fix MozillaMochitest bug

I reconfig'ed production-master for this.

Comment 24

[[e-mail] (:bhearsum)]

Attachment: fix MozillaMochitest kwarg digging

In bug 469518 catlee pointed out that we don't need to do kwarg digging like my previous patch does. Here's a patch to avoid doing that, and make MozillaMochitest consistent with MozillaReftest once again.

Sorry for all the patch noise due to my mistaken assumptions.

Comment 25

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

Comment on attachment 374127 [details] [diff] [review]
fix MozillaMochitest kwarg digging

Yay for simplification.

Comment 26

[[e-mail] (:bhearsum)]

Comment on attachment 372665 [details] [diff] [review]
talos patch - same as before w/out lists as defaults parameters

changeset:   1121:44cb4fec2f2b

Comment 27

[[e-mail] (:bhearsum)]

Attachment: follow-up to make Talos class commands overridable again

Comment 28

[[e-mail] (:bhearsum)]

Attachment: for real.

Comment 29

[[e-mail] (:bhearsum)]

Comment on attachment 375040 [details] [diff] [review]
for real.

changeset:   1122:3a50347f0bbd

Comment 30

[[e-mail] (:bhearsum)]

Comment on attachment 374127 [details] [diff] [review]
fix MozillaMochitest kwarg digging

changeset:   266:dfc46d9683ab

Comment 31

[[e-mail] (:bhearsum)]

production-master master has been reconfig'ed for the MozillaMochitest patch. Looks like we're all done here.