451043 - build script should not depend on machine locale

Status: RESOLVED FIXED

(addons.mozilla.org Graveyard :: Administration, defect)

Description

[Fred Wenzel [:wenzel]]

build.sh assumes the dev machine is set to an English locale, because it parses the output of svn info, which is localized.

While I added the German string as an option in r17705, instead of assuming a box's locale, we should call the command with 'svn info --xml', then parse the xml output.

Comment 1

[Fred Wenzel [:wenzel]]

I know, I know: You file the bug, you fix it yourself ;)

Comment 2

[Fred Wenzel [:wenzel]]

Since I have to fix the build script anyway (maybe I do it in Python, as it allows me to parse XML easily), I *may* fix bug 464625 in the process as well.

Comment 3

[Fred Wenzel [:wenzel]]

Attachment: Python build script

This is a build script rewrite. Major changes:
- it (obviously) fixes the problem described here by parsing svn info --xml instead of relying on its regular, "human readable" output.
- it's in Python
- it opens the way to fixing bug 464625 by not throwing intermediate build products in the source tree anymore

Lars, can you please review this? I am mainly looking for:
- not doing stupid things that make your eyes hurt
- equivalence to the current build script (build.sh)

Let me know what you think, thanks.

Comment 4

[K Lars Lohn [:lars] [:klohn]]

Overall, I'd say my comments here are pretty minor - my eyes only bled a little bit.  I looked at the code with respect to Python syntax and idioms.  I did not do a comparison to the current build script because I am both lazy and incompetent when it comes to shell scripts.

Notes:

While it makes no functional difference in this simple code, it is generally better form to use “new” style classes in Python: all classes should inherit from “object”

RevisionParser.getMaxRevision: could be simplied to:
  return max([self.getLatestRevision(repo) for repo in repos])

Minifier.concatFiles: the open call should not be within the try:...finally block.  If the open were to fail, then it would attempt to close a not opened file.  In fact, the name binding would not even happen.  This would result in a second exception being raised that would mask the original and more relevant error.  

Minifier.concatFiles: There is an idiom in Python where it is better to ask forgiveness than for permission.  It stems from the fact that Python exception handling is very efficient in comparison to exception handling in C++ or Java.  Rather than testing for the existence of a condition before trying something, just try it and catch the exception if things go wrong. In that spirit I suggest  concatFiles might better look like this:

    if not sourceNames: return None
    try:
        try:
            destinationFile = open(destName, "w")
        except TypeError:  #can't open None
            tempfile = mkstemp()
            destinationFile = os.fdopen(tempfile[0], "w")
            destName = tempfile[1]
        try:            
            for source in sourceNames:
                try:
                    prefix = source['prefix']
                    sourceFileName = source['file']
                    suffix = source['suffix']
                except KeyError:
                    prefix = suffix = ''
                    sourceFileName = source
                sourceFile = open(sourceFileName)
                try:
                    destinationFile.write(prefix)
                    destinationFile.writelines(sourceFile)
                    destinationFile.write(suffix)
                finally:
                    sourceFile.close()
        finally:
            destinationFile.close()
        return destName
    except Exception, e:
        try:
            os.remove(tempfile[1])
        except:
            pass
        print e

concatAndMinify: using the + operator on more than two strings isn't efficient in Python.  While it makes little performance difference in this application, consider using either templates or os.path.join for creating paths of three or more parts:
 webroot + '/js/' + js_file  #ugly
 “%s/js/%s” % (webroot, js_file) #better
os.path.join(webroot, 'js', js_file) #best


main: globals is a function call and not indexable
globals['java'] = argv[1] #probably a TypeError
globals()['java'] = argv[1] #better
java = argv[1] #best

Comment 5

[Fred Wenzel [:wenzel]]

Thanks for your helpful comments, Lars. I actually really appreciate your comments even if you are right and the performance impact is negligible in this context: I don't want to just write mediocre code so I am always happy when I get helpful reviews.

I'll work through your comments and add a new patch.

Comment 6

[Fred Wenzel [:wenzel]]

Attachment: Build script, rev 2

This incorporates all suggested changes, and it also uses os.path.join() for all file paths. Also note that for the dictionary vs. string thing I had to replace KeyError with TypeError because it won't throw a KeyError but a TypeError, as string[something] is valid for something being an integer.

Comment 7

[K Lars Lohn [:lars] [:klohn]]

If you'd like to eliminate the '..' from paths, you can call os.path.normpath.  Again, that's not necessary, as paths with '..' in the middle are perfectly legal.  

Otherwise, everything looks fine.

Comment 8

[Fred Wenzel [:wenzel]]

(In reply to comment #7)
> If you'd like to eliminate the '..' from paths, you can call os.path.normpath. 
> Again, that's not necessary, as paths with '..' in the middle are perfectly
> legal.

Yeah, it'll work how it is for now, I think.

> Otherwise, everything looks fine.

All right! If you r+ my patch, I'll check it in and have it replace the old shell script (will need to file an IT bug to have the cron job on preview changed).

Comment 9

[Fred Wenzel [:wenzel]]

Thanks, checked into r20289. I also filed bug 466879 in order to have the cron job on preview changed accordingly.