fix(strict-deps): fail if venv is inconsistent (#1784)

This runs `pip check` to ensure that all the packages in the charm
virtualenv have all of their required dependencies, failing the build if
they do not.

Fixes #1781

charmcraft/charm_builder.py

@@ -1,4 +1,4 @@
-# Copyright 2020-2023 Canonical Ltd.
+# Copyright 2020-2024 Canonical Ltd.
 #
 # Licensed under the Apache License, Version 2.0 (the "License");
 # you may not use this file except in compliance with the License.
@@ -61,18 +61,18 @@ def __init__(
         installdir: pathlib.Path,
         entrypoint: pathlib.Path,
         allow_pip_binary: bool = None,
-        binary_python_packages: list[str] = None,
-        python_packages: list[str] = None,
-        requirements: list[pathlib.Path] = None,
+        binary_python_packages: list[str] | None = None,
+        python_packages: list[str] | None = None,
+        requirements: list[pathlib.Path] | None = None,
         strict_dependencies: bool = False,
     ) -> None:
         self.builddir = builddir
         self.installdir = installdir
         self.entrypoint = entrypoint
         self.allow_pip_binary = allow_pip_binary
-        self.binary_python_packages = binary_python_packages
-        self.python_packages = python_packages
-        self.requirement_paths = requirements
+        self.binary_python_packages = binary_python_packages or []
+        self.python_packages = python_packages or []
+        self.requirement_paths = requirements or []
         self.strict_dependencies = strict_dependencies
         self.ignore_rules = self._load_juju_ignore()
         self.ignore_rules.extend_patterns([f"/{const.STAGING_VENV_DIRNAME}"])
@@ -227,7 +227,7 @@ def _calculate_dependencies_hash(self):
         return hashlib.sha1(deps_mashup.encode("utf8")).hexdigest()
 
     @instrum.Timer("Installing dependencies")
-    def _install_dependencies(self, staging_venv_dir):
+    def _install_dependencies(self, staging_venv_dir: pathlib.Path):
         """Install all dependencies in a specific directory."""
         # create virtualenv using the host environment python
         with instrum.Timer("Creating venv"):
@@ -309,6 +309,8 @@ def _install_strict_dependencies(self, pip_cmd: str) -> None:
                 binary_deps=self.binary_python_packages or [],
             )
         )
+        # Validate that the environment is consistent.
+        _process_run([pip_cmd, "check"])
 
     def handle_dependencies(self):
         """Handle from-directory and virtualenv dependencies."""

tests/integration/test_charm_builder.py

@@ -0,0 +1,85 @@
+# Copyright 2024 Canonical Ltd.
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+# For further info, check https://github.com/canonical/charmcraft
+"""Integration tests for CharmBuilder."""
+
+
+import pathlib
+
+import pytest
+
+from charmcraft import charm_builder
+
+
+@pytest.mark.parametrize(
+    "requirements",
+    [
+        ["ops==2.15.0"],  # Requires pyyaml and websocket-client
+    ],
+)
+def test_install_strict_dependencies_pip_check_error(
+    new_path: pathlib.Path, requirements: list[str]
+):
+    build_dir = new_path / "build"
+    install_dir = new_path / "install"
+    entrypoint = build_dir / "entrypoint.py"
+
+    build_dir.mkdir()
+    install_dir.mkdir()
+
+    requirements_file = build_dir / "requirements.txt"
+    requirements_file.write_text("\n".join(requirements))
+
+    builder = charm_builder.CharmBuilder(
+        builddir=build_dir,
+        installdir=install_dir,
+        entrypoint=entrypoint,
+        requirements=[requirements_file],
+        strict_dependencies=True,
+    )
+
+    with pytest.raises(RuntimeError, match="failed with retcode 1"):
+        builder.handle_dependencies()
+
+
+@pytest.mark.parametrize(
+    "requirements",
+    [
+        ["craft-platforms==0.1.0"],  # No dependencies
+    ],
+)
+def test_install_strict_dependencies_pip_check_success(
+    new_path: pathlib.Path, requirements: list[str]
+):
+    build_dir = new_path / "build"
+    install_dir = new_path / "install"
+    entrypoint = build_dir / "entrypoint.py"
+
+    build_dir.mkdir()
+    install_dir.mkdir()
+
+    requirements_file = build_dir / "requirements.txt"
+    requirements_file.write_text("\n".join(requirements))
+
+    builder = charm_builder.CharmBuilder(
+        builddir=build_dir,
+        installdir=install_dir,
+        entrypoint=entrypoint,
+        requirements=[requirements_file],
+        strict_dependencies=True,
+    )
+
+    with pytest.raises(RuntimeError, match="failed with retcode 1"):
+        builder.handle_dependencies()

