PythonDeps: move cli_only handling logic to _parse_requirements (#2547)

* [test case] tox4: fails to process requirement files with --hash

Add a test case for issue #2373

* Specifying `--hash` in the deps list doesn't work (pip would
  reject this anyway).
* Specifying `--hash` in a requirements.txt file named in the deps list
  should work, and recursive parsing should correctly extract the hash.

* PythonDeps: move cli_only handling logic to _parse_requirements

Remove `cli_only` parameter from `build_parser`.

Remove special case handling for `--hash` option (only valid in
requirements.txt files, not `pip install`).

Validate options in PythonDeps._parse_requirements:
  * Only check "cli_only" logic for ParsedRequirement objects that
    directly come from the PythonDeps.
  * Allow included requirements.txt lines to correctly parse `--hash` (Fix #2373).
  * Provides a more contextual error message to end users when `--hash`
    is used in the deps list.

* changelog for issue #2373

Author: masenf

docs/changelog/2373.bugfix.rst

@@ -0,0 +1 @@
+Allow ``--hash`` to be specified in requirements.txt files. - by :user:`masenf`.

src/tox/tox_env/python/pip/req/args.py

@@ -18,10 +18,10 @@ def exit(self, status: int = 0, message: str | None = None) -> NoReturn:  # noqa
         raise ValueError(msg)
 
 
-def build_parser(cli_only: bool) -> ArgumentParser:
+def build_parser() -> ArgumentParser:
     parser = _OurArgumentParser(add_help=False, prog="", allow_abbrev=False)
     _global_options(parser)
-    _req_options(parser, cli_only)
+    _req_options(parser)
     return parser
 
 
@@ -47,11 +47,10 @@ def _global_options(parser: ArgumentParser) -> None:
     )
 
 
-def _req_options(parser: ArgumentParser, cli_only: bool) -> None:
+def _req_options(parser: ArgumentParser) -> None:
     parser.add_argument("--install-option", action=AddSortedUniqueAction)
     parser.add_argument("--global-option", action=AddSortedUniqueAction)
-    if not cli_only:
-        parser.add_argument("--hash", action=AddSortedUniqueAction, type=_validate_hash)
+    parser.add_argument("--hash", action=AddSortedUniqueAction, type=_validate_hash)
 
 
 _HASH = re.compile(r"sha(256:[a-f0-9]{64}|384:[a-f0-9]{96}|512:[a-f0-9]{128})")

src/tox/tox_env/python/pip/req/file.py

@@ -156,7 +156,7 @@ def requirements(self) -> list[ParsedRequirement]:
     @property
     def _parser(self) -> ArgumentParser:
         if self._parser_private is None:
-            self._parser_private = build_parser(False)
+            self._parser_private = build_parser()
         return self._parser_private
 
     def _ensure_requirements_parsed(self) -> None:

src/tox/tox_env/python/pip/req_file.py

@@ -1,14 +1,17 @@
 from __future__ import annotations
 
 import re
-from argparse import ArgumentParser
+from argparse import Namespace
 from pathlib import Path
 
-from .req.args import build_parser
-from .req.file import ReqFileLines, RequirementsFile
+from .req.file import ParsedRequirement, ReqFileLines, RequirementsFile
 
 
 class PythonDeps(RequirementsFile):
+    # these options are valid in requirements.txt, but not via pip cli and
+    # thus cannot be used in the testenv `deps` list
+    _illegal_options = ["hash"]
+
     def __init__(self, raw: str, root: Path):
         super().__init__(root / "tox.ini", constraint=False)
         self._raw = self._normalize_raw(raw)
@@ -28,12 +31,6 @@ def _pre_process(self, content: str) -> ReqFileLines:
                 line = f"{line[0:2]} {line[2:]}"
             yield at, line
 
-    @property
-    def _parser(self) -> ArgumentParser:
-        if self._parser_private is None:
-            self._parser_private = build_parser(cli_only=True)  # e.g. no --hash for cli only
-        return self._parser_private
-
     def lines(self) -> list[str]:
         return self._raw.splitlines()
 
@@ -68,6 +65,20 @@ def _normalize_raw(raw: str) -> str:
         raw = f"{adjusted}\n" if raw.endswith("\\\n") else adjusted  # preserve trailing newline if input has it
         return raw
 
+    def _parse_requirements(self, opt: Namespace, recurse: bool) -> list[ParsedRequirement]:
+        # check for any invalid options in the deps list
+        # (requirements recursively included from other files are not checked)
+        requirements = super()._parse_requirements(opt, recurse)
+        for r in requirements:
+            if r.from_file != str(self.path):
+                continue
+            for illegal_option in self._illegal_options:
+                if r.options.get(illegal_option):
+                    raise ValueError(
+                        f"Cannot use --{illegal_option} in deps list, it must be in requirements file. ({r})",
+                    )
+        return requirements
+
     def unroll(self) -> tuple[list[str], list[str]]:
         if self._unroll is None:
             opts_dict = vars(self.options)

tests/tox_env/python/pip/test_req_file.py

@@ -14,3 +14,31 @@ def test_legacy_requirement_file(tmp_path: Path, legacy_flag: str) -> None:
     assert python_deps.as_root_args == [legacy_flag, "a.txt"]
     assert vars(python_deps.options) == {}
     assert [str(i) for i in python_deps.requirements] == ["b" if legacy_flag == "-r" else "-c b"]
+
+
+def test_deps_with_hash(tmp_path: Path) -> None:
+    """deps with --hash should raise an exception."""
+    python_deps = PythonDeps(
+        raw="foo==1 --hash sha256:97a702083b0d906517b79672d8501eee470d60ae55df0fa9d4cfba56c7f65a82",
+        root=tmp_path,
+    )
+    with pytest.raises(ValueError, match="Cannot use --hash in deps list"):
+        _ = python_deps.requirements
+
+
+def test_deps_with_requirements_with_hash(tmp_path: Path) -> None:
+    """deps can point to a requirements file that has --hash."""
+    exp_hash = "sha256:97a702083b0d906517b79672d8501eee470d60ae55df0fa9d4cfba56c7f65a82"
+    requirements = tmp_path / "requirements.txt"
+    requirements.write_text(
+        f"foo==1 --hash {exp_hash}",
+    )
+    python_deps = PythonDeps(
+        raw="-r requirements.txt",
+        root=tmp_path,
+    )
+    assert len(python_deps.requirements) == 1
+    parsed_req = python_deps.requirements[0]
+    assert str(parsed_req.requirement) == "foo==1"
+    assert parsed_req.options == {"hash": [exp_hash]}
+    assert parsed_req.from_file == str(requirements)