Step name cleanups for build database

RESOLVED FIXED

Status

Release Engineering
General
P2
normal
RESOLVED FIXED
9 years ago
5 years ago

People

(Reporter: catlee, Assigned: lsblakk)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 5 obsolete attachments)

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'.
(Assignee)

Updated

9 years ago
Assignee: nobody → lsblakk
Priority: -- → P2
(Assignee)

Comment 1

9 years ago
Created attachment 381823 [details] [diff] [review]
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.
Attachment #381823 - Flags: review?(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'.
Attachment #381823 - Flags: review?(catlee) → review-
(Assignee)

Comment 3

9 years ago
Created attachment 381841 [details] [diff] [review]
Adding names to shell commands, with hg_clone consistent

All instances of hg clone are now named hg_clone.
Attachment #381823 - Attachment is obsolete: true
Attachment #381841 - Flags: review?(catlee)
Attachment #381841 - Flags: review?(catlee) → review+
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.
(Assignee)

Comment 5

9 years ago
Created attachment 382308 [details] [diff] [review]
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.
Attachment #381841 - Attachment is obsolete: true
(Assignee)

Comment 6

9 years ago
Created attachment 382797 [details] [diff] [review]
Adding names to shell commands, with fixes

This one cleans up a missing ,

Currently testing this on staging
Attachment #382308 - Attachment is obsolete: true
(Assignee)

Updated

9 years ago
Attachment #382797 - Flags: review?(catlee)
Attachment #382797 - Flags: review?(catlee) → review+
Attachment #382797 - Flags: checked‑in+
Comment on attachment 382797 [details] [diff] [review]
Adding names to shell commands, with fixes

changeset:   335:7b91b763cb4e
(Assignee)

Updated

9 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 8

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

Comment 9

9 years ago
Created attachment 385362 [details] [diff] [review]
Follow up step names not caught in last patch

Tested on staging, works.
Attachment #385362 - Flags: review?(bhearsum)
(Assignee)

Updated

9 years ago
Attachment #385362 - Flags: review?(bhearsum) → review?(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.
Attachment #385362 - Flags: review?(catlee) → review-
We have steps called clean_configs and rm_configs that should probably be standardized too.
Created attachment 395062 [details] [diff] [review]
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.
Attachment #385362 - Attachment is obsolete: true
Attachment #395062 - Flags: review?(ccooper)
(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.
Created attachment 395096 [details] [diff] [review]
Step Names updated - take 2

I see what you mean now.  Fixed in this patch.
Attachment #395062 - Attachment is obsolete: true
Attachment #395096 - Flags: review?(ccooper)
Attachment #395062 - Flags: review?(ccooper)

Updated

9 years ago
Attachment #395096 - Flags: review?(ccooper) → review+
(Assignee)

Updated

9 years ago
Attachment #395096 - Flags: checked-in?
Comment on attachment 395096 [details] [diff] [review]
Step Names updated - take 2

http://hg.mozilla.org/build/buildbotcustom/rev/a03271decb50
Attachment #395096 - Flags: checked-in? → checked-in+
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.
Will wait until catlee is back to see if this can be closed now or if there is more to do.
Closing this.  Another bug can be filed for future naming needs.
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Product: mozilla.org → Release Engineering
You need to log in before you can comment on or make changes to this bug.