Closed
Bug 1145175
Opened 10 years ago
Closed 9 years ago
Include the beta version in "About Firefox"
Categories
(Firefox Build System :: General, defect)
Firefox Build System
General
Tracking
(firefox40 affected, firefox41 fixed)
RESOLVED
FIXED
mozilla41
People
(Reporter: Sylvestre, Assigned: Sylvestre)
References
Details
Attachments
(1 file, 4 obsolete files)
5.10 KB,
patch
|
glandium
:
review+
|
Details | Diff | Splinter Review |
Similar to bug #1145171, in the release management process (and also for QE and UA), having the beta number displayed in About Firefox would help a lot. For now, we just have the version, no information about the beta we are running. In release, we are displaying dot releases too (example: 36.0.1)
Assignee | ||
Comment 1•10 years ago
|
||
Ben, does that sound the correct place for you? Thanks
Attachment #8602695 -
Flags: review?(bhearsum)
Assignee | ||
Updated•10 years ago
|
Component: General → Release Automation
Product: Toolkit → Release Engineering
QA Contact: bhearsum
Comment 2•10 years ago
|
||
Comment on attachment 8602695 [details] [diff] [review] bug-1145175.diff I'm pretty sure this will break a whole bunch of things, including add-on version checks and possibly updates. This will change the version in application.ini, the update ping, add-on pings, etc. I don't think we want to do this without a lot of forethought. If the only goal is to show this in the dialog, it's probably best to add it as new thing, and change the About dialog to use that. It might be good to talk to build system person if you want to go that route. Ted might be a good starting point.
Attachment #8602695 -
Flags: review?(bhearsum) → review-
Updated•10 years ago
|
Component: Release Automation → Build Config
Product: Release Engineering → Core
QA Contact: bhearsum
Assignee | ||
Comment 3•10 years ago
|
||
OK, at least I tried :) Ted, can you help here? Thanks It is hard to find which one to fix *. And I didn't find out in build log how the versions are updated or set. The about windows uses MOZ_APP_VERSION. * $ grep 38.0 config.status (''' MOZILLA_VERSION ''', r''' "38.0" '''), (''' MOZILLA_VERSION_U ''', r''' 38.0 '''), (''' MOZILLA_UAVERSION ''', r''' "38.0" '''), (''' MOZ_APP_UA_VERSION ''', r''' "38.0" '''), (''' FIREFOX_VERSION ''', r''' 38.0 '''), (''' GRE_MILESTONE ''', r''' 38.0 '''), (''' MOZ_APP_VERSION ''', r''' 38.0 '''), (''' FIREFOX_VERSION ''', r''' 38.0 '''), (''' MOZILLA_VERSION ''', r''' 38.0 '''),
Flags: needinfo?(ted)
Comment 4•9 years ago
|
||
The beta version was removed for a reason that I don't remember. Please make sure you're not going against that reason. That said, application.ini and about:buildconfig both have the revision the build was built from, isn't that enough? In about:buildconfig, it's even an hyperlink to hg.mozilla.org, which, if you follow it, will also give you the mercurial tags, which do tell what specific beta it is.
Comment 5•9 years ago
|
||
For example, the link in about:buildconfig for 38.0.5b3 is https://hg.mozilla.org/releases/mozilla-release/rev/f81271987769
Assignee | ||
Comment 6•9 years ago
|
||
(In reply to Mike Hommey [:glandium] from comment #4) > The beta version was removed for a reason that I don't remember. Please make > sure you're not going against that reason. Some various people told me that it was removed to "hide" the version number in Firefox itself (with the idea that we should not mention the version). As we failed on this, I would like to bring that back into the product itself.
Comment 7•9 years ago
|
||
(In reply to Sylvestre Ledru [:sylvestre] from comment #6) > (In reply to Mike Hommey [:glandium] from comment #4) > > The beta version was removed for a reason that I don't remember. Please make > > sure you're not going against that reason. > Some various people told me that it was removed to "hide" the version number > in Firefox itself (with the idea that we should not mention the version). I /think/ this was orthogonal to try to get rid of the version and had more to do with having a cycle of testing in the train where the builds have the actual same behavior as releases wrt versioning. Because 39.0bn is not the same as 39.0.
Comment 8•9 years ago
|
||
Now, it /could/ be argued that the version in the about box could be different than the one used for everything else, but that sounds like one more way to screw up with the build system.
Comment 9•9 years ago
|
||
(In reply to Mike Hommey [:glandium] from comment #8) > Now, it /could/ be argued that the version in the about box could be > different than the one used for everything else, but that sounds like one > more way to screw up with the build system. And by that, I mean: - you need either another file with the beta number or to put the beta number in browser/config/version.txt - depending on the above, you need to feed a variable that will be specific to the about dialog and use it there - if you go with browser/config/version.txt, you need to remove the beta version for all the other variables, but you also need to make that optional for third-party apps that *do* want the beta number everywhere and are already using beta numbers in their version.txt. - if you go with a separate file, you need to add that beta number to the version for about dialog, but you also need to support not doing so for the same reason as above. The whole thing handling versions, as you can see from the number of variables, is a mess. If it's only for QE and release management, I'm not convinced it's worth doing it. If there's actual will to make it visible to the beta user population, that's a different story.
Comment 10•9 years ago
|
||
Sounds like glandium has this under control from the build system perspective.
Flags: needinfo?(ted)
Assignee | ||
Comment 11•9 years ago
|
||
Is that what you proposed? Thanks
Attachment #8602695 -
Attachment is obsolete: true
Attachment #8616710 -
Flags: review?(mh+mozilla)
Comment 12•9 years ago
|
||
Comment on attachment 8616710 [details] [diff] [review] beta-show.diff Review of attachment 8616710 [details] [diff] [review]: ----------------------------------------------------------------- ::: configure.in @@ +1925,5 @@ > MOZILLA_SYMBOLVERSION=`$PYTHON $srcdir/python/mozbuild/mozbuild/milestone.py --topsrcdir $srcdir --symbolversion` > > dnl Get version of various core apps from the version files. > FIREFOX_VERSION=`cat $_topsrcdir/browser/config/version.txt` > +FIREFOX_VERSION_COMPLETE=`cat $_topsrcdir/browser/config/version-complete.txt` Do we really want to maintain a separate file for this? Also, the file is not in the patch, so applied as is, all this does is remove the version from the about box. ::: extensions/confvars.sh @@ +5,5 @@ > > MOZ_APP_NAME=mozilla > MOZ_APP_DISPLAYNAME=Mozilla > MOZ_APP_VERSION=$MOZILLA_VERSION > +MOZ_APP_VERSION_COMPLETE=$MOZILLA_VERSION_COMPLETE MOZILLA_VERSION_COMPLETE is never defined.
Attachment #8616710 -
Flags: review?(mh+mozilla) → review-
Assignee | ||
Comment 13•9 years ago
|
||
here it is. I am adding a new line in version.txt
Attachment #8616710 -
Attachment is obsolete: true
Attachment #8617456 -
Flags: review?(mh+mozilla)
Comment 14•9 years ago
|
||
Comment on attachment 8617456 [details] [diff] [review] beta-show.diff Review of attachment 8617456 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/config/version.txt @@ +1,2 @@ > 39.0 > +39.0bX You obviously don't want this version landed on m-c. ::: configure.in @@ +1925,5 @@ > MOZILLA_SYMBOLVERSION=`$PYTHON $srcdir/python/mozbuild/mozbuild/milestone.py --topsrcdir $srcdir --symbolversion` > > dnl Get version of various core apps from the version files. > +FIREFOX_VERSION=`cat $_topsrcdir/browser/config/version.txt|head -1` > +FIREFOX_VERSION_COMPLETE=`cat $_topsrcdir/browser/config/version.txt|tail -1` There's at least browser/installer/windows/Makefile.in and webapprt/win/Makefile.in reading this file. They probably shouldn't read it directly, and use FIREFOX_VERSION instead, but changing the expected contents of that file require those to be handled. @@ +8696,5 @@ > AC_SUBST(MOZ_APP_MAXVERSION) > AC_DEFINE_UNQUOTED(FIREFOX_VERSION,$FIREFOX_VERSION) > AC_SUBST(FIREFOX_VERSION) > +AC_DEFINE_UNQUOTED(FIREFOX_VERSION_COMPLETE,$FIREFOX_VERSION_COMPLETE) > +AC_SUBST(FIREFOX_VERSION_COMPLETE) There's probably not much use in making this particular variable available to the build system.
Attachment #8617456 -
Flags: review?(mh+mozilla) → feedback+
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → sledru
Assignee | ||
Comment 15•9 years ago
|
||
Here it is. However, I am not sure how version.txt is updated (by hand after a merge?) I renamed FIREFOX_VERSION_COMPLETE by FIREFOX_VERSION_ABOUT to make it more obvious.
Attachment #8617456 -
Attachment is obsolete: true
Attachment #8620860 -
Flags: review?(mh+mozilla)
Assignee | ||
Comment 16•9 years ago
|
||
Rebased
Attachment #8620860 -
Attachment is obsolete: true
Attachment #8620860 -
Flags: review?(mh+mozilla)
Attachment #8620927 -
Flags: review?(mh+mozilla)
Comment 17•9 years ago
|
||
Comment on attachment 8620927 [details] [diff] [review] beta.diff Review of attachment 8620927 [details] [diff] [review]: ----------------------------------------------------------------- r+ with the change below. > However, I am not sure how version.txt is updated (by hand after a merge?) I don't know. How it's going to be updated on beta is something you need to figure out with releng (I guess?) before you land this. ::: configure.in @@ +1947,5 @@ > MOZILLA_SYMBOLVERSION=`$PYTHON $srcdir/python/mozbuild/mozbuild/milestone.py --topsrcdir $srcdir --symbolversion` > > dnl Get version of various core apps from the version files. > +FIREFOX_VERSION=`cat $_topsrcdir/browser/config/version.txt|head -1` > +FIREFOX_VERSION_ABOUT=`cat $_topsrcdir/browser/config/version.txt|sed -n 2p` The tail -1 version had the nice property of working without changing the version.txt file. Then, on beta, the file could look like: 39.0 # Version to display in the about box: 39.0b6
Attachment #8620927 -
Flags: review?(mh+mozilla) → review+
Assignee | ||
Comment 18•9 years ago
|
||
Thanks. Fixed and pushed: https://hg.mozilla.org/integration/mozilla-inbound/rev/3c6d24af0ac5
Assignee | ||
Comment 20•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/95b6ab6e9fa6
Comment 21•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/3c6d24af0ac5 https://hg.mozilla.org/mozilla-central/rev/95b6ab6e9fa6
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox41:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Assignee | ||
Updated•9 years ago
|
status-firefox40:
--- → affected
Assignee | ||
Comment 22•9 years ago
|
||
Comment on attachment 8620927 [details] [diff] [review] beta.diff For once, requesting an uplift of my one of my patch. Approval Request Comment [Feature/regressing bug #]: NA [User impact if declined]: Harder to see which version the user is running. I would like to have this change in 40 (not 39 as it will probably requires some changes on the releng side and we are late in the cycle) [Describe test coverage new/current, TreeHerder]: Mostly a build changes (ie, it builds or it doesn't build), a day in m-c and a lot of manual testing. [Risks and why]: Low: update of the build system + update of a variable in a XUL file. Worst case scenario, the "about" window shows an empty version. [String/UUID change made/needed]: None Lawrence, as I don't want to auto approve myself an uplift, could you accept (or reject that)?
Flags: needinfo?(lmandel)
Attachment #8620927 -
Flags: approval-mozilla-aurora?
Comment 23•9 years ago
|
||
Comment on attachment 8620927 [details] [diff] [review] beta.diff A variable name like __MOZ_APP_VERSION_DISPLAY__ may have been a better option in case there is another place in the UI that we choose to display the version. However, that's a nit and you already have your r+ and I think this is good as is. We won't start to see the benefit of this change until it hits Beta. Now that this has been on Nightly for a couple of days, let's push it to Aurora. Aurora+
Flags: needinfo?(lmandel)
Attachment #8620927 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 24•9 years ago
|
||
Comment on attachment 8620927 [details] [diff] [review] beta.diff Sorry, I marked the regression as blocking this but didn't get around to commenting because I hadn't decided about the propriety of cancelling the approval request on my own. Cancelling the granted approval is an easier call, because you Do Not Want this on aurora right now: we know it will break the periodic updates of the blocklist and HSTS and HPKP, and it may or may not also break merges and release automation.
Attachment #8620927 -
Flags: approval-mozilla-aurora+
Comment 25•9 years ago
|
||
(In reply to Phil Ringnalda (:philor) from comment #24) > Comment on attachment 8620927 [details] [diff] [review] > beta.diff > > we know it will > break the periodic updates of the blocklist and HSTS and HPKP, and it may or > may not also break merges and release automation. Can you be more specific? The change only affects the version displayed in the about dialog. Not the version programatically available from inside Firefox, nor the version exposed in the UA, nor the version in application.ini. Does any of those things you are referring to take the version number directly from browser/config/version.txt?
Comment 26•9 years ago
|
||
Every one of them does, see bug 1174558 and the linked mxr search in it. The periodic update already failed, http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-central-linux64/mozilla-central-linux64-periodicupdate-bm73-build1-build2.txt.gz and release tagging and the mergeday script update the version number in version.txt with currently-unknown results. Maybe they will now update both to the same thing (which, if they do, seems like it would break this feature for this feature to have any point), maybe they will only update one or the other, maybe they'll halt and catch fire. I don't know, somebody needs to check before we find out by breaking a release or breaking a mergeday. I do know, however, that it completely breaks the way the blocklist update currently works.
Comment 27•9 years ago
|
||
See bug 1174506 for another regression, MOZ_APP_VERSION_ABOUT isn't defined in android builds.
Comment 28•9 years ago
|
||
Bug 1175331 for changes to release tagging and merge day code, to support the new format of browser/config/version.txt. The tagging one may block uplift to beta, if you were planning on automated update of the about: version.
Assignee | ||
Comment 29•9 years ago
|
||
> A variable name like __MOZ_APP_VERSION_DISPLAY__ may have been a better
> option in case there is another place in the UI that we choose to display
> the version.
Yeh, I am planning to rename it.
Assignee | ||
Updated•9 years ago
|
Updated•7 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•