383167 - need build id in an external file

Status: RESOLVED FIXED

(Firefox Build System :: General, defect, P1)

Description

[Rob Campbell [:rc] (:robcee)]

Currently, on builds shipping with talkback, we're using the master.ini file to grab the id of the build in external tests (e.g., firefox performance tests). When talkback is removed, we'll lose this.

We'd like to have this value stored somewhere accessible outside the application. Putting it in an ini file or just a text file would be ideal.

Comment 1

[Benjamin Smedberg]

While I'm at it, I'd like to make the buildid unique. The idea I floated on mozilla.dev.quality was

YYYYMMDDHH-machinename-branchname

Ray Kiddy suggested adding a GUID:

YYYYMMDDHH-machinename-branchname-GUID

I also have been pondering whether the locale should be encoded as well:
YYYYMMDDHH-machinename-branchname-locale-GUID
That would of course require altering the buildid during locale repackaging, which is relatively easy but I want to think about the implications a bit more.

Right now I know we have buildid dependencies in AUS and probably in the tinderbox client code... do we have others?

Comment 2

[Rob Campbell [:rc] (:robcee)]

those are the biggies that I'm aware of, but there are probably others. I saw the thread on m.d.q and am not really sure a GUID buys us any more uniqueness there. Also, which machinename are you thinking of including? Builder?

Looks like the timing on this bug was right on. :)

Comment 3

[Benjamin Smedberg]

yeah... although that shows that we have delimeter issues. How about using colons instead:

2007060405:fx-linux-tbox:trunk:en-US for nightly trunk builds
2007060405:fx-linux-tbox:trunk-release:en-US for alpha release builds
2007060405:argo-vm:1.8:en-US for nightly branch

As for the GUID, the defaults for buildid would probably be pretty generic:

2007060417:unspecified:unspecified:en-US

The GUID might provide some additional uniqueness, although I'm not sure that telling various self-built creations apart is all that important.

Comment 4

[Benjamin Smedberg]

Attachment: platform.ini

This patch does a bunch of stuff, mainly having to do with getting rid of nsBuildID.h. All references to the platform or application buildid are gated through nsIXULAppInfo, which is loaded from application.ini and platform.ini.

This also loads the firefox xulappinfo from application.ini instead of hardcoding it in the binary.

For the moment, I am forcing the app buildid to be the same as the platform buildid. I'm not sure what we want to do about that.

Comment 5

[Benjamin Smedberg]

Attachment: Firefox buildid-in-a-file, rev. 1

Comment 6

[(not currently active) Ted Mielczarek]

Comment on attachment 268237 [details] [diff] [review]
Firefox buildid-in-a-file, rev. 1

>Index: browser/app/Makefile.in
>+# GRE_MILESTONE is only available in platform.ini in a form that we can't use
> # directly. So munge it. Beware makefile and shell escaping

Yuck.  Ok, so I realize you're just replacing one awk mess with another, but is there a way to make this better?  Your make-platformini.py and the associated code removal are fantastic, seems a shame to keep stuff like this.  At least file a follow-up on it.

>Index: browser/app/application.ini
> [Crash Reporter]
>-Enabled=0
>+#if MOZILLA_OFFICIAL
>+#if XP_WIN
>+Enabled=1
>+#elif XP_MACOSX
>+Enabled=1
>+#endif
>+#endif
> ServerURL=https://crash-reports.mozilla.com/submit

Does our preprocessor not support #if defined(XP_WIN) || defined(XP_MACOSX) or some construct like that?

Still gets r-, but only because you'll break this:
http://mxr.mozilla.org/mozilla/source/Makefile.in#206
And tinderbox:
http://mxr.mozilla.org/mozilla/source/tools/tinderbox/post-mozilla-rel.pl#439

Otherwise, looks good.

Comment 7

[Benjamin Smedberg]

Attachment: Firefox buildid-in-a-file, rev. 2

Comment 8

[Benjamin Smedberg]

Comment on attachment 269090 [details] [diff] [review]
Firefox buildid-in-a-file, rev. 2

jst, the major question I want your input on is whether the "navigator.buildID" property ought to be returning the application buildid or the platform buildid. Right now for Firefox they are always the same, but for other apps including XR apps they can be different.

Comment 9

[Johnny Stenback (:jst)]

Comment on attachment 269090 [details] [diff] [review]
Firefox buildid-in-a-file, rev. 2

The reason for navigator.buildID existing is to make identifying the app easier for testers, and for that purpose I would imagine the app id is more relevant than the platform id, so r=jst for that part of the patch.

Comment 10

[Benjamin Smedberg]

Attachment: Supplement

oops, forgot Makefile.in in the last patch

Comment 11

[(not currently active) Ted Mielczarek]

Comment on attachment 269118 [details] [diff] [review]
Supplement

You need to kill the BUILDID= line at the end of the Makefile, too.

Comment 12

[(not currently active) Ted Mielczarek]

Comment on attachment 269090 [details] [diff] [review]
Firefox buildid-in-a-file, rev. 2

Please also fix tinderbox as indicated in my last review.

Comment 13

[Benjamin Smedberg]

Attachment: Fix the tinderbox with additional checks for the new location, rev. 1

Comment 14

[Benjamin Smedberg]

Attachment: Fix for unified builds buildate unification

Comment 15

[Benjamin Smedberg]

I'm going to back this out: there were some additional usages of the old buildid code which I didn't catch before, and I don't have time to track them all right now. I'll try again tomorrow.

