872212 - dmcli launchfennec "--mozenv" option broken

Status: RESOLVED FIXED

(Testing :: Mozbase, defect)

Description

[William Lachance (:wlach)]

mozenv needs to be passed as a dictionary to the underlying implementation, but we have no way of doing this currently. Let's parse this argument as json and then do that. This will let us do things like easily launch fennec with network profiling enabled as follows:

dm launchfennec org.mozilla.fennec_wlach--mozenv '{ "NSPR_LOG_MODULES":"timestamp,nsHttp:5,nsSocketTransport:5,nsHostResolver:5","NSPR_LOG_FILE":"/mnt/sdcard/http.log"}' --url http://www.nytimes.com

Comment 1

[William Lachance (:wlach)]

Attachment: Fix --mozenv option to dmcli's launchfennec command

Comment 2

[Mark Côté [:mcote]]

So this works but strikes me as a bit awkward, in that environment variables only map to a small subset of JSON (as well as being somewhat annoying to type).  What about just passing a space-separated list, as you would use at the beginning of the line when executing a function from the shell? E.g.

--mozenv 'NSPR_LOG_MODULES=timestamp,nsHttp:5,nsSocketTransport:5,nsHostResolver:5 NSPR_LOG_FILE=/mnt/sdcard/http.log'

Comment 3

[William Lachance (:wlach)]

Attachment: Updated patch per mcote's suggestion

I like this idea. Updated patch per your suggestion.

Comment 4

[Mark Côté [:mcote]]

Comment on attachment 750696 [details] [diff] [review]
Updated patch per mcote's suggestion

Review of attachment 750696 [details] [diff] [review]:
-----------------------------------------------------------------

This is good with a few little changes.

::: mozdevice/mozdevice/dmcli.py
@@ +14,4 @@
>  import mozdevice
>  import mozlog
>  import argparse
> +import json

No longer needed.

@@ +110,5 @@
>                                                          'default': 'android.intent.action.VIEW' },
>                                                        { 'name': '--url', 'action': 'store' },
>                                                        { 'name': '--extra-args', 'action': 'store' },
> +                                                      { 'name': '--mozenv', 'action': 'store',
> +                                                        'help': 'Gecko environment variables to set in "KEY1=VAL1;KEY2=VAL2" format' },

The code in launchfennec() below separates variables by whitespace (which I think is the correct approach), so the example format should be "KEY1=VAL1 KEY2=VAL2" (i.e. no semicolon).

@@ +322,5 @@
> +        if args.mozenv:
> +            mozEnv = {}
> +            keyvals = args.mozenv.split()
> +            for keyval in keyvals:
> +                (key, val) = keyval.split("=")

So it seems that, on POSIXy systems at least, subsequent '=' characters in an environment variable assignment are treated as part of the value.  For example, "FOO=BAR=BAZ" results in "FOO" being set to "BAR=BAZ".

You could replicate that behaviour here with '(key, _, val) = keyval.partition("=")'.

@@ +324,5 @@
> +            keyvals = args.mozenv.split()
> +            for keyval in keyvals:
> +                (key, val) = keyval.split("=")
> +                mozEnv[key] = val
> +            print mozEnv

Debug print statement?

Comment 5

[William Lachance (:wlach)]

Pushed with suggested changes: https://github.com/mozilla/mozbase/commit/42ba6a97c6010d61b30b0940992db3ac2b767c8b