project: introduce build-snaps #1518

sergiusens · 2017-08-30T19:34:26Z

Introduce build-snaps with support for advanced grammar.

come-maiz

I am eager to see this released.
I left a few comments. And this is missing the recording part. Do you plan to do it as part of this branch, or leave it for (me, probably) later?

come-maiz · 2017-08-30T19:59:06Z

schema/snapcraft.yaml

@@ -276,6 +276,9 @@ properties:
          stage-packages:
            $ref: "#/definitions/grammar-array"
            default: [] # For some reason this doesn't work if in the ref
+          build-snaps:


¡viva! this is the end of compiling go1.8 on every build.

go 1.9 now, already...

come-maiz · 2017-08-30T19:59:56Z

schema/snapcraft.yaml

@@ -276,6 +276,9 @@ properties:
          stage-packages:
            $ref: "#/definitions/grammar-array"
            default: [] # For some reason this doesn't work if in the ref
+          build-snaps:
+            $ref: "#/definitions/grammar-array"
+            default: [] # For some reason this doesn't work if in the ref


hum, I haven't seen this before. It's ok to leave a comment, but it needs more information.
What doesn't work, specifically? I guess this is a question for @kyrofa.

When I initially wrote the grammar back in the day I wasn't sure what was happening, and decided it was a jsonschema quirk. However, now that you ask I looked into it with a fresh mind and I think I know what's actually happening: a jsonschema ref isn't expanded in any fancy way by the YAML (which is why I'd like to use a recursive YAML anchor for this, but jsonschema causes things to explode that way since it wants to walk the whole thing). This means that we can't actually access the chunk of YAML representing that bit of the schema very easily in code. Like we do here to expand defaults. So this is kind of an ugly hack to allow specifying the defaults to continue actually resulting in defaults. Thankfully that's the only field for which I imagine we'll need this hack.

come-maiz · 2017-08-30T20:02:45Z

snapcraft/internal/project_loader/_config.py

@@ -89,6 +89,7 @@ def __init__(self, project_options=None):
        if project_options is None:
            project_options = snapcraft.ProjectOptions()

+        self.build_snaps = set()
        self.build_tools = []


have I said how much I hate this build_tools? :D
I'm just saying it again to put myself pressure to fix it.

come-maiz · 2017-08-30T20:03:46Z

snapcraft/internal/repo/snaps.py

+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.


missing empty line

this I will indeed ignore ;-)

come-maiz · 2017-08-30T20:07:15Z

snapcraft/internal/repo/snaps.py

+
+    @classmethod
+    def is_valid_snap(cls, snap):
+        return cls(snap).is_valid()


I consider this very weird. A class method that makes an instance. I haven't seen it before, is it an accepted pattern?

I would prefer the caller to make the instance. Or, a little less preferable, to make is_valid_snap a module function instead of a class method.

This is a common pattern, yes

come-maiz · 2017-08-30T20:17:42Z

snapcraft/internal/repo/snaps.py

+            self.get_store_snap_info()
+        return self._is_on_store
+
+    def get_local_snap_info(self):


Any reason for not putting an _ in front of this one?

I have no reason for the fact that it can be used outside even though it isn't right now.

come-maiz · 2017-08-30T20:18:19Z

snapcraft/internal/repo/snaps.py

+    @property
+    def on_store(self):
+        if self._is_on_store is None:
+            self.get_store_snap_info()


same comment about assigning _is_on_store

This means I also need ancillary values for the actual info which I was trying to avoid.

come-maiz · 2017-08-30T20:22:37Z

snapcraft/internal/repo/snaps.py

+        if not self.on_store:
+            return dict()
+
+        return snap_store_info['result'][0]['channels']


Is that 0 because multiple results can be returned? It would be nice to explain that in a comment.

yes, I had it in a comment somewhere else before moving this to a class.

come-maiz · 2017-08-30T20:24:25Z

snapcraft/internal/repo/snaps.py

+        if self._original_channel:
+            snap_install_cmd.extend(['--channel', self._original_channel])
+        if self.is_classic:
+            snap_install_cmd.extend(['--classic'])


I now this would complicate the snapcraft.yaml, but I would prefer the user to declare if she wants to install the snap as classic.

this is not what was agreed to on the forum 😉

come-maiz · 2017-08-30T20:25:38Z

snapcraft/internal/repo/snaps.py

+        try:
+            check_call(snap_refresh_cmd)
+        except CalledProcessError as install_error:
+            raise errors.SnapInstallError(snap_name=self.name,


A Refresh error would be more appropriate here.

kalikiana · 2017-08-31T07:35:33Z

snapcraft/internal/project_loader/_parts_config.py

-    def __init__(self, parts, project_options, validator, build_tools,
-                 snapcraft_yaml):
+    def __init__(self, parts, project_options, validator,
+                 build_snaps, build_tools, snapcraft_yaml):


Can you add a * here and use keywords?

kalikiana · 2017-08-31T07:45:08Z

snapcraft/internal/repo/snaps.py

+        if not snap_pkg.installed and snap_pkg.on_store:
+            snap_pkg.install()
+        elif snap_pkg.get_current_channel() != snap_pkg.channel:
+            snap_pkg.refresh()


This should also check if is_classic is the same and requires a refresh.

ah, right, for people who bounce back and forth; in general terms if installed once with classic you don't need to pass it on for subsequent installs

To be clear, I'm talking about the case where snap foo has strict confinement and a new revision is a classic snap, or vice versa. Afair snapd doesn't change it automatically.

yeah, that is what I meant by bounce back and forth 😉

kalikiana · 2017-08-31T07:49:08Z

integration_tests/test_build_snap_grammar.py

+            self.fail(
+                'This integration test cannot run if you already have the '
+                "'hello' package installed. Please uninstall it before "
+                'running this test.')


This should say "if you already have the 'hello' snap installed", otherwise it makes you think of apt or dnf. Also, print the command that does that.

kalikiana · 2017-08-31T07:58:21Z

snapcraft/internal/repo/snaps.py

+                                          snap_channel=self.channel)
+
+    def refresh(self):
+        """Refreshes a snap onto a channel on the system."""


This should be Refreshes a snap to the latest version, channel and confinement type because 1) it will update the snap if there's a new version 2) as per my comment on install_snaps confinement may have changed.

kalikiana · 2017-08-31T08:00:03Z

snapcraft/internal/repo/snaps.py

+            snap_pkg.refresh()
+
+
+def _snap_command_requires_sudo():


Can we have a unit test for this?

Yeah, unittest for this whole module coming today :-)

sergiusens · 2017-08-31T12:25:35Z

snapcraft/internal/repo/snaps.py

+        snap_install_cmd.extend(['snap', 'install', self.name])
+        if self._original_channel:
+            snap_install_cmd.extend(['--channel', self._original_channel])
+        if self.is_classic:


this is missing the TODO comment to make it a user choice on non ci/container environments

this is done in the is_classic check now

kyrofa · 2017-08-31T15:29:23Z

snapcraft/internal/repo/snaps.py

+    confinment value and channel availability.
+
+    This information can also be used determine if a snap should be
+    installed or refreshed.


There should be a "to" in here: "This information can also be used to determine[...]"

kyrofa · 2017-08-31T15:31:57Z

snapcraft/internal/repo/snaps.py

+        return self._is_installed
+
+    @property
+    def on_store(self):


On the store, or in the store?

kyrofa · 2017-08-31T15:34:48Z

snapcraft/internal/repo/snaps.py

+        snap_install_cmd = []
+        if _snap_command_requires_sudo():
+            snap_install_cmd = ['sudo']
+        snap_install_cmd.extend(['snap', 'install', self.name])


Is there an advantage to shelling out as opposed to using the snapd API for this?

it is much more work as we'd need to take care of "uploading" the snap.

Oh wait, not for these snaps. Maybe not, let me look

I'll leave this for future work, I don't even know how stable that API is.

nor where it is documented

Yeah, fair enough.

kyrofa · 2017-08-31T15:36:01Z

snapcraft/internal/repo/snaps.py

+        store_channels = self._get_store_channels()
+        return self.channel in store_channels.keys()
+
+    def install(self):


What if the snap I want to use to build requires devmode? Are we choosing to not support that?

not at the moment, check the forum post on the subject

I did. It says nothing about devmode.

right, so it would need a discussion first, right now it shouldn't work. In all fairness, if we discuss this, I'd say it shouldn't just like we are not auto connecting interfaces.

kyrofa · 2017-08-31T15:38:10Z

snapcraft/internal/repo/snaps.py

+def _get_parsed_snap(snap):
+    if '/' in snap:
+        snap_name = snap[:snap.find('/')]
+        snap_channel = snap[snap.find('/')+1:]


The two snap.find('/')s here are bothering me-- perhaps it should be saved in a variable?

ok, fair enough

kyrofa · 2017-08-31T15:39:19Z

