Ubuntu Application Launcher

Merge lp:~ted/ubuntu-app-launch/lp1626028-legacy-stop into lp:ubuntu-app-launch/16.10

  1. lp1626028-legacy-stop
  2. Merge into trunk.16.10
Proposed by Ted Gould
Status: Merged
Approved by: Albert Astals Cid
Approved revision: 266
Merged at revision: 264
Proposed branch: lp:~ted/ubuntu-app-launch/lp1626028-legacy-stop
Merge into: lp:ubuntu-app-launch/16.10
Diff against target: 332 lines (+70/-33)
10 files modified
libubuntu-app-launch/application-impl-base.cpp (+32/-11)
libubuntu-app-launch/application-impl-base.h (+1/-0)
libubuntu-app-launch/application-impl-click.cpp (+4/-4)
libubuntu-app-launch/application-impl-legacy.cpp (+18/-6)
libubuntu-app-launch/application-impl-legacy.h (+2/-0)
libubuntu-app-launch/application-impl-libertine.cpp (+3/-3)
libubuntu-app-launch/application-impl-snap.cpp (+4/-4)
libubuntu-app-launch/registry-impl.cpp (+4/-4)
libubuntu-app-launch/registry-impl.h (+1/-1)
upstart-jobs/application-legacy.conf.in (+1/-0)
To merge this branch: bzr merge lp:~ted/ubuntu-app-launch/lp1626028-legacy-stop
Reviewer Review Type Date Requested Status
Albert Astals Cid (community) Abstain
Marcus Tomlinson (community) Approve
unity-api-1-bot continuous-integration Approve
Review via email: mp+306679@code.launchpad.net

Commit message

Make use of the instance parameter consistent through all the backends

Description of the change

The legacy stop reported in the bug showed the issue in that we couldn't refind legacy apps in some cases. But as I started to fix things other things broke, so it was clear not all the backends were thinking the same. I think this fixes it so they're all using it the same way. And it fixes the bug.

To post a comment you must log in.
Revision history for this message
unity-api-1-bot (unity-api-1-bot) wrote :

PASSED: Continuous integration, rev:266
https://jenkins.canonical.com/unity-api-1/job/lp-ubuntu-app-launch-ci/106/
Executed test runs:
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build/765
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-0-fetch/771
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=amd64,release=vivid+overlay/579
        deb: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=amd64,release=vivid+overlay/579/artifact/output/*zip*/output.zip
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=amd64,release=xenial+overlay/579
        deb: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=amd64,release=xenial+overlay/579/artifact/output/*zip*/output.zip
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=amd64,release=yakkety/579
        deb: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=amd64,release=yakkety/579/artifact/output/*zip*/output.zip
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=armhf,release=vivid+overlay/579
        deb: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=armhf,release=vivid+overlay/579/artifact/output/*zip*/output.zip
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=armhf,release=xenial+overlay/579
        deb: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=armhf,release=xenial+overlay/579/artifact/output/*zip*/output.zip
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=armhf,release=yakkety/579
        deb: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=armhf,release=yakkety/579/artifact/output/*zip*/output.zip
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=i386,release=vivid+overlay/579
        deb: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=i386,release=vivid+overlay/579/artifact/output/*zip*/output.zip
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=i386,release=xenial+overlay/579
        deb: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=i386,release=xenial+overlay/579/artifact/output/*zip*/output.zip
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=i386,release=yakkety/579
        deb: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=i386,release=yakkety/579/artifact/output/*zip*/output.zip

Click here to trigger a rebuild:
https://jenkins.canonical.com/unity-api-1/job/lp-ubuntu-app-launch-ci/106/rebuild

review: Approve (continuous-integration)
Revision history for this message
Marcus Tomlinson (marcustomlinson) wrote :

Diff looks good, and bug confirmed fixed.

review: Approve
Revision history for this message
Albert Astals Cid (aacid) wrote :

Doesn't seem to work

phablet@ubuntu-phablet:~/lp1626028-legacy-stop/build$ ./tools/ubuntu-app-stop messaging-app

** (process:13790): WARNING **: Unable to stop job application-legacy app_id messaging-app instance_id : GDBus.Error:org.freedesktop.DBus.Error.InvalidArgs: Unknown parameter: INSTANCE_ID

** (process:13790): WARNING **: Unable to stop Upstart instance

