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

Revision history for this message
Michal Hruby (mhr3) wrote :

- scoperegistry, scoperunner, and smartscopesproxy are now installed in /usr/lib/<arch> (instead of a subdir of /usr/lib/arch).

Why this change? There is loads of stuff in the /usr/lib/arch/, we should keep some namespacing - at least a ../arch/unity-scopes/ dir.

77 + The default value is "$XDG_RUNTIME_DIR/<effective UID>/zmq". If XDG_RUNTIME_DIR is not
78 + set, "/run/user/<effective UID>/zmq" is used.

The fallback note seems out-of-date, plus $XDG_RUNTIME_DIR already includes UID.

80 +- EndpointDir.Private (string)

Can you remind you why do we even have this? Scopes will only have access to their own endpoint directory and aggregating scopes (+registry, +dash) need to access them all, so isn't this option obsolete?

111 + The default value is "@CMAKE_INSTALL_PREFIX@/@SCOPES_DEFAULT_CONFIGDIR@/scoperunner".
116 + The default value is "@CMAKE_INSTALL_PREFIX@/@SCOPES_DEFAULT_CONFIGDIR@/scopes".

Arch-specific binaries can't go into /usr/share/ (and SCOPES_DEFAULT_CONFIGDIR == /usr/share/unity-scopes-api). Moreover the default config files still point to @CMAKE_INSTALL_PREFIX@/@CMAKE_INSTALL_LIBDIR@/scoperunner/scoperunner and @CMAKE_INSTALL_PREFIX@/@CMAKE_INSTALL_LIBDIR@/unity-scopes.

121 + by OEMs. The default value is "@CMAKE_INSTALL_PREFIX@/custom/@LIB_INSTALL_PREFIX@/scopes".

Shouldn't this be without the CMAKE_INSTALL_PREFIX prefix? :)

126 + The defaul value is "$HOME/.local/sahre/unity-scopes".

Typos - "defaul", "sahre".

I also noticed Registry.ini's endpoint option disappeared, is it now just Zmq.ini's EndpointDir.Public + "Registry"?

422 - void run_scope(ScopeBase *const scope_base, std::string const& scope_ini_file);
423 + void run_scope(ScopeBase *const scope_base, std::string const& runtime_ini_file, std::string const& scope_ini_file);

:/ Break in public API. How about overload instead?

477 +static constexpr char const* DFLT_SCOPERUNNER_PATH = "@CMAKE_INSTALL_PREFIX@/@SCOPES_DEFAULT_CONFIGDIR@/scoperunner";
478..484

Please keep in sync with the default config files.

review: Needs Fixing

« Back to merge proposal