Repository: canonical/charmcraft

Issue:
Enforce deprecation DN05: Use charm-requirements in charmcraft.yaml parts to define requirements
See [the Deprecations page](https://juju.is/docs/sdk/charmcraft-deprecations) for more info.


Discussion:

Patch:
diff --git a/charmcraft/commands/build.py b/charmcraft/commands/build.py
index 1332333c..b96cfe0c 100644
--- a/charmcraft/commands/build.py
+++ b/charmcraft/commands/build.py
@@ -96,7 +96,6 @@ class Builder:
 
     def __init__(self, args, config):
         self.charmdir = args["from"]
-        self.requirement_paths = args["requirement"]
         self.force_packing = args["force"]
         self.debug = args["debug"]
         self.shell = args["shell"]
@@ -177,7 +176,7 @@ class Builder:
 
         if self._special_charm_part:
             # all current deprecated arguments set charm plugin parameters
-            self._handle_deprecated_cli_arguments()
+            self._pre_lifecycle_validation()
 
             # add charm files to the prime filter
             self._set_prime_filter()
@@ -215,37 +214,16 @@ class Builder:
         emit.message(f"Created '{zipname}'.", intermediate=True)
         return zipname
 
-    def _handle_deprecated_cli_arguments(self):
-        """Handle the mix of deprecated CLI args and config options.
-
-        This function also validate the result files from the mix when they are needed for
-        the lifecycle process itself (e.g. requirements.txt).
-        """
-        # XXX Facundo 2022-06-07: after these CLI args are removed, we could transform this
-        # function in a `_pre_lifecycle_validation` one.
-
-        # verify if deprecated --requirement is used and update the plugin property
-        if self._special_charm_part.get("charm-requirements"):
-            if self.requirement_paths:
-                raise CraftError(
-                    "--requirement not supported when charm-requirements "
-                    "specified in charmcraft.yaml"
-                )
-        else:
-            if self.requirement_paths:
-                self._special_charm_part["charm-requirements"] = [
-                    str(p) for p in self.requirement_paths
-                ]
-                self.requirement_paths = None
-            else:
-                default_reqfile = self.charmdir / "requirements.txt"
-                if default_reqfile.is_file():
-                    self._special_charm_part["charm-requirements"] = ["requirements.txt"]
-                else:
-                    self._special_charm_part["charm-requirements"] = []
-
-        # XXX Facundo 2022-06-27: this default should come from the config itself,
+    def _pre_lifecycle_validation(self):
+        """Handle pre lifecycle validations."""
+        # XXX Facundo 2022-06-27: these defaults should come from the config itself,
         # we need to refactor how the config is loaded.
+        if "charm-requirements" not in self._special_charm_part:
+            default_reqfile = self.charmdir / "requirements.txt"
+            if default_reqfile.is_file():
+                self._special_charm_part["charm-requirements"] = ["requirements.txt"]
+            else:
+                self._special_charm_part["charm-requirements"] = []
         entrypoint = self._special_charm_part.get("charm-entrypoint", "src/charm.py")
 
         # store the entrypoint always relative to the project's path (no matter if the origin
@@ -380,9 +358,6 @@ class Builder:
         if not managed_mode and not destructive_mode:
             self.provider.ensure_provider_is_available()
 
-        if self.requirement_paths:
-            notify_deprecation("dn05")
-
         build_plan = self.plan(
             bases_indices=bases_indices,
             destructive_mode=destructive_mode,
@@ -518,7 +493,6 @@ class Validator:
     _options = [
         "from",  # this needs to be processed first, as it's a base dir to find other files
         "destructive_mode",
-        "requirement",
         "bases_indices",
         "force",
         "debug",
@@ -585,17 +559,6 @@ class Validator:
         self.basedir = dirpath
         return dirpath
 
-    def validate_requirement(self, filepaths):
-        """Validate that the given requirement(s) (if any) exist."""
-        if filepaths is None:
-            return []
-
-        filepaths = [x.expanduser().absolute() for x in filepaths]
-        for fpath in filepaths:
-            if not fpath.exists():
-                raise CraftError("the requirements file was not found: {!r}".format(str(fpath)))
-        return filepaths
-
     def validate_shell(self, value):
         """Validate the value (just convert to bool to make None explicit)."""
         return bool(value)
@@ -641,14 +604,6 @@ class BuildCommand(BaseCommand):
             help="Charm directory with metadata.yaml where the build "
             "takes place; defaults to '.'",
         )
-        parser.add_argument(
-            "-r",
-            "--requirement",
-            action="append",
-            type=pathlib.Path,
-            help="File(s) listing needed PyPI dependencies (can be used multiple "
-            "times); defaults to 'requirements.txt'",
-        )
 
     def run(self, parsed_args):
         """Run the command."""
diff --git a/charmcraft/commands/pack.py b/charmcraft/commands/pack.py
index 2f080a4a..d56d57a5 100644
--- a/charmcraft/commands/pack.py
+++ b/charmcraft/commands/pack.py
@@ -29,7 +29,7 @@ from charmcraft.cmdbase import BaseCommand
 from charmcraft.commands import build
 from charmcraft.manifest import create_manifest
 from charmcraft.parts import Step
-from charmcraft.utils import load_yaml, useful_filepath
+from charmcraft.utils import load_yaml
 
 # the minimum set of files in a bundle
 MANDATORY_FILES = ["bundle.yaml", "README.md"]
@@ -92,16 +92,6 @@ class PackCommand(BaseCommand):
                 "changes to system configuration"
             ),
         )
