408134 - (srcsrv) Source Server for Windows builds integrated into symbolstore.py

Status: RESOLVED FIXED

(Firefox Build System :: General, enhancement)

Description

[Lukas Blakk [:lsblakk] use ?needinfo]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9b2pre) Gecko/2007120604 Minefield/3.0b2pre
Build Identifier: 

Here is a patch for symbolstore.py to gather up and index pdb files created when a debug-info build is made.  It uses pdbstr.exe which comes with the Debugging Tools for Windows and assumes that pdbstr.exe is in mozilla/toolkit/crashreporter/tools/win32

When make buildsymbols is called, this new version of symbolstore.py creates .stream files with the proper syntax for source indexed data blocks and calls on pdbstr to write it into the pdd files.

On the user end, your PATH must contain cvs.exe and in your debugger (Visual Studio or Windbg) you must enable source server as well.

Currently there is just one sample available to test out (an installer and a package of corresponding symbols) here:
http://crashopensource.wordpress.com/2007/12/12/03-release-of-the-source-server-for-mozilla-firefox/trackback/

Reproducible: Always

Steps to Reproduce:
1.
2.
3.

Comment 1

[Lukas Blakk [:lsblakk] use ?needinfo]

Attachment: Patch for symbolstore.py to enable source indexing with pdbstr.exe

Comment 2

[Benjamin Smedberg]

Quick question: when you say "your PATH must contain cvs.exe and ... you must enable source server as well", you mean if you want source-level debugging, right? If you don't want source-level debugging you could continue using the symbol server the exact same way as today?

Comment 3

[Benjamin Smedberg]

Oh, and I think this is rockingly awesome, in case I didn't make that clear...

Comment 4

[(not currently active) Ted Mielczarek]

Yeah, you have to explicitly enable source server support in your debugger.  See http://crashopensource.wordpress.com/2007/11/14/source-server-02-release-howto-and-contribution-opportunity/ for a step-by-step (with pictures!) guide for VS2k5.

Comment 5

[(not currently active) Ted Mielczarek]

Comment on attachment 292890 [details] [diff] [review]
Patch for symbolstore.py to enable source indexing with pdbstr.exe

This patch looks so small for all the work you put into it!  :-)

First, some general comments.  You're going to have to change this a bit so that the source indexing is controlled by a commandline argument, maybe something like --source-server.  We obviously can't do this on non-Win32 platforms!  Second, we're going to have to figure out what to do about pdbstr.exe, I don't think we can redistribute that, and having to have WinDBG installed on the Tinderboxes seems like overkill.  I may be able to hack up a small replacement program, I'll look into it.

>Index: toolkit/crashreporter/tools/symbolstore.py
>===================================================================
>+                            # gather up files with cvs for indexing

Ideally we'd be able to handle files from any VCS, but I think mixing VCSes is going to be hard given the way the source server works.  I would like to at least consider how we're going to make this work in Mozilla 2 land, with mercurial as the VCS.

>+                            if filename.startswith("cvs"):
>+                                (ver, checkout, source_file, revision) = filename.split(":", 3)
>+                                sourcefile_path = os.path.normpath(source_file)
>+                                sourcefile_path = os.path.join(self.srcdir, sourcefile_path)

If you save a copy of the filename variable right before we call GetVCSInfo, you won't need to jump through hoops to get the full path back.

>+                                # at the momment there is an "extra" mozilla in the path - so this .replace is a hack
>+                                sourceFileStream += sourcefile_path.replace("mozilla\\", "", 1) + "*MYSERVER*" + source_file + '*' + revision + "\r\n"

Have to figure out a cleaner way to do this, might be that my previous comment makes this unnecessary?

>+                    # Creates a .pdb.stream file in the mozilla\toolkit\crashreporter\tools\win32 dir where pdbstr is located

I'd like you to pull this out into a separate method.  Maybe we can make a method on Dumper called SourceIndex, or something like that, that takes a list of files (or (file, revision) maybe?) and it could be a no-op in the base class, but implemented like this in the Win32 class.  How does that sound?

>+                    pdbStreamFile.write("SRCSRV: ini ------------------------------------------------\r\n")