Comment 16

[Benjamin Smedberg]

Attachment: Fix the UA string, rev. 1

Biesi, this loads the UA string "Gecko/YYYYMMDDHH" from nsIXULAppInfo. Really though, I'd prefer to remove the Gecko/* string altogether, but I don't want to do that yet because I haven't got an alternate solution in place to show the buildid in the app UI.

Comment 17

[Christian :Biesinger (don't email me, ping me on IRC)]

Why does the patch work, given that toolkit is in a different tier than netwerk?

Also, I'd prefer not removing the http-startup category/topic, there may be extensions using it.

Comment 18

[Benjamin Smedberg]

Attachment: Fix the UA string, rev. 1.1

Comment 19

[Peter van der Woude [:Peter6]]

FF on Win32/XP wouldn't start with this checked in at 2007-06-26 09:35 and 2007-06-26 11:10

Comment 20

[Mike Schroepfer]

So we've got no talos perf numbers since Jun 11.  We need to get this fix in or back out the original change.  

Comment 21

[Benjamin Smedberg]

I tried to land this Monday and Tuesday and caused Linux orange. I'll debug that and try to get this landed again Monday.

Comment 22

[Benjamin Smedberg]

Attachment: Actually package the xpcom/system xpts, rev. 1

Comment 23

[(not currently active) Ted Mielczarek]

Comment on attachment 270445 [details] [diff] [review]
Actually package the xpcom/system xpts, rev. 1

Does it always come down to packaging manifests?

Comment 24

[Benjamin Smedberg]

Fixed on trunk, and everything except for Camino is green. Camino is known bustage, I mailed the devs about it a while back and they're going to have to update their use of nsBuildID.h in at least one place.

Comment 25

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

--NEW-LEAKS-----------------------------------leaks------leaks%-----------------------
nsLocalFile                                     240     100.00%
nsStringBuffer                                   32      33.33%
TOTAL                                           272

Comment 26

[[e-mail]]

Comment on attachment 269090 [details] [diff] [review]
Firefox buildid-in-a-file, rev. 2

>+#if defined(XP_WIN) && !MOZ_WINCONSOLE
This isn't going to work because you don't define MOZ_WINCONSOLE anywhere, it's only a Makefile variable.

Comment 27

[Stefan Sitter [:ssitter]]

Comment on attachment 270445 [details] [diff] [review]
Actually package the xpcom/system xpts, rev. 1

Does calendar also needs this fix or was it left out on purpose?

Comment 28

[Benjamin Smedberg]

Presumably Sunbird would need a matching fix, yes.

Comment 29

[[e-mail]]

Comment on attachment 269090 [details] [diff] [review]
Firefox buildid-in-a-file, rev. 2

>+  aBuildID.Truncate();
>+  AppendASCIItoUTF16(buildID, aBuildID);
CopyASCIItoUTF16(buildID, aBuildID);

Comment 30

[Walter Meinl]

benjamin, I'm pretty much sure that the bustage of FF on Xulrunner tinderbox (no rule to make target 'export') http://tinderbox.mozilla.org/showlog.cgi?log=MozillaExperimental/1183511460.5275.gz
is due to
http://lxr.mozilla.org/mozilla/source/browser/app/Makefile.in#386
the endif coming now to late after you checked in the patch. Before it was connected to ifdef LIBXUL_SDK, now probably to ifndef LIBXUL_SDK.
I can rescue the build here when I move the endif up, e.g. after http://lxr.mozilla.org/mozilla/source/browser/app/Makefile.in#111

Comment 31

[Roy Tam]

nsBrowserApp.cpp Version 1.44
+  vsnprintf(msg, sizeof(msg), fmt, ap);
This statement cause compilation error with MSVC 7.1.

Proposed fix:
  * ***** END LICENSE BLOCK ***** */
 
 #include "nsXULAppAPI.h"
 #ifdef XP_WIN
 #include <windows.h>
 #include <stdlib.h>
+#if defined(__MSC_VER) && __MSC_VER <= 1310
+#define vsnprintf _vsnprintf
+#endif
 #endif

Comment 32

[(not currently active) Ted Mielczarek]

Roy: that's already filed as bug 386658

Comment 33

[Jerome Borsboom]

On my linux setup, the check-in from this bug breaks the install:

make -f client.mk DESTDIR=/usr/local/src/firefox-3.0-install install

Shouldn't the last 'install::' rule in the Makefile in mozilla/firefox/toolkit/xre at least have one dependency and a full path to install to?

Comment 34

[alexander :surkov (:asurkov)]

I get compile error in nsBrowserApp.cpp file:

c:/Mozilla\mozilla\browser\app\nsBrowserApp.cpp(60) : error C3861: 'vsnprintf': identifier not found, even with argument-dependent lookup
make[5]: *** [nsBrowserApp.obj] Error 2

stdio.h defines "_vsnprintf" function but there is no vsnprintf function. What's wrong?

Comment 35

[(not currently active) Ted Mielczarek]

That's bug 386658.

Comment 36

[Christian Persch (GNOME) (away; not receiving bug mail)]

After this checkin, the UA string in a gtkmozembed embedding app is missing the build ID: it contains only "Gecko" instead of (e.g.) "Gecko/20070714".

Comment 37

[Mark Mentovai]

You need to provide an nsIXULAppInfo component.  If you can't use the one in nsAppRunner.cpp (becuase you're not using nsAppRunner.cpp), you might want to follow the pattern in bug 386653, where we introduced one for Camino.