424817 - source server tweaks

Status: RESOLVED FIXED

(Toolkit :: Crash Reporting, defect)

Description

[(not currently active) Ted Mielczarek]

The source server support needs a few tweaks to make it work right. I was able to get it working by adding this into my srcsrv.ini:
[variables]
CVSROOT=:pserver:[e-mail]:/cvsroot
CVS_WORKING_DIR=%targ%
SRCSRVTRG=%targ%\%fnbksl%(%var3%)

So:
1) We're not defining CVSROOT anywhere in the source server stream in the pdb. (oops)
2) SRCSRVTARG appears to be a little wrong, at least with the way we're running the cvs command. We have it defined as:
CVS_WORKING_DIR=%targ%\%var2%\%fnbksl%(%var3%)
SRCSRVTRG=%CVS_WORKING_DIR%

Which means that if your local source path is c:\srcsrv, it expands to something like c:\srcsrv\MYSERVER\mozilla\..., which is not where the source gets checked out to.

After applying these local tweaks (and having it work, which was awesome!) I realized that we should probably tweak this one step further. We're not currently encoding the revision number into the path anywhere, so this could cause problems working with multiple revisions of a source file. We should somehow get the revision number into the pathname somewhere. I haven't figured this out yet.

Comment 1

[Lukas Blakk [:lsblakk] use ?needinfo]

Attachment: Tweaks as requested

Here's my stab at addressing those issues.  If there's an easier way to chase root_name around the script, please point that out to me.

Comment 2

[(not currently active) Ted Mielczarek]

Comment on attachment 311503 [details] [diff] [review]
Tweaks as requested

You're supposed to set review? and put someone's bugmail in there. :)

Comment 3

[(not currently active) Ted Mielczarek]

Comment on attachment 311503 [details] [diff] [review]
Tweaks as requested

This is close, but not quite. I'll suggest a bit of refactoring here. We also should really figure out how to get the files stored with the version number in the path, but I'll let that slide to a future patch. It's going to be essential for long-term use of the source server, but we can get it up and running without that for the time being.

Ok, for the refactoring:
I think you should add to |class VCSFileInfo| another method, call it GetCleanRoot or something. Then split up the current implementations of GetRoot so that GetRoot just returns the full unmodified root value, and GetCleanRoot takes self.root, does whatever modifications we currently do to it, and returns it. You'll need to modify the __getattr__ method of VCSFileInfo to handle a new property, let's call it clean_root. Then just replace any existing usage of .root with .clean_root, and you should be ok. Once that's done, you should be able to just use .root directly where you need it.

Ok, now for your actual patch. :)


>@@ -167,17 +168,17 @@ class CVSFileInfo(VCSFileInfo):
>                 # the source filenames come out all lowercase,
>                 # but the srcdir can be mixed case
>                 if file.lower().startswith(self.srcdir.lower()):
>                     file = file[len(self.srcdir):]
>                 (head, tail) = os.path.split(self.srcdir)
>                 if tail == "":
>                     tail = os.path.basename(head)
>                 file = tail + file
>-            return "cvs:%s:%s:%s" % (self.root, file, self.revision)
>+            return "cvs:%s:%s:%s|%s" % (self.root, file, self.revision, self.root_name)
>         return file

I'm not a fan of this, you already stored it as a property on the class above, so you could just get it back from that property below. Especially if you do the refactoring I suggested above.