I think I mentioned before, but I think you should rewrite this as a triple-quoted string:
pdbStreamFile.write("""SRCSRV: ....
...."""

You can use string interpolation if you need to stick variables in the middle.

>+                    pdbStreamFile.write("VERSION=1\r\nINDEXVERSION=2\r\nVERCTRL=CVS\r\nDATETIME=Sat Dec  1 22:01:58 2007\r\n")
I think some of this data is probably unnecessary.  You should experiment some to see what we could remove.

>+                    pdbStreamFile.write("MYSERVER=:pserver:[e-mail]:/cvsroot\r\n")

We're definitely going to have to figure out a way to avoid hardcoding this here.  I'm not sure what the right solution is.


>                         shutil.copyfile(file, full_path)
>+                        # location of pdbstr.exe
>+                        pdbstr = r"c:\ff_clean\mozilla\toolkit\crashreporter\tools\win32\pdbstr.exe"

Obviously can't hardcode this either.  Probably the best solution would be if we get a pdbstr replacement checked in, to pass the path to the exe on the commandline.


>+                        # move to the dir with the stream files to call pdbstr
>+                        os.chdir(os.path.dirname(stream_output_path))
>+                        os.spawnv(os.P_WAIT, pdbstr, [pdbstr, "-w", "-p:" + pdb_filename, "-i:" + streamFilename, "-s:srcsrv"])

Maybe we can make this suck a bit less with a pdbstr replacement.  That tool is pretty crappy.

Overall, a great start.  Needs some work to get to a state where we can check it in, but I think we knew that.

Comment 6

[Lukas Blakk [:lsblakk] use ?needinfo]

Attachment: Work in progress on the patch to add source indexing

Okay, so I have made the following changes:

1.  Re-wrote the pdbStreamFile.write as a triple-quoted string 
2.  Removed unecessary information, and tested it to be sure it still worked (which it does from the indexing end of things)
3.  Pulled out the creating of the stream file to a separate method called SourceIndex

The issues I am still having are:

1.  The method is not being recognized - and I would appreciate tips on how to make that work - I get an error message that says the global name SourceIndex is not defined
2.  I did not move the gathering of filenames because as I see it, if I do this before the calling of GetVCSFilename then I have a list of all files and not just the ones that have cvs info.  Any thoughts or suggestions about this?
3.  I need clarification about the calling from the command line - is this something that would involve changes in the make buildsymbols command?  Any additional info about where to look for this, perhaps an example of something that does use calling different variables at the command line level?
4.  Obviously I cannot change the hardcoding of pdbstr.exe yet, once this patch is cleaned up I can start working on the pdbstr.exe replacement
5.  In terms of the cvs checkout username and password - what is the solution to something like that?  Doesn't everyone use the pserver:anonymous login?  Is there a reason to not have that in the code?

Okay.  I look forward to feedback.  Thanks.

Comment 7

[Lukas Blakk [:lsblakk] use ?needinfo]

Attachment: Current status of symbolstore.py patch

Okay, please disregard the previous comments, this is the current status:

Done:

1.  Re-wrote the pdbStreamFile.write as a triple-quoted string 
2.  Removed unecessary information, and tested it to be sure it still worked
(which it does from the indexing end of things)
3.  Pulled out the creating of the stream file to a separate method called
SourceIndex

The issues I am still having are:

1.  I did not move the gathering of filenames because as I see it, if I do this
before the calling of GetVCSFilename then I have a list of all files and not
just the ones that have cvs info.  Any thoughts or suggestions about this?
2.  I need clarification about the calling from the command line - is this
something that would involve changes in the make buildsymbols command?  Any
additional info about where to look for this, perhaps an example of something
that does use calling different variables at the command line level?
3.  Obviously I cannot change the hardcoding of pdbstr.exe yet, once this patch
is cleaned up I can start working on the pdbstr.exe replacement
4.  In terms of the cvs checkout username and password - what is the solution
to something like that?  Doesn't everyone use the pserver:anonymous login?  Is
there a reason to not have that in the code?

Okay.  I look forward to feedback.  Thanks.

Comment 8

[Lukas Blakk [:lsblakk] use ?needinfo]

Attachment: Replacement patch with checking for Win32

Okay - so I think that this covers most of the issues.  What's still hanging is the passing of the path for pdbstr.  Without knowing where this is going to be in the end, I am unsure how to pass this - can I use a global in Makefile?

Comment 9

[(not currently active) Ted Mielczarek]

Comment on attachment 302031 [details] [diff] [review]
Replacement patch with checking for Win32

Ok, getting much closer!

>Index: Makefile.in
>===================================================================
> # we want to copy PDB files on Windows
> MAKE_SYM_STORE_ARGS := -c
>+MAKE_SYM_STORE_ARGS += -i

You can just put all of those on one line:
MAKE_SYM_STORE_ARGS := -c -i

But actually, now that I think about it, we'll need to leave that out for the moment, since we won't be able to turn this on right away.  More on this at the end...

>Index: toolkit/crashreporter/tools/symbolstore.py
>===================================================================
>+import subprocess

I started to get worried about what version of Python we needed to support (subprocess is only in 2.4 or newer), but then I noticed you aren't actually using this anywhere.  Just remove this line if you're not using it.

>                             filename = self.FixFilenameCase(filename.rstrip())
>                             if self.vcsinfo:
>                                 filename = GetVCSFilename(filename, self.srcdir)
>+                            # gather up files with cvs for indexing   
>+                            if filename.startswith("cvs"):
>+                                (ver, checkout, source_file, revision) = filename.split(":", 3)
>+                                sourcefile_path = os.path.normpath(source_file)
>+                                sourcefile_path = os.path.join(self.srcdir, sourcefile_path)
>+                                # at the momment there is an "extra" mozilla in the path - so this .replace is a hack
>+                                sourceFileStream += sourcefile_path.replace("mozilla\\", "", 1) + "*MYSERVER*" + source_file + '*' + revision + "\r\n"

Pretty sure I mentioned this before, but what I'd like to see here is:
1) Right before |if self.vcsinfo:|, store |filename| in a temp variable, since that contains the full file path.  Then in your |if filename.startswith("cvs"):| block, just use that temp variable instead of |sourcefile_path| that you're jumping through hoops to construct.


