496509 - Step name cleanups for build database

Status: RESOLVED FIXED

(Release Engineering :: General, defect, P2)

Description

[Chris AtLee [:catlee]]

If we want to do meaningful analysis of how individual steps are behaving via the build database, it would be easier if they had meaningful names.

e.g. instead of 'shell_8' for the compile step, it should be called 'compile'.

Comment 1

[Lukas Blakk [:lsblakk] use ?needinfo]

Attachment: Adds name to 211 shell commands in factory.py

Tried to set a standard for name where there are no spaces - so some of the existing names got changed to match.

Comment 2

[Chris AtLee [:catlee]]

Comment on attachment 381823 [details] [diff] [review]
Adds name to 211 shell commands in factory.py

Looks pretty good!  Thanks for tackling this.

This will probably take a few iterations to get right, but I'm ok with these names at this point, except for inconsistencies between steps named 'clone', and 'hg_clone'.

Comment 3

[Lukas Blakk [:lsblakk] use ?needinfo]

Attachment: Adding names to shell commands, with hg_clone consistent

All instances of hg clone are now named hg_clone.

Comment 4

[Nick Thomas [:nthomas] (UTC+12)]

Comment on attachment 381841 [details] [diff] [review]
Adding names to shell commands, with hg_clone consistent

First off, I think that it's awesome that we're doing this, and that this patch appeared so quickly. Here are some drive-by comments for further improvements.

