Merge pull request #357 from anaconda/v0.1.5

V0.1.5

CHANGELOG.md

@@ -1,6 +1,9 @@
 # Changelog
 Note: version releases in the 0.x.y range may introduce breaking changes.
 
+## 0.1.5
+- Bug fix: calls to get_read_only_parser result in unhandled errors on some recipes
+
 ## 0.1.4
 - Make check_url 403 results a warning
 - Add python build tools

anaconda_linter/__init__.py

@@ -3,7 +3,7 @@
 Description:    Module configurations for the `anaconda-linter` project
 """
 __name__ = "anaconda_linter"  # pylint: disable=redefined-builtin
-__version__ = "0.1.4"
+__version__ = "0.1.5"
 __author__ = "Anaconda, Inc."
 __email__ = "distribution_team@anaconda.com"
 __license__ = "BSD-3-Clause"

anaconda_linter/lint/__init__.py

@@ -356,10 +356,7 @@ def fix(self, message: LintMessage, data: Any) -> bool:  # pylint: disable=unuse
         """
         Attempt to automatically fix the linting error.
         :param message: Linting message emitted by the rule
-        :param data: Information vector for the rule to communicate to the fix.
-                     TODO: All callers should fill out a custom `dict[str, Any]` and then cast the types to a variable
-                           inside of their overridden `fix()` function. Then the data structure is easily expandable but
-                           still custom to each function call.
+        :param data: Information vector for the rule to communicate to the fix. TODO: standardize typing for all callers
         :returns: True if the fix succeeded. False otherwise
         """
         return False

anaconda_linter/lint/check_build_help.py

@@ -8,7 +8,7 @@
 import os
 import re
 from pathlib import Path
-from typing import Any, Final
+from typing import Any
 
 from percy.parser.recipe_parser import RecipeParser, SelectorConflictMode
 from percy.render.recipe import Recipe
@@ -953,15 +953,8 @@ class missing_python(LintCheck):
     """
 
     def check_recipe(self, recipe: Recipe) -> None:
-        ro_parser: Final[RecipeParser] = recipe.get_read_only_parser()
         is_pypi = is_pypi_source(recipe)
-        # TODO: Refactor and test this with the new parser work. PAT-273 tracks adding a comparable feature
-        #       to `recipe.packages` in the RecipeParser.
         for package in recipe.packages.values():
-            # PAT-270: Do not add `python` if the package is `noarch: generic`
-            if ro_parser.get_value(f"/{package.path_prefix}build/noarch", "") == "generic":
-                continue
-
             if (
                 is_pypi
                 or recipe.contains(f"{package.path_prefix}build/script", "pip install", "")
@@ -975,7 +968,6 @@ def check_recipe(self, recipe: Recipe) -> None:
                         )
 
     def fix(self, message, data) -> bool:
-        # TODO: Refactor and test this with the new parser work
         (recipe, package) = data
         op = [
             {

anaconda_linter/lint/check_syntax.py

@@ -5,9 +5,6 @@
 from __future__ import annotations
 
 import re
-from typing import Final, cast
-
-from percy.parser.recipe_parser import RecipeParser
 
 from anaconda_linter.lint import LintCheck
 
@@ -23,48 +20,45 @@ class version_constraints_missing_whitespace(LintCheck):
 
     """
 
-    # TODO Future: percy should eventually have support for constraints. This regex may not be sufficient enough to
-    # to catch all accepted formats. The spec can be found at:
-    #   https://docs.conda.io/projects/conda-build/en/stable/resources/package-spec.html#build-version-spec
-    _CONSTRAINTS_RE: Final[re.Pattern[str]] = re.compile("(.*?)([!<=>].*)")
-
     def check_recipe(self, recipe) -> None:
-        # Whitespace will be the same in all rendered forums of the recipe, so we use the RecipeParser infrastructure.
-        # TODO future: in these instances, recipes should not be rendered in all formats AND only look at the
-        # raw recipe file.
-        ro_parser: Final[RecipeParser] = recipe.get_read_only_parser()
-        check_paths: Final[list[str]] = ro_parser.get_dependency_paths()
+        check_paths = []
+        for section in ("build", "run", "host"):
+            check_paths.append(f"requirements/{section}")
+        if outputs := recipe.get("outputs", None):
+            for n in range(len(outputs)):
+                for section in ("build", "run", "host"):
+                    check_paths.append(f"outputs/{n}/requirements/{section}")
 
-        # Search all dependencies for missing whitespace
+        constraints = re.compile("(.*?)([!<=>].*)")
         for path in check_paths:
-            output: Final[int] = -1 if not path.startswith("/outputs") else int(path.split("/")[2])
-            spec = cast(str, ro_parser.get_value(path))
-            has_constraints = version_constraints_missing_whitespace._CONSTRAINTS_RE.search(spec)
-            if not has_constraints:
-                continue
-            # The second condition is a fallback.
-            # See: https://github.com/anaconda-distribution/anaconda-linter/issues/113
-            space_separated = has_constraints[1].endswith(" ") or " " in has_constraints[0]
-            if space_separated:
-                continue
-            dependency, version = has_constraints.groups()
-            new_dependency: Final[str] = f"{dependency} {version}"
-            self.message(
-                section=path,
-                data={"path": path, "new_dependency": new_dependency},
-                output=output,
-            )
+            output = -1 if not path.startswith("outputs") else int(path.split("/")[1])
+            for n, spec in enumerate(recipe.get(path, [])):
+                has_constraints = constraints.search(spec)
+                if has_constraints:
+                    # The second condition is a fallback.
+                    # See: https://github.com/anaconda-distribution/anaconda-linter/issues/113
+                    space_separated = has_constraints[1].endswith(" ") or " " in has_constraints[0]
+                    if not space_separated:
+                        self.message(section=f"{path}/{n}", data=True, output=output)
 
     def fix(self, message, data) -> bool:
-        path = cast(str, data["path"])
-        new_dependency = cast(str, data["new_dependency"])
+        check_paths = []
+        for section in ("build", "run", "host"):
+            check_paths.append(f"requirements/{section}")
+        if outputs := self.recipe.get("outputs", None):
+            for n in range(len(outputs)):
+                for section in ("build", "run", "host"):
+                    check_paths.append(f"outputs/{n}/requirements/{section}")
 
-        def _add_whitespace(parser: RecipeParser):
-            selector: Final[str] = (
-                "" if not parser.contains_selector_at_path(path) else parser.get_selector_at_path(path)
-            )
-            parser.patch({"op": "replace", "path": path, "value": new_dependency})
-            if selector:
-                parser.add_selector(path, selector)
-
-        return self.recipe.patch_with_parser(_add_whitespace)
+        constraints = re.compile("(.*?)([!<=>].*)")
+        for path in check_paths:
+            for spec in self.recipe.get(path, []):
+                has_constraints = constraints.search(spec)
+                if has_constraints:
+                    # The second condition is a fallback.
+                    # See: https://github.com/anaconda-distribution/anaconda-linter/issues/113
+                    space_separated = has_constraints[1].endswith(" ") or " " in has_constraints[0]
+                    if not space_separated:
+                        dep, ver = has_constraints.groups()
+                        self.recipe.replace(spec, f"{dep} {ver}", within="requirements")
+        return True

tests/conftest.py

@@ -133,7 +133,7 @@ def check_dir(check_name: str, feedstock_dir: str | Path, recipe_str: str, arch:
     return messages
 
 
-def assert_on_auto_fix(check_name: str, suffix: str, arch: str, num_occurrences: int) -> None:
+def assert_on_auto_fix(check_name: str, suffix: str, arch: str) -> None:
     """
     Utility function executes a fix function against an offending recipe file. Then asserts the resulting file against
     a known fixed equivalent of the offending recipe file.
@@ -142,8 +142,6 @@ def assert_on_auto_fix(check_name: str, suffix: str, arch: str, num_occurrences:
                             variants of input-to-expected-output files. If non-empty, the files should be named:
                             `<check_name>_<suffix>.yaml` and `<check_name>_<suffix>_fixed.yaml`, respectively.
     :param arch:            Target architecture to render recipe as
-    :param num_occurrences: How many times a rule should be expected to be found in the file. This allows for 1 set of
-                            test files to validate multiple variations of a rule.
     """
     suffix_adjusted: Final[str] = f"_{suffix}" if suffix else ""
     broken_file: Final[str] = f"{TEST_AUTO_FIX_FILES_PATH}/{check_name}{suffix_adjusted}.yaml"
@@ -156,14 +154,9 @@ def assert_on_auto_fix(check_name: str, suffix: str, arch: str, num_occurrences:
     with patch("builtins.open", mock_open()):
         messages = linter_obj.check_instances[check_name].run(recipe=recipe, fix=True)
 
-    # Ensure that:
-    #   - The rule triggered the expected number of times
-    #   - That the correct rule triggered
-    #   - And that the rule was actually fixed
-    # This supports
-    assert len(messages) == num_occurrences
-    for i in range(num_occurrences):
-        assert messages[i].auto_fix_state == AutoFixState.FIX_PASSED
-        assert str(messages[i].check) == check_name
+    # Ensure that the rule triggered, that the correct rule triggered, and that the rule was actually fixed
+    assert len(messages) == 1
+    assert messages[0].auto_fix_state == AutoFixState.FIX_PASSED
+    assert str(messages[0].check) == check_name
     # Ensure that the output matches the expected output
     assert recipe.dump() == load_file(fixed_file)

tests/lint/test_auto_fix_rules.py

@@ -18,32 +18,19 @@
 # Common file suffixes:
 #   - `mo` -> For multi-output test cases
 @pytest.mark.parametrize(
-    "check,suffix,arch,num_occurrences",
+    "check,suffix,arch",
     [
-        ## license_file_overspecified ##
-        # TODO add multi-output-test
-        ("license_file_overspecified", "", "linux-64", 1),
-        ("license_file_overspecified", "", "osx-arm64", 1),
-        ("license_file_overspecified", "", "win-64", 1),
-        ## no_git_on_windows ##
-        # TODO add multi-output-test
-        ("no_git_on_windows", "", "win-64", 1),
-        ## version_constraints_missing_whitespace ##
-        ("version_constraints_missing_whitespace", "", "linux-64", 4),
-        ("version_constraints_missing_whitespace", "", "osx-arm64", 4),
-        ("version_constraints_missing_whitespace", "", "win-64", 4),
-        ("version_constraints_missing_whitespace", "mo", "linux-64", 12),
-        ("version_constraints_missing_whitespace", "mo", "osx-arm64", 12),
-        ("version_constraints_missing_whitespace", "mo", "win-64", 12),
+        ("license_file_overspecified", "", "linux-64"),
+        ("license_file_overspecified", "", "win-64"),
+        ("no_git_on_windows", "", "win-64"),
     ],
 )
-def test_auto_fix_rule(check: str, suffix: str, arch: str, num_occurrences: int):
+def test_auto_fix_rule(check: str, suffix: str, arch: str):
     """
     Tests auto-fixable rules by passing in a file with the issue and checking the output with an expected file.
     Files must be stored in the `test_aux_files/auto_fix/` directory, following our naming conventions.
     :param check: Name of the linter check
     :param suffix: File suffix, allowing developers to create multiple test files against a linter check.
     :param arch: Architecture to run the test against.
-    :param num_occurrences: Number of times this rule should be triggered in the test file.
     """
-    assert_on_auto_fix(check, suffix, arch, num_occurrences)
+    assert_on_auto_fix(check, suffix, arch)

tests/lint/test_build_help.py

@@ -2559,73 +2559,6 @@ def test_missing_test_requirement_pip_script_bad_multi(base_yaml: str, recipe_di
     assert len(messages) == 2 and all("pip is required" in msg.title for msg in messages)
 
 
-def test_missing_python_does_not_trigger_on_noarch_generic(base_yaml: str) -> None:
-    """
-    Regression from PAT-270: Packages with `noarch: generic` should not fail this lint check.
-    """
-    yaml_str = (
-        base_yaml
-        + """
-        build:
-          noarch: generic
-        source:
-          url: https://pypi.io/packages/source/D/Django/Django-4.1.tar.gz
-        requirements:
-          host:
-            - foo
-          run:
-            - bar
-        """
-    )
-    lint_check = "missing_python"
-    messages = check(lint_check, yaml_str)
-    assert len(messages) == 0
-
-
-def test_missing_python_does_not_trigger_on_noarch_generic_multi_output(base_yaml: str) -> None:
-    """
-    Regression from PAT-270: Packages with `noarch: generic` should not fail this lint check. This checks for
-    multi-output recipes.
-    """
-    yaml_str = (
-        base_yaml
-        + """
-        outputs:
-          - name: test0
-            source:
-              url: https://github.com/foo/bar/bar-1.2.3.tar.gz
-            requirements:
-              host:
-                - foo
-              run:
-                - bar
-          - name: test1
-            build:
-              noarch: generic
-            source:
-              url: https://pypi.io/packages/source/D/Django/Django-4.1.tar.gz
-            requirements:
-              host:
-                - foo
-              run:
-                - bar
-          - name: test2
-            build:
-              noarch: python
-            source:
-              url: https://pypi.io/packages/source/D/Django/Django-4.1.tar.gz
-            requirements:
-              host:
-                - python
-              run:
-                - python
-        """
-    )
-    lint_check = "missing_python"
-    messages = check(lint_check, yaml_str)
-    assert len(messages) == 0
-
-
 def test_missing_python_url_good(base_yaml: str) -> None:
     yaml_str = (
         base_yaml