Closed Bug 1174506 Opened 5 years ago Closed 5 years ago

Version and build date are no longer included on about: page of Android Nightly builds

Categories

(Firefox for Android :: General, defect, blocker)

ARM
Android
defect
Not set
blocker

Tracking

()

RESOLVED FIXED
Firefox 41
Tracking Status
firefox41 --- fixed
fennec 41+ ---

People

(Reporter: wgianopoulos, Assigned: Sylvestre)

References

Details

Attachments

(1 file, 5 obsolete files)

Build date is no longer included on about: page on Android Nightly builds.
Still present in the 2015-06-12 build, on m-c rev 0093691d3715. Looking for the regression window.
Summary: Build date is no longer included on about: page on Android Nightly builds → Version and build date are no longer included on about: page of Android Nightly builds
(In reply to Nick Thomas [:nthomas] from comment #3)
> Lets try that regression window again
> http://hg.mozilla.org/mozilla-central/
> pushloghtml?fromchange=0093691d3715&tochange=b7ee8e13145a

Based on that window I would suspect Bug 1145175.
OH I guess you already figured that out.
Looks like MOZ_APP_VERSION_ABOUT isn't defined on Android. This is a (partial) diff of OK and busted builds, apk and omni.ja unzipped:

diff -rU8 0612/assets/chrome/chrome/content/about.js 0613/assets/chrome/chrome/content/about.js
--- 0612/assets/chrome/chrome/content/about.js  2010-01-01 00:00:00.000000000 +1300
+++ 0613/assets/chrome/chrome/content/about.js  2010-01-01 00:00:00.000000000 +1300
@@ -4,17 +4,17 @@

 let Ci = Components.interfaces, Cc = Components.classes, Cu = Components.utils, Cr = Components.results;
 Cu.import("resource://gre/modules/XPCOMUtils.jsm");
 Cu.import("resource://gre/modules/Services.jsm");

 function init() {
   // Include the build date and a warning about Telemetry
   // if this is an "a#" (nightly or aurora) build
-const version = "41.0a1";
+const version = "";
   if (/a\d+$/.test(version)) {
     let buildID = Services.appinfo.appBuildID;
     let buildDate = buildID.slice(0, 4) + "-" + buildID.slice(4, 6) + "-" + buildID.slice(6, 8);
     let br = document.createElement("br");
     let versionPara = document.getElementById("version");
     versionPara.appendChild(br);
     let date = document.createTextNode("(" + buildDate + ")");
     versionPara.appendChild(date);

diff -ru 0612/assets/chrome/chrome/content/about.xhtml 0613/assets/chrome/chrome/content/about.xhtml
--- 0612/assets/chrome/chrome/content/about.xhtml       2010-01-01 00:00:00.000000000 +1300
+++ 0613/assets/chrome/chrome/content/about.xhtml       2010-01-01 00:00:00.000000000 +1300
@@ -24,7 +24,7 @@
 <body dir="&locale.dir;">
   <div id="header">
     <div id="wordmark"></div>
-<p id="version">41.0a1</p>
+<p id="version"></p>
   </div>

   <div id="banner">
tracking-fennec: --- → ?
That is indeed my fault. Working on it.
Assignee: nobody → sledru
Attached patch bug-1174506.diff (obsolete) — Splinter Review
This fixes it.
To took the liberty to replace that MOZ_APP_VERSION=41.0a1 by the variable.
Attachment #8623025 - Flags: review?(mh+mozilla)
(In reply to Sylvestre Ledru [:sylvestre] from comment #8)
> Created attachment 8623025 [details] [diff] [review]
> bug-1174506.diff
> 
> This fixes it.
> To took the liberty to replace that MOZ_APP_VERSION=41.0a1 by the variable.

I realize no one asked for feedback, but I can report this DOES fix the issue I originally reported.
Comment on attachment 8623025 [details] [diff] [review]
bug-1174506.diff

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

::: mobile/android/confvars.sh
@@ +4,5 @@
>  
>  MOZ_APP_BASENAME=Fennec
>  MOZ_APP_VENDOR=Mozilla
>  
> +MOZ_APP_VERSION=$FIREFOX_VERSION

Since the version is currently hardcoded, I'm going to assume something is updating it after merges. If that something is not a person, they're going to break.

@@ +5,5 @@
>  MOZ_APP_BASENAME=Fennec
>  MOZ_APP_VENDOR=Mozilla
>  
> +MOZ_APP_VERSION=$FIREFOX_VERSION
> +MOZ_APP_VERSION_ABOUT=$FIREFOX_VERSION_ABOUT

Come to think of it, it would be better if MOZ_APP_VERSION_ABOUT was falling back to use MOZ_APP_VERSION if it's not explicitly set. The large comment describing the various variables like MOZ_APP_VERSION should be updated too.
Attachment #8623025 - Flags: review?(mh+mozilla) → feedback+
Bug 1175331 tracks changes needed in merging & release tagging code.
Blocks: 1175331
Severity: normal → major
Attached file foo (obsolete) —
> Come to think of it, it would be better if MOZ_APP_VERSION_ABOUT was falling back to use MOZ_APP_VERSION if it's not explicitly set.
Indeed. I implemented in this patch.
Let me know if you prefer a separate bug

> The large comment describing the various variables like MOZ_APP_VERSION should be updated too.
Sure, where is it?
Attachment #8623025 - Attachment is obsolete: true
Attachment #8623517 - Flags: review?(mh+mozilla)
(In reply to Sylvestre Ledru [:sylvestre] from comment #12)
> > The large comment describing the various variables like MOZ_APP_VERSION should be updated too.
> Sure, where is it?

https://dxr.mozilla.org/mozilla-central/source/configure.in#8684
Attached patch foo (obsolete) — Splinter Review
With the doc updated
Attachment #8623517 - Attachment is obsolete: true
Attachment #8623517 - Flags: review?(mh+mozilla)
Attachment #8623547 - Flags: review?(mh+mozilla)
Attachment #8623547 - Attachment is patch: true
Comment on attachment 8623547 [details] [diff] [review]
foo

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

::: configure.in
@@ +1965,5 @@
>      AC_MSG_ERROR([FIREFOX_VERSION is unexpectedly blank.])
>  fi
>  
>  if test -z "$FIREFOX_VERSION_ABOUT"; then
> +    AC_MSG_WARN([FIREFOX_VERSION_ABOUT is unexpectedly blank. Using FIREFOX_VERSION as version])

AC_MSG_ERROR was fine here, it's not supposed to happen, since that's reading a fixed file in the tree.

What I was referring to is MOZ_APP_VERSION_ABOUT, which is only set in firefox's confvars.sh. So it's effectively empty for *other* apps (think seamonkey, thunderbird...)

@@ +8696,5 @@
>  # Mac Bundle name, Updater, Installer), it is typically used for nightly
>  # builds (e.g. Aurora for Firefox).
>  # - MOZ_APP_VERSION: Defines the application version number.
> +# - MOZ_APP_VERSION_ABOUT: Defines the application version number. Used
> +# in the "About" Window. If not found, MOZ_APP_VERSION will be used.

Which is not true :)

::: mobile/android/confvars.sh
@@ +5,5 @@
>  MOZ_APP_BASENAME=Fennec
>  MOZ_APP_VENDOR=Mozilla
>  
> +MOZ_APP_VERSION=$FIREFOX_VERSION
> +MOZ_APP_VERSION_ABOUT=$FIREFOX_VERSION_ABOUT

Same comment as before. Make sure this doesn't break whatever updates it.
Attachment #8623547 - Flags: review?(mh+mozilla) → feedback-
Attached patch bug-version (obsolete) — Splinter Review
is it what you are suggesting?
Attachment #8623588 - Flags: review?(mh+mozilla)
Attachment #8623588 - Attachment is patch: true
(In reply to Sylvestre Ledru [:sylvestre] from comment #16)
> Created attachment 8623588 [details] [diff] [review]
> bug-version
> 
> is it what you are suggesting?

Nope. What I'm suggesting is that after confvars.sh/configure.sh are read, if MOZ_APP_VERSION_ABOUT is not set, then set it from MOZ_APP_VERSION. That would match what the description says, and would match what is done for e.g. MOZ_APP_REMOTINGNAME
tracking-fennec: ? → 41+
So, sorry I think this is more important than you do, but with automatic updates being disabled for android nitghtlies it is important for nightly testers who don't understand abut:buildconfig or changeset IDs to figure out what day's nightly they are running when they report issues.
Severity: major → blocker
I will fix that tomorrow with Mike's comments, however, attachment 8623025 [details] [diff] [review] is good enough to be merged if urgent
(In reply to Sylvestre Ledru [:sylvestre] from comment #19)
> I will fix that tomorrow with Mike's comments, however, attachment 8623025 [details] [diff] [review]
> [details] [diff] [review] is good enough to be merged if urgent

NO, just trying to point out that this in conjunction with no automatic updating is impacting proper nightly user testing and reporting issues.  Trying to make people who don't get it this is more of an issue than cosmetics.
(In reply to Bill Gianopoulos [:WG9s] from comment #20)
> (In reply to Sylvestre Ledru [:sylvestre] from comment #19)
> > I will fix that tomorrow with Mike's comments, however, attachment 8623025 [details] [diff] [review]
> > [details] [diff] [review] is good enough to be merged if urgent
> 
> NO, just trying to point out that this in conjunction with no automatic
> updating is impacting proper nightly user testing and reporting issues. 
> Trying to make people who don't get it this is more of an issue than
> cosmetics.

My feeling is if this wee an issue on desktop, the tree would be closed.
Don't worry, I understood your point when you updated the severity twice. ;)
(In reply to Sylvestre Ledru [:sylvestre] from comment #22)
> Don't worry, I understood your point when you updated the severity twice. ;)

I know but some people don't get with the issues form the 2 bugs we only have automated testing and the normal real nightly users I am not sure are either really testing or can reliably tell us what version they are seeing the issue on.

We need to get these properly time-stamped and have nightlies properly updating ASAP.
Attached patch foo.diff (obsolete) — Splinter Review
here it is
Attachment #8623547 - Attachment is obsolete: true
Attachment #8623588 - Attachment is obsolete: true
Attachment #8623588 - Flags: review?(mh+mozilla)
Attachment #8624667 - Flags: review?(mh+mozilla)
Comment on attachment 8624667 [details] [diff] [review]
foo.diff

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

::: configure.in
@@ +8695,5 @@
>  # Mac Bundle name, Updater, Installer), it is typically used for nightly
>  # builds (e.g. Aurora for Firefox).
>  # - MOZ_APP_VERSION: Defines the application version number.
> +# - MOZ_APP_VERSION_ABOUT: Defines the application version number. Used
> +# in the "About" window.

If not set, defaults to MOZ_APP_VERSION.

@@ +8715,5 @@
>     MOZ_APP_REMOTINGNAME=$MOZ_APP_NAME
>  fi
>  
> +if test -z "$MOZ_APP_VERSION_ABOUT"; then
> +   MOZ_APP_VERSION_ABOUT=$MOZ_APP_NAME

$MOZ_APP_VERSION

::: mobile/android/confvars.sh
@@ +5,5 @@
>  MOZ_APP_BASENAME=Fennec
>  MOZ_APP_VENDOR=Mozilla
>  
> +MOZ_APP_VERSION=$FIREFOX_VERSION
> +MOZ_APP_VERSION_ABOUT=$FIREFOX_VERSION_ABOUT

cf. irc discussion, please leave this file alone.
Attachment #8624667 - Flags: review?(mh+mozilla) → feedback+
Attached patch foo.diffSplinter Review
sure :)
Attachment #8624667 - Attachment is obsolete: true
Attachment #8624676 - Flags: review?(mh+mozilla)
Attachment #8624676 - Flags: review?(mh+mozilla) → review+
https://hg.mozilla.org/mozilla-central/rev/8dabe64cdd24
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
You need to log in before you can comment on or make changes to this bug.