-        parser.add_argument(
-            "-r",
-            "--requirement",
-            action="append",
-            type=useful_filepath,
-            help=(
-                "File(s) listing needed PyPI dependencies (can be used multiple "
-                "times); defaults to 'requirements.txt'"
-            ),
-        )
         parser.add_argument(
             "--shell",
             action="store_true",
@@ -131,8 +121,6 @@ class PackCommand(BaseCommand):
         if self.config.type == "charm":
             self._pack_charm(parsed_args)
         elif self.config.type == "bundle":
-            if parsed_args.requirement is not None:
-                raise CraftError("The -r/--requirement option is valid only when packing a charm")
             self._pack_bundle(parsed_args)
         else:
             raise CraftError("Unknown type {!r} in charmcraft.yaml".format(self.config.type))
@@ -146,7 +134,6 @@ class PackCommand(BaseCommand):
                 "debug": parsed_args.debug,
                 "destructive_mode": parsed_args.destructive_mode,
                 "from": self.config.project.dirpath,
-                "requirement": parsed_args.requirement,
                 "shell": parsed_args.shell,
                 "shell_after": parsed_args.shell_after,
                 "bases_indices": parsed_args.bases_index,
diff --git a/charmcraft/deprecations.py b/charmcraft/deprecations.py
index e0c57bf1..a0c35298 100644
--- a/charmcraft/deprecations.py
+++ b/charmcraft/deprecations.py
@@ -32,7 +32,6 @@ from charmcraft.env import is_charmcraft_running_in_managed_mode
 # the message to show for each deprecation ID (this needs to be in sync with the
 # documentation)
 _DEPRECATION_MESSAGES = {
-    "dn05": "Use 'charm-requirements' in charmcraft.yaml parts to define requirements.",
     "dn06": "The build command is deprecated, use 'pack' instead.",
 }
 
diff --git a/completion.bash b/completion.bash
index b3309d3c..b41c4821 100644
--- a/completion.bash
+++ b/completion.bash
@@ -85,14 +85,11 @@ _charmcraft()
             ;;
         build)
             case "$prev" in
-                -r|--requirement)
-                    _filedir txt
-                    ;;
                 -f|--from)
                     _filedir -d
                     ;;
                 *)
-                    COMPREPLY=( $(compgen -W "${globals[*]} --from --requirement" -- "$cur") )
+                    COMPREPLY=( $(compgen -W "${globals[*]} --from" -- "$cur") )
                     ;;
             esac
             ;;
@@ -107,14 +104,7 @@ _charmcraft()
             esac
             ;;
         pack)
-            case "$prev" in
-                -r|--requirement)
-                    _filedir txt
-                    ;;
-                *)
-                    COMPREPLY=( $(compgen -W "${globals[*]} --requirement --force --format" -- "$cur") )
-                    ;;
-            esac
+            COMPREPLY=( $(compgen -W "${globals[*]} --force --format" -- "$cur") )
             ;;
         release)
             COMPREPLY=( $(compgen -W "${globals[*]} --revision --channel --resource" -- "$cur") )


