build_providers: new build provider using multipass #2100

sergiusens · 2018-04-23T15:59:01Z

build_providers is a new package to handle different providers snapcraft
can use to create a snap.

In addition to the scaffolding, an implementation to handle
multipass is provided. This is the MVP for multipass integration.

Have you followed the guidelines for contributing?
Have you signed the CLA?
If this is a bugfix. Have you checked that there is a bug report open for the issue you are trying to fix on bug reports?
If this is a new feature. Have you discussed the design on the forum?
Have you successfully run ./runtests.sh static?
Have you successfully run ./runtests.sh unit?

sergiusens · 2018-04-23T22:00:08Z

To test out this drop

snap install snapcraft --channel edge/multipass
cd <project>
SNAPCRAFT_BUILD_ENVIRONMENT=multipass snapcraft cleanbuild

popey · 2018-04-23T22:17:46Z

Observations. The naming scheme isn't consistent with lxd. On lxd I end up with snapcraft-foo-bar-baz. With multipass I get foo-bar.

Also, it failed. Took 10 minutes to get to this failure.

$ SNAPCRAFT_BUILD_ENVIRONMENT=multipass snapcraft cleanbuild                                                                                                    
Creating a build environment named 'unreelable-kyndall'
Starting unreelable-kyndall                                                  
failed to launch: Attempt to access value of a disengaged optional object       
An error occurred when trying to launch the instance with 'multipass'.

popey · 2018-04-23T22:27:49Z

Tried a second time with the same result.

kalikiana · 2018-04-24T09:47:26Z

snapcraft/cli/env.py

            raise errors.SnapcraftEnvironmentError(
-                'SNAPCRAFT_BUILD_ENVIRONMENT must be one of: host or lxd.')
+                'SNAPCRAFT_BUILD_ENVIRONMENT must be one of: {}.'.format(
+                    humanize_list(items=valid_providers, conjunction='or')))


Can we have a proper error class for this?

I do not think it is necessary to create an error class for a feature flag variable

What about testing? Having #1705 in mind where we want to get rid of messages in tests... or are you suggesting to not test this case?

kalikiana · 2018-04-24T09:50:57Z

snapcraft/internal/build_providers/_factory.py

+    if provider_name == 'multipass':
+        return Multipass
+    else:
+        raise errors.ProviderNotSupported(provider=provider_name)


This should probably be ProviderNotSupportedError? Note the suffix.

kalikiana · 2018-04-24T09:56:08Z

snapcraft/internal/build_providers/errors.py

