unity-scopes-api

Merge lp:~mterry/unity-scopes-api/snap-root into lp:unity-scopes-api/devel

  1. snap-root
  2. Merge into devel
Proposed by Michael Terry
Status: Merged
Approved by: Michi Henning
Approved revision: 375
Merged at revision: 692
Proposed branch: lp:~mterry/unity-scopes-api/snap-root
Merge into: lp:unity-scopes-api/devel
Diff against target: 155 lines (+36/-9)
7 files modified
data/scope-registry.conf.in (+1/-1)
data/smart-scopes-proxy.conf.in (+1/-1)
include/unity/scopes/internal/ConfigBase.h (+2/-0)
src/scopes/internal/ConfigBase.cpp (+18/-1)
src/scopes/internal/RegistryConfig.cpp (+2/-2)
src/scopes/internal/RuntimeConfig.cpp (+4/-4)
src/scopes/internal/ScopeConfig.cpp (+8/-0)
To merge this branch: bzr merge lp:~mterry/unity-scopes-api/snap-root
Reviewer Review Type Date Requested Status
Michi Henning (community) Approve
unity-api-1-bot continuous-integration Approve
Review via email: mp+308609@code.launchpad.net

This proposal supersedes a proposal from 2016-10-06.

Commit message

Handle running inside a snap by paying attention to the $SNAP prefix.

Description of the change

Handle running inside a snap by paying attention to the $SNAP prefix.

To post a comment you must log in.
Revision history for this message
Marcus Tomlinson (marcustomlinson) wrote : Posted in a previous version of this proposal

Thanks Michael! Michi and I do acknowledged this MP, but we'd like to investigate the possibility of a cleaner approach whereby we reduce some of the redundancy (especially with getenv()) that's introduced here.

This is not a stab at your work by any means ;) Thanks for your help with this!

Revision history for this message
Michi Henning (michihenning) wrote : Posted in a previous version of this proposal

Could you re-target this MR at the devel branch instead of trunk please? That allows us to use our normal staging process.

Is $SNAP always going to have a trailing slash? If not, constructs such as

$SNAP@CMAKE_INSTALL_PREFIX@/@CMAKE_INSTALL_LIBDIR@/@UNITY_SCOPES_LIB@/scoperegistry

and

snap_root + base_dflt_file

won't create the path name correctly. Wouldn't it be safer to append a slash regardless? If we end up with two adjacent slashes, they won't do any harm.

This does not work:

const string snap_root = getenv("SNAP");

If SNAP isn't set, getenv() returns a nullptr, which causes the string constructor to throw. That causes a bunch of tests to fail.

The repeated calls to getenv("SNAP") are a bit of a pain (and dangerous because of the needed check for a nullptr return). It would be better to do this is ConfigBase and and provide a protected accessor for the value. That puts the code to sanitize the value in a single place.

review: Needs Fixing
Revision history for this message
Michi Henning (michihenning) wrote : Posted in a previous version of this proposal

Here is a diff against this branch that makes some changes. Could you review please?

Thanks,

Michi.

Revision history for this message
Michael Terry (mterry) wrote : Posted in a previous version of this proposal