>     # This regex separates protocol and optional username/password from a url.
>@@ -250,49 +251,52 @@ def GetVCSFilename(file, srcdir):
>     vcs:vcs location:filename:revision
>     For example:
>     cvs:cvs.mozilla.org/cvsroot:mozilla/browser/app/nsBrowserApp.cpp:1.36"""
>     (path, filename) = os.path.split(file)
>     if path == '' or filename == '':
>         return file
> 
>     fileInfo = None
>+    root_name = None
>     if file in vcsFileInfoCache:
>         # Already cached this info, use it.
>         fileInfo = vcsFileInfoCache[file]
>     else:
>         if os.path.isdir(os.path.join(path, "CVS")):
>             fileInfo = CVSFileInfo(file, srcdir)
>         elif os.path.isdir(os.path.join(path, ".svn")) or \
>              os.path.isdir(os.path.join(path, "_svn")):
>             fileInfo = SVNFileInfo(file);
>         vcsFileInfoCache[file] = fileInfo
> 
>     if fileInfo:
>-        file = fileInfo.filename
>+        file, root_name = fileInfo.filename.split('|')

If you do the refactoring I suggest, you can just get fileInfo.root here. Even without doing that, you could just do fileInfo.root_name (which you set above).

> 
>     # we want forward slashes on win32 paths
>-    return file.replace("\\", "/")
>+    return (file.replace("\\", "/"), root_name)

Can you add something to the function documentation about the return value, since it's changing? (The triple-quoted string right after the def line.)

>                             if self.vcsinfo:
>-                                filename = GetVCSFilename(filename, self.srcdir)
>+                                (filename, rootname) = GetVCSFilename(filename, self.srcdir)
>+                                if rootname:
>+                                    root_name = rootname

I don't understand why you're using two different variables here?

Comment 4

[Lukas Blakk [:lsblakk] use ?needinfo]

Attachment: Source Server Tweaks - now with clean_root and refactoring

I believe this addressed your comments.  I've done the refactoring and cleaned it up.  Tested on my local machine and it's working.

Comment 5

[(not currently active) Ted Mielczarek]

Comment on attachment 312420 [details] [diff] [review]
Source Server Tweaks - now with clean_root and refactoring

+    def GetCleanRoot(self):
+        return None

This is going to need to return something valid, at the very least, just make it return self.root. Ideally you'd refactor the bit in GetRoot that does the regex match, and move that down into CleanRoot, but given that we don't have source server support for SVN yet, I could live with an XXX comment and a followup bug on that.

nit: you have a couple instances like:
if (foo):
The parentheses are not required in Python, so ditch them for consistency.

Comment 6

[Lukas Blakk [:lsblakk] use ?needinfo]

Attachment: Source server tweaks, with SVN GetCleanRoot() returning self.root for now

Uses temporary return self.root for SVNInfo's GetCleanRoot()

Comment 7

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

Comment on attachment 313221 [details] [diff] [review]
Source server tweaks, with SVN GetCleanRoot() returning self.root for now

a1.9=beltzner

Comment 8

[Reed Loden [:reed]]

Checking in toolkit/crashreporter/tools/symbolstore.py;
/cvsroot/mozilla/toolkit/crashreporter/tools/symbolstore.py,v  <--  symbolstore.py
new revision: 1.12; previous revision: 1.11
done

Comment 9

[(not currently active) Ted Mielczarek]

I backed this out because it broke buildsymbols on non-windows platforms this morning:
Checking in toolkit/crashreporter/tools/symbolstore.py;
/cvsroot/mozilla/toolkit/crashreporter/tools/symbolstore.py,v  <--  symbolstore.py
new revision: 1.13; previous revision: 1.12
done

Mac:
http://tinderbox.mozilla.org/showlog.cgi?tree=Firefox&errorparser=unix&logfile=1207307100.1207309519.10527.gz&buildtime=1207307100&buildname=MacOSX%20Darwin%208.8.4%20bm-xserve08%20Depend%20Universal%20Nightly&fulltext=#err3
Linux:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1207307400.1207310395.11859.gz#err1

Comment 10

[Lukas Blakk [:lsblakk] use ?needinfo]

Attachment: Source Server Tweaks - now with clean_root and refactoring and not breaking Mac/Linux buildsymbols

So it turned out that in GetVCSFilename, there was a return file instead of return (file, None) so that a tuple is expected and received on the calling end.

I tested this on a Mac build and was able to make buildsymbols without error.

Comment 11

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

Comment on attachment 314129 [details] [diff] [review]
Source Server Tweaks - now with clean_root and refactoring and not breaking Mac/Linux buildsymbols

a=beltzner; I'm assuming we really need this.

Comment 12

[Reed Loden [:reed]]

Checking in toolkit/crashreporter/tools/symbolstore.py;
/cvsroot/mozilla/toolkit/crashreporter/tools/symbolstore.py,v  <--  symbolstore.py
new revision: 1.14; previous revision: 1.13
done