tests/spread/dependencies/strict-dependencies-errors/missing-packages/charmcraft.yaml

@@ -0,0 +1,14 @@
+name: charm
+type: charm
+title: Strict dependencies test charm
+summary: a test charm for checking strict dependency build failures.
+description: |
+  This charm fails to build because it has strict dependencies enabled but does not
+  include the full dependency tree in its requirements file.
+bases:
+  - name: ubuntu
+    channel: "22.04"
+
+parts:
+  charm:
+    charm-strict-dependencies: true

tests/spread/dependencies/strict-dependencies-errors/missing-packages/pyproject.toml

@@ -0,0 +1,46 @@
+# Testing tools configuration
+[tool.coverage.run]
+branch = true
+
+[tool.coverage.report]
+show_missing = true
+
+[tool.pytest.ini_options]
+minversion = "6.0"
+log_cli_level = "INFO"
+
+# Formatting tools configuration
+[tool.black]
+line-length = 99
+target-version = ["py38"]
+
+# Linting tools configuration
+[tool.ruff]
+line-length = 99
+lint.select = ["E", "W", "F", "C", "N", "D", "I001"]
+lint.extend-ignore = [
+    "D203",
+    "D204",
+    "D213",
+    "D215",
+    "D400",
+    "D404",
+    "D406",
+    "D407",
+    "D408",
+    "D409",
+    "D413",
+]
+lint.ignore = ["E501", "D107"]
+extend-exclude = ["__pycache__", "*.egg_info"]
+lint.per-file-ignores = {"tests/*" = ["D100","D101","D102","D103","D104"]}
+
+[tool.ruff.lint.mccabe]
+max-complexity = 10
+
+[tool.codespell]
+skip = "build,lib,venv,icon.svg,.tox,.git,.mypy_cache,.ruff_cache,.coverage"
+
+[tool.pyright]
+include = ["src/**.py"]
+

tests/spread/dependencies/strict-dependencies-errors/missing-packages/requirements.txt

@@ -0,0 +1 @@
+pytest-time==0.3.1  # missing pytest

tests/spread/dependencies/strict-dependencies-errors/missing-packages/src/charm.py

