Merge lp://staging/~mardy/location-service/providers-dir into lp://staging/location-service/trunk

Proposed by Alberto Mardegan
Status: Work in progress
Proposed branch: lp://staging/~mardy/location-service/providers-dir
Merge into: lp://staging/location-service/trunk
Diff against target: 890 lines (+420/-141)
20 files modified
include/location_service/com/ubuntu/location/default_provider_selection_policy.h (+4/-4)
include/location_service/com/ubuntu/location/provider_collection.h (+34/-12)
include/location_service/com/ubuntu/location/provider_selection_policy.h (+2/-3)
src/location_service/com/ubuntu/location/CMakeLists.txt (+2/-0)
src/location_service/com/ubuntu/location/default_provider_selection_policy.cpp (+10/-10)
src/location_service/com/ubuntu/location/engine.h (+6/-25)
src/location_service/com/ubuntu/location/filesystem_provider_loader.cpp (+97/-0)
src/location_service/com/ubuntu/location/filesystem_provider_loader.h (+69/-0)
src/location_service/com/ubuntu/location/non_selecting_provider_selection_policy.cpp (+2/-2)
src/location_service/com/ubuntu/location/non_selecting_provider_selection_policy.h (+1/-1)
src/location_service/com/ubuntu/location/provider_loader.h (+54/-0)
src/location_service/com/ubuntu/location/provider_manifest.cpp (+45/-0)
src/location_service/com/ubuntu/location/provider_manifest.h (+76/-0)
src/location_service/com/ubuntu/location/service/daemon.cpp (+14/-70)
src/location_service/com/ubuntu/location/service/daemon.h (+0/-4)
src/location_service/com/ubuntu/location/service/default_configuration.cpp (+1/-6)
src/location_service/com/ubuntu/location/service/default_configuration.h (+0/-1)
tests/engine_test.cpp (+1/-1)
tests/null_provider_selection_policy.h (+1/-1)
tests/provider_selection_policy_test.cpp (+1/-1)
To merge this branch: bzr merge lp://staging/~mardy/location-service/providers-dir
Reviewer Review Type Date Requested Status
Thomas Voß (community) Needs Fixing
Review via email: mp+280724@code.staging.launchpad.net

Description of the change

WIP

Manifest-based provider loading

Don't hardcode the list of available providers. Instead, scan some directories from the filesystem ($XDG_DATA_DIRS/location/providers) for <provider>.json files, which tell which providers are available.

The goal is to allow a simple way of configuring providers; for instance, we might have a file

  /usr/share/location/providers/google-location-service.json

with these contents:

  {
    "plugin": "gls",
    "configuration": {
      "endpoint": "https://www.googleapis.com/geolocation/v1/geolocate",
      "api-key": "xyz"
    }
  }

which will cause the location service to load a "gls" plugin and initialize it with the given configuration, suitable for the Google service. Also, a user might create a file

  ~/.local/share/location/providers/mozilla.json

with these contents:

  {
    "plugin": "gls",
    "configuration": {
      "endpoint": "https://location.services.mozilla.com/v1/geolocate",
      "api-key": "abc"
    }
  }

By doing this, the user is asking ULS to create a second instance of the "gls" plugin, this time setup to use the similar service provided by Mozilla.

The ULS remembers which manifest files have been loaded (using the basename of the manifest file as a key), and will refuse to load the same provider twice. So, continuing our example above, the user could create a file

  ~/.local/share/location/providers/google-location-service.json

which would override the system one. This can be useful when a different configuration is desired (for example, we the plugin might accept a configuration option to decide which of WIFI, CellId, or GeoIP positioning methods are allowed, and the user might want to force a specific one).

Some providers are built into the location service; in order to enable them, their plugin name must be prefixed with a "@". So, to enable the GPS provider, we should create a file

  /usr/share/location/providers/gps.json

with contents:

  {
    "plugin": "@gps"
  }

(note that the filename is irrelevant in this case: what matters is the value of the "plugin" key)
The current implementation assumes that if the plugin name does not start with a "@", the plugin must be loaded using the "remote::Provider"; so, in the "configuration" key one should store the options needed by the remote provider. So, for example:

  {
    "plugin": "espoo",
    "configuration": {
      "name": "com.ubuntu.espoo.Service.Provider",
      "path": "/com/ubuntu/espoo/Service/Provider"
    }
  }

========== OPEN QUESTIONS =========

The current WIP implementation still supports passing provider options from the command line (and actually, requires that at least one provider is specified). Command line options do override those from the manifest file. However, despite having implemented this, I think that there is no point in having this code, and I'd propose removing it and letting all the provider configuration happen in the manifest. Do you agree?

Then, about remote providers, to make landing this branch as easy as possible, I'd propose (at least for the time being) to keep the existing mechanism of loading remove providers via the remote::Provider interface; we can actually get rid of the D-Bus name and path from the "configuration" key and always assume that they are in the form:
    "com.ubuntu.<plugin>.Service.Provider"
    "/com/ubuntu/<plugin>/Service/Provider"