>+                        if self.srcsrv:
>+                            # location of pdbstr.exe for now, until a replacement exists and is checked in 
>+                            pdbstr = r"c:\ff_clean\mozilla\toolkit\crashreporter\tools\win32\pdbstr.exe"

I think for now we should just require an environment variable to be set containing the full path to pdbstr.exe.  That will be easy to use until we figure out a replacement strategy.

>+                            pdb_rel_path = os.path.join(debug_file, guid, debug_file).replace("\\", "/")

Is this replace really needed?  Doesn't os.path.normpath on the next line just give you backslashes anyway?

>+                            pdb_filename = os.path.normpath(os.path.join(self.symbol_path, pdb_rel_path))


>+    parser.add_option("-i", "--source-index",
>+                      action="store_true", dest="srcsrv", default=False,
>+                      help="Runs pdbstr on the debug info files and adds source index information for debugging")

Make the doc string here say "Add source index information to debug files, making them suitable for use in a source server."

Since we're going to use an environment variable for pdbstr, you should check to make sure it's set and the file exists in main().  After parser.parse_args(), you can check |if options.srcsrv| and then check the existence of the env var and that the file it points to exists, and error if either of those is not true.

Fix those comments and your next patch gets an r+, I swear!

Comment 10

[(not currently active) Ted Mielczarek]

