Closed
Bug 496509
Opened 15 years ago
Closed 15 years ago
Step name cleanups for build database
Categories
(Release Engineering :: General, defect, P2)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: catlee, Assigned: lsblakk)
Details
Attachments
(2 files, 5 obsolete files)
58.52 KB,
patch
|
catlee
:
review+
catlee
:
checked-in+
|
Details | Diff | Splinter Review |
8.53 KB,
patch
|
coop
:
review+
coop
:
checked-in+
|
Details | Diff | Splinter Review |
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•15 years ago
|
Assignee: nobody → lsblakk
Priority: -- → P2
Assignee | ||
Comment 1•15 years ago
|
||
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)
Reporter | ||
Comment 2•15 years ago
|
||
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•15 years ago
|
||
All instances of hg clone are now named hg_clone.
Attachment #381823 -
Attachment is obsolete: true
Attachment #381841 -
Flags: review?(catlee)
Reporter | ||
Updated•15 years ago
|
Attachment #381841 -
Flags: review?(catlee) → review+
Comment 4•15 years ago
|
||
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•15 years ago
|
||
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•15 years ago
|
||
This one cleans up a missing , Currently testing this on staging
Attachment #382308 -
Attachment is obsolete: true
Assignee | ||
Updated•15 years ago
|
Attachment #382797 -
Flags: review?(catlee)
Reporter | ||
Updated•15 years ago
|
Attachment #382797 -
Flags: review?(catlee) → review+
Reporter | ||
Updated•15 years ago
|
Attachment #382797 -
Flags: checked‑in+
Reporter | ||
Comment 7•15 years ago
|
||
Comment on attachment 382797 [details] [diff] [review] Adding names to shell commands, with fixes changeset: 335:7b91b763cb4e
Assignee | ||
Updated•15 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 8•15 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•15 years ago
|
||
Tested on staging, works.
Attachment #385362 -
Flags: review?(bhearsum)
Assignee | ||
Updated•15 years ago
|
Attachment #385362 -
Flags: review?(bhearsum) → review?(catlee)
Reporter | ||
Comment 10•15 years ago
|
||
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-
Comment 11•15 years ago
|
||
We have steps called clean_configs and rm_configs that should probably be standardized too.
Assignee | ||
Comment 12•15 years ago
|
||
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)
Comment 13•15 years ago
|
||
(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.
Assignee | ||
Comment 14•15 years ago
|
||
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•15 years ago
|
Attachment #395096 -
Flags: review?(ccooper) → review+
Assignee | ||
Updated•15 years ago
|
Attachment #395096 -
Flags: checked-in?
Comment 15•15 years ago
|
||
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+
Comment 16•15 years ago
|
||
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.
Assignee | ||
Comment 17•15 years ago
|
||
Will wait until catlee is back to see if this can be closed now or if there is more to do.
Assignee | ||
Comment 18•15 years ago
|
||
Closing this. Another bug can be filed for future naming needs.
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Updated•11 years ago
|
Product: mozilla.org → Release Engineering
You need to log in
before you can comment on or make changes to this bug.
Description
•