Merge lp:~michihenning/thumbnailer/snap into lp:thumbnailer/devel

Proposed by Michi Henning on 2017-01-25
Status: Merged
Approved by: James Henstridge on 2017-01-31
Approved revision: 371
Merged at revision: 369
Proposed branch: lp:~michihenning/thumbnailer/snap
Merge into: lp:thumbnailer/devel
Diff against target: 127 lines (+90/-5)
4 files modified
.bzrignore (+5/-1)
snapcraft.yaml (+80/-0)
tests/copyright/check_copyright.sh (+1/-1)
tests/whitespace/CMakeLists.txt (+4/-3)
To merge this branch: bzr merge lp:~michihenning/thumbnailer/snap
Reviewer Review Type Date Requested Status
James Henstridge 2017-01-25 Approve on 2017-01-31
unity-api-1-bot continuous-integration Approve on 2017-01-30
Review via email: mp+315538@code.launchpad.net

Commit message

First stab at snapcraft.yaml.

Description of the change

First stab at snapcraft.yaml.

It's not clear yet whether we really want to use the generic DBus interface or add our own interface to snapd. For now, we use the generic one; this works, in that thumbnailer-admin can talk to the (hand-started) thumbnailer-service. Presumably, that's because both are in the same snap. Next step will be to try with thumbnailer-admin in a separate snap.

To try this (from the top-level dir):

$ snapcraft
$ thumbnailer.thumbnailer-service &
$ thumbnailer.thumbnailer-admin stats

To post a comment you must log in.
James Henstridge (jamesh) wrote :

One small problem, and a few questions.

review: Needs Fixing
Michi Henning (michihenning) wrote :

Replied inline, thanks!

James Henstridge (jamesh) wrote :

Looks good. I am a bit concerned about the "control" slot you're exposing though: the dbus interface auto connects, and I don't think we want this one auto-connecting.

Also, we don't ever acquire the name "com.canonical.ThumbnailerAdmin", so that doesn't sound quite right either.

I wonder if there is anything to allow snaps to communicate with other binaries from the same snap via dbus? If so, then we wouldn't even need this second slot.

James Henstridge (jamesh) wrote :

I've added a few inline comments.

These are the changes I made while testing out the snap: http://paste.ubuntu.com/23873664/. With those changes, I tried using my second snap to access the thumbnailer:

  $ sudo snap install --dangerous thumbnailer-admin-test_0.1_amd64.snap
  $ sudo connect thumbnailer-admin-test:thumbnailer thumbnailer:thumbnailer
  $ thumbnailer-admin-test.admin get-album Radiohead 'Ok Computer'
  thumbnailer-admin: Handler::createFinished(): could not get thumbnail for album: Radiohead/Ok Computer (0,0): ERROR

On the service side, I got the following:

  $ thumbnailer.thumbnailer-service
  thumbnailer-service: [15:57:10.144] Initializing
  thumbnailer-service: [15:57:10.440] image cache: 0 entries, 0 bytes, hit rate 0.00 (0/0), avg hit run 0.00, avg miss run 0.00
  thumbnailer-service: [15:57:10.440] thumbnail cache: 0 entries, 0 bytes, hit rate 0.00 (0/0), avg hit run 0.00, avg miss run 0.00
  thumbnailer-service: [15:57:10.440] failure cache: 0 entries, 0 bytes, hit rate 0.00 (0/0), avg hit run 0.00, avg miss run 0.00

  (process:13511): GdkPixbuf-WARNING **: Cannot open pixbuf loader module file '/home/james/snap/thumbnailer/x1/.cache/gdk-pixbuf-loaders.cache': No such file or directory

  This likely means that your installation is broken.
  Try running the command
    gdk-pixbuf-query-loaders > /home/james/snap/thumbnailer/x1/.cache/gdk-pixbuf-loaders.cache
to make things work again for the time being.

  (process:13511): GdkPixbuf-WARNING **: Cannot open pixbuf loader module file '/home/james/snap/thumbnailer/x1/.cache/gdk-pixbuf-loaders.cache': No such file or directory

  This likely means that your installation is broken.
  Try running the command
    gdk-pixbuf-query-loaders > /home/james/snap/thumbnailer/x1/.cache/gdk-pixbuf-loaders.cache