tools/travis/run_lxd_container.sh

@@ -32,7 +32,7 @@ name="$1"
 lxc="/snap/bin/lxc"

 echo "Starting the LXD container."
-$lxc launch --ephemeral ubuntu:xenial "$name"
+$lxc launch --ephemeral --config security.privileged=true ubuntu:xenial "$name"


Why do we need privileged containers here?

The commit that added this has the info, but here it is: https://bugs.launchpad.net/snapd/+bug/1709536

Ah yes, I hit that myself. I fixed it by removing the Nice value from the systemd unit file though as opposed to making the container privileged. It's probably not a big deal, let's just keep an eye on the bug and change back to unprivileged once it's fixed. Can we add a FIXME comment or something, though, with a link to the bug?

sparkiegeek · 2017-09-01T14:06:02Z

snapcraft/internal/repo/snaps.py

+            snap_install_cmd.extend(['--channel', self._original_channel])
+        if self.is_classic:
+            # TODO make this a user explicit choice
+            snap_install_cmd.extend(['--classic'])


This should just be .append() no need to .extend() with a single item list

sparkiegeek · 2017-09-01T14:06:22Z

snapcraft/internal/repo/snaps.py

+                                 '--channel', self.channel])
+        if self.is_classic:
+            # TODO make this a user explicit choice
+            snap_refresh_cmd.extend(['--classic'])


More .extend() waste

sparkiegeek · 2017-09-01T15:52:10Z

snapcraft/internal/repo/snaps.py

+def _get_store_snap_info(snap_name):
+    # This logic uses /v2/find returns an array of results, given that
+    # we do a strict search either 1 result or a 404 will be returned.
+    slug = '{}{}'.format('find?name=', urllib.parse.quote(snap_name, safe=''))


huh? why are you interpolating a fixed string?

