/snapcraft

Create Plainbox Provider plugin #364

Closed
wants to merge 1 commit into
from

Conversation

Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
6 participants
@jocave @snappy-m-o @elopio @coveralls @sergiusens @zyga
@jocave
Contributor

jocave commented Mar 3, 2016

lp: #1552369

It would be useful if this plugin could be released on to the 1.x series as well.
@snappy-m-o

snappy-m-o commented Mar 3, 2016

Can one of the admins verify this patch?
@snappy-m-o

snappy-m-o commented Mar 3, 2016

Can one of the admins verify this patch?
snapcraft/plugins/plainbox_provider.py
+deb http://${security}.ubuntu.com/${suffix} ${release}-security main restricted
+deb http://${security}.ubuntu.com/${suffix} ${release}-security universe
+deb http://${security}.ubuntu.com/${suffix} ${release}-security multiverse
+deb http://ppa.launchpad.net/checkbox-dev/ppa/ubuntu ${release} main
@zyga

zyga Mar 4, 2016

Contributor

I'd use stable PPA here really or make this customizable with an explicit knob.

...n_tests/snaps/simple-plainbox-provider/2016.com.example:simple/manage.py
+
+
+setup(
+ name='2016.com.example:simple',
@zyga

zyga Mar 4, 2016

Contributor

I'd use plainbox-provider-example here with an explicit namespace="2016.com.example"

@jocave
Contributor

jocave commented Mar 4, 2016

I've updated the code to make it possible to disable the Checkbox Development PPA. Also, switched to using an explicit namespace (kept the provider name "Simple" though").
@elopio
Member

elopio commented Mar 16, 2016

OK to test
@elopio
Member

elopio commented Mar 18, 2016

retest this please
@coveralls

coveralls commented Mar 21, 2016

Coverage Status

Coverage remained the same at 95.213% when pulling 1b738a2 on jocave:plainbox-provider-plugin into 3cc0049 on ubuntu-core:master.
@coveralls

coveralls commented Mar 21, 2016

Coverage Status

Coverage remained the same at 95.213% when pulling 2f45197 on jocave:plainbox-provider-plugin into 3cc0049 on ubuntu-core:master.
@coveralls

coveralls commented Mar 21, 2016

Coverage Status

Coverage remained the same at 95.213% when pulling db13ab5 on jocave:plainbox-provider-plugin into 3cc0049 on ubuntu-core:master.
@jocave
Contributor

jocave commented Mar 21, 2016

retest this please
@elopio
Member

elopio commented Mar 21, 2016

retest this please
@elopio
Member

elopio commented Mar 21, 2016

add to whitelist
@elopio
Member

elopio commented Mar 21, 2016

retest this please
@coveralls

coveralls commented Mar 23, 2016

Coverage Status

Coverage decreased (-0.1%) to 95.325% when pulling 27bacbd on jocave:plainbox-provider-plugin into 889fe87 on ubuntu-core:master.
snapcraft/plugins/plainbox_provider.py
+ super().__init__(name, options)
+ self.build_packages.extend(['python3-plainbox', 'intltool'])
+ if self.options.checkbox_dev_ppa:
+ self._PLUGIN_STAGE_SOURCES = self._DEV_PPA_PLUGIN_STAGE_SOURCES
@elopio

elopio Mar 23, 2016

Member

this _PLUGIN_STAGE_SOURCES is not a constant, so it shouldn't be all capital letters.
_plugin_stage_sources.

@elopio

elopio Mar 23, 2016

Member

sorry, not from your branch. After you merge I'll propose something to change it.

For you, it seems safer to overwrite the PLUGIN_STAGE_SOURCES @property, instead of dealing with the internal attribute.

@sergiusens

sergiusens Mar 23, 2016

Collaborator

Yes, don't take advantage of internal implementation details which are subject to change 😉

@elopio
Member

elopio commented Mar 23, 2016

👍 from me.
Can you please remove the icon? It's not needed, and I think it's better to keep the test snaps with only the required bits.
integration_tests/snaps/simple-plainbox-provider/snapcraft.yaml
+description: |
+ Create a snap of a very simple plainbox that could
+ then be used by a checkbox application
+icon: icon.png
@sergiusens

sergiusens Mar 23, 2016

Collaborator

remove this entry please

@sergiusens
Collaborator

sergiusens commented Mar 23, 2016

git rm integration_tests/snaps/simple-plainbox-provider/icon.png
@jocave
Contributor

jocave commented Mar 23, 2016

Pushed commit with icon file removed from simple-plainbox-provider integration test.
@sergiusens
Collaborator

sergiusens commented Mar 23, 2016

git mv integration_tests/test_plainbox_provider_plugin.py integration_tests/test_plugin_plainbox_provider.py
@coveralls

coveralls commented Mar 23, 2016

Coverage Status

Coverage decreased (-0.1%) to 95.325% when pulling 885ef55 on jocave:plainbox-provider-plugin into 889fe87 on ubuntu-core:master.
snapcraft/plugins/plainbox_provider.py
+ schema['properties']['checkbox-dev-ppa'] = {
+ 'type': 'boolean',
+ 'default': 'true',
+ }
@sergiusens

sergiusens Mar 23, 2016

Collaborator

since you aren't overriding properties it seems you are making use of the common source options, can you add that piece of information to the doc string?

snapcraft/plugins/plainbox_provider.py
+
+ def __init__(self, name, options):
+ super().__init__(name, options)
+ self.build_packages.extend(['python3-plainbox', 'intltool'])
@sergiusens

sergiusens Mar 23, 2016

Collaborator

build-packages are installed onto the host from the hosts defined sources.list[.d]; in the documentation above you mention that plainbox features are not completely in the archive. How does adding python3-plainbox here play into this?

@sergiusens
Collaborator

sergiusens commented Mar 23, 2016

Thanks for this, I have a bunch of open questions in there though.

Also, does this rely on python provided by the os snap? It is fine from my PoV, just want to make sure.
@sergiusens
Collaborator

sergiusens commented Mar 23, 2016

Given the previous comment I would like to see an example and an example_test (which would run on a snappy system)
@elopio
Member

elopio commented Mar 23, 2016

The example test would be so nice, indeed.
@jocave take a look at the examples_tests dir. Give me a shout if you need a hand with that one.
@jocave
Contributor

jocave commented Mar 23, 2016

Thank you for the reviews, I will put an example together and try and address the questions ASAP.
@coveralls

coveralls commented Mar 24, 2016

Coverage Status

Coverage decreased (-0.1%) to 95.739% when pulling 318332b on jocave:plainbox-provider-plugin into 3c2081a on ubuntu-core:master.
@coveralls

coveralls commented Mar 24, 2016

Coverage Status

Coverage decreased (-0.1%) to 95.739% when pulling 0b894a7 on jocave:plainbox-provider-plugin into 3c2081a on ubuntu-core:master.
@coveralls

coveralls commented Mar 24, 2016

Coverage Status

Coverage decreased (-0.1%) to 95.744% when pulling 19140e7 on jocave:plainbox-provider-plugin into 56894e8 on ubuntu-core:master.
@coveralls

coveralls commented Mar 24, 2016

Coverage Status

Coverage decreased (-0.1%) to 95.744% when pulling 6b1bde0 on jocave:plainbox-provider-plugin into 56894e8 on ubuntu-core:master.
@coveralls

coveralls commented Mar 24, 2016

Coverage Status

Coverage decreased (-0.1%) to 95.744% when pulling 5b07527 on jocave:plainbox-provider-plugin into 56894e8 on ubuntu-core:master.
@coveralls

coveralls commented Mar 29, 2016

Coverage Status

Coverage decreased (-0.1%) to 95.753% when pulling aad99ce on jocave:plainbox-provider-plugin into b18e4e9 on ubuntu-core:master.
@jocave
Create Plainbox Provider plugin 
lp: #1552369

Integration tests provided that check the plugin can be
used in several common Checkbox use cases.

Some checks were not successful

2 failing and 2 successful checks
@snappy-m-o
Examples tests
Details
@snappy-m-o
autopkgtest
Details
continuous-integration/travis-ci/pr — The Travis CI build passed
Details
coverage/coveralls — Coverage decreased (-0.1%) to 96.131%
Details
af05e90
@jocave
Contributor

jocave commented May 9, 2016

I have added integration tests to cover a set of what I think are expected use cases for this plugin. One of these fails with a python import error for module 'plainbox'. This is provided by the package python3-plainbox which is explicitly added to the build-package list by the plugin.

I would like to know how to guarantee that 'build-package' works as advertised.
@sergiusens
Collaborator

sergiusens commented May 27, 2016

@jocave build-packages will just land on the host and be used to build the system, there is additional magic to crawl everything ELF and bring in any .so it needs not provided in the snap/part area.

stage-packages however, which is what you use in your examples are a bit tricky, specially when it comes to python as you will need to define a PYTHONPATH for this.

Given this still needs a few twists and turns I am going to close it for now. Feel free to reopen when ready.

@sergiusens sergiusens closed this May 27, 2016

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