1145175 - Include the beta version in "About Firefox"

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Sylvestre Ledru [:Sylvestre]]

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)

Comment 1

[Sylvestre Ledru [:Sylvestre]]

Attachment: bug-1145175.diff

Ben, does that sound the correct place for you? Thanks

Comment 2

[[e-mail] (:bhearsum)]

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.

Comment 3

[Sylvestre Ledru [:Sylvestre]]

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 '''),

Comment 4

[Mike Hommey [:glandium]]

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

[Mike Hommey [:glandium]]

For example, the link in about:buildconfig for 38.0.5b3 is https://hg.mozilla.org/releases/mozilla-release/rev/f81271987769

Comment 6

[Sylvestre Ledru [:Sylvestre]]

(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

[Mike Hommey [:glandium]]

(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

[Mike Hommey [:glandium]]

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

[Mike Hommey [:glandium]]

(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

[(not currently active) Ted Mielczarek]

Sounds like glandium has this under control from the build system perspective.

Comment 11

[Sylvestre Ledru [:Sylvestre]]

Attachment: beta-show.diff

Is that what you proposed? Thanks

Comment 12

[Mike Hommey [:glandium]]

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.

Comment 13

[Sylvestre Ledru [:Sylvestre]]

Attachment: beta-show.diff

here it is. I am adding a new line in version.txt

Comment 14

[Mike Hommey [:glandium]]

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.

Comment 15

[Sylvestre Ledru [:Sylvestre]]

Attachment: beta-show.diff

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.

Comment 16

[Sylvestre Ledru [:Sylvestre]]

Attachment: beta.diff

Rebased

Comment 17

[Mike Hommey [:glandium]]

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

Comment 18

[Sylvestre Ledru [:Sylvestre]]

Thanks.
Fixed and pushed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3c6d24af0ac5

Comment 19

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/95b6ab6e9fa6

Comment 20

[Sylvestre Ledru [:Sylvestre]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/95b6ab6e9fa6

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3c6d24af0ac5
https://hg.mozilla.org/mozilla-central/rev/95b6ab6e9fa6

Comment 22

[Sylvestre Ledru [:Sylvestre]]

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)?

Comment 23

[Lawrence Mandel [:lmandel] (use needinfo)]

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+

Comment 24

[Phil Ringnalda (:philor)]

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.

Comment 25

[Mike Hommey [:glandium]]

(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

[Phil Ringnalda (:philor)]

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

[Nick Thomas [:nthomas] (UTC+12)]

See bug 1174506 for another regression, MOZ_APP_VERSION_ABOUT isn't defined in android builds.

Comment 28

[Nick Thomas [:nthomas] (UTC+12)]

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.

Comment 29

[Sylvestre Ledru [:Sylvestre]]

> 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.