+class ProviderNotSupported(_SnapcraftError):
+
+    fmt = (
+        'The {provider!r} provider is not supported, please choose a '


Shouldn't this follow the what/how/fix pattern?

kalikiana · 2018-04-24T10:03:40Z

snapcraft/internal/build_providers/_multipass/_multipass_command.py

+logger = logging.getLogger(__name__)
+
+
+def _run(command: List) -> None:


Seems like a candidate for sharing with other modules.

is there anything else that needs it, if so, let's identify them and create a follow up PR

The lxd module would be an obvious candidate. Plugin commands could use it as well. But yeah, it can be in a follow-up.

kyrofa · 2018-04-25T15:37:11Z

snapcraft/internal/build_providers/_multipass/_instance_info.py

+
+from snapcraft.internal.build_providers import errors
+
+T = TypeVar('T', bound='InstanceInfo')


Is there a need for this? You can just use 'InstanceInfo' (the string) as the type; as long as it's valid after this file is processed, mypy will be happy with it.

kyrofa · 2018-04-25T15:39:08Z

snapcraft/internal/build_providers/_multipass/_instance_info.py

+class InstanceInfo:
+
+    @classmethod
+    def new_instance_info_from_json(cls: Type[T], *, instance_name: str,


Callers will be using this as InstanceInfo.new_instance_info_from_json(...) which seems redundant. I find it reads very well to name these types of methods from_X, so InstanceInfo.from_json(...). Thoughts?

kyrofa · 2018-04-25T15:42:13Z

snapcraft/internal/build_providers/_multipass/_instance_info.py

+
+        :param str name: the instance name.
+        :param str state: the state of the instance which can be any one of
+                          RUNNING, STOPPED, DELETED.


This seems a perfect use-case for enums. Thoughts?

kyrofa · 2018-04-25T15:43:07Z

snapcraft/internal/build_providers/_multipass/_multipass.py

+
+
+class Multipass(BaseProvider):
+    """A multipass provider for snapcraft execute its lifecycle."""


To execute?

kyrofa · 2018-04-25T15:45:25Z

snapcraft/internal/build_providers/_multipass/_multipass.py

+        try:
+            instance_info = self._get_instance_info()
+        except errors.ProviderInfoError as info_error:
+            self.echoer.warning(


Is this not an error? We weren't able to do as requested, but to the caller, this will look like a success. This comment applies to the cases below as well.

Let's live chat about this one

kyrofa · 2018-04-25T16:11:07Z

snapcraft/internal/build_providers/_multipass/_multipass_command.py

+            raise errors.ProviderCommandNotFound(command=provider_cmd)
+        self.provider_cmd = provider_cmd
+        # Workaround for https://github.com/CanonicalLtd/multipass/issues/221
+        signal.signal(signal.SIGUSR1, _ignore_signal)


That was most of my Saturday!

Fixed in edge ;)

kyrofa · 2018-04-25T16:12:49Z

snapcraft/internal/build_providers/_multipass/_multipass_command.py

+                provider_name=self.provider_name,
+                command=command) from process_error
+
+    def copy_files(self, *, source: str, destination: str):


Missing a -> None here

Saviq

Works for me!

Saviq · 2018-04-25T15:05:15Z

snapcraft/internal/build_providers/_multipass/_instance_info.py

+        try:
+            return cls(name=instance_name,
+                       state=instance_info['state'],
+                       image_release=instance_info['image_release'])


This gives you the release of the image from which this instance got launched. Is this enough? OTOH release is only currently available when the instance is running, so probably best to use that anyway.

Saviq · 2018-04-25T16:13:12Z

snapcraft/internal/build_providers/_multipass/_multipass_command.py

+import signal
+import shutil
+import subprocess
+from typing import List


Uh oh, /me needs to school up on new python...

kyrofa · 2018-04-25T16:17:12Z

snapcraft/internal/build_providers/errors.py

+class ProviderCommandNotFound(_SnapcraftError):
+
+    fmt = (
+        'The {command!r} command is necessary to be able to build in this '


This jumps straight to why this is a problem instead of starting with the actual problem. Consider "{command!r} command not found: this command is necessary to build in this environment.\nInstall {command!r}...".

kyrofa · 2018-04-25T16:18:44Z

snapcraft/internal/build_providers/errors.py

+
+    fmt = (
+        'An error occurred when trying to launch the instance with '
+        '{provider_name!r}.'


I realize multipass errors should be above this exception, but would it be helpful to have the multipass exit codes here? This comment applies for all these command error classes. Similarly, should we follow Evan's advice and say something like "this is likely not a problem in Snapcraft, please check {provider_name}" ?

Should be optional, these errors should be usable by things that talk over protocols as well (which do have error codes!)

kyrofa · 2018-04-25T16:30:17Z

snapcraft/internal/build_providers/errors.py

+
+    fmt = (
+        'The data returned by {provider_name!r} was not expected. '
+        'It is missing some key data {key_info!r} in {data!r}.'


I don't quite parse this. "key data {key_info!r}" -> is key_info a key in data? What is key data? Like... key = important?

kyrofa · 2018-04-25T16:34:44Z

snapcraft/internal/build_providers/_factory.py

+from ._multipass import Multipass
+
+
+def get_provider_for(provider_name: str) -> Union[Type[Multipass]]:


This should probably return a BaseProvider, no?

kyrofa · 2018-04-25T16:35:54Z

snapcraft/internal/build_providers/_base_provider.py

+
+
+# This class is a mix of a parent and a mixin.
+class BaseProvider:


I'd personally rather see this called Provider. "Multipass is a provider" versus "multipass is a base provider".

kyrofa · 2018-04-25T16:36:31Z

snapcraft/internal/lifecycle/_containers.py

+    with build_provider_class(project=project, echoer=echoer) as instance:
+        instance.provision_project(tar_filename)
+        instance.build_project()
+        instance.retrieve_snap()


Given the factory nature of this, the interface used here should probably be implemented by the BaseProvider, or this quickly falls down as we add other providers.

kyrofa · 2018-04-25T16:46:02Z

snapcraft/project/__init__.py

@@ -15,3 +15,4 @@
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.

 from ._project import Project                          # noqa F401
+from ._project_info import ProjectInfo                 # noqa F401


Eventually we want to get to the point where the Project can fill out the ProjectInfo based on its __init__, which means this won't need to be imported here. Would it make sense to just import the private module in the test instead of making it publicly available now?

Fwiw test_config imports the private module by name today as "from snapcraft.project._project_info import ProjectInfo" for that reason.

build_providers is a new package to handle different providers snapcraft can use to create a snap. In addition to the scaffolding, an implementation to handle multipass is provided. This is the MVP for multipass integration.

also add unit test

kyrofa

The interface is coming together, but I have a few more suggestions.

kyrofa · 2018-04-25T20:51:44Z

snapcraft/internal/build_providers/_base_provider.py

+
+    @property
+    @abc.abstractmethod
+    def run(self) -> Callable[[str], None]:


It seems odd to have this as a property, especially considering that it's actually a callable, so it LOOKS like a method when used. Why not just make it a method, and the implementer can just pass it on to their command instance?

kyrofa · 2018-04-25T20:52:02Z

snapcraft/internal/build_providers/_base_provider.py

+
+    @property
+    @abc.abstractmethod
+    def launch(self) -> Callable[[], None]:


Same here, feels like it should be a method.

kyrofa · 2018-04-25T20:59:37Z

snapcraft/internal/build_providers/_multipass/_multipass.py

+            self.echoer.warning('Could not stop {!r}: {}.'.format(
+                self.instance_name, stop_error))
+        except errors.ProviderDeleteError as stop_error:
+            self.echoer.warning('Could not stop {!r}: {}.'.format(


Could not delete?

kyrofa · 2018-04-25T21:02:54Z

snapcraft/internal/build_providers/_base_provider.py

+import petname
+
+
+class Provider():


This is heading the right direction, but I'm losing track of what is and isn't public API ("public" defined to be "meant to be called by a user of Provider). For example, run and launch are both public, but I don't believe they're actually meant to be called by anything except Provider, right?

kyrofa · 2018-04-25T21:03:21Z

snapcraft/internal/build_providers/_base_provider.py

+    @abc.abstractmethod
+    def retrieve_snap(self) -> str:
+        """
+        Provider steps needed to retrieve the built snap from the instance.


Can you please document what this should return?

kyrofa · 2018-04-25T21:03:57Z

snapcraft/internal/build_providers/_base_provider.py

+        """Return a callable that can be used to launch an instance."""
+
+    @abc.abstractmethod
+    def create(self):


Missing -> None for both of these.

kyrofa

This is quite nice! Great work 👏 .

sergiusens force-pushed the build-providers branch from a7cb3f9 to 23b67c4 Compare April 23, 2018 21:17

kalikiana reviewed Apr 24, 2018

View reviewed changes

sergiusens force-pushed the build-providers branch from 23b67c4 to 5a16699 Compare April 25, 2018 03:44

kyrofa reviewed Apr 25, 2018

View reviewed changes

Saviq approved these changes Apr 25, 2018

View reviewed changes

kyrofa reviewed Apr 25, 2018

View reviewed changes

sergiusens added 9 commits April 25, 2018 17:05

build_providers: new build provider using multipass

8c709e0

build_providers is a new package to handle different providers snapcraft can use to create a snap. In addition to the scaffolding, an implementation to handle multipass is provided. This is the MVP for multipass integration.

cleanup environment

69201b6

remove use-multipass option

5ed406a

comment about the reason for the current petname

2619eaa

refactor some more to make the entire Multipass class context aware.

2596c79

also add unit test

ProviderError

d0ffae3

fix messaging for missing provider command

f0823f3

fixup exceptions

69bfd69

use string for instanceinfo type

e2efc4b

sergiusens added 6 commits April 25, 2018 17:05

from_json

69d95b5

fix docstring for the Multipass class

2317aea

return type for MultipassCommand.copy_files

cff6ef5

use private ProjectInfo

de0823e

refactor provider and multipass

3ebd7fb

instance_status on create

6d6f976

sergiusens force-pushed the build-providers branch from aaa8451 to 6d6f976 Compare April 25, 2018 20:05

kyrofa reviewed Apr 25, 2018

View reviewed changes

last review, thanks @kyrofa

5a67061

kyrofa approved these changes Apr 25, 2018

View reviewed changes

test fix

0a0bbad

kyrofa merged commit c44a4ec into canonical:master Apr 26, 2018

sergiusens deleted the build-providers branch April 26, 2018 01:57

		logger = logging.getLogger(__name__)


		def _run(command: List) -> None:


		from snapcraft.internal.build_providers import errors

		T = TypeVar('T', bound='InstanceInfo')



		class Multipass(BaseProvider):
		"""A multipass provider for snapcraft execute its lifecycle."""

		from ._multipass import Multipass


		def get_provider_for(provider_name: str) -> Union[Type[Multipass]]:



		# This class is a mix of a parent and a mixin.
		class BaseProvider:

build_providers: new build provider using multipass #2100

build_providers: new build provider using multipass #2100

Conversation

sergiusens commented Apr 23, 2018 • edited

sergiusens commented Apr 23, 2018

popey commented Apr 23, 2018

popey commented Apr 23, 2018

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 Apr 25, 2018 • edited

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

Saviq 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 Apr 25, 2018 • edited

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 Apr 25, 2018 • edited

Choose a reason for hiding this comment

kyrofa Apr 25, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kyrofa 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kyrofa left a comment

Choose a reason for hiding this comment

sergiusens commented Apr 23, 2018 •

edited

kyrofa Apr 25, 2018 •

edited

kyrofa Apr 25, 2018 •

edited

kyrofa Apr 25, 2018 •

edited

kyrofa Apr 25, 2018 •

edited