Closed Bug 383167 Opened 17 years ago Closed 17 years ago

need build id in an external file

Categories

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

defect

Tracking

(Not tracked)

RESOLVED FIXED
mozilla1.9alpha8

People

(Reporter: rcampbell, Assigned: benjamin)

References

Details

Attachments

(7 files, 2 obsolete files)

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.
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?
Assignee: nobody → benjamin
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. :)
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.
Attached patch platform.iniSplinter Review
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.
Blocks: 384174
Attachment #268237 - Flags: superreview?(dbaron)
Attachment #268237 - Flags: review?(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.
Attachment #268237 - Flags: review?(ted.mielczarek) → review-
Attachment #268237 - Attachment is obsolete: true
Attachment #269090 - Flags: superreview?(dbaron)
Attachment #269090 - Flags: review?(ted.mielczarek)
Attachment #268237 - Flags: superreview?(dbaron)
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.
Attachment #269090 - Flags: review?(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.
Attachment #269090 - Flags: review?(jst) → review+
Attached patch SupplementSplinter Review
oops, forgot Makefile.in in the last patch
Comment on attachment 269118 [details] [diff] [review]
Supplement

You need to kill the BUILDID= line at the end of the Makefile, too.
Attachment #269118 - Flags: review+
Comment on attachment 269090 [details] [diff] [review]
Firefox buildid-in-a-file, rev. 2

Please also fix tinderbox as indicated in my last review.
Attachment #269090 - Flags: review?(ted.mielczarek) → review+
Attachment #269526 - Flags: review?(rhelmer) → review+
Attachment #269090 - Flags: superreview?(dbaron)
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.
Attached patch Fix the UA string, rev. 1 (obsolete) — Splinter Review
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.
Attachment #269845 - Flags: review?(cbiesinger)
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.
Attachment #269845 - Attachment is obsolete: true
Attachment #269849 - Flags: review?(cbiesinger)
Attachment #269845 - Flags: review?(cbiesinger)
Attachment #269849 - Flags: review?(cbiesinger) → review+
FF on Win32/XP wouldn't start with this checked in at 2007-06-26 09:35 and 2007-06-26 11:10
So we've got no talos perf numbers since Jun 11.  We need to get this fix in or back out the original change.  
Blocks: 386081
I tried to land this Monday and Tuesday and caused Linux orange. I'll debug that and try to get this landed again Monday.
Priority: -- → P1
Target Milestone: --- → mozilla1.9beta1
Flags: blocking1.9+
Attachment #270445 - Flags: review?(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?
Attachment #270445 - Flags: review?(ted.mielczarek) → review+
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.
Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
--NEW-LEAKS-----------------------------------leaks------leaks%-----------------------
nsLocalFile                                     240     100.00%
nsStringBuffer                                   32      33.33%
TOTAL                                           272
Depends on: 386740
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 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?
Presumably Sunbird would need a matching fix, yes.
Comment on attachment 269090 [details] [diff] [review]
Firefox buildid-in-a-file, rev. 2

>+  aBuildID.Truncate();
>+  AppendASCIItoUTF16(buildID, aBuildID);
CopyASCIItoUTF16(buildID, aBuildID);
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
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
Roy: that's already filed as bug 386658
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?
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?
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".
Depends on: 387726
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.
Depends on: 387163
Depends on: 392204
Flags: in-testsuite-
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.