review: Needs Fixing
Revision history for this message
Albert Astals Cid (aacid) wrote :

Ok, in conjunction with the silo it works.

review: Abstain

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'libubuntu-app-launch/application-impl-base.cpp'
2--- libubuntu-app-launch/application-impl-base.cpp 2016-09-13 17:42:49 +0000
3+++ libubuntu-app-launch/application-impl-base.cpp 2016-09-23 22:41:29 +0000
4@@ -116,6 +116,13 @@
5
6 return registry_->impl->thread.executeOnThread<pid_t>([this, &jobpath]() -> pid_t {
7 GError* error = nullptr;
8+
9+ std::string instancename = std::string(appId_);
10+ if (job_ != "application-click")
11+ {
12+ instancename += "-" + instance_;
13+ }
14+
15 g_debug("Getting instance by name: %s", instance_.c_str());
16 GVariant* vinstance_path =
17 g_dbus_connection_call_sync(registry_->impl->_dbus.get(), /* connection */
18@@ -123,7 +130,7 @@
19 jobpath.c_str(), /* object path */
20 DBUS_INTERFACE_UPSTART_JOB, /* iface */
21 "GetInstanceByName", /* method */
22- g_variant_new("(s)", instance_.c_str()), /* params */
23+ g_variant_new("(s)", instancename.c_str()), /* params */
24 G_VARIANT_TYPE("(o)"), /* return type */
25 G_DBUS_CALL_FLAGS_NONE, /* flags */
26 -1, /* timeout: default */
27@@ -201,6 +208,27 @@
28 });
29 }
30
31+/** Generate the full name of the Upstart job for the job, the
32+ instance and how all those fit together.
33+
34+ Handles the special case of application-click which isn't designed
35+ to have multi-instance apps.
36+*/
37+std::string UpstartInstance::upstartJobPath()
38+{
39+ std::string path = job_ + "-" + std::string(appId_);
40+ if (job_ != "application-click")
41+ {
42+ path += "-";
43+ }
44+ if (!instance_.empty())
45+ {
46+ path += instance_;
47+ }
48+
49+ return path;
50+}
51+
52 /** Looks at the PIDs in the instance cgroup and checks to see if @pid
53 is in the set.
54
55@@ -208,7 +236,7 @@
56 */
57 bool UpstartInstance::hasPid(pid_t pid)
58 {
59- for (auto testpid : registry_->impl->pidsFromCgroup(job_, instance_))
60+ for (auto testpid : registry_->impl->pidsFromCgroup(upstartJobPath()))
61 if (pid == testpid)
62 return true;
63 return false;
64@@ -217,14 +245,7 @@
65 /** Gets the path to the log file for this instance */
66 std::string UpstartInstance::logPath()
67 {
68- std::string logfile = job_;
69- if (!instance_.empty())
70- {
71- logfile += "-";
72- logfile += instance_;
73- }
74-
75- logfile += ".log";
76+ std::string logfile = upstartJobPath() + ".log";
77
78 gchar* cpath = g_build_filename(g_get_user_cache_dir(), "upstart", logfile.c_str(), nullptr);
79 std::string path(cpath);
80@@ -236,7 +257,7 @@
81 /** Returns all the PIDs that are in the cgroup for this application */
82 std::vector<pid_t> UpstartInstance::pids()
83 {
84- auto pids = registry_->impl->pidsFromCgroup(job_, instance_);
85+ auto pids = registry_->impl->pidsFromCgroup(upstartJobPath());
86 g_debug("Got %d PIDs for AppID '%s'", int(pids.size()), std::string(appId_).c_str());
87 return pids;
88 }
89
90=== modified file 'libubuntu-app-launch/application-impl-base.h'
91--- libubuntu-app-launch/application-impl-base.h 2016-08-26 15:24:44 +0000
92+++ libubuntu-app-launch/application-impl-base.h 2016-09-23 22:41:29 +0000
93@@ -114,6 +114,7 @@
94 void oomValueToPid(pid_t pid, const oom::Score oomvalue);
95 void oomValueToPidHelper(pid_t pid, const oom::Score oomvalue);
96 void pidListToDbus(const std::vector<pid_t>& pids, const std::string& signal);
97+ std::string upstartJobPath();
98
99 static std::shared_ptr<gchar*> urlsToStrv(const std::vector<Application::URL>& urls);
100 static void application_start_cb(GObject* obj, GAsyncResult* res, gpointer user_data);
101
102=== modified file 'libubuntu-app-launch/application-impl-click.cpp'
103--- libubuntu-app-launch/application-impl-click.cpp 2016-09-06 16:13:51 +0000
104+++ libubuntu-app-launch/application-impl-click.cpp 2016-09-23 22:41:29 +0000
105@@ -331,7 +331,7 @@
106 there or return an empty vector */
107 if (sappid == instancename)
108 {
109- vect.emplace_back(std::make_shared<UpstartInstance>(appId(), "application-click", sappid,
110+ vect.emplace_back(std::make_shared<UpstartInstance>(appId(), "application-click", std::string{},
111 std::vector<Application::URL>{}, _registry));
112 break;
113 }
114@@ -360,15 +360,15 @@
115 std::shared_ptr<Application::Instance> Click::launch(const std::vector<Application::URL>& urls)
116 {
117 std::function<std::list<std::pair<std::string, std::string>>(void)> envfunc = [this]() { return launchEnv(); };
118- return UpstartInstance::launch(appId(), "application-click", std::string(appId()), urls, _registry,
119+ return UpstartInstance::launch(appId(), "application-click", {}, urls, _registry,
120 UpstartInstance::launchMode::STANDARD, envfunc);
121 }
122
123 std::shared_ptr<Application::Instance> Click::launchTest(const std::vector<Application::URL>& urls)
124 {
125 std::function<std::list<std::pair<std::string, std::string>>(void)> envfunc = [this]() { return launchEnv(); };
126- return UpstartInstance::launch(appId(), "application-click", std::string(appId()), urls, _registry,
127- UpstartInstance::launchMode::TEST, envfunc);
128+ return UpstartInstance::launch(appId(), "application-click", {}, urls, _registry, UpstartInstance::launchMode::TEST,
129+ envfunc);
130 }
131
132 } // namespace app_impls
133
134=== modified file 'libubuntu-app-launch/application-impl-legacy.cpp'
135--- libubuntu-app-launch/application-impl-legacy.cpp 2016-09-13 17:42:49 +0000
136+++ libubuntu-app-launch/application-impl-legacy.cpp 2016-09-23 22:41:29 +0000
137@@ -33,6 +33,9 @@
138 /** Path that snapd puts desktop files, we don't want to read those directly
139 in the Legacy backend. We want to use the snap backend. */
140 const std::string snappyDesktopPath{"/var/lib/snapd"};
141+/** Special characters that could be an application name that
142+ would activate in a regex */
143+const static std::regex regexCharacters("([\\.\\-])");
144
145 /***********************************
146 Prototypes
147@@ -66,6 +69,14 @@
148 {
149 throw std::runtime_error{"Looking like a legacy app, but should be a Snap: " + appname.value()};
150 }
151+
152+ /* Build a regex that'll match instances of the applications which
153+ roughly looks like: $(appid)-2345345
154+
155+ It is important to filter out the special characters that are in
156+ the appid.
157+ */
158+ instanceRegex_ = std::regex("^(?:" + std::regex_replace(_appname.value(), regexCharacters, "\\$&") + ")\\-(\\d*)$");
159 }
160
161 std::tuple<std::string, std::shared_ptr<GKeyFile>, std::string> keyfileForApp(const AppID::AppName& name)
162@@ -277,10 +288,11 @@
163
164 for (auto instance : _registry->impl->upstartInstancesForJob("application-legacy"))
165 {
166+ std::smatch instanceMatch;
167 g_debug("Looking at legacy instance: %s", instance.c_str());
168- if (std::equal(startsWith.begin(), startsWith.end(), instance.begin()))
169+ if (std::regex_match(instance, instanceMatch, instanceRegex_))
170 {
171- vect.emplace_back(std::make_shared<UpstartInstance>(appId(), "application-legacy", instance,
172+ vect.emplace_back(std::make_shared<UpstartInstance>(appId(), "application-legacy", instanceMatch[1].str(),
173 std::vector<Application::URL>{}, _registry));
174 }
175 }
176@@ -358,8 +370,8 @@
177 std::function<std::list<std::pair<std::string, std::string>>(void)> envfunc = [this, instance]() {
178 return launchEnv(instance);
179 };
180- return UpstartInstance::launch(appId(), "application-legacy", std::string(appId()) + "-" + instance, urls,
181- _registry, UpstartInstance::launchMode::STANDARD, envfunc);
182+ return UpstartInstance::launch(appId(), "application-legacy", instance, urls, _registry,
183+ UpstartInstance::launchMode::STANDARD, envfunc);
184 }
185
186 /** Create an UpstartInstance for this AppID using the UpstartInstance launch
187@@ -373,8 +385,8 @@
188 std::function<std::list<std::pair<std::string, std::string>>(void)> envfunc = [this, instance]() {
189 return launchEnv(instance);
190 };
191- return UpstartInstance::launch(appId(), "application-legacy", std::string(appId()) + "-" + instance, urls,
192- _registry, UpstartInstance::launchMode::TEST, envfunc);
193+ return UpstartInstance::launch(appId(), "application-legacy", instance, urls, _registry,
194+ UpstartInstance::launchMode::TEST, envfunc);
195 }
196
197 } // namespace app_impls
198
199=== modified file 'libubuntu-app-launch/application-impl-legacy.h'
200--- libubuntu-app-launch/application-impl-legacy.h 2016-08-24 21:21:11 +0000
201+++ libubuntu-app-launch/application-impl-legacy.h 2016-09-23 22:41:29 +0000
202@@ -18,6 +18,7 @@
203 */
204
205 #include <gio/gdesktopappinfo.h>
206+#include <regex>
207
208 #include "application-impl-base.h"
209 #include "application-info-desktop.h"
210@@ -82,6 +83,7 @@
211 std::shared_ptr<GKeyFile> _keyfile;
212 std::shared_ptr<app_info::Desktop> appinfo_;
213 std::string desktopPath_;
214+ std::regex instanceRegex_;
215
216 std::list<std::pair<std::string, std::string>> launchEnv(const std::string& instance);
217 std::string getInstance();
218
219=== modified file 'libubuntu-app-launch/application-impl-libertine.cpp'
220--- libubuntu-app-launch/application-impl-libertine.cpp 2016-08-26 15:30:28 +0000
221+++ libubuntu-app-launch/application-impl-libertine.cpp 2016-09-23 22:41:29 +0000
222@@ -266,7 +266,7 @@
223 for (auto instancename : _registry->impl->upstartInstancesForJob("application-legacy"))
224 {
225 if (std::equal(sappid.begin(), sappid.end(), instancename.begin()))
226- vect.emplace_back(std::make_shared<UpstartInstance>(appId(), "application-legacy", sappid + "-",
227+ vect.emplace_back(std::make_shared<UpstartInstance>(appId(), "application-legacy", std::string{},
228 std::vector<Application::URL>{}, _registry));
229 }
230
231@@ -312,14 +312,14 @@
232 std::shared_ptr<Application::Instance> Libertine::launch(const std::vector<Application::URL>& urls)
233 {
234 std::function<std::list<std::pair<std::string, std::string>>(void)> envfunc = [this]() { return launchEnv(); };
235- return UpstartInstance::launch(appId(), "application-legacy", std::string(appId()) + "-", urls, _registry,
236+ return UpstartInstance::launch(appId(), "application-legacy", {}, urls, _registry,
237 UpstartInstance::launchMode::STANDARD, envfunc);
238 }
239
240 std::shared_ptr<Application::Instance> Libertine::launchTest(const std::vector<Application::URL>& urls)
241 {
242 std::function<std::list<std::pair<std::string, std::string>>(void)> envfunc = [this]() { return launchEnv(); };
243- return UpstartInstance::launch(appId(), "application-legacy", std::string(appId()) + "-", urls, _registry,
244+ return UpstartInstance::launch(appId(), "application-legacy", {}, urls, _registry,
245 UpstartInstance::launchMode::TEST, envfunc);
246 }
247
248
249=== modified file 'libubuntu-app-launch/application-impl-snap.cpp'
250--- libubuntu-app-launch/application-impl-snap.cpp 2016-08-25 17:56:40 +0000
251+++ libubuntu-app-launch/application-impl-snap.cpp 2016-09-23 22:41:29 +0000
252@@ -397,7 +397,7 @@
253 {
254 if (std::equal(startsWith.begin(), startsWith.end(), instance.begin()))
255 {
256- vect.emplace_back(std::make_shared<UpstartInstance>(appid_, "application-snap", instance,
257+ vect.emplace_back(std::make_shared<UpstartInstance>(appid_, "application-snap", std::string{},
258 std::vector<Application::URL>{}, _registry));
259 }
260 }
261@@ -438,7 +438,7 @@
262 std::shared_ptr<Application::Instance> Snap::launch(const std::vector<Application::URL>& urls)
263 {
264 std::function<std::list<std::pair<std::string, std::string>>(void)> envfunc = [this]() { return launchEnv(); };
265- return UpstartInstance::launch(appid_, "application-snap", std::string(appid_) + "-", urls, _registry,
266+ return UpstartInstance::launch(appid_, "application-snap", {}, urls, _registry,
267 UpstartInstance::launchMode::STANDARD, envfunc);
268 }
269
270@@ -450,8 +450,8 @@
271 std::shared_ptr<Application::Instance> Snap::launchTest(const std::vector<Application::URL>& urls)
272 {
273 std::function<std::list<std::pair<std::string, std::string>>(void)> envfunc = [this]() { return launchEnv(); };
274- return UpstartInstance::launch(appid_, "application-snap", std::string(appid_) + "-", urls, _registry,
275- UpstartInstance::launchMode::TEST, envfunc);
276+ return UpstartInstance::launch(appid_, "application-snap", {}, urls, _registry, UpstartInstance::launchMode::TEST,
277+ envfunc);
278 }
279
280 } // namespace app_impls
281
282=== modified file 'libubuntu-app-launch/registry-impl.cpp'
283--- libubuntu-app-launch/registry-impl.cpp 2016-09-06 16:13:19 +0000
284+++ libubuntu-app-launch/registry-impl.cpp 2016-09-23 22:41:29 +0000
285@@ -270,18 +270,18 @@
286 all of the PIDs. It is important to note that this is an IPC call, so it can
287 by its nature, be racy. Once the message has been sent the group can change.
288 You should take that into account in your usage of it. */
289-std::vector<pid_t> Registry::Impl::pidsFromCgroup(const std::string& job, const std::string& instance)
290+std::vector<pid_t> Registry::Impl::pidsFromCgroup(const std::string& jobpath)
291 {
292 initCGManager();
293 auto lmanager = cgManager_; /* Grab a local copy so we ensure it lasts through our lifetime */
294
295- return thread.executeOnThread<std::vector<pid_t>>([&job, &instance, lmanager]() -> std::vector<pid_t> {
296+ return thread.executeOnThread<std::vector<pid_t>>([&jobpath, lmanager]() -> std::vector<pid_t> {
297 GError* error = nullptr;
298 const gchar* name = g_getenv("UBUNTU_APP_LAUNCH_CG_MANAGER_NAME");
299 std::string groupname;
300- if (!job.empty())
301+ if (!jobpath.empty())
302 {
303- groupname = "upstart/" + job + "-" + instance;
304+ groupname = "upstart/" + jobpath;
305 }
306
307 g_debug("Looking for cg manager '%s' group '%s'", name, groupname.c_str());
308
309=== modified file 'libubuntu-app-launch/registry-impl.h'
310--- libubuntu-app-launch/registry-impl.h 2016-09-06 16:14:08 +0000
311+++ libubuntu-app-launch/registry-impl.h 2016-09-23 22:41:29 +0000
312@@ -73,7 +73,7 @@
313
314 void zgSendEvent(AppID appid, const std::string& eventtype);
315
316- std::vector<pid_t> pidsFromCgroup(const std::string& job, const std::string& instance);
317+ std::vector<pid_t> pidsFromCgroup(const std::string& jobpath);
318
319 /* Upstart Jobs */
320 std::list<std::string> upstartInstancesForJob(const std::string& job);
321
322=== modified file 'upstart-jobs/application-legacy.conf.in'
323--- upstart-jobs/application-legacy.conf.in 2015-08-17 21:34:39 +0000
324+++ upstart-jobs/application-legacy.conf.in 2016-09-23 22:41:29 +0000
325@@ -12,6 +12,7 @@
326 env APP_URIS
327 env APP_DESKTOP_FILE_PATH
328 env APP_XMIR_ENABLE
329+env INSTANCE_ID=""
330
331 # This will be set to "unconfined" by desktop-exec if there is no confinement defined
332 apparmor switch $APP_EXEC_POLICY

Subscribers

People subscribed via source and target branches