Merge ~jugmac00/lpci:use-pluggy-for-plugins into lpci:main

[Jürgen Gmach]

Introduce pluggy to manage internal and external plugins

- pluggy has not been type annotated yet
- the documentation will be extended once all hooks are defined/created; e.g. tox uses a sphinx plugin to auto-document the hooks (this will be a new MP)

The internal hook `lpcraft_install_packages` needs access to the job configuration, the external plugins do not.

This "mismatch" could be solved by:
- creating two different hooks, e.g. `_lpcraft_install_packages` which takes a job configuration as argument, and `lpcraft_install_packages` which does not, and then combining the result; that is what `tox` does
- using a class as namespace, which I did, so the internal hook can access the job configuration via `self.config`
- just pass the job configuration to all hooks, and ignore the information where it is not needed; that is what pytest does a lot; I did not use this approach as the signature/interface of the hook would be more complex than needed

[Colin Watson (cjwatson)]

What I'm not sure I understand here is how this is going to hook into the configuration file syntax. The spec says that `.launchpad.yaml` can say something like `plugin: tox` to cause the `tox` plugin, and only the `tox` plugin, to be used for the current build. This implementation seems to load and execute all discoverable plugins, which certainly makes sense for some plugin systems but it's not what we're looking for here.

This might be something you're going to address in a later branch; could you elaborate on your plans?

review: Needs Information

[Colin Watson (cjwatson)]

Incidentally, I can definitely see the appeal of being able to load more than one plugin and have it take effect; that seems like a good composable kind of design. It's just the discovery mechanic that I'm unsure about here.

[Jürgen Gmach (jugmac00)]

My plan was as outlined in LP-568 and LP-584:
- decide which functionality should be exposed via hooks (e.g. here the list of system packages)
- convert internal functions into plugins (e.g. lpcraft/plugin/lib.py -> lpcraft_install_packages)

And here the work for the current MP should have stopped, but I was nosy how external plugins work, so I added one, and then there was the discussion about how to test external plugins, and so I added a test for it.

Until then, I had a vague idea how to install plugins on runtime, which seems to be necessary if we ever want to support third party plugins. My idea, originating back from the very first thoughts about a plugin system was to use a subprocess to install the plugins via pip or similar, but as lpcraft no longer runs in the guest and has to run different jobs with different plugins, this is no longer viable.

Let's quickly define what I mean with internal and external plugins:

internal plugin = replace data/function with a hook, and provide the original data/function via an implementation

external plugin = every plugin, no matter whether created by us (e.g. tox plugin), or a future third party plugin, which can add data or even change behavior

It is true, that all hooks of all installed plugins are loaded, but not necessarily executed.

All (external) hooks have names, and they can be filtered by e.g. name.

```
(Pdb++) list(pm.get_plugins())[0].__name__
'lp_tox'
```

So when there is the `tox` plugin mentioned in the configuration file, all other plugins could be filtered out, and only the tox hook gets executed.

This may sound a bit complicated, but this way all external plugins could have the same structure (and currently we could just install all plugins in the same environment).

For this approach I assumed that all external plugins are outside the lpcraft repository.

An alternative approach could be to dynamically load plugins from a module/class from within lpcraft, e.g. we add a tox plugin to the lpcraft repository.

Additionally to loading "internal" plugins via `pm.register(HookImplementations(job))` we could also load additional plugins, e.g. `pm.register(<plugin>(job))` - for this approach we'd possibly need a mapping from strings from the configuration file (plugin: tox) to class names, where the classes provide all the implementations for the relevant hooks.

This all may or may not be too complex, and maybe even a simple data structure would be enough for a plugin? e.g. a list of additional deb and snap packages, a list of environment variables... Then a plugin could be a simple dictionary, or a data class, or an ini file? This should be clarified, too.

From LP-568 there is at least one question unanswered - how will we install third party plugins?

Maybe something like this?

```
pipeline:
    - test

jobs:
    test:
        series: focal
        architectures: amd64
        run: check-python-versions
        plugin: lp/check-python-versions
```

... where `lp/check-python-versions` would translate to a git repository on Launchpad, which would be cloned and installed -> and then the above approach with setuptools entry_points would pick it up.

While third party plugins ar...

[Colin Watson (cjwatson)]

I think I get it now, and this all looks basically fine - it just needs a few comment improvements. Thanks for working on this.

Could you add copyright/licence statements to the top of all the new files?

review: Approve

[Jürgen Gmach (jugmac00)]

Thanks for the review!

I applied all the suggestions.

> Could you add copyright/licence statements to the top of all the new files?

This is the third time... oO

... almost got it... :-)

```
- repo: local
    hooks:
    - id: python-check-license-statements
      name: Check license statements
      types: [python]
      entry: "Canonical"
      language: pygrep
      args: [--negate]
```

