Closed Bug 724787 Opened 10 years ago Closed 10 years ago

Convert SeaMonkey Release Configs to use a python dictionary rather than global vars.

Categories

(SeaMonkey :: Release Engineering, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: Callek, Assigned: ewong)

Details

Attachments

(3 files, 6 obsolete files)

66.91 KB, patch
Callek
: review+
Details | Diff | Splinter Review
7.46 KB, patch
Details | Diff | Splinter Review
22.47 KB, patch
Callek
: review+
Details | Diff | Splinter Review
Firefox long ago switched to using a ReleaseConfig python dict for their release configs.

Many of the new release syntax stuff requires that, and it would also allow us to do multiple releases in parallel.

As a first step we need to convert our Configs to use the dict syntax, and make our release-factory code support it.

From convo on IRC ewong felt he could handle this, and will do so.
Comment on attachment 594952 [details] [diff] [review]
Convert SeaMonkey release configs to use a python dictionary. (part 1) (v1)

Skimming this looks fine by itself for this file, BUT we can't do just this one file at once, we need all the release-* config files, and the file updates that reference them...
Attachment #594952 - Flags: review?(bugspam.Callek) → review-
Did it for all release configs, incl 2.0 and 1.9.1.
Attachment #594952 - Attachment is obsolete: true
Attachment #595303 - Flags: review?(bugspam.Callek)
changes also includes the release_master.py
Attachment #595303 - Attachment is obsolete: true
Attachment #595303 - Flags: review?(bugspam.Callek)
Attachment #595326 - Flags: review?(bugspam.Callek)
Comment on attachment 595326 [details] [diff] [review]
Convert SeaMonkey release configs to use a python dictionary. (part 1) (v3)

Review of attachment 595326 [details] [diff] [review]:
-----------------------------------------------------------------

Overall, looks good, some things I caught in release_master.py need fixing, I mostly skimmed the other release*.py's here, but they look good. (I didn't char for character check everything, but I did check the major intent of this patch and where it may have been missed)

::: seamonkey/release-comm-release.py
@@ +68,1 @@
>                                'macosx64': 'mozRelease-seamonkey-mac64.cfg',

I have whitespace nits which I'll call out here, but I'm not too worried since I still intend to re-order/reformat these whole files anyway.

Basically for the current order of the file, I wish we can keep the ='s signs lined up like previously.

Also I wish the newlines right above this comment lined up ' signs at the start of the key names.

::: seamonkey/release_master.py
@@ +176,4 @@
>          'bumpFiles': []
>      }
>  
>  if len(l10nPlatforms) > 0:

missed this one.

@@ +237,2 @@
>      # Disable cvsroot on comm-central/comm-2.0 builds
> +    #releaseConfig['cvsroot']=cvsroot,

wrong thing wrapped here, but it is commented out, so doesn't *really* matter.

@@ +255,3 @@
>      # shorthand
>      pf = branchConfig['platforms'][platform]
>      mozconfig = '%s/%s/release' % (platform, sourceRepoName)

need to fix sourceRepoName here

@@ +255,4 @@
>      # shorthand
>      pf = branchConfig['platforms'][platform]
>      mozconfig = '%s/%s/release' % (platform, sourceRepoName)
>      l10nmozconfig = '%s/%s/release-l10n' % (platform, sourceRepoName)

and here

@@ +255,5 @@
>      # shorthand
>      pf = branchConfig['platforms'][platform]
>      mozconfig = '%s/%s/release' % (platform, sourceRepoName)
>      l10nmozconfig = '%s/%s/release-l10n' % (platform, sourceRepoName)
>      if platform in talosTestPlatforms:

...talosTestPlatforms

@@ +260,5 @@
>          talosMasters = branchConfig['talos_masters']
>      else:
>          talosMasters = None
>  
>      if platform in unittestPlatforms:

...unittestPlatforms

@@ +335,2 @@
>              # Disable cvsroot on comm-central/comm-2.0 builds
>              #cvsroot=cvsroot,

Since you wrap it above, might as well do it here... But if you revert above, no need for here

@@ +338,1 @@
>              mergeLocales=mergeLocales,

...mergeLocales

@@ +449,1 @@
>      schema=globals().get("snippetSchema", 1), # Bug 682805

I know my syntax here caught you off guard, but we need to change this as well.

releaseConfig.get("snippetSchema", 1),

instead of globals().get(...),

@@ -477,1 @@
>          'factory': update_verify_factory,

Not shown in my context here, but |for platform in sorted(verifyConfigs.key):| needs to have verifyConfigs wrapped as well

Same with |verifyConfig=verifyConfigs[platform],| needs to change to |verifyConfig=releaseConfig['verifyConfigs'][platform]|

@@ +539,1 @@
>          schema=globals().get("majorSnippetSchema", 1), # Bug 682805

same thing as with the globals.get() change above.
Attachment #595326 - Flags: review?(bugspam.Callek) → review-
Fixed nits and the missing wrappers.
Attachment #595326 - Attachment is obsolete: true
Attachment #595344 - Flags: review?(bugspam.Callek)
Comment on attachment 595344 [details] [diff] [review]
Convert SeaMonkey release configs to use a python dictionary. (part 1) (v4)

Review of attachment 595344 [details] [diff] [review]:
-----------------------------------------------------------------

Ok, skimming this all looks great. All my nits are addressed!
Attachment #595344 - Flags: review?(bugspam.Callek) → review+
Status: NEW → ASSIGNED
Aside for the already attached patch, there were a series of bustage fixes
as given in the following csets:

1) http://hg.mozilla.org/build/buildbot-configs/rev/9cf36415790d
2) http://hg.mozilla.org/build/buildbot-configs/rev/1785eef818fb
3) http://hg.mozilla.org/build/buildbot-configs/rev/495c7eda2dab

All were r+/rs+ by Callek over IRC.
release-comm-beta.py changes only for feedback.
Attachment #595724 - Flags: feedback?(bugspam.Callek)
Comment on attachment 595724 [details] [diff] [review]
Convert SeaMonkey Release Configs to use a python dictionary. (part 2 - reorder configs)

Review of attachment 595724 [details] [diff] [review]:
-----------------------------------------------------------------

I *think* this is all the nits I have/will have, any further Q's ask me in IRC, I'll review the final patch for sure.

::: seamonkey/release-comm-beta.py
@@ +4,5 @@
> +#  not used yet
> +# Basic product configuration
> +#  Names for the product/files
> +#releaseConfig['productVersionFile']        = 'suite/config/version-20.txt'
> +releaseConfig['productVersionFile']         = ''

move these two lines (productVersionFile) to just below the sourceRepo section

@@ +8,5 @@
> +releaseConfig['productVersionFile']         = ''
> +releaseConfig['productName']                = 'seamonkey'
> +releaseConfig['brandName']                  = 'SeaMonkey'
> +releaseConfig['appName']                    = 'suite'
> +releaseConfig['skip_tag']                   = False

Stick this right at the start of the tagging-section (above 'relbranchprefix')

@@ +52,5 @@
>  releaseConfig['l10nRepoPath']               = 'releases/l10n/mozilla-beta'
>  releaseConfig['l10nRelbranchOverride']      = ''
>  releaseConfig['l10nRevisionFile']           = 'l10n-changesets-comm-beta'
> +
> +# Support repositories

kill that blank line, and indent "Support Repositories" just line "L10n ..." above

@@ +60,2 @@
>  releaseConfig['enUSPlatforms']              = ('linux', 'linux64', 'win32', 'macosx64')
>  releaseConfig['l10nPlatforms']              = ('linux', 'win32', 'macosx64')

put this in "L10n config..." above mergeLocales

@@ +70,5 @@
> +# Mercurial account
> +releaseConfig['hgUsername']                 = 'seabld'
> +releaseConfig['hgSshKey']                   = '~seabld/.ssh/seabld_dsa'
> +
> +# CVSROOT : for patcher, etc

get rid of this comment and toss 'cvsroot' into 'Update Specific...." section below

@@ +87,1 @@
>  releaseConfig['verifyConfigs']              = {'linux':  'mozBeta-seamonkey-linux.cfg',

my prefence here is to start a new line at { and do a 4 space indent for all those keys, then a new line for the }
Attachment #595724 - Flags: feedback?(bugspam.Callek) → feedback+
Attachment #595724 - Attachment is obsolete: true
Attachment #596415 - Flags: feedback?(bugspam.Callek)
Comment on attachment 596415 [details] [diff] [review]
Convert SeaMonkey Release Configs to use a python dictionary. (part 2 - reorder configs) (v2)

Review of attachment 596415 [details] [diff] [review]:
-----------------------------------------------------------------

bah, splinter made this confusing... let me edit the first version for how I want then you can pickup (I think thats easier)
ewong this is the minor adjustments I was meaning with my f+ of part 2 v1.
Attachment #596415 - Attachment is obsolete: true
Attachment #596415 - Flags: feedback?(bugspam.Callek)
Incl changes to release-comm-release.py, release-comm-beta.py and release-comm-1.9.1.py
Attachment #596471 - Flags: review?(bugspam.Callek)
Comment on attachment 596471 [details] [diff] [review]
Convert SeaMonkey Release Configs to use a python dictionary. (part 2 - reorder configs) (v3)

Review of attachment 596471 [details] [diff] [review]:
-----------------------------------------------------------------

::: seamonkey/release-comm-1.9.1.py
@@ +99,3 @@
>  releaseConfig['majorUpdateToVersion']       = '2.6.1'
>  releaseConfig['majorUpdateAppVersion']      = '2.6.1'
>  releaseConfig['majorUpdateBaseTag']         = 'SEAMONKEY_2_6_1'

Hrm Tuxedo is not listed here (though we don't actually need it at all, nevermind this file, so as-is is good)

Just be sure that my changes at the end of these files (over this weekend) don't bitrot you and mess things up.

But looks good.
Attachment #596471 - Flags: review?(bugspam.Callek) → review+
Unbit rot previous patch.
Attachment #596471 - Attachment is obsolete: true
Attachment #596549 - Flags: review?(bugspam.Callek)
Attachment #596549 - Flags: review?(bugspam.Callek) → review+
Pushed to buildbot/buildbot-configs:
http://hg.mozilla.org/build/buildbot-configs/rev/99da3ba7a076
This is done, thanks ewong!
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.