>diff --git a/process/factory.py b/process/factory.py
>         self.addStep(ShellCommand,
>+         name='copy_bootsrap',

Typo in bootstrap.

>@@ -497,12 +526,14 @@
>                     (self.stageServer, self.productName, self.logUploadDir)]
>         )
>         self.addStep(ShellCommand,
>+         name='mv_malloc_log',
>          env=self.env,
>          command=['mv',
>                   '%s/_leaktest/malloc.log' % self.mozillaObjdir,
>                   '../malloc.log'],
>         )
>         self.addStep(ShellCommand,
>+         name='mv_sdleak',
>          env=self.env,
>          command=['mv',
>                   '%s/_leaktest/sdleak.log' % self.mozillaObjdir,

Use 'sdleak_log' instead of 'sdleak', so that bloat, malloc, and sdleak are consistent. eg get_sdleak_log, mv_sdleak_log, upload_sdleak_log.

Would be great if buildbotcustom.test.steps.AliveTest was augmented so that name could be customized, then we can tell the 5 calls in a leak build apart.

>@@ -604,6 +644,7 @@
>          )
>         if self.createSnippet:
>          self.addStep(ShellCommand,
>+             name='mk_update_pkg',
>              command=['make', '-C',
>                       '%s/tools/update-packaging' % self.mozillaObjdir],
>              env=self.env,
>@@ -625,10 +666,12 @@
> 
>     def addCodesighsSteps(self):
>         self.addStep(ShellCommand,
>+         name='make_codesighs',
>          command=['make'],
>          workdir='build/%s/tools/codesighs' % self.mozillaObjdir
>         )

Consistent use of make or mk  (I'd vote for the former).

>@@ -727,10 +777,12 @@
> 
>         if self.buildRevision:
>             self.addStep(ShellCommand,
>+             name='hg_get_revision',
>              command=['hg', 'up', '-C', '-r', self.buildRevision],
>              haltOnFailure=True

This is an hg update rather than ident.

>@@ -1013,6 +1074,7 @@
>         )
>         for dir in ('nsprpub', 'config'):
>             self.addStep(ShellCommand,
>+             name='make',

You could do |name='make_%s' % dir,| here, and check for other addSteps's in for loops.

>     def tinderboxPrint(self, propName, propValue):
>         self.addStep(ShellCommand,
>+                     name='tinderboxprint_properties',
>                      command=['echo',
>                               'TinderboxPrint:',
>                               '%s:' % propName,

And something similar here, supposing we want more unique names for each step.

>         self.addStep(ShellCommand,
>+         name='compare_locale',
>          command=['python',
>                   '../../../compare-locales/scripts/compare-locales',
>                   '-m', 'merged',

You could maybe argue that 'merge_locale' would be better here, as that's the main functionality.

>@@ -1168,6 +1239,7 @@
> 
>     def updateSources(self):
>         self.addStep(ShellCommand,
>+         name='update_workdir',
>          command=['hg', 'up', '-C', '-r', 'default'],
>          description='update workdir',
>          workdir=WithProperties('build/' + self.l10nRepoPath + '/%(locale)s'),

Updating the locale source really.

>         self.addStep(ShellCommand,
>+                     name='update_revision',

update_enUS_revision ?

Ran out of puff at this point.

Comment 5

[Lukas Blakk [:lsblakk] use ?needinfo]

Attachment: Adding names to shell commands, with fixes

This version takes into account nthomas' points - consistent use of make, using variables in for loops for clearer output and spelling fixes.

Comment 6

[Lukas Blakk [:lsblakk] use ?needinfo]

Attachment: Adding names to shell commands, with fixes

This one cleans up a missing ,

Currently testing this on staging

Comment 7

[Chris AtLee [:catlee]]

Comment on attachment 382797 [details] [diff] [review]
Adding names to shell commands, with fixes

changeset:   335:7b91b763cb4e

Comment 8

[Lukas Blakk [:lsblakk] use ?needinfo]

First (big) patch is up.  A second one is on its way to pick up the few steps that were missed.

Comment 9

[Lukas Blakk [:lsblakk] use ?needinfo]

Attachment: Follow up step names not caught in last patch

Tested on staging, works.

Comment 10

[Chris AtLee [:catlee]]

Comment on attachment 385362 [details] [diff] [review]
Follow up step names not caught in last patch

>         self.addStep(CompareBloatLogs,
>+         name='compare_bloatLog',
>          bloatLog='bloat.log',
>          env=self.env,
>          workdir='.',
>@@ -540,6 +544,7 @@
>                   '../sdleak.log'],
>         )
>         self.addStep(CompareLeakLogs,
>+         name='compare_leakLogs',
>          mallocLog='../malloc.log',
>          platform=self.platform,
>          env=self.env,
>@@ -556,6 +561,7 @@
>              resultsname=self.baseName
>             )
>         self.addStep(CompareLeakLogs,
>+         name='compare_leakLogs',
>          mallocLog='../malloc.log.old',
>          platform=self.platform,
>          env=self.env,

These look to be the only instances of using camel case in the step names, I'd rather they be called 'compare_leaklogs', or 'compare_leak_logs'.  Also, there are two steps with the same name above, I'm not sure what the difference between the two CompareLeakLogs steps are, but the name should probably reflect that.

Comment 11

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

We have steps called clean_configs and rm_configs that should probably be standardized too.

Comment 12

[Lukas Blakk [:lsblakk] use ?needinfo]

Attachment: Step Names updated

Fixed camel case issue and added the branchName to clobber step names for additional clarity.

I don't see how clean_configs and rm_configs can be any more standardized. At the moment these steps only happen once in any given factory so there would not be a repeat in a particular build - plus they all use the same command and deal with the same files (mozconfigs). Let me know what you were thinking with that comment coop and I'll make the necessary changes.

Comment 13

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

(In reply to comment #12) 
> I don't see how clean_configs and rm_configs can be any more standardized. At
> the moment these steps only happen once in any given factory so there would not
> be a repeat in a particular build - plus they all use the same command and deal
> with the same files (mozconfigs). Let me know what you were thinking with that
> comment coop and I'll make the necessary changes.

My point was: if the two steps are doing the same thing, why do they have different names? Ideally they should be rolled into a single def, but at least they should have the same name so people can search for all instances of that code.

Comment 14

[Lukas Blakk [:lsblakk] use ?needinfo]

Attachment: Step Names updated - take 2

I see what you mean now.  Fixed in this patch.

Comment 15

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

Comment on attachment 395096 [details] [diff] [review]
Step Names updated - take 2

http://hg.mozilla.org/build/buildbotcustom/rev/a03271decb50

Comment 16

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

I missed a quoting error in my review:

http://hg.mozilla.org/build/buildbotcustom/rev/0e28f3ee92c2

Both production masters have been updated and reconfig-ed.

Comment 17

[Lukas Blakk [:lsblakk] use ?needinfo]

Will wait until catlee is back to see if this can be closed now or if there is more to do.

Comment 18

[Lukas Blakk [:lsblakk] use ?needinfo]

Closing this.  Another bug can be filed for future naming needs.