@@ -0,0 +1,103 @@
+#!/usr/bin/env python3
+# Copyright 2023 Canonical Ltd.
+# See LICENSE file for licensing details.
+#
+# Learn more at: https://juju.is/docs/sdk
+
+"""Charm the service.
+
+Refer to the following tutorial that will help you
+develop a new k8s charm using the Operator Framework:
+
+https://juju.is/docs/sdk/create-a-minimal-kubernetes-charm
+"""
+
+import logging
+
+import ops
+
+# Log messages can be retrieved using juju debug-log
+logger = logging.getLogger(__name__)
+
+VALID_LOG_LEVELS = ["info", "debug", "warning", "error", "critical"]
+
+
+class CharmCharm(ops.CharmBase):
+    """Charm the service."""
+
+    def __init__(self, *args):
+        super().__init__(*args)
+        self.framework.observe(self.on["httpbin"].pebble_ready, self._on_httpbin_pebble_ready)
+        self.framework.observe(self.on.config_changed, self._on_config_changed)
+
+    def _on_httpbin_pebble_ready(self, event: ops.PebbleReadyEvent):
+        """Define and start a workload using the Pebble API.
+
+        Change this example to suit your needs. You'll need to specify the right entrypoint and
+        environment configuration for your specific workload.
+
+        Learn more about interacting with Pebble at at https://juju.is/docs/sdk/pebble.
+        """
+        # Get a reference the container attribute on the PebbleReadyEvent
+        container = event.workload
+        # Add initial Pebble config layer using the Pebble API
+        container.add_layer("httpbin", self._pebble_layer, combine=True)
+        # Make Pebble reevaluate its plan, ensuring any services are started if enabled.
+        container.replan()
+        # Learn more about statuses in the SDK docs:
+        # https://juju.is/docs/sdk/constructs#heading--statuses
+        self.unit.status = ops.ActiveStatus()
+
+    def _on_config_changed(self, event: ops.ConfigChangedEvent):
+        """Handle changed configuration.
+
+        Change this example to suit your needs. If you don't need to handle config, you can remove
+        this method.
+
+        Learn more about config at https://juju.is/docs/sdk/config
+        """
+        # Fetch the new config value
+        log_level = self.model.config["log-level"].lower()
+
+        # Do some validation of the configuration option
+        if log_level in VALID_LOG_LEVELS:
+            # The config is good, so update the configuration of the workload
+            container = self.unit.get_container("httpbin")
+            # Verify that we can connect to the Pebble API in the workload container
+            if container.can_connect():
+                # Push an updated layer with the new config
+                container.add_layer("httpbin", self._pebble_layer, combine=True)
+                container.replan()
+
+                logger.debug("Log level for gunicorn changed to '%s'", log_level)
+                self.unit.status = ops.ActiveStatus()
+            else:
+                # We were unable to connect to the Pebble API, so we defer this event
+                event.defer()
+                self.unit.status = ops.WaitingStatus("waiting for Pebble API")
+        else:
+            # In this case, the config option is bad, so block the charm and notify the operator.
+            self.unit.status = ops.BlockedStatus("invalid log level: '{log_level}'")
+
+    @property
+    def _pebble_layer(self) -> ops.pebble.LayerDict:
+        """Return a dictionary representing a Pebble layer."""
+        return {
+            "summary": "httpbin layer",
+            "description": "pebble config layer for httpbin",
+            "services": {
+                "httpbin": {
+                    "override": "replace",
+                    "summary": "httpbin",
+                    "command": "gunicorn -b 0.0.0.0:80 httpbin:app -k gevent",
+                    "startup": "enabled",
+                    "environment": {
+                        "GUNICORN_CMD_ARGS": f"--log-level {self.model.config['log-level']}"
+                    },
+                }
+            },
+        }
+
+
+if __name__ == "__main__":  # pragma: nocover
+    ops.main(CharmCharm)  # type: ignore

tests/spread/dependencies/strict-dependencies-errors/task.yaml

@@ -4,6 +4,7 @@ environment:
   CHARM/no_requirements: no-requirements/
   CHARM/extra_packages: extra-packages/
   CHARM/extra_binary_packages: extra-binary-packages/
+  CHARM/missing_packages: missing-packages/
 
 execute: |
   cd $CHARM

tests/spread/dependencies/strict-dependencies/charm/charmcraft.yaml

@@ -4,13 +4,14 @@ title: Strict dependencies test charm
 summary: a test charm for checking that strict dependencies are used.
 description: |
   This charm should build successfully, even with strict dependencies, but
-  will only use the dependencies from requirements.txt. The ops package, declared
-  in charm-binary-python-packages, is allowed to be installed from a wheel.
+  will only use the dependencies from requirements.txt. requirements.txt must include
+  the full dependency tree. The three packages are allowed as binary here to speed
+  up the test.
 bases:
   - name: ubuntu
     channel: "22.04"
 
 parts:
   charm:
     charm-strict-dependencies: true
-    charm-binary-python-packages: ["ops"]
+    charm-binary-python-packages: ["ops", "pyyaml", "websocket-client"]

tests/spread/dependencies/strict-dependencies/charm/requirements.txt

@@ -1 +1,3 @@
 ops==2.5.1
+pyyaml==6.0.1
+websocket-client==1.8.0

tests/unit/test_charm_builder.py

@@ -1,4 +1,4 @@
-# Copyright 2023 Canonical Ltd.
+# Copyright 2023-2024 Canonical Ltd.
 #
 # Licensed under the Apache License, Version 2.0 (the "License");
 # you may not use this file except in compliance with the License.
@@ -105,6 +105,10 @@ def test_install_strict_dependencies_success(
         "--no-binary=:all:",
         "--requirement=requirements.txt",
     ]
-    fake_process.register(expected_command, returncode=0)
+    install_cmd = fake_process.register(expected_command, returncode=0)
+    check_cmd = fake_process.register(["/pip", "check"], returncode=0)
 
     builder._install_strict_dependencies("/pip")
+
+    assert install_cmd.call_count() == 1
+    assert check_cmd.call_count() == 1