'find?name={}'.format(...)` would be more intuitive

I don't know 🙃

any reason for not using urllib.parse.urlencode here?

FWIW using urllib.parse.quote like that results in unnecessary %-encoding of characters that don't need it. If you'd rather not use urlencode (maybe building a dict isn't to your taste?), just use urllib.parse.quote_plus which is the quote function for the query part of a URL.

requests_unixsockets doesn't support the dict as params, but I guess you are implying that urlencode can do that so i'll take a look. The only one I sort of have problems with is urljoin.

switched to urlencode on bec56ed

kyrofa · 2017-09-01T19:49:09Z

snapcraft/internal/repo/snaps.py

-            if current_channel == 'stable':
-                current_channel = 'latest/stable'
+            if current_channel in ('stable', 'candidate', 'beta', 'edge'):
+                current_channel = 'latest/{}'.format(current_channel)


This check won't catch branches, think build-snaps: go/hotfix-211. Perhaps instead of a whitelist of risks, just check to see if a track is specified (e.g. searching for /)?

kyrofa · 2017-09-01T19:57:13Z

snapcraft/tests/repo/test_snaps.py

+            return 'email: -'.encode()
+
+
+class FakeSnapdServer(BaseHTTPRequestHandler):


Having @chipaca look this fake over to make sure everything is sane would be helpful.

I sort of asked him the necessary questions about API and showed him the original implementation so I don't think he is needed her @kyrofa

can confirm, was sort of asked.

kalikiana · 2017-09-04T10:04:00Z

snapcraft/tests/repo/test_snaps.py

+        return (request, client_address)
+
+
+class FakeSnapd(fixtures.Fixture):


I like your new FakeSnapd a lot. But we already have a FakeSnapd fixture in snapcraft/tests/fixture_setup.py so please merge the two.

I would rather not do it here as it would require substantial changes. I'll do it in a follow up PR though.

kalikiana · 2017-09-04T10:07:52Z

snapcraft/tests/repo/test_snaps.py

+        self.fake_snap_command.refresh_success = False
+        self.assertRaises(errors.SnapRefreshError, snap_pkg.refresh)
+
+    def test_bundle_install(self):


Nit: the term "bundle" confused me. I think test_install_multiple would be much clearer.

kalikiana · 2017-09-04T10:11:19Z

snapcraft/tests/repo/test_snaps.py

+        self.assertThat(snap_pkg.in_store, Is(self.expected))
+
+
+class SnapPackageIsClasicTest(SnapPackageBaseTestCase):


Typo: SnapPackageIsClassicTest should have two s.

kalikiana · 2017-09-04T10:13:10Z

snapcraft/tests/repo/test_snaps.py

+        snap_pkg = snaps.SnapPackage('fake-snap/classic/stable')
+        snap_pkg.install()
+        self.assertThat(self.fake_snap_command.calls, Equals([
+            ['snap', 'whoami'],


There's a test missing for the case of a logged in user where sudo won't be used.

come-maiz

Looks very good to me.
I tested it with a few real snaps. Now we need for the go plugin to not install golang-go from the archive.

I'm a couple of integration tests that install a snap with track and risk.

come-maiz · 2017-09-04T19:56:45Z

integration_tests/test_build_snap_grammar.py

+class BuildSnapGrammarTestCase(testscenarios.WithScenarios,
+                               integration_tests.TestCase):
+
+    scenarios = _construct_scenarios()


yeah, I would use multiply here, but if this makes you happy, ok :)

Yeah, I saw multiply, this feels much more readable IMO, the other options was as @sparkiegeek mentioned, subTest but iirc, testscenarios had issues with that, right?

right: testing-cabal/testtools#235

come-maiz · 2017-09-04T20:02:31Z

snapcraft/internal/repo/snaps.py

+
+    It uses information provided by snapd implicitly referring to the local
+    and remote stores to obtain information about the snap, such as its
+    confinment value and channel availability.


typo: confinement.

come-maiz · 2017-09-04T20:03:20Z

integration_tests/test_build_snap_grammar.py

+        except subprocess.CalledProcessError as e:
+            self.fail("unable to remove 'hello': {}".format(e.output))
+
+    def _hello_is_installed(self):


_is_hello_installed should be the name of this function.

I replicated what was in test_build_package_grammar. Let's change this in a different PR.

come-maiz · 2017-09-04T20:10:21Z

snapcraft/tests/repo/test_snaps.py

+                self.calls.append(cmd)
+                return self._fake_snap_command(cmd, *args, **kwargs)
+            else:
+                return original(cmd, *args, **kwargs)


Hey, I like this! ;)

come-maiz · 2017-09-04T20:13:49Z

snapcraft/tests/repo/test_snaps.py

+        elif cmd == 'refresh' and not self.refresh_success:
+            raise subprocess.CalledProcessError(returncode=1, cmd=cmd)
+        elif cmd == 'whoami':
+            return 'email: {}'.format(self._email).encode()


I've been reflecting upon these fakes. I think things would be a lot easier if we always called check_output, even when we don't care about the output. That way we only have to make one patch. And if there is only one patch, and in good theory the test should only call the patched function once, then the test itself could inject the exception to raise. That would be clearer to me. But well, all random thoughts for future improvement, we can discuss about the fake tomorrow.

Using check_output only will not stream the output to stdout which is a feature. If you must, we should migrate to our own runner based on Popen

come-maiz · 2017-09-04T20:15:30Z

snapcraft/tests/repo/test_snaps.py

+            return 'email: {}'.format(self._email).encode()
+
+
+class FakeSnapdServer(BaseHTTPRequestHandler):


I would move this fake to snapcraft/tests/fake_servers/snapd.

My next step after this PR is to refactor fixtures_setup and others to make it more readable, I was lost inside those modules while trying to make out some structure from it

come-maiz · 2017-09-04T20:17:27Z

snapcraft/tests/repo/test_snaps.py

+            params = {'channel': 'track/stable/branch'}
+        elif parsed_url.path.endswith('/fake-snap-edge'):
+            status_code = 200
+            params = {'channel': 'edge'}


This unending stream of elifs caused many problems when we left the fake servers evolve. That's why on the new ones we are using pyramid to route the requests. This is not a lot better, just a little, but it will help for when we are finally able to use https://github.com/pretenders/pretenders

I don't like complicating things at the start though.

come-maiz · 2017-09-04T20:20:37Z

tools/travis/build_snapcraft_snap.sh

@@ -25,9 +25,12 @@ lxc="/snap/bin/lxc"

 "$script_path/setup_lxd.sh"
 "$script_path/run_lxd_container.sh" snap-builder
+# Workaround for
+# - Setup snap "core" (2462) security profiles (cannot reload udev rules: exit status 2


is there a bug for this one?

no idea, I was told it was known

come-maiz · 2017-09-04T20:38:05Z

snapcraft/tests/repo/test_snaps.py

+         dict(snap='fake-snap-branch/candidate/branch',
+              expected='latest/candidate/branch')),
+        ('track/stable/branch',
+         dict(snap='fake-snap-track-stable-branch/trakc/stable/branch',


There is a typo here: trakc/track.
This is one of the good reasons to migrate out of those hard-coded fake servers.

Fixed, but in the case of this test fixture the request for the future is never made, if I had called it, I would of gotten an error (like install or is_classic)

Introduce build-snaps with support for advanced grammar. Fixes canonical#1442

This is a workaround for LP: #1709536

come-maiz

+1 from the phone, like kids do \•/

Introduce build-snaps with support for advanced grammar. Fixes canonical#1442

come-maiz reviewed Aug 30, 2017

View reviewed changes

sergiusens force-pushed the build-snap branch 3 times, most recently from 2444f3c to c26600a Compare August 31, 2017 02:46

kalikiana suggested changes Aug 31, 2017

View reviewed changes

kalikiana assigned sergiusens Aug 31, 2017

sergiusens commented Aug 31, 2017

View reviewed changes

kyrofa reviewed Aug 31, 2017

View reviewed changes

snapcraft/internal/repo/snaps.py Outdated

return self._is_installed

@property

def on_store(self):

Copy link

Contributor

kyrofa Aug 31, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the store, or in the store?

kyrofa reviewed Aug 31, 2017

View reviewed changes

sergiusens force-pushed the build-snap branch from b160719 to a427c09 Compare September 1, 2017 12:49

sparkiegeek reviewed Sep 1, 2017

View reviewed changes

kyrofa reviewed Sep 1, 2017

View reviewed changes

sergiusens force-pushed the build-snap branch from abadb8c to 91737a3 Compare September 2, 2017 03:47

kalikiana suggested changes Sep 4, 2017

View reviewed changes

sergiusens force-pushed the build-snap branch from bec56ed to 45bebb6 Compare September 4, 2017 14:38

kalikiana approved these changes Sep 4, 2017

View reviewed changes

come-maiz suggested changes Sep 4, 2017

View reviewed changes

sergiusens added 8 commits September 4, 2017 21:03

project: introduce build-snaps

c4a835a

Introduce build-snaps with support for advanced grammar. Fixes canonical#1442

switching to privileged containers for test runs

e2add95

This is a workaround for LP: #1709536

force kwargs for PartsConfig

cff356b

Clearer hello fail error in test

9e7897e

classic fixes

3f756d9

typo and in_store instead of on_store

bc658cf

satisfy kyle

f4fec13

install squashfuse

c9449ae

sergiusens added 13 commits September 4, 2017 21:03

add fixme note for container testing in testbed

bb00790

unit tests

6c48791

workaround

b7189e1

adam's review fixes

01fd337

fix local branch normalization

ff6a36d

workaround 2

9e76582

urlencode find args

6a20af3

typo in test name

5060e0d

rename test for clarity

0a2831d

logged in tests

d806dd7

fix test typo

0efff71

fix for docstring typo

f9129d1

move the fake server

a211b19

sergiusens force-pushed the build-snap branch from abb8763 to a211b19 Compare September 5, 2017 00:03

come-maiz approved these changes Sep 5, 2017

View reviewed changes

sergiusens merged commit 69dceb8 into canonical:master Sep 5, 2017

sergiusens deleted the build-snap branch September 5, 2017 02:13

sergiusens mentioned this pull request Sep 11, 2017

build-snaps #1442

Closed

kalikiana pushed a commit to kalikiana/snapcraft that referenced this pull request Sep 21, 2017

project: introduce build-snaps (canonical#1518)

5fd282f

Introduce build-snaps with support for advanced grammar. Fixes canonical#1442

		return 'email: -'.encode()


		class FakeSnapdServer(BaseHTTPRequestHandler):

		return (request, client_address)


		class FakeSnapd(fixtures.Fixture):

		self.assertThat(snap_pkg.in_store, Is(self.expected))


		class SnapPackageIsClasicTest(SnapPackageBaseTestCase):

		return 'email: {}'.format(self._email).encode()


		class FakeSnapdServer(BaseHTTPRequestHandler):

project: introduce build-snaps #1518

project: introduce build-snaps #1518

Conversation

sergiusens commented Aug 30, 2017

come-maiz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kyrofa Aug 31, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kalikiana Aug 31, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kyrofa Sep 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kyrofa Aug 31, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

come-maiz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kyrofa Aug 31, 2017 •

edited

Loading

kalikiana Aug 31, 2017 •

edited

Loading

kyrofa Sep 1, 2017 •

edited

Loading

kyrofa Aug 31, 2017 •

edited

Loading