> Is $SNAP always going to have a trailing slash? If not, constructs such as
> [snip[
> won't create the path name correctly. Wouldn't it be safer to append a slash
> regardless? If we end up with two adjacent slashes, they won't do any harm.

@CMAKE_INSTALL_PREFIX@ is guaranteed to either be empty or start with a slash, right? So that's fine. And base_dflt_file always starts with a slash. So I think we're fine there.

> If SNAP isn't set, getenv() returns a nullptr, which causes the string
> constructor to throw. That causes a bunch of tests to fail.

Ick, right you are. I wrote so many of these in a row, I got confused between QString and std::string.

> It would be better to do this is ConfigBase and and provide a protected
> accessor for the value. That puts the code to sanitize the value in a single
> place.

OK.

> Here is a diff against this branch that makes some changes. Could you review please?

I would go make your suggested changes, but it sounds like you were trying to upload a diff to make them for me? I don't see one. I don't think LP MPs can have attachments.

Revision history for this message
Michi Henning (michihenning) wrote : Posted in a previous version of this proposal

So sorry, got distracted by my wife with an imap issue in the middle of things.

It's a it of a let-down that I can't add an attachment to comments here.

Here's a link to the diff:

https://www.dropbox.com/s/qbbs3yeo6f5ovom/snap_root.diff?dl=0

Revision history for this message
Michi Henning (michihenning) wrote : Posted in a previous version of this proposal

> @CMAKE_INSTALL_PREFIX@ is guaranteed to either be empty or start with a slash,
> right? So that's fine. And base_dflt_file always starts with a slash. So I
> think we're fine there.

Not necessarily. I can set it to anything when I run the configure step.

I think having the slash there is safer, and it won't do any harm if there is an extra one.

Revision history for this message
Michael Terry (mterry) wrote : Posted in a previous version of this proposal

Thanks! I've applied your patch.

Revision history for this message
Michi Henning (michihenning) wrote : Posted in a previous version of this proposal

Still need to re-target this at the devel branch, otherwise Jenkins won't test it. Could you do that please?

Revision history for this message
Michael Terry (mterry) wrote :

Whoops, done.

Revision history for this message
unity-api-1-bot (unity-api-1-bot) wrote :

PASSED: Continuous integration, rev:375
https://jenkins.canonical.com/unity-api-1/job/lp-unity-scopes-api-ci/50/
Executed test runs:
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build/889
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-0-fetch/896
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=amd64,release=vivid+overlay/702
        deb: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=amd64,release=vivid+overlay/702/artifact/output/*zip*/output.zip
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=amd64,release=xenial+overlay/702
        deb: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=amd64,release=xenial+overlay/702/artifact/output/*zip*/output.zip
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=amd64,release=yakkety/702
        deb: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=amd64,release=yakkety/702/artifact/output/*zip*/output.zip
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=armhf,release=vivid+overlay/702
        deb: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=armhf,release=vivid+overlay/702/artifact/output/*zip*/output.zip
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=armhf,release=xenial+overlay/702
        deb: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=armhf,release=xenial+overlay/702/artifact/output/*zip*/output.zip
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=armhf,release=yakkety/702
        deb: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=armhf,release=yakkety/702/artifact/output/*zip*/output.zip
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=i386,release=vivid+overlay/702
        deb: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=i386,release=vivid+overlay/702/artifact/output/*zip*/output.zip
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=i386,release=xenial+overlay/702
        deb: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=i386,release=xenial+overlay/702/artifact/output/*zip*/output.zip
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=i386,release=yakkety/702
        deb: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=i386,release=yakkety/702/artifact/output/*zip*/output.zip

Click here to trigger a rebuild:
https://jenkins.canonical.com/unity-api-1/job/lp-unity-scopes-api-ci/50/rebuild

review: Approve (continuous-integration)
Revision history for this message
Michi Henning (michihenning) wrote :

Cool, all good, thanks!

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'data/scope-registry.conf.in'
2--- data/scope-registry.conf.in 2014-05-07 11:45:50 +0000
3+++ data/scope-registry.conf.in 2016-10-17 08:49:46 +0000
4@@ -7,4 +7,4 @@
5 respawn
6 respawn limit 10 60
7
8-exec @CMAKE_INSTALL_PREFIX@/@CMAKE_INSTALL_LIBDIR@/@UNITY_SCOPES_LIB@/scoperegistry
9+exec $SNAP/@CMAKE_INSTALL_PREFIX@/@CMAKE_INSTALL_LIBDIR@/@UNITY_SCOPES_LIB@/scoperegistry
10
11=== modified file 'data/smart-scopes-proxy.conf.in'
12--- data/smart-scopes-proxy.conf.in 2014-05-07 11:45:50 +0000
13+++ data/smart-scopes-proxy.conf.in 2016-10-17 08:49:46 +0000
14@@ -9,4 +9,4 @@
15
16 expect stop
17
18-exec @CMAKE_INSTALL_PREFIX@/@CMAKE_INSTALL_LIBDIR@/@UNITY_SCOPES_LIB@/smartscopesproxy upstart
19+exec $SNAP/@CMAKE_INSTALL_PREFIX@/@CMAKE_INSTALL_LIBDIR@/@UNITY_SCOPES_LIB@/smartscopesproxy upstart
20
21=== modified file 'include/unity/scopes/internal/ConfigBase.h'
22--- include/unity/scopes/internal/ConfigBase.h 2014-11-03 05:31:30 +0000
23+++ include/unity/scopes/internal/ConfigBase.h 2016-10-17 08:49:46 +0000
24@@ -59,6 +59,7 @@
25 virtual std::string get_middleware(std::string const& group, std::string const& key) const;
26
27 protected:
28+ std::string snap_root() const;
29 void throw_ex(::std::string const& reason) const;
30 bool path_exists(::std::string const& path) const;
31
32@@ -70,6 +71,7 @@
33 private:
34 unity::util::IniParser::SPtr parser_;
35 std::string configfile_;
36+ std::string snap_root_;
37 };
38
39 } // namespace internal
40
41=== modified file 'src/scopes/internal/ConfigBase.cpp'
42--- src/scopes/internal/ConfigBase.cpp 2014-08-28 00:20:56 +0000
43+++ src/scopes/internal/ConfigBase.cpp 2016-10-17 08:49:46 +0000
44@@ -43,10 +43,22 @@
45 // If configfile is the empty string, we create a default instance that returns "Zmq" for the middleware
46 // and throws for the other methods.
47
48-ConfigBase::ConfigBase(string const& configfile, string const& dflt_file) :
49+ConfigBase::ConfigBase(string const& configfile, string const& base_dflt_file) :
50 parser_(nullptr),
51 configfile_(configfile)
52 {
53+ char const* sroot = getenv("SNAP");
54+ if (sroot)
55+ {
56+ snap_root_ = sroot;
57+ if (!snap_root_.empty() && snap_root_[snap_root_.size() - 1] != '/')
58+ {
59+ snap_root_ += '/';
60+ }
61+ }
62+
63+ const string dflt_file = base_dflt_file.empty() ? "" : snap_root_ + base_dflt_file;
64+
65 if (!configfile.empty())
66 {
67 boost::filesystem::path path(configfile);
68@@ -157,6 +169,11 @@
69 return val;
70 }
71
72+string ConfigBase::snap_root() const
73+{
74+ return snap_root_;
75+}
76+
77 void ConfigBase::throw_ex(string const& reason) const
78 {
79 string s = "\"" + configfile_ + "\": " + reason;
80
81=== modified file 'src/scopes/internal/RegistryConfig.cpp'
82--- src/scopes/internal/RegistryConfig.cpp 2016-08-31 17:54:04 +0000
83+++ src/scopes/internal/RegistryConfig.cpp 2016-10-17 08:49:46 +0000
84@@ -53,7 +53,7 @@
85 identity_ = identity;
86 mw_kind_ = get_middleware(registry_config_group, mw_kind_key);
87 mw_configfile_ = get_optional_string(registry_config_group, mw_kind_ + configfile_key);
88- scope_installdir_ = get_optional_string(registry_config_group, scope_installdir_key, DFLT_SCOPE_INSTALL_DIR);
89+ scope_installdir_ = get_optional_string(registry_config_group, scope_installdir_key, snap_root() + DFLT_SCOPE_INSTALL_DIR);
90 oem_installdir_ = get_optional_string(registry_config_group, oem_installdir_key, DFLT_OEM_INSTALL_DIR);
91 click_installdir_ = get_optional_string(registry_config_group, click_installdir_key);
92 if (click_installdir_.empty())
93@@ -65,7 +65,7 @@
94 }
95 click_installdir_ = string(home) + "/.local/share/unity-scopes/";
96 }
97- scoperunner_path_ = get_optional_string(registry_config_group, scoperunner_path_key, DFLT_SCOPERUNNER_PATH);
98+ scoperunner_path_ = get_optional_string(registry_config_group, scoperunner_path_key, snap_root() + DFLT_SCOPERUNNER_PATH);
99 if (scoperunner_path_[0] != '/')
100 {
101 throw ConfigException(configfile + ": " + scoperunner_path_key + " must be an absolute path");
102
103=== modified file 'src/scopes/internal/RuntimeConfig.cpp'
104--- src/scopes/internal/RuntimeConfig.cpp 2016-02-22 01:29:01 +0000
105+++ src/scopes/internal/RuntimeConfig.cpp 2016-10-17 08:49:46 +0000
106@@ -63,11 +63,11 @@
107 if (configfile.empty()) // Default config
108 {
109 registry_identity_ = DFLT_REGISTRY_ID;
110- registry_configfile_ = DFLT_REGISTRY_INI;
111+ registry_configfile_ = snap_root() + DFLT_REGISTRY_INI;
112 ss_registry_identity_ = DFLT_SS_REGISTRY_ID;
113- ss_configfile_ = DFLT_SS_REGISTRY_INI;
114+ ss_configfile_ = snap_root() + DFLT_SS_REGISTRY_INI;
115 default_middleware_ = DFLT_MIDDLEWARE;
116- default_middleware_configfile_ = DFLT_ZMQ_MIDDLEWARE_INI;
117+ default_middleware_configfile_ = snap_root() + DFLT_ZMQ_MIDDLEWARE_INI;
118 reap_expiry_ = DFLT_REAP_EXPIRY;
119 reap_interval_ = DFLT_REAP_INTERVAL;
120 cache_directory_ = default_cache_directory();
121@@ -83,7 +83,7 @@
122 default_middleware_ = get_middleware(runtime_config_group, default_middleware_key);
123 default_middleware_configfile_ = get_optional_string(runtime_config_group,
124 default_middleware_ + default_middleware_configfile_key,
125- DFLT_MIDDLEWARE_INI);
126+ snap_root() + DFLT_MIDDLEWARE_INI);
127 reap_expiry_ = get_optional_int(runtime_config_group, reap_expiry_key, DFLT_REAP_EXPIRY);
128 if (reap_expiry_ < 1 && reap_expiry_ != -1)
129 {
130
131=== modified file 'src/scopes/internal/ScopeConfig.cpp'
132--- src/scopes/internal/ScopeConfig.cpp 2016-10-12 12:34:58 +0000
133+++ src/scopes/internal/ScopeConfig.cpp 2016-10-17 08:49:46 +0000
134@@ -115,6 +115,10 @@
135 try
136 {
137 string art = parser()->get_string(scope_config_group, art_key);
138+ if (!art.empty() && art[0] == '/')
139+ {
140+ art = snap_root() + art;
141+ }
142 art_.reset(new string(art));
143 }
144 catch (LogicException const&)
145@@ -123,6 +127,10 @@
146 try
147 {
148 string icon = parser()->get_string(scope_config_group, icon_key);
149+ if (!icon.empty() && icon[0] == '/')
150+ {
151+ icon = snap_root() + icon;
152+ }
153 icon_.reset(new string(icon));
154 }
155 catch (LogicException const&)

Subscribers

People subscribed via source and target branches

to all changes: