Closed Bug 410006 Opened 13 years ago Closed 13 years ago

PatcherConfig should generate pretty version strings

Categories

(Release Engineering :: General, defect, P2)

defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: rhelmer, Assigned: nthomas)

References

Details

Attachments

(4 files, 3 obsolete files)

Over in bug 359082, there's a desire to have nicer-looking version strings in the Firefox update UI. For example, instead of "Firefox 3.0b2" the update dialog would prompt the user to update to "Firefox 3 Beta 2".

The product name comes from the client, but AUS can provide the "3 Beta 2" part. There have been requests for more fanciful strings too.

We could attempt to automatically expand this (e.g. "3.0a2" == "3 Alpha 2") but maybe it's just better and safer to provide a way to override the "version" string in the snippet, and fall back to the old (current) behavior if not specified.

Took a quick look in PatcherConfig, I think we'd just need to do a check here:
http://mxr.mozilla.org/mozilla/source/tools/release/Bootstrap/Step/PatcherConfig.pm#212

extension-version should continue to point to $version, but version could point to e.g. "aus_version" in bootstrap.cfg if present, otherwise fall back.
Assignee: nobody → rhelmer
Blocks: 359082
Status: NEW → ASSIGNED
Priority: -- → P2
I think this is all it should take to allow a gussied-up "version" string for the version in the client UI. 

If "pretty_aus_version" var is present in bootstrap.cfg use it, otherwise fall back to current behavior.
Attachment #294702 - Flags: review?(nrthomas)
Whiteboard: waiting for review
Comment on attachment 294702 [details] [diff] [review]
untested, add ability to override patcher version with pretty version

This looks fine, glad to hear that it just works in patcher. Would it be useful to use a var called prettyVersion, and also use this for file renaming while staging (rather than hard coding it). It would also match the interCaps style for other vars.
See also Ben's changes in bug 409394.
(In reply to comment #3)
> See also Ben's changes in bug 409394.

Hmm, yeah I thought about this, but I'm not sure if we should auto-generate the prettyName, because we may want it to say something fancier. Maybe we should auto-generate the fallback name? That makes less work for us in the beta/alpha case.
Attachment #294702 - Flags: review?(nrthomas)
Attachment #294702 - Attachment is obsolete: true
Attachment #295848 - Flags: review?(nrthomas)
Comment on attachment 295848 [details] [diff] [review]
[checked in] use prettyAusVersion not pretty_aus_version

Looks good, r+. Do you think we are settled enough on "Firefox 3 Beta 3" to specify prettyAusVersion in fx-moz19-*bootstrap.cfg ?
Attachment #295848 - Flags: review?(nrthomas) → review+
Blocks: 409880
Comment on attachment 295848 [details] [diff] [review]
[checked in] use prettyAusVersion not pretty_aus_version

Checking in Bootstrap/Step/PatcherConfig.pm;
/cvsroot/mozilla/tools/release/Bootstrap/Step/PatcherConfig.pm,v  <--  PatcherConfig.pm
new revision: 1.13; previous revision: 1.12
done
Attachment #295848 - Attachment description: use prettyAusVersion not pretty_aus_version → [checked in] use prettyAusVersion not pretty_aus_version
Whiteboard: waiting for review
Whiteboard: testing
Hm, so as-is this will cause the version in the detailsURL to be "3.0 Beta 3" as well. Not sure if this is what we want, it's different than what we do know for sure.

Patcher uses SubstitutePath to set detailsUrl with %version% coming from appv, which is the same string that gets used in the update.xml

If we want detailsUrl/extv to be 3.0b3 and appv to be "3.0 Beta 3", there are a couple ways I can think of to do this:

a) substitute %version% with extv in detailsUrl
b) don't substitute %version% at all, make PatcherConfig specify the desired version

(a) seems like it's using extv (extension compatibility version) in an unintended and potentially surprising way. I'm thinking (b) for this, what do you think Nick? Am I missing a better way here?
Attached patch bump relnotes from PatcherConfig (obsolete) — Splinter Review
Bump details url version explicitly in PatcherConfig. This also gives us the option of pointing detailsUrl at the staging boxes, which might be nice to do later for auto-verification purposes.
Attachment #297593 - Flags: review?(nrthomas)
Whiteboard: testing → waiting for review
Comment on attachment 297593 [details] [diff] [review]
bump relnotes from PatcherConfig

Of the two proposals, I don't really mind, so this patch seems fine. AFAIK, extv was only different from appv for the 1.5 betas, and AMO has come a long way since then.

Why is there no %locale% between http:// and www. ? We use that in all the 1.8 and 1.8.0-major configs, but not for 1.8.0 and 1.9.
(In reply to comment #10)
> (From update of attachment 297593 [details] [diff] [review])
> Of the two proposals, I don't really mind, so this patch seems fine. AFAIK,
> extv was only different from appv for the 1.5 betas, and AMO has come a long
> way since then.
> 
> Why is there no %locale% between http:// and www. ? We use that in all the 1.8
> and 1.8.0-major configs, but not for 1.8.0 and 1.9. 

I just copied this line from the moz19 config; happy to change this.

Now with http://%locale% !
Attachment #297593 - Attachment is obsolete: true
Attachment #297774 - Flags: review?(nrthomas)
Attachment #297593 - Flags: review?(nrthomas)
Comment on attachment 297774 [details] [diff] [review]
[checked in] bump relnotes URL from PatcherConfig

clouserw confirmed that this is the way to go.
Attachment #297774 - Flags: review?(nrthomas) → review+
Comment on attachment 297774 [details] [diff] [review]
[checked in] bump relnotes URL from PatcherConfig

Checking in Bootstrap/Step/PatcherConfig.pm;
/cvsroot/mozilla/tools/release/Bootstrap/Step/PatcherConfig.pm,v  <--  PatcherConfig.pm
new revision: 1.14; previous revision: 1.13
done
Attachment #297774 - Attachment description: bump relnotes URL from PatcherConfig → [checked in] bump relnotes URL from PatcherConfig
Whiteboard: waiting for review → testing
Attached file result from staging
Looks good to me. Note in this tests I've attached, version should be "3 Beta 3" not "Firefox 3 Beta 3", but that's a configuration issue. I'll retest and either verify or reopen this bug, but I think that this test is enough to show that the patch works as intended.
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Whiteboard: testing
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment 297774 [details] [diff] caused bug 418450 by hardcoding firefox in the URL (super bit of reviewing that!). Switch to using $product instead.
Assignee: rhelmer → nrthomas
Status: REOPENED → ASSIGNED
Attachment #304323 - Flags: review?(rhelmer)
Urgh.
Attachment #304323 - Attachment is obsolete: true
Attachment #304327 - Flags: review?(rhelmer)
Attachment #304323 - Flags: review?(rhelmer)
Comment on attachment 304327 [details] [diff] [review]
[checked in] Remove hard coded "firefox" in details - take 2

Ouch, sorry :)
Attachment #304327 - Flags: review?(rhelmer) → review+
Comment on attachment 304327 [details] [diff] [review]
[checked in] Remove hard coded "firefox" in details - take 2

Checking in Bootstrap/Step/PatcherConfig.pm;
/cvsroot/mozilla/tools/release/Bootstrap/Step/PatcherConfig.pm,v  <--  PatcherConfig.pm
new revision: 1.17; previous revision: 1.16
done
Attachment #304327 - Attachment description: Remove hard coded "firefox" in details - take 2 → [checked in] Remove hard coded "firefox" in details - take 2
Status: ASSIGNED → RESOLVED
Closed: 13 years ago13 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.