Test patch:
diff --git a/tests/commands/test_build.py b/tests/commands/test_build.py
index b32a55cd..fd84249a 100644
--- a/tests/commands/test_build.py
+++ b/tests/commands/test_build.py
@@ -15,7 +15,6 @@
 # For further info, check https://github.com/canonical/charmcraft
 
 import logging
-import os
 import pathlib
 import re
 import subprocess
@@ -50,7 +49,6 @@ def get_builder(
     config,
     *,
     project_dir=None,
-    requirement=None,
     force=False,
     debug=False,
     shell=False,
@@ -59,15 +57,11 @@ def get_builder(
     if project_dir is None:
         project_dir = config.project.dirpath
 
-    if requirement is None:
-        requirement = []
-
     return Builder(
         {
             "debug": debug,
             "force": force,
             "from": project_dir,
-            "requirement": requirement,
             "shell": shell,
             "shell_after": shell_after,
         },
@@ -309,92 +303,6 @@ def test_validator_bases_index_invalid(bases_indices, config):
         validator.validate_bases_indices(bases_indices)
 
 
-def test_validator_requirement_simple(tmp_path, config):
-    """'requirement' param: simple validation."""
-    testfile = tmp_path / "testfile"
-    testfile.touch()
-
-    validator = Validator(config)
-    resp = validator.validate_requirement([testfile])
-    assert resp == [testfile]
-
-
-def test_validator_requirement_multiple(tmp_path, config):
-    """'requirement' param: multiple files."""
-    testfile1 = tmp_path / "testfile1"
-    testfile1.touch()
-    testfile2 = tmp_path / "testfile2"
-    testfile2.touch()
-
-    validator = Validator(config)
-    resp = validator.validate_requirement([testfile1, testfile2])
-    assert resp == [testfile1, testfile2]
-
-
-def test_validator_requirement_none(tmp_path, config):
-    """'requirement' param: default value when a requirements.txt is there and readable."""
-    validator = Validator(config)
-    validator.basedir = tmp_path
-    resp = validator.validate_requirement(None)
-    assert resp == []
-
-
-def test_validator_requirement_default_present_not_readable(tmp_path, config):
-    """'requirement' param: default value when a requirements.txt is there but not readable."""
-    default_requirement = tmp_path / "requirements.txt"
-    default_requirement.touch(0o230)
-
-    validator = Validator(config)
-    validator.basedir = tmp_path
-    resp = validator.validate_requirement(None)
-    assert resp == []
-
-
-def test_validator_requirement_default_missing(tmp_path, config):
-    """'requirement' param: default value when no requirements.txt is there."""
-    validator = Validator(config)
-    validator.basedir = tmp_path
-    resp = validator.validate_requirement(None)
-    assert resp == []
-
-
-def test_validator_requirement_absolutized(tmp_path, monkeypatch, config):
-    """'requirement' param: check it's made absolute."""
-    # change dir to the temp one, where we will have the reqs file
-    testfile = tmp_path / "reqs.txt"
-    testfile.touch()
-    monkeypatch.chdir(tmp_path)
-
-    validator = Validator(config)
-    resp = validator.validate_requirement([pathlib.Path("reqs.txt")])
-    assert resp == [testfile]
-
-
-@pytest.mark.skipif(sys.platform == "win32", reason="Windows not [yet] supported")
-def test_validator_requirement_expanded(tmp_path, config):
-    """'requirement' param: expands the user-home prefix."""
-    fake_home = tmp_path / "homedir"
-    fake_home.mkdir()
-
-    requirement = fake_home / "requirements.txt"
-    requirement.touch(0o230)
-
-    validator = Validator(config)
-
-    with patch.dict(os.environ, {"HOME": str(fake_home)}):
-        resp = validator.validate_requirement([pathlib.Path("~/requirements.txt")])
-    assert resp == [requirement]
-
-
-@pytest.mark.skipif(sys.platform == "win32", reason="Windows not [yet] supported")
-def test_validator_requirement_exist(config):
-    """'requirement' param: checks that the file exists."""
-    validator = Validator(config)
-    expected_msg = "the requirements file was not found: '/not_really_there.txt'"
-    with pytest.raises(CraftError, match=expected_msg):
-        validator.validate_requirement([pathlib.Path("/not_really_there.txt")])
-
-
 @pytest.mark.parametrize(
     "inp_value,out_value",
     [
@@ -1512,7 +1420,7 @@ def test_build_postlifecycle_validation_entrypoint_missing(basic_project):
     config = load(basic_project)
 
     builder = get_builder(config, force=True)
-    builder._handle_deprecated_cli_arguments()  # load from config
+    builder._pre_lifecycle_validation()  # load from config
     with pytest.raises(CraftError) as cm:
         builder._post_lifecycle_validation(basic_project)
     entrypoint = basic_project / "missing.py"
@@ -1544,26 +1452,12 @@ def test_build_postlifecycle_validation_entrypoint_nonexec(basic_project):
     entrypoint.chmod(0o666)
 
     builder = get_builder(config, force=True)
-    builder._handle_deprecated_cli_arguments()  # load from config
+    builder._pre_lifecycle_validation()  # load from config
     with pytest.raises(CraftError) as cm:
         builder._post_lifecycle_validation(basic_project)
     assert str(cm.value) == f"Charm entry point must be executable: {str(entrypoint)!r}"
 
 
-def test_build_with_requirement_argment_issues_dn05(basic_project, emitter):
-    """Test cases for base-index parameter."""
-    config = load(basic_project)
-    builder = get_builder(config, requirement=["reqs.txt"])
-
-    with patch("charmcraft.commands.build.Builder.build_charm"):
-        builder.run(destructive_mode=True)
-
-    emitter.assert_message(
-        "DEPRECATED: Use 'charm-requirements' in charmcraft.yaml parts to define requirements.",
-        intermediate=True,
-    )
-
-
 def test_build_requirements_from_parts(basic_project, monkeypatch):
     """Test cases for base-index parameter."""
     host_base = get_host_as_base()
@@ -1629,70 +1523,6 @@ def test_build_requirements_from_parts(basic_project, monkeypatch):
     )
 
 
-def test_build_requirements_from_commandline(basic_project, monkeypatch, emitter):
-    """Test cases for base-index parameter."""
-    host_base = get_host_as_base()
-    charmcraft_file = basic_project / "charmcraft.yaml"
-    charmcraft_file.write_text(
-        dedent(
-            f"""\
-                type: charm
-                bases:
-                  - build-on:
-                      - name: {host_base.name!r}
-                        channel: {host_base.channel!r}
-                    run-on:
-                      - name: {host_base.name!r}
-                        channel: {host_base.channel!r}
-
-                parts:
-                  charm:
-                    charm-entrypoint: src/charm.py
-                """
-        )
-    )
-    config = load(basic_project)
-    builder = get_builder(config, force=True, requirement=["reqs.txt"])
-
-    reqs = basic_project / "reqs.txt"
-    reqs.touch()
-
-    monkeypatch.setenv("CHARMCRAFT_MANAGED_MODE", "1")
-    with patch("charmcraft.parts.PartsLifecycle", autospec=True) as mock_lifecycle:
-        mock_lifecycle.side_effect = SystemExit()
-        with pytest.raises(SystemExit):
-            builder.run([0])
-    mock_lifecycle.assert_has_calls(
-        [
-            call(
-                {
-                    "charm": {
-                        "plugin": "charm",
-                        "prime": [
-                            "src",
-                            "venv",
-                            "metadata.yaml",
-                            "dispatch",
-                            "hooks",
-                            "lib",
-                            "LICENSE",
-                            "icon.svg",
-                            "README.md",
-                        ],
-                        "charm-entrypoint": "src/charm.py",
-                        "charm-requirements": ["reqs.txt"],
-                        "source": str(basic_project),
-                    }
-                },
-                work_dir=pathlib.Path("/root"),
-                project_dir=basic_project,
-                project_name="name-from-metadata",
-                ignore_local_sources=["*.charm"],
-            )
-        ]
-    )
-
-
 def test_build_requirements_default(basic_project, monkeypatch, emitter):
     """Test cases for base-index parameter."""
     host_base = get_host_as_base()
@@ -1817,42 +1647,6 @@ def test_build_requirements_no_requirements_txt(basic_project, monkeypatch, emit
     )
 
 
-def test_build_requirements_from_both(basic_project, monkeypatch, emitter):
-    """Test cases for base-index parameter."""
-    host_base = get_host_as_base()
-    charmcraft_file = basic_project / "charmcraft.yaml"
-    charmcraft_file.write_text(
-        dedent(
-            f"""\
-                type: charm
-                bases:
-                  - build-on:
-                      - name: {host_base.name!r}
-                        channel: {host_base.channel!r}
-                    run-on:
-                      - name: {host_base.name!r}
-                        channel: {host_base.channel!r}
-
-                parts:
-                  charm:
-                    charm-requirements: ["reqs.txt"]
-                """
-        )
-    )
-    config = load(basic_project)
-    builder = get_builder(config, force=True, requirement=["reqs.txt"])
-
-    reqs = basic_project / "reqs.txt"
-    reqs.touch()
-
-    monkeypatch.setenv("CHARMCRAFT_MANAGED_MODE", "1")
-    with pytest.raises(CraftError) as raised:
-        builder.run([0])
-    assert str(raised.value) == (
-        "--requirement not supported when charm-requirements specified in charmcraft.yaml"
-    )
-
-
 def test_build_python_packages_from_parts(basic_project, monkeypatch):
     """Test cases for base-index parameter."""
     host_base = get_host_as_base()
diff --git a/tests/commands/test_pack.py b/tests/commands/test_pack.py
index ff651e87..a4e0e699 100644
--- a/tests/commands/test_pack.py
+++ b/tests/commands/test_pack.py
@@ -18,7 +18,7 @@ import datetime
 import pathlib
 import sys
 import zipfile
-from argparse import ArgumentParser, Namespace
+from argparse import Namespace
 from textwrap import dedent
 from unittest import mock
 from unittest.mock import MagicMock, call, patch
@@ -31,7 +31,6 @@ from charmcraft.cmdbase import JSON_FORMAT
 from charmcraft.commands import pack
 from charmcraft.commands.pack import PackCommand, build_zip
 from charmcraft.config import Project, load
-from charmcraft.utils import useful_filepath
 
 
 def get_namespace(
@@ -40,7 +39,6 @@ def get_namespace(
     debug=False,
     destructive_mode=False,
     force=None,
-    requirement=None,
     shell=False,
     shell_after=False,
     format=None,
@@ -53,7 +51,6 @@ def get_namespace(
         debug=debug,
         destructive_mode=destructive_mode,
         force=force,
-        requirement=requirement,
         shell=shell,
         shell_after=shell_after,
         format=format,
@@ -131,16 +128,6 @@ def test_resolve_no_config_packs_charm(config, tmp_path):
     mock.assert_called_with(noargs)
 
 
-def test_resolve_bundle_with_requirement(config):
-    """The requirement option is not valid when packing a bundle."""
-    config.set(type="bundle")
-    args = Namespace(requirement="reqs.txt")
-
-    with pytest.raises(CraftError) as cm:
-        PackCommand(config).run(args)
-    assert str(cm.value) == "The -r/--requirement option is valid only when packing a charm"
-
-
 # -- tests for main bundle building process
 
 
@@ -695,15 +682,6 @@ def test_zipbuild_symlink_outside(tmp_path):
 # infrastructure, until we migrate the (adapted) behaviour to this command
 
 
-def test_charm_parameters_requirement(config):
-    """The --requirement option implies a set of validations."""
-    cmd = PackCommand(config)
-    parser = ArgumentParser()
-    cmd.fill_parser(parser)
-    (action,) = [action for action in parser._actions if action.dest == "requirement"]
-    assert action.type is useful_filepath
-
-
 def test_charm_parameters_validator(config, tmp_path):
     """Check that build.Builder is properly called."""
     args = Namespace(
@@ -711,7 +689,6 @@ def test_charm_parameters_validator(config, tmp_path):
         debug=True,
         destructive_mode=True,
         force=True,
-        requirement="test-reqs",
         shell=True,
         shell_after=True,
         format=None,
@@ -732,7 +709,6 @@ def test_charm_parameters_validator(config, tmp_path):
                 "destructive_mode": True,
                 "from": tmp_path,
                 "force": True,
-                "requirement": "test-reqs",
                 "shell": True,
                 "shell_after": True,
             }