Bug 32075 – Add buildbot workaround for json parsed as unicode

• Component: Tools / Tests
• Status: NEW

[Marc-Antoine Ruel]

Created attachment 44162 [details]
master.cfg patch

I hit this issue while testing locally.

Also, add 2 parameters to loadBuildConfig() to specify the configuration files to use.

The next patch will split the logic code out of master.cfg into webkit_master.py so it can be reused by the try server. I need to have this patch in before sending the other one so the svn copy history is kept sane. I also didn't want to make changes while moving code around, so this is why I make this patch first.

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 44162 [details] without any errors.

[Mark Rowe (bdash)]

Comment on attachment 44162 [details]
master.cfg patch

> Index: WebKitTools/BuildSlaveSupport/build.webkit.org-config/master.cfg
> ===================================================================
> --- WebKitTools/BuildSlaveSupport/build.webkit.org-config/master.cfg	(revision 51602)
> +++ WebKitTools/BuildSlaveSupport/build.webkit.org-config/master.cfg	(working copy)
> @@ -317,10 +317,32 @@ class BuildAndTestLeaksFactory(BuildAndT
>      TestClass = RunWebKitLeakTests
>  
>  
> -def loadBuilderConfig(c):
> -    passwords = simplejson.load(open('passwords.json'))
> +def convertToString(json_data):

This method name is way too general and doesn’t provide enough information about what it actually does.

> +    if isinstance(json_data, dict):
> +        retval = {}
> +        for (k,v) in json_data.iteritems():
> +            retval[str(k)] = convertToString(v)
> +        return retval
> +    elif isinstance(json_data, list):
> +        retval = []
> +        for i in json_data:
> +            retval.append(convertToString(i))
> +        return retval
> +    elif isinstance(json_data, unicode):
> +        return str(json_data)

This isn’t safe as it will encode the unicode string using the system encoding.  On many machines this is ASCII which means it will throw if any non-ASCII data is present.

> +    else:
> +        return json_data

The variable naming convention within this code is internally inconsistent.  It’s probably best to match the existing convention used within this file.

> +def loadBuilderConfig(c, config_file, passwords_file):
> +    passwords = convertToString(simplejson.load(open(passwords_file)))
> +
> +    config = convertToString(simplejson.load(open(config_file)))

Ditto here.


Per your comment the only reason we’re doing this is so that the dictionary keys are instances of str rather than unicode.  Given that it seems as though it would be much cleaner to do something like:

def hook(newObject):
    for key in newObject:
        value = newObject.pop(key)
        newObject[key.encode('utf-8')] = value
    return newObject

simplejson.load(open('config.json'), object_hook=hook)

Is there some reason this wouldn’t be suitable?

[Marc-Antoine Ruel]

Created attachment 44172 [details]
master.cfg patch, second try

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 44172 [details] without any errors.

[Marc-Antoine Ruel]

The hook method you give is insufficient because the values must be string too. I just get the assert elsewhere by setting an object hook.

I updated the code to enforce ascii-only encoding and fixed the variable naming.

[Eric Seidel (no email)]

Comment on attachment 44172 [details]
master.cfg patch, second try

OK.  I think in general this looks fine.

Typo:
 323     Some simplejson version will always return unicode strings. Python

"versions"

Do we know which versions?  It would be best if the comment documented which versions were bad so that we would have some idea when we could rip out this hack.  We could even make the hack a noop for newer versions, although that seems unecesary optimization.

I'm not sure that your isinstance checks are the slickest way to do this via python, but I don't know how else you'd differentiate between a dict and a sequence.

In the end Mark Rowe will have to push this new version to the master server, so he'll see this code go by anyway.

I'm happy to r+ a version with the fixed comment (ideally one which includes version information).  Feel free to ping me via email if I don't see your review go by right away.