Closed
Bug 633054
Opened 14 years ago
Closed 13 years ago
audit the mozilla/ mozilla-tests/ configs have a lot of duplicate code, is it possible get them into the same config?
Categories
(Release Engineering :: General, defect, P3)
Tracking
(Not tracked)
RESOLVED
WONTFIX
People
(Reporter: lsblakk, Assigned: armenzg)
References
Details
Attachments
(3 files, 2 obsolete files)
(deleted),
patch
|
lsblakk
:
review+
armenzg
:
checked-in+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
armenzg
:
review+
armenzg
:
checked-in+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
lsblakk
:
review+
catlee
:
review+
armenzg
:
checked-in+
|
Details | Diff | Splinter Review |
Try to get them into one config file if possible or at least pull out some of the duplication - tinderbox_tree for example and also do we still need packaged_unittest_tinderbox_tree?
Assignee | ||
Comment 1•14 years ago
|
||
Feel free to pass reviews my way.
I had few patches months ago but I would have bit-rotten bear and aki's work.
A lot can be moved there but we will need catlee's review as he touched a lot of the code under mozilla/.
catlee what you say?
Reporter | ||
Updated•14 years ago
|
Assignee: lsblakk → armenzg
Assignee | ||
Updated•14 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Updated•14 years ago
|
Priority: P3 → P4
Assignee | ||
Comment 2•13 years ago
|
||
I started working on bug 660124 and chrome_mac started getting on the way for project branches so I started refactoring... I think I got a little carried away.
This is a no-op patch which I have run through test-masters.sh any trouble but I would like to run it through dump_master.sh.
Attachment #547089 -
Flags: review?(catlee)
Reporter | ||
Comment 3•13 years ago
|
||
Comment on attachment 547089 [details] [diff] [review]
refactor mozilla-tests/config.py a little
Just to note, this is not noop since you're enabling the jetpack tests for mozilla-beta and mozilla-release and those are not currently on, though they are requested in bug 669428 so if you land this please feel free to go close that bug.
Assignee | ||
Comment 5•13 years ago
|
||
Comment on attachment 547089 [details] [diff] [review]
refactor mozilla-tests/config.py a little
lsblakk is right. I tested incorrectly. I will attach the correct patch soon.
lsblakk I would prefer to keep this patch as no-op and do a follow up for jetpack when we are ready; is that alright?
Attachment #547089 -
Flags: review?(catlee)
Reporter | ||
Comment 6•13 years ago
|
||
> lsblakk I would prefer to keep this patch as no-op and do a follow up for
> jetpack when we are ready; is that alright?
fine by me, I can still wait for this to land since it will make enabling the other branches easier and cleaner :)
Assignee | ||
Comment 7•13 years ago
|
||
After several iterations on dump_masters.sh and test-masters.sh, here is the no-op change for mozilla-tests/config.py (except for adding jetpack for Win7 x64).
It might be bitrotten by bug 674228 but I will adjust it again once I meet your review requirements.
Attachment #547089 -
Attachment is obsolete: true
Attachment #548471 -
Flags: review?(lsblakk)
Assignee | ||
Comment 8•13 years ago
|
||
Attachment #548475 -
Flags: review?(lsblakk)
Reporter | ||
Comment 9•13 years ago
|
||
Comment on attachment 548471 [details] [diff] [review]
refactor mozilla-tests/config.py
Review of attachment 548471 [details] [diff] [review]:
-----------------------------------------------------------------
::: mozilla-tests/config.py
@@ +857,5 @@
> + for pf in PLATFORMS:
> + if pf in ('linux-android', ):
> + continue
> + for slave_pf in PLATFORMS[pf]['slave_platforms']:
> + # yucky exceptions for 10.5
change comment to
# to get around leopard-o having only debug key and leopard having only opt
or something more useful :)
r+ with that nit addressed
Attachment #548471 -
Flags: review?(lsblakk) → review+
Reporter | ||
Updated•13 years ago
|
Attachment #548475 -
Flags: review?(lsblakk) → review+
Assignee | ||
Comment 10•13 years ago
|
||
Attachment #548471 -
Attachment is obsolete: true
Attachment #548753 -
Flags: review+
Attachment #548753 -
Flags: checked-in+
Assignee | ||
Comment 11•13 years ago
|
||
Comment on attachment 548475 [details] [diff] [review]
adjusting for previous attachment
http://hg.mozilla.org/build/buildbotcustom/rev/58ae380d5231
Attachment #548475 -
Flags: checked-in+
Comment 12•13 years ago
|
||
This is in production now.
Assignee | ||
Comment 13•13 years ago
|
||
Attachment #556557 -
Flags: review?(lsblakk)
Attachment #556557 -
Flags: review?(catlee)
Reporter | ||
Comment 14•13 years ago
|
||
Comment on attachment 556557 [details] [diff] [review]
It makes the try branch to always match other branches plus some more refactoring
> BRANCHES['mozilla-central']['build_branch'] = "1.9.2"
>-BRANCHES['mozilla-central']['platforms']['linux']['enable_mobile_unittests'] = True
> BRANCHES['mozilla-central']['platforms']['linux']['fedora']['opt_unittest_suites'] += [('reftest-no-accel', ['opengl-no-accel'])]
> BRANCHES['mozilla-central']['platforms']['linux-android']['enable_debug_unittests'] = True
Did you mean to turn off the mobile unittests here? I see you have them in your release_branch loop for the others, but it looks like you're just turning them off for m-c.
Assignee | ||
Comment 15•13 years ago
|
||
m-c is also a release branch even though we really don't release anything off it.
> 'mozilla-central': { 'release_branch': True },
Assignee | ||
Comment 16•13 years ago
|
||
FTR this is a no-op change.
Reporter | ||
Comment 17•13 years ago
|
||
Comment on attachment 556557 [details] [diff] [review]
It makes the try branch to always match other branches plus some more refactoring
in that case you will end up with the wrong repo path for mozilla-central:
+ BRANCHES[branch]['repo_path'] = "releases/%s" % branch
mozilla-central is a top-level repo.
Assignee | ||
Comment 18•13 years ago
|
||
I deal with it in the exceptions section:
> # The following are exceptions to the defaults
>
> ######## mozilla-central
> BRANCHES['mozilla-central']['branch_name'] = "Firefox"
> BRANCHES['mozilla-central']['repo_path'] = "mozilla-central"
Reporter | ||
Comment 19•13 years ago
|
||
Comment on attachment 556557 [details] [diff] [review]
It makes the try branch to always match other branches plus some more refactoring
very well then! :)
Attachment #556557 -
Flags: review?(lsblakk) → review+
Comment 20•13 years ago
|
||
Comment on attachment 556557 [details] [diff] [review]
It makes the try branch to always match other branches plus some more refactoring
Review of attachment 556557 [details] [diff] [review]:
-----------------------------------------------------------------
What does a diff of dump master output say?
r+ with comments addressed, and a clean diff of dump master.
::: mozilla-tests/config.py
@@ +375,5 @@
> BRANCHES[branch]['support_url_base'] = branchConfig.get('support_url_base', 'http://build.mozilla.org/talos')
> BRANCHES[branch]['enable_unittests'] = branchConfig.get('enable_unittests', True)
>
> def loadCustomTalosSuites(BRANCHES, SUITES, branch, branchConfig):
> + coallesceJobs = BRANCHES.get('coallesce_jobs', True)
I think you mean branchConfig.get('coallesce_jobs', True)?
@@ +393,5 @@
>
> for suite in SUITES.keys():
> if not SUITES[suite]['enable_by_default']:
> # Suites that are turned off by default
> + BRANCHES[branch][suite + '_tests'] = (talosConfig.get(suite, 0), ) + (coallesceJobs, ) + SUITES[suite]['options']
This could be better written as
BRANCHES[branch][suite + '_tests'] = (talosConfig.get(suite, 0), coallesceJobs) + SUITES[suite]['options']
@@ +403,5 @@
> '''
> This is very similar to loadCustomTalosSuites and is to deal with branches that are not in project_branches.py
> but in config.py. Both functions could be unified later on when we do further refactoring.
> '''
> + coallesceJobs = BRANCHES.get('coallesce_jobs', True)
Probably should be BRANCHES[branch].get('callesce_jobs', True)
Attachment #556557 -
Flags: review?(catlee) → review+
Assignee | ||
Comment 21•13 years ago
|
||
Comment on attachment 556557 [details] [diff] [review]
It makes the try branch to always match other branches plus some more refactoring
I addresses the comments. dump_masters.sh's diff returned no differences.
http://hg.mozilla.org/build/buildbot-configs/rev/cd4361138ffb
./test-masters.sh && ~/repos/releng/braindump/buildbot-related/dump_masters.sh > new && hg up -C && ~/repos/releng/braindump/buildbot-related/dump_masters.sh > old && diff -pU 8 old new > armen
armenzg-laptop $ ls -l old new
-rw-r--r-- 1 armenzg staff 614477680 31 Aug 15:01 new
-rw-r--r-- 1 armenzg staff 614477680 31 Aug 15:12 old
armenzg-laptop $ md5sum new
5ce56a3fc1cdcf6e64a383e7b7d04d6a new
armenzg-laptop $ md5sum old
5ce56a3fc1cdcf6e64a383e7b7d04d6a old
Attachment #556557 -
Flags: checked-in+
Assignee | ||
Updated•13 years ago
|
Assignee: armenzg → nobody
Assignee | ||
Updated•13 years ago
|
Assignee: nobody → armenzg
Priority: P4 → P3
Assignee | ||
Comment 22•13 years ago
|
||
I have not been able to work on this.
I don't see us gaining value since we will eventually be switching to Large-Wooden-Rabbit.
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → WONTFIX
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
•