Closed
Bug 452866
Opened 16 years ago
Closed 15 years ago
Teach Socorro/crash-stats about comm-central
Categories
(MailNews Core :: Build Config, defect, P3)
MailNews Core
Build Config
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: standard8, Assigned: gozer)
Details
(Whiteboard: [notacrash])
Attachments
(3 files, 5 obsolete files)
555 bytes,
patch
|
Details | Diff | Splinter Review | |
6.66 KB,
patch
|
ted
:
review+
|
Details | Diff | Splinter Review |
10.31 KB,
patch
|
Details | Diff | Splinter Review |
I've just been able to see some crash reports from Thunderbird since it moved from cvs to comm-central. For instance: http://crash-stats.mozilla.com/report/index/8c2f4059-756b-11dd-9e81-001321b13766?date=2008-08-29-01 On the report, only files that are in mozilla-central have links. I'm assuming that crash-stats doesn't know about the files in comm-central and therefore doesn't show the links for them.
Comment 2•16 years ago
|
||
See bug 440074, where we did this for m-c.
Comment 3•16 years ago
|
||
Hm, it looks like the symbolstore script might not be handling your source files correctly. Compare: 0|1|thunderbird.exe|nsTreeSelection::RangedSelect(int,int,int)|hg:hg.mozilla.org/mozilla-central:layout/xul/base/src/tree/src/nsTreeSelection.cpp:5e208ee3e1cc|433|0xc vs: 0|2|thunderbird.exe|nsAbView::ReselectCards(nsIArray *,nsIAbCard *)|e:/builds/buildbot/comm-central-win32-nightly/build/mailnews/addrbook/src/nsAbView.cpp|1056|0xc (two frames in a row from your linked report). Note that the mailnews code doesn't have its filename mangled nicely.
Reporter | ||
Comment 4•16 years ago
|
||
(In reply to comment #3) > Hm, it looks like the symbolstore script might not be handling your source > files correctly. Compare: ... So it looks like there might be two problems here: 1) Socorro needs teaching about comm-central in the same way as bug 440074 2) make buildsymbols isn't throwing out the right data for Socorro to use. I'm going to pass this to gozer to take a look at.
Assignee: nobody → gozer
Comment 5•16 years ago
|
||
Yep, exactly. We should be able to preemptively put the comm-central bits into the server config, but that's not going to do you any good if the source files aren't coming out in the right format. Actually, I think I see your problem, although I'm not sure how to fix it. When we run buildsymbols, we pass $(topsrcdir) to the symbolstore.py script from the top-level (mozilla-central) Makefile.in: http://mxr.mozilla.org/mozilla-central/source/Makefile.in#187 When we check to see if files are in a Hg repo in the script, we only check $srcdir/.hg: http://mxr.mozilla.org/mozilla-central/source/toolkit/crashreporter/tools/symbolstore.py#359 Since your files are in $srcdir/../, they're never getting found here. I guess we could add another else condition there, that just took the filename and walked up each directory in its pathname, looking for another .hg repo? It wouldn't be a great solution, but it'd work.
Reporter | ||
Comment 6•16 years ago
|
||
Ok, from comment 3 and comment 5 this is more a mailnews build config problem in the first instance, so moving for now but keeping gozer on the assigned in the hopes that he can look at this sometime.
Component: Socorro → Build Config
Priority: -- → P3
Product: Webtools → MailNews Core
QA Contact: socorro → build-config
Comment 7•16 years ago
|
||
bug 462159 is about a similar problem, where source/header files in the objdir wind up with source links to hgweb.
Assignee | ||
Comment 8•16 years ago
|
||
(In reply to comment #4) > [...] > So it looks like there might be two problems here: > > 1) Socorro needs teaching about comm-central in the same way as bug 440074 That's bug 468965 now
No longer depends on: 468965
Assignee | ||
Comment 9•16 years ago
|
||
(In reply to comment #5) > [...] > Since your files are in $srcdir/../, they're never getting found here. I guess > we could add another else condition there, that just took the filename and > walked up each directory in its pathname, looking for another .hg repo? It > wouldn't be a great solution, but it'd work. A better approach would be to add support for _multiple_ source directories to symbolstore,py and tell it about /foo/bar/comm-central _and_ /foo/bar/comm-central/mozilla and search them until a match for the file in question is found.
Assignee | ||
Comment 10•16 years ago
|
||
This change adds support for multiple source (-s) directories to symbolstore.py. They are searched in order, from left to right, and the first one to contain version information for a file is picked. In this example, for comm-central, you'd specify: -s comm-central/mozilla -s comm-central
Comment 11•16 years ago
|
||
I thought up that exact same solution, but apparently never mentioned it here. Glad to see you sorted it out. :)
Assignee | ||
Comment 12•16 years ago
|
||
It works somewhat as you'd expect if invoked directly, I've been able to see hg.mozilla.org/comm-central references in generated .sym files. The next thing I'd need to sort out is how to change mozilla/Makefile.in:buildsymbols so that comm-central/Makefile.in:buildsymbols could add it's own source path in there, when building comm-central. But this patch/fix could land on it's own.
Comment 13•16 years ago
|
||
We could add an $(EXTRA_SYM_STORE_ARGS) variable to m-c's Makefile.in to let you pass arbitrary arguments in there, and just put it before all the other args on the symbolstore.py commandline. Would that work for you?
Assignee | ||
Comment 14•16 years ago
|
||
Hrm, positioning of such a variable always caused me headaches. How about a more direct solution? ${SYM_STORE_SOURCE_DIRS} ? Pseudo-code would end up something like: <comm-central/Makefile> SYM_STORE_SOURCE_DIRS="comm-central/mozilla:comm-central/" <comm-central/mozilla/Makefile> SYM_STORE_SOURCE_DIRS="." IFS=":" for p in $SYM_STORE_SOURCE_DIRS; do SYM_STORE_SOURCE_ARGS="$SYM_STORE_SOURCE_ARGS -s $p" done
Comment 15•16 years ago
|
||
more like $(foreach dir,$(SYM_STORE_SOURCE_DIRS),-s $dir), and SYM_STORE_SOURCE_DIRS=$topsrcdir/mozilla $topsrcdir I think should do the trick.
Assignee | ||
Comment 16•16 years ago
|
||
Yes, for some reason, I had my shell hat on, not my make one. So, do you want me to make such a patch and submit it for review as part of this bug or a new one? Second question, should I keep the existing patch and this coming one separate or combined ?
Comment 17•16 years ago
|
||
Might as well do it in this bug. You're welcome to do a new patch or combine them, whatever's easiest for you.
Comment 18•16 years ago
|
||
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1b3pre) Gecko/20081211 SeaMonkey/2.0a3pre - Build ID: 20081211000454 When I crash, Breakpad tells me "There was a problem sending your crash data". I think it's because Breakpad/Socorro don't know about SeaMonkey 2.0a3pre. Should I file a new bug or is it part of this one?
Comment 19•16 years ago
|
||
Socorro doesn't care about what your app thinks its version is. If your report failed it was probably a network issue.
Comment 20•16 years ago
|
||
(In reply to comment #19) > Socorro doesn't care about what your app thinks its version is. If your report > failed it was probably a network issue. Hm. Recently it's every time, including a few minutes ago, and my connection was up (I didn't need to reconnect before posting these comments). -- Ah well...
Assignee | ||
Comment 21•16 years ago
|
||
Attachment #352386 -
Attachment is obsolete: true
Attachment #353063 -
Flags: review?
Assignee | ||
Updated•16 years ago
|
Attachment #353063 -
Flags: review? → review?(ted.mielczarek)
Assignee | ||
Comment 22•16 years ago
|
||
This patch, assumes the previous one (attachment353063 [details] [diff] [review]) has already landed. It just tells buildsymbols about the other hg repos it should scan as source directories when building symbols.
Attachment #353064 -
Flags: review?(ted.mielczarek)
Comment 23•16 years ago
|
||
Comment on attachment 353063 [details] [diff] [review] Add support for multiple source directories to buildsymbols +SYM_STORE_SOURCE_ARGS = $(foreach dir,$(SYM_STORE_SOURCE_DIRS),-s $(dir)) + Just put the $(foreach) inline in the rule commands. You can put it on its own line if it gets too long. +def GetVCSFilename(file, srcdirs): """Given a full path to a file, and the top source directory, Modify the comment here to mention multiple source dirs, please. + for srcdir in srcdirs: + if os.path.isdir(os.path.join(path, "CVS")): + fileInfo = CVSFileInfo(file, srcdir) + if fileInfo: + root = fileInfo.root Can you break out of the loop early after the if blocks if fileInfo is set, or do you rely on it finding things in the last srcdir in the list? if srcdir is not None: - self.srcdir = os.path.normpath(srcdir) + self.srcdir = [os.path.normpath(a) for a in srcdir] With optparse and the "append" action, does srcdir wind up as None or just [] if it's not specified? Also, can you change the member variable name to |self.srcdirs| to be clearer? I think there are a few other comments in the file that didn't make it into your patch context that mention srcdir. Please fix those up as well. Looks like you'll break this Solaris-only case, please try to fix it (although it's ok if you can't test it): http://mxr.mozilla.org/mozilla-central/source/toolkit/crashreporter/tools/symbolstore.py#513
Attachment #353063 -
Flags: review?(ted.mielczarek) → review+
Updated•16 years ago
|
Attachment #353064 -
Flags: review?(ted.mielczarek) → review+
Comment 24•16 years ago
|
||
Comment on attachment 353064 [details] [diff] [review] Tell buildsymbols about comm-central & mozilla-central for mozilla-central builds $(topsrcdir)/mozilla $(topsrcdir) " $@ If that extra space at the end of the quoted string is not necessary, please remove it.
Assignee | ||
Comment 25•16 years ago
|
||
Address comment 24
Attachment #353064 -
Attachment is obsolete: true
Attachment #353524 -
Flags: review?(ted.mielczarek)
Assignee | ||
Comment 26•16 years ago
|
||
(In reply to comment #23) > (From update of attachment 353063 [details] [diff] [review]) > +SYM_STORE_SOURCE_ARGS = $(foreach dir,$(SYM_STORE_SOURCE_DIRS),-s $(dir)) > + > > Just put the $(foreach) inline in the rule commands. You can put it on its own > line if it gets too long. Fine, what's the standard for aligning the '\' character in continued lines ? > +def GetVCSFilename(file, srcdirs): > """Given a full path to a file, and the top source directory, > > Modify the comment here to mention multiple source dirs, please. > > + for srcdir in srcdirs: > + if os.path.isdir(os.path.join(path, "CVS")): > + fileInfo = CVSFileInfo(file, srcdir) > + if fileInfo: > + root = fileInfo.root > > Can you break out of the loop early after the if blocks if fileInfo is set, or > do you rely on it finding things in the last srcdir in the list? Nope, we stop at the first place we find something, but you had to follow the if/elseif chain to the end to see the break: if fileInfo: vcsFileInfoCache[file] = fileInfo break > if srcdir is not None: > - self.srcdir = os.path.normpath(srcdir) > + self.srcdir = [os.path.normpath(a) for a in srcdir] > > With optparse and the "append" action, does srcdir wind up as None or just [] > if it's not specified? It ends up None, unless specified default=[] > Also, can you change the member variable name to |self.srcdirs| to be clearer? Yes, that was part of the carefull, change as little as possible, process of implementing this. Renamed. > I think there are a few other comments in the file that didn't make it into > your patch context that mention srcdir. Please fix those up as well. I'll look for those. > Looks like you'll break this Solaris-only case, please try to fix it (although > it's ok if you can't test it): > http://mxr.mozilla.org/mozilla-central/source/toolkit/crashreporter/tools/symbolstore.py#513 Interesting, for some reason, it gets passed in full path names as opposed to relative ones ? Easily fixed.
Comment 27•16 years ago
|
||
(In reply to comment #26) > Fine, what's the standard for aligning the '\' character in continued lines ? I like them lined up out to whatever the longest line is. > Nope, we stop at the first place we find something, but you had to follow the > if/elseif chain to the end to see the break: Oops, missed that. > Interesting, for some reason, it gets passed in full path names as opposed to > relative ones ? Easily fixed. Yeah, I dunno, Solaris is weird.
Comment 28•16 years ago
|
||
Comment on attachment 353524 [details] [diff] [review] Tell buildsymbols about comm-central & mozilla-central for mozilla-central builds v2 FWIW, you don't need to request review if I r+ed with comments. Just carry forward the review when you fix what I asked for.
Attachment #353524 -
Flags: review?(ted.mielczarek) → review+
Assignee | ||
Comment 29•15 years ago
|
||
Update to latest comm-central status
Attachment #353524 -
Attachment is obsolete: true
Assignee | ||
Comment 30•15 years ago
|
||
Added all recommended changes.
Attachment #353063 -
Attachment is obsolete: true
Assignee | ||
Updated•15 years ago
|
Attachment #360935 -
Flags: review?(ted.mielczarek)
Comment 31•15 years ago
|
||
Comment on attachment 360935 [details] [diff] [review] Add support for multiple source directories to buildsymbols v2 if sys.platform == "sunos5": - start = filename.find(self.srcdir) + start = filename.find(self.srcdirs) You're going to need to fix this somehow. You could just hack it to be self.srcdirs[0] or something (but check |if self.srcdirs:| first?) - action="store", dest="srcdir", + action="append", dest="srcdir", default=[], Make that dest="srcdirs", and change the one reference to options.srcdir below to options.srcdirs.
Attachment #360935 -
Flags: review?(ted.mielczarek) → review+
Assignee | ||
Comment 32•15 years ago
|
||
This fixes the Solaris-specific code, but I am not entirely certain what it was for in the first place. The only thing it would ever do is strip a mysterious prefix from the source files. i.e. if srcdir=/home/gozer/mozilla-central filename=/home/gozer/mozilla-central/somefile The only time it would do something is if at one point filename=/mysterious/prefix/home/gozer/mozilla-central/somefile and would strip the /mysterious/prefix part. In any case, this patch preserves that behaviour across multiple source directories.
Attachment #360935 -
Attachment is obsolete: true
Attachment #361996 -
Flags: review?(ted.mielczarek)
Assignee | ||
Comment 33•15 years ago
|
||
Just for reference, the special sunos case I am talking about in comment #32 was introduced by bug 391361.
Updated•15 years ago
|
Attachment #361996 -
Flags: review?(ted.mielczarek) → review+
Comment 34•15 years ago
|
||
Comment on attachment 361996 [details] [diff] [review] [checked in] Add support for multiple source directories to buildsymbols v3 (mozilla-1.9.1 patch) Looks good, thanks.
Assignee | ||
Comment 35•15 years ago
|
||
Attachment 361996 [details] [diff] needs checkin-in in mozilla-central/mozilla-1.9.1 before attachment 360934 [details] [diff] [review] can land in comm-central.
Keywords: checkin-needed
Comment 36•15 years ago
|
||
Comment on attachment 361996 [details] [diff] [review] [checked in] Add support for multiple source directories to buildsymbols v3 (mozilla-1.9.1 patch) Does not apply to mozilla-central: { patching file toolkit/crashreporter/tools/symbolstore.py Hunk #3 FAILED at 348 Hunk #6 FAILED at 510 2 out of 8 hunks FAILED }
Updated•15 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 37•15 years ago
|
||
Original patch was based on mozilla-1.9.1, rebased to mozilla-central. Mostly whitespace changes.
Attachment #361996 -
Attachment is obsolete: true
Assignee | ||
Updated•15 years ago
|
Keywords: checkin-needed
Reporter | ||
Comment 38•15 years ago
|
||
Comment on attachment 362667 [details] [diff] [review] [checked in] Add support for multiple source directories to buildsymbols v4 (mozilla-central) Checked in: http://hg.mozilla.org/mozilla-central/rev/d2d0110cd69a
Attachment #362667 -
Attachment description: Add support for multiple source directories to buildsymbols v4 (mozilla-central) → [checked in] Add support for multiple source directories to buildsymbols v4 (mozilla-central)
Reporter | ||
Comment 39•15 years ago
|
||
This is now in on mozilla-central, lets give it a few hours before pushing to 1.9.1.
Keywords: checkin-needed
Assignee | ||
Comment 40•15 years ago
|
||
(In reply to comment #39) > This is now in on mozilla-central, lets give it a few hours before pushing to > 1.9.1. Just realized this didn't make it to 1.9.1 yet, that's unfortunate, as I just tagged for TB Beta 2 RC1. Can we try and get this landed soon, so we might catch it if/when we do RC2 ?
Reporter | ||
Updated•15 years ago
|
Attachment #361996 -
Attachment description: Add support for multiple source directories to buildsymbols v3 → Add support for multiple source directories to buildsymbols v3 (mozilla-1.9.1 patch)
Attachment #361996 -
Attachment is obsolete: false
Reporter | ||
Comment 41•15 years ago
|
||
Comment on attachment 361996 [details] [diff] [review] [checked in] Add support for multiple source directories to buildsymbols v3 (mozilla-1.9.1 patch) Un-obsoleting as this is required for mozilla-1.9.1
Reporter | ||
Comment 42•15 years ago
|
||
Comment on attachment 361996 [details] [diff] [review] [checked in] Add support for multiple source directories to buildsymbols v3 (mozilla-1.9.1 patch) Checked into 1.9.1: http://hg.mozilla.org/releases/mozilla-1.9.1/rev/0ed07e0df2ec
Attachment #361996 -
Attachment description: Add support for multiple source directories to buildsymbols v3 (mozilla-1.9.1 patch) → [checked in] Add support for multiple source directories to buildsymbols v3 (mozilla-1.9.1 patch)
Reporter | ||
Comment 43•15 years ago
|
||
Comment on attachment 360934 [details] [diff] [review] [checked in] Tell buildsymbols about comm-central & mozilla-central for mozilla-central builds v3 Checked in: http://hg.mozilla.org/comm-central/rev/ce0e94b8b433
Attachment #360934 -
Attachment description: Tell buildsymbols about comm-central & mozilla-central for mozilla-central builds v3 → [checked in] Tell buildsymbols about comm-central & mozilla-central for mozilla-central builds v3
Reporter | ||
Comment 44•15 years ago
|
||
This should be fixed now.
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Comment 45•15 years ago
|
||
(In reply to comment #42) > (From update of attachment 361996 [details] [diff] [review]) > Checked into 1.9.1: > http://hg.mozilla.org/releases/mozilla-1.9.1/rev/0ed07e0df2ec A little disingenuous, as this is in fact part of the build. (Not that it gets built into Firefox, but we certainly use it.)
Reporter | ||
Comment 46•15 years ago
|
||
Fresh off the press: http://crash-stats.mozilla.com/report/index/58b6007e-0e56-4cc2-8718-200fa2090219?p=1 We've got links for comm-central now :-) but the ones for mozilla-1.9.1 are missing, although most of the backend data seems to be there.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 47•15 years ago
|
||
I filed bug 479055 on that yesterday.
Status: REOPENED → RESOLVED
Closed: 15 years ago → 15 years ago
Resolution: --- → FIXED
Updated•15 years ago
|
Whiteboard: [notacrash]
You need to log in
before you can comment on or make changes to this bug.
Description
•