diff --git a/.mypy.ini b/.mypy.ini
index 3b71bdb..60a899d 100644
--- a/.mypy.ini
+++ b/.mypy.ini
@@ -1,5 +1,4 @@
 [mypy]
-plugins = pydantic.mypy
 python_version = 3.8
 
 [mypy-*.tests.*]
@@ -7,7 +6,7 @@ disallow_subclassing_any = false
 disallow_untyped_calls = false
 disallow_untyped_defs = false
 
-[mypy-fixtures.*,systemfixtures.*,testtools.*]
+[mypy-fixtures.*,systemfixtures.*,testtools.*,pluggy.*]
 ignore_missing_imports = true
 
 [mypy-craft_cli.*]
diff --git a/docs/index.rst b/docs/index.rst
index 8d171d8..c37c2fc 100644
--- a/docs/index.rst
+++ b/docs/index.rst
@@ -46,4 +46,5 @@ Example configuration
 
     self
     configuration
+    plugins
     CONTRIBUTING
diff --git a/docs/plugins.rst b/docs/plugins.rst
new file mode 100644
index 0000000..4c644d6
--- /dev/null
+++ b/docs/plugins.rst
@@ -0,0 +1,22 @@
+Extending lpcraft
+=================
+
+lpcraft uses `pluggy <https://pluggy.readthedocs.io/>`_ to customize the
+default behaviour. For example the following code snippet would extend the
+packages which need to be installed into an environment by additional
+requirements:
+
+.. code-block:: python
+
+    from lpcraft.plugin import hookimpl
+
+
+    @hookimpl
+    def lpcraft_install_packages():
+        return ["tox"]
+
+Plugin discovery
+----------------
+
+As currently only builtin plugins are supported,
+you need to define a plugin in ``lpcraft/plugins/<plugin>``.
diff --git a/lpcraft/commands/run.py b/lpcraft/commands/run.py
index ec99c11..3c53eef 100644
--- a/lpcraft/commands/run.py
+++ b/lpcraft/commands/run.py
@@ -3,6 +3,7 @@
 
 import fnmatch
 import io
+import itertools
 import json
 import os
 from argparse import Namespace
@@ -17,6 +18,7 @@ from dotenv import dotenv_values
 from lpcraft import env
 from lpcraft.config import Config, Job, Output
 from lpcraft.errors import CommandError
+from lpcraft.plugin.manager import get_plugin_manager
 from lpcraft.providers import Provider, get_provider
 from lpcraft.utils import get_host_architecture
 
@@ -193,8 +195,10 @@ def _run_job(
                     channel="stable",
                     classic=True,
                 )
-        if job.packages:
-            packages_cmd = ["apt", "install", "-y"] + job.packages
+        pm = get_plugin_manager(job)
+        packages = list(itertools.chain(*pm.hook.lpcraft_install_packages()))
+        if packages:
+            packages_cmd = ["apt", "install", "-y"] + packages
             emit.progress("Installing system packages")
             with emit.open_stream(f"Running {packages_cmd}") as stream:
                 proc = instance.execute_run(
diff --git a/lpcraft/config.py b/lpcraft/config.py
index cdfda54..ed6ae7f 100644
--- a/lpcraft/config.py
+++ b/lpcraft/config.py
@@ -67,6 +67,7 @@ class Job(ModelConfigDefaults):
     output: Optional[Output]
     snaps: Optional[List[StrictStr]]
     packages: Optional[List[StrictStr]]
+    plugin: Optional[StrictStr]
 
     @pydantic.validator("architectures", pre=True)
     def validate_architectures(
diff --git a/lpcraft/plugin/__init__.py b/lpcraft/plugin/__init__.py
new file mode 100644
index 0000000..d514ac6
--- /dev/null
+++ b/lpcraft/plugin/__init__.py
@@ -0,0 +1,6 @@
+# Copyright 2021 Canonical Ltd.  This software is licensed under the
+# GNU General Public License version 3 (see the file LICENSE).
+
+import pluggy
+
+hookimpl = pluggy.HookimplMarker("lpcraft")
diff --git a/lpcraft/plugin/hookspecs.py b/lpcraft/plugin/hookspecs.py
new file mode 100644
index 0000000..f16f909
--- /dev/null
+++ b/lpcraft/plugin/hookspecs.py
@@ -0,0 +1,13 @@
+# Copyright 2021 Canonical Ltd.  This software is licensed under the
+# GNU General Public License version 3 (see the file LICENSE).
+
+from __future__ import annotations
+
+import pluggy
+
+hookspec = pluggy.HookspecMarker("lpcraft")
+
+
+@hookspec  # type: ignore
+def lpcraft_install_packages() -> list[str]:
+    """system packages to be installed"""
diff --git a/lpcraft/plugin/lib.py b/lpcraft/plugin/lib.py
new file mode 100644
index 0000000..b22be5a
--- /dev/null
+++ b/lpcraft/plugin/lib.py
@@ -0,0 +1,25 @@
+"""`InternalPlugins` is the namespace for the internal plugins.
+
+see https://pluggy.readthedocs.io/en/stable/#define-and-collect-hooks
+
+Internal plugins provide the base functionality for lpcraft, which means that
+these plugins cannot be removed without breaking the application.
+
+The provided functionality can be extended by additional plugins.
+"""
+
+from __future__ import annotations
+
+from lpcraft.config import Job
+from lpcraft.plugin import hookimpl
+
+
+class InternalPlugins:
+    def __init__(self, config: Job) -> None:
+        self.config = config
+
+    @hookimpl  # type: ignore
+    def lpcraft_install_packages(self) -> list[str]:
+        if self.config.packages:
+            return self.config.packages
+        return []
diff --git a/lpcraft/plugin/manager.py b/lpcraft/plugin/manager.py
new file mode 100644
index 0000000..ad8ec66
--- /dev/null
+++ b/lpcraft/plugin/manager.py
@@ -0,0 +1,23 @@
+import pluggy
+
+from lpcraft.config import Job
+from lpcraft.errors import YAMLError
+from lpcraft.plugin import hookspecs
+from lpcraft.plugin.lib import InternalPlugins
+from lpcraft.plugins import PLUGINS
+
+
+def get_plugin_manager(job: Job) -> pluggy.PluginManager:
+    pm = pluggy.PluginManager("lpcraft")
+    pm.add_hookspecs(hookspecs)
+
+    # register internal plugins
+    pm.register(InternalPlugins(job))
+
+    # register builtin plugins
+    if job.plugin:
+        if job.plugin not in PLUGINS:
+            raise YAMLError("Unknown plugin")
+        pm.register(PLUGINS[job.plugin](job))
+
+    return pm
diff --git a/lpcraft/plugin/tests/__init__.py b/lpcraft/plugin/tests/__init__.py
new file mode 100644
index 0000000..e69de29
--- /dev/null
+++ b/lpcraft/plugin/tests/__init__.py
diff --git a/lpcraft/plugin/tests/test_builtin_plugins.py b/lpcraft/plugin/tests/test_builtin_plugins.py
new file mode 100644
index 0000000..79b0a1d
--- /dev/null
+++ b/lpcraft/plugin/tests/test_builtin_plugins.py
@@ -0,0 +1,112 @@
+# Copyright 2021 Canonical Ltd.  This software is licensed under the
+# GNU General Public License version 3 (see the file LICENSE).
+
+import subprocess
+from pathlib import Path, PosixPath
+from textwrap import dedent
+from unittest.mock import ANY, Mock, call, patch
+
+from craft_providers.lxd import LXC, launch
+
+from lpcraft.commands.tests import CommandBaseTestCase
+from lpcraft.errors import YAMLError
+from lpcraft.providers._lxd import LXDProvider, _LXDLauncher
+from lpcraft.providers.tests import FakeLXDInstaller
+
+
+class TestBuiltinPlugins(CommandBaseTestCase):
+    def makeLXDProvider(
+        self,
+        is_ready=True,
+        lxd_launcher=_LXDLauncher,
+    ):
+        lxc = Mock(spec=LXC)
+        lxc.remote_list.return_value = {}
+        lxd_installer = FakeLXDInstaller(is_ready=is_ready)
+        return LXDProvider(
+            lxc=lxc,
+            lxd_installer=lxd_installer,
+            lxd_launcher=lxd_launcher,
+        )
+
+    @patch("lpcraft.commands.run.get_provider")
+    @patch("lpcraft.commands.run.get_host_architecture", return_value="amd64")
+    def test_builtin_plugin(
+        self, mock_get_host_architecture, mock_get_provider
+    ):
+        launcher = Mock(spec=launch)
+        provider = self.makeLXDProvider(lxd_launcher=launcher)
+        mock_get_provider.return_value = provider
+        execute_run = launcher.return_value.execute_run
+        execute_run.return_value = subprocess.CompletedProcess([], 0)
+        # XXX jugmac00 2021-12-16: the current implementation of the `tox`
+        # plugin does not provide a run command, so we need to specify it
+        # here in the configuration file
+        config = dedent(
+            """
+            pipeline:
+                - test
+
+            jobs:
+                test:
+                    series: focal
+                    architectures: amd64
+                    packages: [nginx, apache2]
+                    run: tox
+                    plugin: tox
+            """
+        )
+        Path(".launchpad.yaml").write_text(config)
+
+        self.run_command("run")
+
+        self.assertEqual(
+            [
+                call(
+                    ["apt", "install", "-y", "tox", "nginx", "apache2"],
+                    cwd=PosixPath("/root/project"),
+                    env=None,
+                    stdout=ANY,
+                    stderr=ANY,
+                ),
+                call(
+                    ["bash", "--noprofile", "--norc", "-ec", "tox"],
+                    cwd=PosixPath("/root/project"),
+                    env=None,
+                    stdout=ANY,
+                    stderr=ANY,
+                ),
+            ],
+            execute_run.call_args_list,
+        )
+
+    @patch("lpcraft.commands.run.get_provider")
+    @patch("lpcraft.commands.run.get_host_architecture", return_value="amd64")
+    def test_execute_unknown_plugin(
+        self, mock_get_host_architecture, mock_get_provider
+    ):
+        launcher = Mock(spec=launch)
+        provider = self.makeLXDProvider(lxd_launcher=launcher)
+        mock_get_provider.return_value = provider
+        execute_run = launcher.return_value.execute_run
+        execute_run.return_value = subprocess.CompletedProcess([], 0)
+        config = dedent(
+            """
+            pipeline:
+                - test
+
+            jobs:
+                test:
+                    series: focal
+                    architectures: amd64
+                    packages: [nginx, apache2]
+                    run: non-existing
+                    plugin: non-existing
+            """
+        )
+        Path(".launchpad.yaml").write_text(config)
+
+        result = self.run_command("run")
+
+        self.assertEqual(1, result.exit_code)
+        self.assertEqual([YAMLError("Unknown plugin")], result.errors)
diff --git a/lpcraft/plugins/__init__.py b/lpcraft/plugins/__init__.py
new file mode 100644
index 0000000..58966af
--- /dev/null
+++ b/lpcraft/plugins/__init__.py
@@ -0,0 +1,5 @@
+from lpcraft.plugins.tox import ToxPlugin
+
+# XXX jugmac00 2021-12-16: The plugin mapping should be autogenerated by a
+# decorator, e.g. @register_plugin(name="<name>")
+PLUGINS = {"tox": ToxPlugin}
diff --git a/lpcraft/plugins/tox.py b/lpcraft/plugins/tox.py
new file mode 100644
index 0000000..2952371
--- /dev/null
+++ b/lpcraft/plugins/tox.py
@@ -0,0 +1,17 @@
+# Copyright 2021 Canonical Ltd.  This software is licensed under the
+# GNU General Public License version 3 (see the file LICENSE).
+
+from __future__ import annotations
+
+from lpcraft.config import Job
+from lpcraft.plugin import hookimpl
+
+
+class ToxPlugin:
+    # XXX jugmac00 2021-12-16: this plugin is not yet fully implemented
+    def __init__(self, config: Job) -> None:
+        self.config = config
+
+    @hookimpl  # type: ignore
+    def lpcraft_install_packages(self) -> list[str]:
+        return ["tox"]
diff --git a/lpcraft/tests/test_config.py b/lpcraft/tests/test_config.py
index 8280223..f07136d 100644
--- a/lpcraft/tests/test_config.py
+++ b/lpcraft/tests/test_config.py
@@ -339,3 +339,24 @@ class TestConfig(TestCase):
         )
         config = Config.load(path)
         self.assertEqual(None, config.jobs["test"][0].packages)
+
+    def test_load_plugin(self):
+        path = self.create_config(
+            dedent(
+                """
+                pipeline:
+                    - test
+
+                jobs:
+                    test:
+                        series: focal
+                        architectures: amd64
+                        packages: [nginx, apache2]
+                        plugin: tox
+                """
+            )
+        )
+
+        config = Config.load(path)
+
+        self.assertEqual("tox", config.jobs["test"][0].plugin)
diff --git a/requirements.in b/requirements.in
index 422bc41..b721260 100644
--- a/requirements.in
+++ b/requirements.in
@@ -3,3 +3,4 @@ pydantic
 PyYAML
 python-dotenv
 git+git://github.com/canonical/craft-cli.git@93db67c7bc357f0b6d9f5793b7c2381c306f9ca3
+pluggy
diff --git a/requirements.txt b/requirements.txt
index f81e3ac..ee685db 100644
--- a/requirements.txt
+++ b/requirements.txt
@@ -16,6 +16,8 @@ idna==3.3
     # via requests
 platformdirs==2.4.0
     # via craft-cli
+pluggy==1.0.0
+    # via -r requirements.in
 pydantic==1.8.2
     # via
     #   -r requirements.in
diff --git a/setup.cfg b/setup.cfg
index 7f9e2d8..02c6816 100644
--- a/setup.cfg
+++ b/setup.cfg
@@ -25,6 +25,7 @@ install_requires =
     PyYAML
     craft-cli
     craft-providers
+    pluggy
     pydantic
     python-dotenv
 python_requires = >=3.8