(In reply to comment #9)
> >Index: Makefile.in
> >===================================================================
> > # we want to copy PDB files on Windows
> > MAKE_SYM_STORE_ARGS := -c
> >+MAKE_SYM_STORE_ARGS += -i
> 
> You can just put all of those on one line:
> MAKE_SYM_STORE_ARGS := -c -i
> 
> But actually, now that I think about it, we'll need to leave that out for the
> moment, since we won't be able to turn this on right away.  More on this at the
> end...

Which I conveniently forgot to finish. What I'm thinking is that you can ifdef this second line based on the environment variable that points to pdbstr.exe. Let's just call it PDBSTR_PATH, so you can do
MAKE_SYM_STORE_ARGS := -c
ifdef PDBSTR_PATH
MAKE_SYM_STORE_ARGS += -i
endif

Comment 11

[Lukas Blakk [:lsblakk] use ?needinfo]

Attachment: Symbolstore.py patch that addresses the previous comments

I believe this addresses the previous comments.

In the call to pdbstr.exe in ProcessFile - I took out the hardcoding and replaced it with the environment variable.

Comment 12

[(not currently active) Ted Mielczarek]

Comment on attachment 303910 [details] [diff] [review]
Symbolstore.py patch that addresses the previous comments

> 
>+
> uploadsymbols:
> ifdef MOZ_CRASHREPORTER
> 	$(topsrcdir)/toolkit/crashreporter/tools/upload_symbols.sh $(topsrcdir)/../$(BUILDID)/crashreporter-symbols-$(SYMBOL_ARCHIVE_BASENAME).zip
> endif

Can you kill that stray blank line you introduced above uploadsymbols?

>Index: toolkit/crashreporter/tools/symbolstore.py
>===================================================================
>+                    if self.srcsrv:
>+                        # Creates a .pdb.stream file in the mozilla\objdir to be used for source indexing
>+                        cwd = os.getcwd()
>+                        streamFilename = debug_file + ".stream"
>+                        stream_output_path = os.path.join(cwd, streamFilename)
>+                        # Call SourceIndex to create the .stream file
>+                        result = SourceIndex(sourceFileStream, stream_output_path)
>                     f.close()
>                     cmd.close()
>                     # we output relative paths so callers can get a list of what
>                     # was generated
>                     print rel_path
>                     if self.copy_debug:
>                         rel_path = os.path.join(debug_file,
>                                                 guid,
>                                                 debug_file).replace("\\", "/")
>                         print rel_path
>                         full_path = os.path.normpath(os.path.join(self.symbol_path,
>                                                                   rel_path))
>                         shutil.copyfile(file, full_path)
>+                        if self.srcsrv:
>+                            # location of pdbstr.exe for now, until a replacement exists and is checked in 
>+                            pdbstr_path = os.environ.get("PDBSTR_PATH")
>+                            pdbstr = os.path.normpath(pdbstr_path)
>+                            pdb_rel_path = os.path.join(debug_file, guid, debug_file)
>+                            pdb_filename = os.path.normpath(os.path.join(self.symbol_path, pdb_rel_path))
>+                            # move to the dir with the stream files to call pdbstr
>+                            os.chdir(os.path.dirname(stream_output_path))
>+                            os.spawnv(os.P_WAIT, pdbstr, [pdbstr, "-w", "-p:" + pdb_filename, "-i:" + streamFilename, "-s:srcsrv"])
>+                            # clean up all the .stream files when done
>+                            os.remove(stream_output_path)

Could you pull everything under |if self.srcsrv:| into a separate method on the Dumper_Win32 class? You'll have to define it on the base Dumper class as well, but you can just make it empty, sort of like FixFilenameCase, but you can just put |pass| as the method body. Since this is Win32 specific code, it'd be better if it lived in the Win32 specific class.


>+    #check to see if the pdbstr.exe exists
>+    if (options.srcsrv):
>+        pdbstr = os.environ.get("PDBSTR_PATH")
>+        if (os.path.exists(pdbstr)):
>+            print >> sys.stderr, "pdbstr.exe path exists.\n" 
>+        else:
>+            print >> sys.stderr, "No path to pdbstr.exe exists - please set a PDBSTR_PATH variable.\n"

No need to print anything if it exists, you can just reverse the case of the test here (also you don't need parentheses around your tests in Python, so ditch those):
if not os.path.exists(pdbstr):
  ...

but you should have a sys.exit(1) or something after the print statement, so the app quits if you try to pass it a nonexistent pdbstr.exe.

Looks good! r=me with those fixes.

Comment 13

[Lukas Blakk [:lsblakk] use ?needinfo]

Attachment: Fixed Patch - ready to check in

Everything from the last comment was addressed - this should be good to go.

Comment 14

[Reed Loden [:reed]]

Please do not add the checkin-needed keyword until you have all required reviews and approvals.

Comment 15

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 305853 [details] [diff] [review]
Fixed Patch - ready to check in

a=beltzner

Comment 16

[Reed Loden [:reed]]

Checking in Makefile.in;
/cvsroot/mozilla/Makefile.in,v  <--  Makefile.in
new revision: 1.401; previous revision: 1.400
done
Checking in toolkit/crashreporter/tools/symbolstore.py;
/cvsroot/mozilla/toolkit/crashreporter/tools/symbolstore.py,v  <--  symbolstore.py
new revision: 1.9; previous revision: 1.8
done