Andrew Halberstadt [:ahal] Reporter
	Description • 12 years ago

In bug 830430 we are attempting to pull preferences out of the harness files and into json manifests. Preferences aren't the only profile-centric information that the test harnesses are populating though, they are also populating permissions, addons and proxy rules. Therefore it would make sense to have all of this information available in a single "profile_data.json" file.

Looking at https://github.com/mozilla/mozbase/blob/master/mozprofile/mozprofile/profile.py#L27, I would propose the format:

{ "preferences": dict,
  "addons": list,
  "addon_manifests": list,
  "locations": list,
  "proxy": dict,
  "permissions": dict,
  "profile": string,
  "restore": bool
}

All of these would be optional and most of them wouldn't be used by production harnesses.

Specifically in mozprofile we need to:
a) Add ability to specify custom permissions via the profile constructor
b) Add method to parse these kinds of files
c) Allow consumers to pass in a list of profile.json file paths

Open question: How does this tie in to all the other ways of specifying profile data?

	Andrew Halberstadt [:ahal] Reporter
	Comment 1 • 12 years ago

Actually looks like permissions.py handles permissions via the locations and proxy dict, so step a) should be unnecessary.

	Jonathan Griffin (:jgriffin)
	Comment 2 • 12 years ago

A lot of profile data in automation.py.in is generated dynamically; it wouldn't fit in a static json file.  How will we store that kind of data?  Would we be better off with a Python package to store default profile data?

	Jeff Hammel
	Comment 3 • 12 years ago

(In reply to Jonathan Griffin (:jgriffin) from comment #2)
> A lot of profile data in automation.py.in is generated dynamically; it
> wouldn't fit in a static json file.  How will we store that kind of data? 
> Would we be better off with a Python package to store default profile data?

My general feeling is that there are two kinds of issues here:

1. Preferences that we want set in the base profile but that need information to be able to set.  My feeling is that for these what we do here is that we do for problems like PAC what we've already done in mozprofile: namely, we introduce an API (in the PAC case and probably most cases as arguments to Profile.__init__) that allows the instantiator to specify the programmatic details needed to set these up.  If there's complicated or time/resource intensive stuff, perhaps you subclass (either in mozprofile if its generic or elsewhere if its not).
1.a Okay, fine, there are three kinds of issues ;) So automation.py.in has a bunch of parameters that depend on the server variable: http://mxr.mozilla.org/mozilla-central/source/build/automation.py.in#507 . These are "dynamic" in the most rudimentary way possible: they just need string interpolation.  While I have internal debates about this, I'm going to consolidate them into one idea that I'll present here but with the caveat that the details are mostly a cartoon:

- have a class-level variable e.g. `variables = ['server', ...]` that defines the variables to be interpolated, e.g. `pref_value = pref_value % dict([(i,getattr(self, i) for i in self.variables])`.  (I say class-level because its friendlier to extensibility and introspection for reasons that I hope are out of scope but I'm happy to elaborate on.)

- take these as arguments to the ctor with default values if possible. If we have this at class level: `for i self.variables: setattr(self, i, locals()[i])`. Obviously if this ends up being dozens of variables then this approach is probably not the right one but I'm hoping that never becomes the case.  What I definitively *DON'T* want is passing free form variables into Profile for interpolation.

- interpolate all (string?) prefs with these variables

I'm waving hands over hopefully non-important details but thats mostly what I would do.

The second class of issues is:

2. Stuff that is harness specific.  Punt. The harness can have its own subclass as desired.

	Jeff Hammel
	Comment 4 • 12 years ago

> Looking at https://github.com/mozilla/mozbase/blob/master/mozprofile/mozprofile/profile.py#L27, I would propose the format:
> 
> { "preferences": dict,
>   "addons": list,
>   "addon_manifests": list,
>   "locations": list,
>   "proxy": dict,
>   "permissions": dict,
>   "profile": string,
>   "restore": bool
> }

I'm actually unsure and go back and forth on whether we need all data in profile.json, but at this point you've probably looked closer than I.  As an instance (though not the only) of the sort of thing I wonder, if you give a relative file path in the list of addons, is that relative to the manifest?  When will/can that be used?  Likewise, would anyone specify restore in a JSON a struct?  When would you specify the path to profile?  

> Open question: How does this tie in to all the other ways of specifying profile data?'

So I was wondering the same thing.  I would probably have a "factory" classmethod:

@classmethod
def from_json(cls, *json_file_or_urls, **kwargs):
    """instantiates a profile instance given JSON blob locations; **kwargs are arguments to the ctor applied atop the JSON data"""

The question then becomes: what to do when e.g. kwargs overlap with keys from the JSON?  My personal opinion is that lists should .extend(), dicts should .update(), and if we care about restore and profile (FWIW, I'd rather not specify profile; IMHO at that point you might as well just make a directory where the profile lives; similarly, i'd leave out restore and leave that to the instantiator. But if someone could make a strong use case I'd bend.)

So something like:

retval = {}
def augment(dict1, dict2):
     methods = {list: list.extend, dict: dict.update}
    for key, value in dict2:
        if key in dict1: # TODO sanity checking
            methods.get(type(value), lambda x, y: None)(dict1, value)
for i in [json.loads(urllib.urlopen(url)).read() for url in json_files_or_urls] + [kwargs]:
    # XXX ^ this is unsafe for a coupla reasons
    augment(retval, i)
return cls(**retval)

I could probably get this into one completely unreadable list comprehension with some effort ;) This is again a cartoon, not how I would actually write it.

This certainly ain't the only way and I could be fine with other solutions, but it seems like what fits profile data the best.

	Andrew Halberstadt [:ahal] Reporter
	Comment 5 • 12 years ago

So I have a patch that is almost finished which I wrote before reading these replies. Some of it is similar to what you suggested, some not so much. I think it's easiest to upload it and then we can iterate on it until it makes sense. It definitely tends towards being a generic solution and making the consumers responsible for their mistakes.

	Andrew Halberstadt [:ahal] Reporter
	Comment 6 • 12 years ago

So this is attempt 1. The parse_profile_json method is a module level function that any harness can call to get a profile object which it can then modify or pass directly into the Profile constructor as it wishes. The harness is responsible for providing a proper interpolation_dict. Mozprofile does no validation on whether or not this dict is suitable for the manifests given (a KeyError is raised if a value is missing, maybe this should be a more specific exception). I like having the mozprofile bits stay as simple as possible and put the burden of making sure the manifest format and interpolation_dict are correct on the consumer.

That being said I'm sure you will have many improvements/suggestions and I am open to other approaches.

	Andrew Halberstadt [:ahal] Reporter
	Comment 7 • 12 years ago

The more I think about it, I'm not really sure if it's worth having non-pref data in these files either. The advantage of only doing prefs is that mozprofile can already read prefs.js style files which are familiar to everyone and also allow comments.

	Jeff Hammel
	Comment 8 • 12 years ago

My general feeling is that we shouldn't add new features unless the give us some advantage.  Talking with :ahal on IRC, I think we're on the same page here.  If prefs.js gives us all that we need for now, i'm in favor of not adding the factory (again, unless/until we need it).

	Jeff Hammel
	Comment 9 • 12 years ago

Comment on attachment 709068 [details] [diff] [review]
Patch 1.0 - simple implementation

Removing r flag if we don't want to take this.  I would have r-ed as IMHO we really should add the ability to read from a URL (we'll still need this anyway).

I would also tend to put this as a classmethod factory unless it is intended for use other than instantiating a profile.  For one, it makes instantiating a profile from these .json files a two-liner instead of a one-liner.  For another, if it doesn't live the class and we do add error checking or what not, we can't do that with a unbound method (you could inspect Profile within the method, but if you have a subclass all bets are off).  For yet another, if we wanted a serializer....

Anyway, I won't go on as I believe the intention is to not do this until there is a concrete driver.

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

I'll leave this open in case we want it in the future. If we're just using prefs.js files for now I don't think we need to add anything new to mozprofile.

	Geoff Brown [:gbrown]
	Comment 11 • 6 years ago

Mass closing bugs with no activity in 2+ years. If this bug is important to you, please re-open.