to make things work again for the time being.
  thumbnailer-service: [15:57:30.227] Warning: "Handler::createFinished(): could not get thumbnail for album: Radiohead/Ok Computer (0,0): ERROR"

So it is still not quite right. It looks like there is some support for gdk-pixbuf in the desktop-launch script, but it only comes into play if we ship gdk-pixbuf plus loaders as part of our snap.

review: Needs Fixing
unity-api-1-bot (unity-api-1-bot) wrote :

PASSED: Continuous integration, rev:371
https://jenkins.canonical.com/unity-api-1/job/lp-thumbnailer-ci/19/
Executed test runs:
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build/1535
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-0-fetch/1542
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=amd64,release=xenial+overlay/1320
        deb: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=amd64,release=xenial+overlay/1320/artifact/output/*zip*/output.zip
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=amd64,release=zesty/1320
        deb: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=amd64,release=zesty/1320/artifact/output/*zip*/output.zip
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=armhf,release=xenial+overlay/1320
        deb: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=armhf,release=xenial+overlay/1320/artifact/output/*zip*/output.zip
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=armhf,release=zesty/1320
        deb: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=armhf,release=zesty/1320/artifact/output/*zip*/output.zip
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=i386,release=xenial+overlay/1320
        deb: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=i386,release=xenial+overlay/1320/artifact/output/*zip*/output.zip
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=i386,release=zesty/1320
        deb: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=i386,release=zesty/1320/artifact/output/*zip*/output.zip

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

review: Approve (continuous-integration)
James Henstridge (jamesh) wrote :

Okay, I'm now able to build a thumbnailer snap that can talk to the thumbnailer-admin-test snap without any post install fixups.

To make sure the new version of the part is used, I rebuilt with:

    snapcraft clean
    snapcraft update
    snapcraft

(The "update" operation downloads a new copy of parts.yaml).

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file '.bzrignore'
2--- .bzrignore 2013-09-20 14:31:57 +0000
3+++ .bzrignore 2017-01-30 03:06:38 +0000
4@@ -1,4 +1,8 @@
5-build
6+./build
7 .project
8 .cproject
9 .settings
10+./parts
11+./prime
12+./stage
13+./thumbnailer*.snap
14
15=== added file 'snapcraft.yaml'
16--- snapcraft.yaml 1970-01-01 00:00:00 +0000
17+++ snapcraft.yaml 2017-01-30 03:06:38 +0000
18@@ -0,0 +1,80 @@
19+name: thumbnailer
20+version: '2.4'
21+summary: Daemon and admin tool to create and cache image thumbnails for media files
22+description: >
23+ This snap provides thumbnailer-service, a DBus daemon that extracts thumbnails from
24+ local media files and retrieves music album artwork from dash.ubuntu.com. It also
25+ includes thumbnailer-admin, a command-line utility that allows for basic cache
26+ maintenance and testing. See lp:thumbnailer for more detail.
27+grade: devel
28+confinement: strict
29+
30+slots:
31+ thumbnailer:
32+ interface: dbus
33+ name: com.canonical.Thumbnailer
34+ bus: session
35+
36+plugs:
37+ platform:
38+ interface: content
39+ content: ubuntu-app-platform1
40+ target: ubuntu-app-platform
41+ default-provider: ubuntu-app-platform
42+ network-manager:
43+ interface: network-manager
44+ thumbnails:
45+ interface: dbus
46+ name: com.canonical.Thumbnailer
47+ bus: session
48+
49+apps:
50+ thumbnailer-service:
51+ command: desktop-launch $SNAP/lib/thumbnailer/thumbnailer-service
52+ # TODO: Currently cannot use a simple daemon with the session bus. For now,
53+ # the service must be started by hand. See https://github.com/snapcore/snapd/pull/2592
54+ # daemon: simple
55+ plugs:
56+ - platform
57+ - network
58+ # TODO: This grants more privileges than we really need but, without this, apparmor
59+ # denies access.
60+ - network-manager
61+ # TODO: The gsettings interface currently does not permit reading.
62+ # See https://bugs.launchpad.net/snappy/+bug/1600154
63+ # For now, the service uses default settings because it's gsettings lookups fail.
64+ - gsettings
65+ slots:
66+ - thumbnailer
67+ thumbnailer-admin:
68+ command: desktop-launch $SNAP/bin/thumbnailer-admin
69+ plugs:
70+ - platform
71+ - thumbnails
72+
73+parts:
74+ thumbnailer:
75+ plugin: cmake
76+ source: .
77+ after:
78+ - desktop-ubuntu-app-platform
79+ filesets:
80+ binaries:
81+ - bin/*
82+ - lib/thumbnailer/*
83+ man:
84+ - share/man/*
85+ etc:
86+ - etc/*
87+ share:
88+ # TODO: This does not seem to have any effect. It is unclear right now how gsettings
89+ # schemas need to be installed.
90+ - share/glib-2.0/*
91+ prime:
92+ - $binaries
93+ - $man
94+ - $etc
95+ - $share
96+ - ubuntu-app-platform
97+ install: |
98+ mkdir $SNAPCRAFT_PART_INSTALL/ubuntu-app-platform
99
100=== modified file 'tests/copyright/check_copyright.sh'
101--- tests/copyright/check_copyright.sh 2016-05-30 09:44:33 +0000
102+++ tests/copyright/check_copyright.sh 2017-01-30 03:06:38 +0000
103@@ -31,7 +31,7 @@
104 [ $# -lt 1 ] && usage
105 [ $# -gt 2 ] && usage
106
107-ignore_pat="\\.sci$|\\.swp$|\\.bzr|debian|qmldir|HACKING|tsan-suppress|valgrind-suppress|testsong_ogg|\\.txt$|\\.xml$|\\.in$|\\.dox$|\\.html$|\\.map$|\\.aiff$|\\.gif$|\\.jpg$|\\.JPG$|\\.mp2$|\\.mp3$|\\.m4a$|\\.m4v$|\\.flac$|\\.oga$|\\.ogg$|\\.opus$|\\.png$|\\.svg$|\\.wav$|UseGSettings.cmake$"
108+ignore_pat="\\.sci$|\\.swp$|\\.bzr|debian|qmldir|HACKING|tsan-suppress|valgrind-suppress|testsong_ogg|\\.txt$|\\.xml$|\\.in$|\\.dox$|\\.html$|\\.map$|\\.aiff$|\\.gif$|\\.jpg$|\\.JPG$|\\.mp2$|\\.mp3$|\\.m4a$|\\.m4v$|\\.flac$|\\.oga$|\\.ogg$|\\.opus$|\\.png$|\\.svg$|\\.wav$|UseGSettings.cmake$|\\.yaml$|parts|prime|stage|~[0-9]+~$"
109
110 #
111 # We don't use the -i option of licensecheck to add ignore_dir to the pattern because Jenkins creates directories
112
113=== modified file 'tests/whitespace/CMakeLists.txt'
114--- tests/whitespace/CMakeLists.txt 2015-05-22 03:13:47 +0000
115+++ tests/whitespace/CMakeLists.txt 2017-01-30 03:06:38 +0000
116@@ -3,7 +3,8 @@
117 #
118
119 add_test(whitespace
120- ${CMAKE_CURRENT_SOURCE_DIR}/check_whitespace.py
121- ${CMAKE_SOURCE_DIR}
122- ${CMAKE_BINARY_DIR}
123+ ${CMAKE_CURRENT_SOURCE_DIR}/check_whitespace.py ${CMAKE_SOURCE_DIR} ${CMAKE_BINARY_DIR}
124+ ${CMAKE_SOURCE_DIR}/parts
125+ ${CMAKE_SOURCE_DIR}/stage
126+ ${CMAKE_BINARY_DIR}/prime
127 )

Subscribers

People subscribed via source and target branches

to all changes: