Andrew Halberstadt [:ahal] Assignee
	Description • 11 years ago

Currently automation.py has the ability to install webapps. This will be needed to get b2g mochitests running on mozprofile.

http://mxr.mozilla.org/mozilla-central/source/build/automation.py.in#299

	Jeff Hammel
	Comment 1 • 11 years ago

I'd be inclined to have this logic live in its own file, e.g. webapps.py . But as usual, whatever works

	Andrew Halberstadt [:ahal] Assignee
	Comment 2 • 11 years ago

Sorry for the big ugly patch. At least it has a test ;)

	Andrew Halberstadt [:ahal] Assignee
	Comment 3 • 11 years ago

Noticed a mistake in clean():
- if os.path.isdir(self.webapps_dir):
+ if len(self._installed_apps) > 0 and os.path.isdir(self.webapps_dir):

I'll add this to the follow up patch I make to address review comments.

	Jeff Hammel
	Comment 4 • 11 years ago

+ if len(self._installed_apps) > 0 and os.path.isdir(self.webapps_dir):

if len(self._installed_apps) and ...

no need to check > 0

	Jeff Hammel
	Comment 5 • 11 years ago

Still in the process of review, but there's something I wanted to call out specifically:

+    def __init__(self, profile, apps=None, json_template=None, manifest_template=None):
+        if not isinstance(profile, basestring):
+            raise TypeError("Must provide path to a profile, received '%s'" % type(profile))
+        self.profile = profile
+        self.webapps_dir = os.path.join(self.profile, 'webapps')
+        self.backup_dir = os.path.join(self.profile, '.webapps_backup')

I just pushed a patch of :mihneadb where, after iteration, it was decided that /tmp was a better place for backing up of stuff from the profile that could be overwritten.

https://bugzilla.mozilla.org/show_bug.cgi?id=759594#c6

I don't know if I have a strong opinion.  My very weak opinion is that, like in this bug, a directory in the profile makes more sense.  This way: A. if Something Bad Happen, you can look in the profile directory, see the backup, and fix yourself; and B. similarly, we can detect and do something (maybe err out, lord knows) in automation.  OTOH, using /tmp doesn't litter the profile with these things which some people will inevitably yell about the same way as I yell about emacs ~files. But as said, that's a weak opinion.

My strong opinion is that we should do this in the same way for everything in the profile.  Either .backup directories in the profile (preferably 1 IMHO, conceivably 1/thingy) -OR- backup directories in e.g. /tmp. 

Anyway, not bringing this up to call either :ahal or :mihneadb out.  We just need to figure it out and change this patch or file a follow-up bug for addons.py.  I'm happy with you two figuring it out, taking a vote between the three of us, or going by the infinite wisdom of the j-Griffin (half-lion, half-eagle, all coder).

	Jeff Hammel
	Comment 6 • 11 years ago

Comment on attachment 710888 [details] [diff] [review]
Patch 1.0 - add webapp handling to mozprofile

So...I tapered off towards the end there, but figured its already a long review for a long patch and I might as well paste my main concerns.

+from prefs import *

If you do this, you should probably define an __all__ in prefs.py.

+        # handle webapps
+        self.webapps = WebappCollection(profile=self.profile, apps=apps)
+        self.webapps.install()
+

This may be thinking too far ahead, but I notice that you don't store
`apps`. Do we care about the use case where an app is added following
instantiation?

+# from nsIPrincipal.idl

Please link to code

Also

+APP_STATUS_NOT_INSTALLED = 0
+APP_STATUS_INSTALLED     = 1
+APP_STATUS_PRIVILEGED    = 2
+APP_STATUS_CERTIFIED     = 3

These don't appear to be used; why are they here?

+    required_keys = ['name', 'description', 'manifestURL']

Should be a set since order isn't important

+            super(Webapp, self).__init__(*args, **kwargs)

Is there any reason to use super vs calling dict.__init__ here?  IMHO
if what you want to call is dict.__init__ you should call it.

+        for key in self.required_keys:
+            if key not in self.keys():

1. AIUI, `if key not in self:` is preferred

2. I think this should be done with set logic:

  if not set(self.required_keys).issubset(self.keys()):

The cast is unnecessary if required keys is a set.

