Merge lp:~ted/ubuntu-app-launch/snappy-backend into lp:ubuntu-app-launch/16.10
| Status: | Merged | ||||
|---|---|---|---|---|---|
| Approved by: | Ted Gould on 2016-09-13 | ||||
| Approved revision: | 383 | ||||
| Merged at revision: | 253 | ||||
| Proposed branch: | lp:~ted/ubuntu-app-launch/snappy-backend | ||||
| Merge into: | lp:ubuntu-app-launch/16.10 | ||||
| Prerequisite: | lp:~ted/ubuntu-app-launch/snappy-backend-no-snap | ||||
| Diff against target: |
2609 lines (+2102/-13) 28 files modified
.bzrignore (+1/-0) CMakeLists.txt (+3/-0) application-failed.c (+2/-1) debian/control (+1/-0) docs/index.rst (+30/-0) libubuntu-app-launch/CMakeLists.txt (+12/-0) libubuntu-app-launch/appid.h (+4/-1) libubuntu-app-launch/application-impl-legacy.cpp (+13/-0) libubuntu-app-launch/application-impl-snap.cpp (+459/-0) libubuntu-app-launch/application-impl-snap.h (+98/-0) libubuntu-app-launch/application.cpp (+14/-0) libubuntu-app-launch/registry-impl.h (+9/-0) libubuntu-app-launch/registry.cpp (+8/-0) libubuntu-app-launch/snapd-info.cpp (+471/-0) libubuntu-app-launch/snapd-info.h (+76/-0) libubuntu-app-launch/ubuntu-app-launch.cpp (+3/-0) tests/CMakeLists.txt (+18/-0) tests/exec-util-test.cc (+1/-0) tests/failure-test.cc (+17/-9) tests/libual-cpp-test.cc (+199/-1) tests/libual-test.cc (+2/-0) tests/list-apps.cpp (+79/-0) tests/snapd-info-test.cpp (+150/-0) tests/snapd-mock.h (+384/-0) upstart-jobs/CMakeLists.txt (+8/-0) upstart-jobs/application-failed.conf.in (+1/-1) upstart-jobs/application-logrotate.conf (+1/-0) upstart-jobs/application-snap.conf.in (+38/-0) |
||||
| To merge this branch: | bzr merge lp:~ted/ubuntu-app-launch/snappy-backend | ||||
| Related bugs: |
|
| Reviewer | Review Type | Date Requested | Status |
|---|---|---|---|
| Charles Kerr (community) | 2016-08-04 | Approve on 2016-09-08 | |
| unity-api-1-bot | continuous-integration | Approve on 2016-09-08 | |
|
Review via email:
|
|||
This proposal supersedes a proposal from 2016-07-25.
Commit Message
Support launching applications installed as snaps
Description of the Change
Adds a new Application Implementation for Snaps. This also includes the snap-info object which is used to connect to snapd. There are also several adjustments to shared classes to make sure they take the new backend into account.
| unity-api-1-bot (unity-api-1-bot) wrote : | # |
FAILED: Continuous integration, rev:347
https:/
Executed test runs:
FAILURE: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
FAILURE: https:/
FAILURE: https:/
FAILURE: https:/
FAILURE: https:/
FAILURE: https:/
FAILURE: https:/
FAILURE: https:/
FAILURE: https:/
FAILURE: https:/
Click here to trigger a rebuild:
https:/
| unity-api-1-bot (unity-api-1-bot) wrote : | # |
FAILED: Continuous integration, rev:348
https:/
Executed test runs:
FAILURE: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
FAILURE: https:/
FAILURE: https:/
FAILURE: https:/
FAILURE: https:/
FAILURE: https:/
FAILURE: https:/
FAILURE: https:/
FAILURE: https:/
FAILURE: https:/
Click here to trigger a rebuild:
https:/
| unity-api-1-bot (unity-api-1-bot) wrote : | # |
FAILED: Continuous integration, rev:350
https:/
Executed test runs:
FAILURE: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
FAILURE: https:/
FAILURE: https:/
FAILURE: https:/
FAILURE: https:/
FAILURE: https:/
FAILURE: https:/
FAILURE: https:/
FAILURE: https:/
FAILURE: https:/
Click here to trigger a rebuild:
https:/
| unity-api-1-bot (unity-api-1-bot) wrote : | # |
FAILED: Continuous integration, rev:354
https:/
Executed test runs:
FAILURE: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
FAILURE: https:/
SUCCESS: https:/
deb: https:/
FAILURE: https:/
FAILURE: https:/
SUCCESS: https:/
deb: https:/
FAILURE: https:/
FAILURE: https:/
SUCCESS: https:/
deb: https:/
FAILURE: https:/
Click here to trigger a rebuild:
https:/
| Charles Kerr (charlesk) wrote : | # |
eod reached, review comments part 1 inline
| Ted Gould (ted) wrote : | # |
On Wed, 2016-08-24 at 23:35 +0000, Charles Kerr wrote:
> > gchar * appid = g_strdup(instance);
> > - if (g_strcmp0(job, "application-
> > + if (g_strcmp0(job, "application-
> > + || g_strcmp0(job, "application-snap") ==
> > 0) {
> > gchar * lasthyphenstanding = g_strrstr(appid, "-
> > ");
> (not-even-
The answer is that everything else was g_, so went with g_.
> > +/** Path that snapd puts desktop files, we don't want to read
> > those directly
> > + in the Legacy backend. We want to use the snap backend. */
> > +const std::string snappyDesktopPa
> This is interesting, I haven't seen this before. Usually when I see a
> file-private variable it's placed inside an unnamed namespace to
> preserve anonymity, eg
>
> namespace
> {
> const std::string snappyDesktopPa
> }
>
> but this variable here, even though not declared in any header, is
> put here inside an ubuntu:
>
> Not a blocker, but I'd like to understand this more, what are the
> advantages/
> namespace advertised in application-
So I've been trying the other way, trying to get someone to say why
those should be in the anonymous namespace. It seems to me, if it's
related and used by the object it should be in the same namespace as
the object. But I haven't seen any rationale other than "it makes the
symbol name smaller" which seems like a premature optimization at best.
> > + if (std::equal(
> > snappyDesktopPa
> (bikeshed) g_str_has_prefix() would be clearer
Eh, yeah, trying to keep C++ techniques with C++ strings and g_
techniques with gchar strings. I do hate that the C++ string class
doesn't have a "bool prefix(
that it seems every other string library has. :-/
> > + /** Return the xMirEnable value based on whether the interface
> > is
> > + in the list of interfaces using XMir */
> > + XMirEnable xMirEnable() override
> > + {
> > + if (XMIR_INTERFACE
> > XMIR_INTERFACES
> > + {
> > + return XMirEnable:
> I should have called this out in snappy-
> I don't see what type-tagging a bool accomplishes. How does this
> improve on just using a bool?
Well, I think it is as useful as type-tagging any variable. Just to
keep the type in the code and say when you're using it. But this is
internal, and so that's not as big a reason. I did it here just to make
it feel like the other variables and have the same semantics of usage.
> > + /** Figures out the exec line for a snappy command. We're not
> > using
> > + the Exec in the desktop file exactly, but assuming that it
> > is kinda
> > + what we want to be run. So we're replacing that with the
> > script, which
> > + we have to use as we can't get the command that is in the
> > snap
> > + metadata as Snapd won't give it to us. So we're parsing
> > the Exec line
> > + and replaci...
| unity-api-1-bot (unity-api-1-bot) wrote : | # |
FAILED: Continuous integration, rev:368
https:/
Executed test runs:
FAILURE: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
FAILURE: https:/
SUCCESS: https:/
deb: https:/
FAILURE: https:/
FAILURE: https:/
SUCCESS: https:/
deb: https:/
FAILURE: https:/
FAILURE: https:/
SUCCESS: https:/
deb: https:/
FAILURE: https:/
Click here to trigger a rebuild:
https:/
| Charles Kerr (charlesk) wrote : | # |
Some more comments
| unity-api-1-bot (unity-api-1-bot) wrote : | # |
FAILED: Continuous integration, rev:369
https:/
Executed test runs:
FAILURE: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
FAILURE: https:/
SUCCESS: https:/
deb: https:/
FAILURE: https:/
FAILURE: https:/
SUCCESS: https:/
deb: https:/
FAILURE: https:/
FAILURE: https:/
SUCCESS: https:/
deb: https:/
FAILURE: https:/
Click here to trigger a rebuild:
https:/
| unity-api-1-bot (unity-api-1-bot) wrote : | # |
FAILED: Continuous integration, rev:370
https:/
Executed test runs:
FAILURE: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
FAILURE: https:/
SUCCESS: https:/
deb: https:/
FAILURE: https:/
FAILURE: https:/
SUCCESS: https:/
deb: https:/
FAILURE: https:/
FAILURE: https:/
SUCCESS: https:/
deb: https:/
FAILURE: https:/
Click here to trigger a rebuild:
https:/
| unity-api-1-bot (unity-api-1-bot) wrote : | # |
FAILED: Continuous integration, rev:371
https:/
Executed test runs:
FAILURE: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
FAILURE: https:/
SUCCESS: https:/
deb: https:/
FAILURE: https:/
FAILURE: https:/
SUCCESS: https:/
deb: https:/
FAILURE: https:/
FAILURE: https:/
SUCCESS: https:/
deb: https:/
FAILURE: https:/
Click here to trigger a rebuild:
https:/
| unity-api-1-bot (unity-api-1-bot) wrote : | # |
FAILED: Continuous integration, rev:372
https:/
Executed test runs:
FAILURE: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
FAILURE: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
FAILURE: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
FAILURE: https:/
Click here to trigger a rebuild:
https:/
| unity-api-1-bot (unity-api-1-bot) wrote : | # |
PASSED: Continuous integration, rev:373
https:/
Executed test runs:
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
Click here to trigger a rebuild:
https:/
| Ted Gould (ted) wrote : | # |
On Thu, 2016-08-25 at 21:55 +0000, Charles Kerr wrote:
> > +
> > + /* Validation of the object we got */
> > +
> > + for (auto member : {"name", "status", "revision", "type",
> > "version", "apps"})
> auto& :-)
Not sure it helps here since the auto resolves to "const char *" it
would just be copying the pointer. But consistency is good.
r374
> > + {
> > + if (!json_
> > + {
> > +
> > '" + std::string(member) + "'");
> > + }
> > + }
> > +
> > +
> > json_object_
> nullptr guard, here and below for 'status', 'type', 'version',
> 'revision', 'name'
r375
> > +static size_t snapd_writefunc
> > nmemb, void *userdata)
> > +{
> > + unsigned int i;
> > + std:
> > *>(userdata);
> redundant type info, suggest auto on lhs
r376
> >
> > + data-
> > */
> > + for (i = 0; i < size * nmemb; i++)
> > + {
> > +
> > + }
> I'd replace 1033-1037 with "data->
> ptr+(size*nmemb));"
r377
> > + if (error != nullptr)
> > + {
> > +
> > +
> > + throw std::runtime_
> marginally better if this contained error->message
r378
> > + std::string statusstr = json_object_
> > "status");
> nullptr guard here and "type" below
r379
| unity-api-1-bot (unity-api-1-bot) wrote : | # |
PASSED: Continuous integration, rev:380
https:/
Executed test runs:
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
Click here to trigger a rebuild:
https:/
| unity-api-1-bot (unity-api-1-bot) wrote : | # |
FAILED: Continuous integration, rev:381
https:/
Executed test runs:
FAILURE: https:/
SUCCESS: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
FAILURE: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
Click here to trigger a rebuild:
https:/
| unity-api-1-bot (unity-api-1-bot) wrote : | # |
PASSED: Continuous integration, rev:382
https:/
Executed test runs:
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
Click here to trigger a rebuild:
https:/

FAILED: Continuous integration, rev:346 /jenkins. canonical. com/unity- api-1/job/ lp-ubuntu- app-launch- ci/31/ /jenkins. canonical. com/unity- api-1/job/ build/302/ console /jenkins. canonical. com/unity- api-1/job/ build-0- fetch/308 /jenkins. canonical. com/unity- api-1/job/ build-1- sourcepkg/ release= vivid+overlay/ 237 /jenkins. canonical. com/unity- api-1/job/ build-1- sourcepkg/ release= xenial+ overlay/ 237 /jenkins. canonical. com/unity- api-1/job/ build-1- sourcepkg/ release= yakkety/ 237 /jenkins. canonical. com/unity- api-1/job/ build-2- binpkg/ arch=amd64, release= vivid+overlay/ 166/console /jenkins. canonical. com/unity- api-1/job/ build-2- binpkg/ arch=amd64, release= xenial+ overlay/ 166/console /jenkins. canonical. com/unity- api-1/job/ build-2- binpkg/ arch=amd64, release= yakkety/ 166/console /jenkins. canonical. com/unity- api-1/job/ build-2- binpkg/ arch=armhf, release= vivid+overlay/ 166/console /jenkins. canonical. com/unity- api-1/job/ build-2- binpkg/ arch=armhf, release= xenial+ overlay/ 166/console /jenkins. canonical. com/unity- api-1/job/ build-2- binpkg/ arch=armhf, release= yakkety/ 166/console /jenkins. canonical. com/unity- api-1/job/ build-2- binpkg/ arch=i386, release= vivid+overlay/ 166/console /jenkins. canonical. com/unity- api-1/job/ build-2- binpkg/ arch=i386, release= xenial+ overlay/ 166/console /jenkins. canonical. com/unity- api-1/job/ build-2- binpkg/ arch=i386, release= yakkety/ 166/console
https:/
Executed test runs:
FAILURE: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
FAILURE: https:/
FAILURE: https:/
FAILURE: https:/
FAILURE: https:/
FAILURE: https:/
FAILURE: https:/
FAILURE: https:/
FAILURE: https:/
FAILURE: https:/
Click here to trigger a rebuild: /jenkins. canonical. com/unity- api-1/job/ lp-ubuntu- app-launch- ci/31/rebuild
https:/