Code review comment for lp:~michihenning/unity-scopes-api/config-fixes

Revision history for this message
Michi Henning (michihenning) wrote :

> Alright, gave this a spin on an actual device, first of all, please merge
> lp:~mhr3/unity-scopes-api/fix-paths so upstart is actually able to run the
> registries

Thanks for that! One change:

- boost::filesystem::path path(configfile);
- if (path.extension() != ".ini")
+ if (configfile.find(".ini") == string::npos)

This isn't the same. The new version tests whether configfile *contains* ".ini", not whether it *ends with* ".ini". I've reverted this for the moment because avoiding boost here and doing a suffix test with std::string only is rather messy. (C++ has pretty much the most dysfunctional and unusable string class I've ever come acros...)

> . Next, smart-scopes-proxy doesn't start and is complaining that it
> can't find its SSRuntime.ini.

My apologies for missing that! I was going to do that as part of stage 2, but it looks like that's not possible, so I've added proper config for smartscopesproxy.

> It's also odd that we still have the DFLT_RUNTIME_INI, DFLT_REGISTRY_INI etc
> consts even though they don't point to real files.

The semantics are:

- if an explicit path is provided, use it.

- if no explicit path is provided, try the DFLT path.

  - If that default path exists, use it, otherwise use the hard-wired default values for attributes

That way, we can run without any config files, but can still drop a config file in place if we want to change something.

> Last thing is that we're putting the binaries in .../unity-scopes - imo that
> dir should be reserved only for scopes. Although it will only ever contain
> scopes that are controlled by us (and packaged as .debs), so not a big deal.

Well, I hear you :-) I wasn't happy about the weird path we had: /usr/lib/<arch>/scoperunner/scoperunner.

I don't think it matters that the three binaries are in the same directory as the scopes. We could make a bin subdirectory or some such, though. Would that improve things? (I'm open to either approach.)

« Back to merge proposal