+    json_template = Template(""""$name": {
....

You're probably not surprised that I don't like this at all. ;)
Why not use actual dicts vs interpolate them vs string templates?  If
you use json.dumps, you can be assured that at least the syntax is
valid.  With string templates, you cannot (without independent calls
to the json module anyway).  You already do some of the
checking/defaulting in code (install()).

+        self._apps = []
+        self._installed_apps = []
+        if apps:
+            if not isinstance(apps, list):
+                apps = [apps]
+
+            for app in apps:
+                if isinstance(app, basestring) and
os.path.isfile(app) and app.endswith('.json'):
+                    self.extend(self.read_json(app))
+                else:
+                    self.append(app)

This is a bit, erm, too much IMHO.  I could reluctantly accept it if
there's precedence, but i'd rather be less implicit (yeah, i know,
*me* saying that).

Firstly, you'll need a docstring on __init__ as you're probably
already aware detail what what is and what can be passed.

While in some cases I'm fine with the "this argument can be one or
many", to me it's probably overkill here and just "many" should be
accepted.  Then you can get rid of the isinstance check and the if
webapps conditional and replace it all with a for loop if you replace
the default signature with an empty tuple which also gives some
indicator of what is expected there.

Even were this not to be done, I disgree with the isinstance check.
What if a tuple is passed in?  Should that really go to [(foo,...)]? Seems
wrong. As it stands, it will add the tuple to self._apps which
also seems wrong.

This check also seems pretty iffy:

+                if isinstance(app, basestring) and os.path.isfile(app) and app.endswith('.json'):

So, I don't know if a grok what exactly self._apps is, but I'm going
to make a wild guess that it is an error that "If one of (or the only
one, if you pass in one) of the apps argument to __init__ is true is a
string that is a path that ends with anything other than '.json' (or
variations on that theme), we want it in self._apps"

It should be clear what is allowed here.  I guessing strings that end
with '.json' should not switch on which path is followed depending on
file existence.

+        if json_template:
+            self.json_template = json_template
+        if manifest_template:
+            self.manifest_template = manifest_template

I would tend to do:

    self.json_template = json_template or self.json_template # etc

but its purely to get it on one line (not important, and no....not as
efficient...maybe?)

+    def __getitem__(self, index):
+        return self._apps.__getitem__(index)
+
+    def __setitem__(self, index, value):
+        return self._apps.__setitem__(index, Webapp(value))
etc

IMHO, this level of syntactic sugar makes the code less readable, not
more.  But opinions vary, so I'm net-netural on it.

+    def install(self):
+        """Installs the webapps represented in this collection to the
profile"""
+
+        # If install is called a subsequent time, there could have
been apps added
+        # or removed to the collection in the interim. Figure out
what these are.

Given the comment, I'm not sure if `install` is the best function
name.  That said, I can't think of a better one.  I would move the
comment to the docstring though in case anyone is looking with `help`.

+        apps_to_install = [app for app in self._apps if app not in
self._installed_apps]
+        apps_to_remove = [app for app in self._installed_apps if app
not in self._apps]

set() ftw

+        elif len(self._installed_apps) == 0:

No need for the ` == 0` part.  Or even the len(), really:

`if not self._installed_apps:`

There are a few things in install that make me wonder how install is
both suppossed to behave and actually does behave with respect to
preexisting webapps. AIUI, the goal is to copy all preexisting webapps
to the backup directory and then copy them back on cleanup.   When you
call install, this will install the set of webapps that are in
self._apps ... (and here is where I get confused) on top of whatever
was in the profile before WebappCollection got to is *BUT* removing
all things that were added? :sigh: I'm probably just being a dummy.
That said, what it actually does should be documented.

+        :param path: Path to a json file defining webapps

Hopefully a file or url in the future :)

In general, I would break up install into several smaller functions.
Since read_json already returns Webapps, perhaps some of the install
logic could go there as well.

I am similarly confused wrt the read_json method.  My confusion
probably mostly comes from:

"""
The json format is either a
+        dictionary where each key represents the name of a webapp
(e.g B2G format) or a list
+        of webapp objects.
"""

I'd like to know more about the difference.

You'll definitely also want to include a link to the canonical
documentation that details what all of this means.  While I've seen a
brownbag here and there, I'm somewhat confused myself.  (You'll want a
module-level docstring anyway.)

	Jeff Hammel
	Comment 7 • 11 years ago

Also, we should figure out the "how to do things" matter of comment 5 too before I r+. While I don't want to shoe-horn things into the same mold that aren't, I would like as much similitude as possible in how things execute the same pattern, both for the sake of refactoring and legibility (especially in the same package).

Also, :ahal, lemme know if anything I propose is unclear and/or wrong ;) (Though I'm sure you will anyway.)

	Andrew Halberstadt [:ahal] Assignee
	Comment 8 • 11 years ago

(In reply to Jeff Hammel [:jhammel] from comment #5)
> My strong opinion is that we should do this in the same way for everything
> in the profile.  Either .backup directories in the profile (preferably 1
> IMHO, conceivably 1/thingy) -OR- backup directories in e.g. /tmp. 

My vote would be 1 backup directory in the profile called ".mozprofile_backup" or something. But I would be fine with /tmp too.

	Andrew Halberstadt [:ahal] Assignee
	Comment 9 • 11 years ago

(In reply to Jeff Hammel [:jhammel] from comment #6)
> Comment on attachment 710888 [details] [diff] [review]
> Patch 1.0 - add webapp handling to mozprofile
>
> +        # handle webapps
> +        self.webapps = WebappCollection(profile=self.profile, apps=apps)
> +        self.webapps.install()
> +
> 
> This may be thinking too far ahead, but I notice that you don't store
> `apps`. Do we care about the use case where an app is added following
> instantiation?

So to add more apps you could do self.profile.webapps.append(app) as the webapps object is list like. I'm not sure if I like having to call "install()" to update the manifests every time webapps get added or removed, but the alternative is to re-write all those files every time something is added or removed to the webapps object which seems wasteful.


> 
> +# from nsIPrincipal.idl
> 
> Please link to code
> 
> Also
> 
> +APP_STATUS_NOT_INSTALLED = 0
> +APP_STATUS_INSTALLED     = 1
> +APP_STATUS_PRIVILEGED    = 2
> +APP_STATUS_CERTIFIED     = 3
> 
> These don't appear to be used; why are they here?

There's not much of a reason. I was thinking it might be nice for consumers to be able to compare the type of their app e.g: "if app["type"] == webapps.APP_STATUS_PRIVILEGED" but that's probably not a terribly common use case. The other pseudo reason is that they were defined in automation.py so maybe there is something out there using it. I can remove it if you want though.

> +            super(Webapp, self).__init__(*args, **kwargs)
> 
> Is there any reason to use super vs calling dict.__init__ here?  IMHO
> if what you want to call is dict.__init__ you should call it.

Sure. Is there a benefit to calling dict.__init__ instead?


> +    json_template = Template(""""$name": {
> ....
> 
> You're probably not surprised that I don't like this at all. ;)
> Why not use actual dicts vs interpolate them vs string templates?  If
> you use json.dumps, you can be assured that at least the syntax is
> valid.  With string templates, you cannot (without independent calls
> to the json module anyway).  You already do some of the
> checking/defaulting in code (install()).

Yes :).. so the templates and about 90% of the install method are just a straight copy from automation.py. My worry was breaking existing code that uses this. That being said, using a dict vs a template is probably pretty safe.

> 
> +        self._apps = []
> +        self._installed_apps = []
> +        if apps:
> +            if not isinstance(apps, list):
> +                apps = [apps]
> +
> +            for app in apps:
> +                if isinstance(app, basestring) and
> os.path.isfile(app) and app.endswith('.json'):
> +                    self.extend(self.read_json(app))
> +                else:
> +                    self.append(app)
> 
> While in some cases I'm fine with the "this argument can be one or
> many", to me it's probably overkill here and just "many" should be
> accepted.  Then you can get rid of the isinstance check and the if
> webapps conditional and replace it all with a for loop if you replace
> the default signature with an empty tuple which also gives some
> indicator of what is expected there.

I don't really think the way it is now is all that complicated, but I guess I don't really have that strong of a preference, so I can change it.

> Even were this not to be done, I disgree with the isinstance check.
> What if a tuple is passed in?  Should that really go to [(foo,...)]? Seems
> wrong. As it stands, it will add the tuple to self._apps which
> also seems wrong.

In this case a WebappFormatException would be raised (append will attempt to cast the tuple to a Webapp object which will fail).
 
> This check also seems pretty iffy:
> 
> +                if isinstance(app, basestring) and os.path.isfile(app) and
> app.endswith('.json'):
> 
> So, I don't know if a grok what exactly self._apps is, but I'm going
> to make a wild guess that it is an error that "If one of (or the only
> one, if you pass in one) of the apps argument to __init__ is true is a
> string that is a path that ends with anything other than '.json' (or
> variations on that theme), we want it in self._apps"
> 
> It should be clear what is allowed here.  I guessing strings that end
> with '.json' should not switch on which path is followed depending on
> file existence.

I'm having trouble parsing this. Are you saying that I shouldn't be checking for ".json"?

> +    def __getitem__(self, index):
> +        return self._apps.__getitem__(index)
> +
> +    def __setitem__(self, index, value):
> +        return self._apps.__setitem__(index, Webapp(value))
> etc
> 
> IMHO, this level of syntactic sugar makes the code less readable, not
> more.  But opinions vary, so I'm net-netural on it.

My goal was to allow the user to do things like self.profile.webapps[0] = app or self.profile.webapps.extend([apps]). It turned out that subclassing list is very tricky so I opted to expose some of the more common properties of self._apps

> +    def install(self):
> +        """Installs the webapps represented in this collection to the
> profile"""
> +
> +        # If install is called a subsequent time, there could have
> been apps added
> +        # or removed to the collection in the interim. Figure out
> what these are.
> 
> Given the comment, I'm not sure if `install` is the best function
> name.  That said, I can't think of a better one.  I would move the
> comment to the docstring though in case anyone is looking with `help`.

Agreed, maybe update or update_manifests?

> +        apps_to_install = [app for app in self._apps if app not in
> self._installed_apps]
> +        apps_to_remove = [app for app in self._installed_apps if app
> not in self._apps]
> 
> set() ftw

I'm not 100% sure that order isn't important as a different order will result in a different set of local id's for the apps. I *think* that doesn't matter, but I want to leave it to be safe.

> There are a few things in install that make me wonder how install is
> both suppossed to behave and actually does behave with respect to
> preexisting webapps. AIUI, the goal is to copy all preexisting webapps
> to the backup directory and then copy them back on cleanup.   When you
> call install, this will install the set of webapps that are in
> self._apps ... (and here is where I get confused) on top of whatever
> was in the profile before WebappCollection got to is *BUT* removing
> all things that were added? :sigh: I'm probably just being a dummy.
> That said, what it actually does should be documented.

No, it's confusing (keep in mind I 90% copy/pasted install from automation.py :p). The goal is to install on top of pre-existing webapps. These pre-existing webapps should outside the control of mozprofile and shouldn't ever be removed (which install will guarantee to happen). I can definitely document it better.

> I am similarly confused wrt the read_json method.  My confusion
> probably mostly comes from:
> 
> """
> The json format is either a
> +        dictionary where each key represents the name of a webapp
> (e.g B2G format) or a list
> +        of webapp objects.
> """
> 
> I'd like to know more about the difference.

So the dictionary of webapp objects is how the official webapps.json is formatted:
{ "webapp_name" : { "type": 3,
                    "developer": "Mozilla"
                     etc... }
}

I thought it also makes sense to allow a format like:
[ { "name": "webapp_name",
    "type": 3,
    "developer": "Mozilla" }]

Thanks for the excellent review, I'll have a follow up patch in a bit.

	Andrew Halberstadt [:ahal] Assignee
	Comment 10 • 11 years ago

I think this addresses most of your comments. There a few things I didn't change:

A) I didn't use set logic in Webapp.validate because I want to have the actual name of the key that's causing the problem in the exception that gets raised.

B) I stuck with string templates instead of a dict + interpolation. First reason being that the json_template format isn't even valid json on it's own. I'd have to do some manual string manipulation to make it parse-able. Second reason being that it's how it's implemented in automation.py so seems like the safest choice for now. We can always file a follow up if need be.

C) I left the constructor as is (but added a check for tuple and set to isinstance). The main reason being is it feels awkward to me to force someone to pass in a list, especially considering how file paths are accepted. Intuitively I would expect WebappCollection(apps="path/to/webapps.json") to work whereas I would not think of doing WebappCollection(apps=["path/to/webapps.json"]). If you really feel strongly about it I could break manifests into it's own argument, but I'd rather not.

Anyway, I'm not saying that these are set in stone or anything, so feel free to continue discussing them.

	Andrew Halberstadt [:ahal] Assignee
	Comment 11 • 11 years ago

Also I made the backup directory profile/.mozprofile_backup but am happy to move it to /tmp if desired.

	Jeff Hammel
	Comment 12 • 11 years ago

(In reply to Andrew Halberstadt [:ahal] from comment #11)
> Also I made the backup directory profile/.mozprofile_backup but am happy to
> move it to /tmp if desired.

Talking to :jgriffin on irc today, and he's another +1 for keeping it with the profile so I think what you have in the patch is good for now. I think there will be some iteration on how to coordinate backup/cleanup between different, shall we call them, Profile-helpers (AddonManager, WebappManager for now, though I can imagine the SQL stuff getting onboard soon), but I think the direction you're pointing to here is good.

	Jeff Hammel
	Comment 13 • 11 years ago

Comment on attachment 714058 [details] [diff] [review]
Patch 1.1 - address review comments

I still have reservations but need to get on with things

	Andrew Halberstadt [:ahal] Assignee
	Comment 14 • 11 years ago

https://github.com/mozilla/mozbase/commit/19ccb177d8c3cea67fa02e225fd15069551f8824

Do you want me to file a follow up bug for getting rid of the string templates? I just don't really want to change how that's done at this stage. I'm trying to keep this refactor as simple as possible.