where "<plugin>" is the value of the "plugin" key in the manifest files.

In the long term, I'd like to get rid of D-Bus and just use a socket (maybe with a p2p D-Bus server on top of it, to minimize changes -- but we should add this to dbuscpp first), because there's no need to route all location updates through the server when all we want is actually a p2p communication. But that's for the far future.

To post a comment you must log in.
Revision history for this message
Thomas Voß (thomas-voss) wrote :

Thanks for the proposal, the general approach looks fine, I left a couple of comments inline.

For your questions:

  (1.) Let's not keep redundant code paths around and go all in.
  (2.) Please keep the keys, and let's avoid implicit conventions only visible in code.
       In addition, let's load providers with { plugin: remote; configuration { plugin ... }}.
       While being a little more verbose, it is absolutely clear what is happening just from
       reading the configuration file.
  (3.) We can think about spawning a private bus instance, but I disagree that the service should
       become a communication hub and take over responsibilities that are in the area of the bus
       daemon or the respective kernel facilities once kdbus is widely available.

With (2.), I would also propose to get rid of the "@" prefix convention as indicated in the corresponding inline comment.

Looking forward to see this landing in the not-too-distant future :)

review: Needs Fixing
Revision history for this message
Alberto Mardegan (mardy) wrote :

Thanks Thomas for the review.

I just want to clarify a couple of things about the FilesystemProviderLoader: why I have the base_dir parameter on the load_providers() method and why it's stateful (the instantiated providers set).

The point is that it needs to load provider manifests from several directories (/usr/share/location/providers and ~/.local/share/location/providers, at least) and at the same time there's an override mechanism so that the manifests defined by the user can override the system ones (as per the description).
So, I think we have three possibilities:
1) keep it as is
2) do as you suggested, but then the constructor needs to take a std::vector<fs::path>, not just a single path
3) no paths visible in the FilesystemProviderLoader API, handle everything internally

Revision history for this message
Thomas Voß (thomas-voss) wrote :

> Thanks Thomas for the review.
>
> I just want to clarify a couple of things about the FilesystemProviderLoader:
> why I have the base_dir parameter on the load_providers() method and why it's
> stateful (the instantiated providers set).
>
> The point is that it needs to load provider manifests from several directories
> (/usr/share/location/providers and ~/.local/share/location/providers, at
> least) and at the same time there's an override mechanism so that the
> manifests defined by the user can override the system ones (as per the
> description).

@stateful: I get why you maintain the set, but that could just be scoped to the function itself iiuc.

> So, I think we have three possibilities:
> 1) keep it as is
> 2) do as you suggested, but then the constructor needs to take a
> std::vector<fs::path>, not just a single path

I think this one is the best way forward, I originally made that proposal in an inline comment:

  "This should take const std::set<boost::filesystem::path>& base_dir at construction time."

The joys of the laucnhpad review UI then :)
> 3) no paths visible in the FilesystemProviderLoader API, handle everything
> internally

229. By Alberto Mardegan

Move provider_loader to src/

230. By Alberto Mardegan

Remove provider_options from load_providers

231. By Alberto Mardegan

Simplify consdruction of path list

232. By Alberto Mardegan

Pass dir list in constructor

233. By Alberto Mardegan

Make load_providers() stateless

234. By Alberto Mardegan

Use boost::fs:path in public methods

235. By Alberto Mardegan

Remove is_internal method

236. By Alberto Mardegan

Add override clause

237. By Alberto Mardegan

no providers from command line

238. By Alberto Mardegan

Don't return reference to temporary

Revision history for this message
Alberto Mardegan (mardy) wrote :

I've updated the MP according to your advice; just a couple of notes:

- I didn't introduce the Receiver class and use it instead of ProviderCollection: I'll do that when the async proivider loading MP has landed.

- regarding ProviderManifest, I cannot add a constructor which takes a stream, because I need the file name; this also answer your question about that name_ variable: it's not a leftover, it's initialized in the constructor's body.

- on a side note, what's the naming convention for member variables? I see both "variable" and "variable_", are you fine with both?

- This is still not building: the src/ directory builds fine, but examples and tests still don't; I'm leaving that for after the holidays.

Unmerged revisions

238. By Alberto Mardegan

Don't return reference to temporary

237. By Alberto Mardegan

no providers from command line

236. By Alberto Mardegan

Add override clause

235. By Alberto Mardegan

Remove is_internal method

234. By Alberto Mardegan

Use boost::fs:path in public methods

233. By Alberto Mardegan

Make load_providers() stateless

232. By Alberto Mardegan

Pass dir list in constructor

231. By Alberto Mardegan

Simplify consdruction of path list

230. By Alberto Mardegan

Remove provider_options from load_providers

229. By Alberto Mardegan

Move provider_loader to src/

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches