Initial addition of ubuntu-app-platform shared snap use for Qt and other Ubuntu's libraries. #18

Merged
merged 3 commits into from Nov 7, 2016

Conversation

Projects
None yet
2 participants

tjyrinki commented Nov 4, 2016

No description provided.

tjyrinki commented Nov 4, 2016

For comments or to be merged after fixes. Not sure how to test this locally, but the working example of using ubuntu-app-runtime snap is at timostestapp3. Integrating the launcher specifities to here should be straight-forward, but I'm not sure about if the content plug can be auto-enabled via cloud part or if the five lines to define it and its (what should be fixed) mount point still need to be copy-pasted by the app developer.

Excellent work Timo! I really like this patch and I would have come up with a very similar solution.
I have a few comments though to enhance it, but the general idea is perfect!

@@ -9,6 +9,8 @@
export QTCHOOSER_NO_GLOBAL_DIR=1
if [ "$USE_qt5" = true ]; then
export QT_SELECT=snappy-qt5
+elif [ "$USE_qt5-app-platform" = true ]; then
@didrocks

didrocks Nov 7, 2016

Owner

Here or above (under that variable condition), I would add a test like:
ls -d $SNAP/ubuntu-app-runtime >/dev/null 2>&1 and it that fails, prints:

You need to connect the ubuntu-app-runtime package with your application to reuse shared assets, please run:
snap install ubuntu-app-runtime
snap connect ubuntu-app-runtime:platform $SNAP_NAME:platform

For instance :)

qt/snapcraft.yaml
@@ -33,3 +33,8 @@ parts:
- libqt5svg5 # for loading icon themes which are svg
- appmenu-qt5
+#plugs:
+# ubuntu-app-runtime:
@didrocks

didrocks Nov 7, 2016

Owner

I would name that plug "platform" (on both sides) as it provides a platform and we want multiple apps to support it. That way, we can standard the plug and slot name for all platform snap. Making sense?

@tjyrinki

tjyrinki Nov 7, 2016

Yes it makes sense and also shorter for connecting and use.

snapcraft.yaml
@@ -29,6 +30,9 @@ description: |
- read/write to gsettings:
plugs: [gsettings, home]
(note that the home plug is needed to read new value)
+ - use of the shared platform snap content:
+ plugs: [ubuntu-app-platform]
+ (content interface plug to use ubuntu-app-platform slot needed)
@didrocks

didrocks Nov 7, 2016

Owner

I would add here the example you provided:

plugs:
  ubuntu-app-runtime:
      content: ubuntu-app-runtime
      interface: content
      target: ubuntu-app-runtime

Indeed, we can't in cloud parts (maybe open a bug for this?) share standard plugs definition. So we need to document it.

A note (and that will impact your content interface as well), please please, version your content interface. It's the only way to ensure that we can't get into a breakage after an API or ABI changes. So I would suggest content: ubuntu-app-runtime1 (or whatever version). The content is what you version on both sides.

@didrocks

didrocks Nov 7, 2016

Owner

oh, as well, please provide default-provider: ubuntu-app-platform (snap name). Once this is implemented, installing the application will install the content interface snap at the same time (it's not implemented though right now)

@tjyrinki

tjyrinki Nov 7, 2016

Done in the second commit. Bug filed at http://pad.lv/1639751

snapcraft.yaml
@@ -286,3 +290,33 @@ parts:
- libglib2.0-dev
stage-packages:
- libglib2.0-bin
+ ubuntu-app-platform:
@didrocks

didrocks Nov 7, 2016

Owner

This stenza isn't necessary. It was for backward compatibility reason with the new desktop- syntax. You can remove it as we never published that new part with the desktop/ syntax.

snapcraft.yaml
+ - qtbase5-dev
+ - dpkg-dev
+ # No staged packages, they are in the platform snap
+ desktop/ubuntu-app-platform:
@didrocks

didrocks Nov 7, 2016

Owner

Same remark than above

snapcraft.yaml
@@ -29,6 +30,9 @@ description: |
- read/write to gsettings:
plugs: [gsettings, home]
(note that the home plug is needed to read new value)
+ - use of the shared platform snap content:
+ plugs: [ubuntu-app-platform]
+ (content interface plug to use ubuntu-app-platform slot needed)
@didrocks

didrocks Nov 7, 2016

Owner

I would add here the example you provided:

plugs:
  ubuntu-app-runtime:
      content: ubuntu-app-runtime
      interface: content
      target: ubuntu-app-runtime

Indeed, we can't in cloud parts (maybe open a bug for this?) share standard plugs definition. So we need to document it.

A note (and that will impact your content interface as well), please please, version your content interface. It's the only way to ensure that we can't get into a breakage after an API or ABI changes. So I would suggest content: ubuntu-app-runtime1 (or whatever version). The content is what you version on both sides.

@didrocks

didrocks Nov 7, 2016

Owner

oh, as well, please provide default-provider: ubuntu-app-platform (snap name). Once this is implemented, installing the application will install the content interface snap at the same time (it's not implemented though right now)

@tjyrinki

tjyrinki Nov 7, 2016

Done in the second commit. Bug filed at http://pad.lv/1639751

Address the review suggestions.
Better example, name the plug "platform", version the content, check if connected,
add default-provider and remove backward compatibility sections.

tjyrinki commented Nov 7, 2016

Ok the second commit hopefully addresses all the comments so far. Also updated the testapp example in other repository which still didn't use "platform" name correctly.

A last nitpick, and once done, good for merging! Excellent work again :)

qt/launcher-specific
@@ -10,6 +10,13 @@ export QTCHOOSER_NO_GLOBAL_DIR=1
if [ "$USE_qt5" = true ]; then
export QT_SELECT=snappy-qt5
elif [ "$USE_qt5-app-platform" = true ]; then
+ ls -d $SNAP/ubuntu-app-platform >/dev/null 2>&1
+ if [ $? -ne 0 ] ; then
@didrocks

didrocks Nov 7, 2016

Owner

It's just a matter of preference, but can you just condense that in one line (just use backticks so that it doesn't depend on bash), as in other part of the launcher, thanks!

Owner

didrocks commented Nov 7, 2016

Thanks for addressing all remarks!

@didrocks didrocks merged commit 15da94f into Ubuntu:master Nov 7, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment