From b2050a40976b295907bd5978f829dc3b3f84dbd8 Mon Sep 17 00:00:00 2001 From: Charles Bousseau Date: Fri, 13 Sep 2024 15:04:54 -0400 Subject: [PATCH 1/6] Revert "Removes stale comment" This reverts commit a7d4de713c028ceb24e36011a4959e402289af41. --- tests/lint/test_auto_fix_rules.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/lint/test_auto_fix_rules.py b/tests/lint/test_auto_fix_rules.py index 957fbae..5d20bdd 100644 --- a/tests/lint/test_auto_fix_rules.py +++ b/tests/lint/test_auto_fix_rules.py @@ -20,6 +20,7 @@ @pytest.mark.parametrize( "check,suffix,arch,num_occurrences", [ + ## avoid_noarch ## license_file_overspecified ## # TODO add multi-output-test ("license_file_overspecified", "", "linux-64", 1), From 8c09a657ffd04117e5727db4454c8d085a716a82 Mon Sep 17 00:00:00 2001 From: Charles Bousseau Date: Fri, 13 Sep 2024 15:06:27 -0400 Subject: [PATCH 2/6] Revert "Fixes PAT-270 bug" This reverts commit d647019620c6a675d06e6cf2285ef01d216ad158. --- anaconda_linter/lint/check_build_help.py | 10 +--- tests/lint/test_auto_fix_rules.py | 1 - tests/lint/test_build_help.py | 67 ------------------------ 3 files changed, 1 insertion(+), 77 deletions(-) diff --git a/anaconda_linter/lint/check_build_help.py b/anaconda_linter/lint/check_build_help.py index 17fc756..a81abe0 100644 --- a/anaconda_linter/lint/check_build_help.py +++ b/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 = [ { diff --git a/tests/lint/test_auto_fix_rules.py b/tests/lint/test_auto_fix_rules.py index 5d20bdd..957fbae 100644 --- a/tests/lint/test_auto_fix_rules.py +++ b/tests/lint/test_auto_fix_rules.py @@ -20,7 +20,6 @@ @pytest.mark.parametrize( "check,suffix,arch,num_occurrences", [ - ## avoid_noarch ## license_file_overspecified ## # TODO add multi-output-test ("license_file_overspecified", "", "linux-64", 1), diff --git a/tests/lint/test_build_help.py b/tests/lint/test_build_help.py index 04b8d36..239c1e9 100644 --- a/tests/lint/test_build_help.py +++ b/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 From aa179218dd3204655a6bb597fd6f31314a7f91ca Mon Sep 17 00:00:00 2001 From: Charles Bousseau Date: Fri, 13 Sep 2024 15:08:10 -0400 Subject: [PATCH 3/6] Revert "Small comment change" This reverts commit ef309daf4a0d72ea1850778baa19671baeedc6d9. --- anaconda_linter/lint/check_syntax.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/anaconda_linter/lint/check_syntax.py b/anaconda_linter/lint/check_syntax.py index 62cda01..f0112df 100644 --- a/anaconda_linter/lint/check_syntax.py +++ b/anaconda_linter/lint/check_syntax.py @@ -23,9 +23,8 @@ 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 + # TODO Future: percy should eventually have support for constraints. This regex may not be + # sufficient enough to catch all accepted formats _CONSTRAINTS_RE: Final[re.Pattern[str]] = re.compile("(.*?)([!<=>].*)") def check_recipe(self, recipe) -> None: From 19f7d396c34c4f0369cd2a97aa5ae824a592be2d Mon Sep 17 00:00:00 2001 From: Charles Bousseau Date: Fri, 13 Sep 2024 15:08:19 -0400 Subject: [PATCH 4/6] Revert "Incorporates JC's feedback" This reverts commit 0f9d650e541b063dfbfb653bf85de86ae7d0bf37. --- anaconda_linter/lint/check_syntax.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/anaconda_linter/lint/check_syntax.py b/anaconda_linter/lint/check_syntax.py index f0112df..5dbc689 100644 --- a/anaconda_linter/lint/check_syntax.py +++ b/anaconda_linter/lint/check_syntax.py @@ -37,6 +37,7 @@ def check_recipe(self, recipe) -> None: # Search all dependencies for missing whitespace for path in check_paths: output: Final[int] = -1 if not path.startswith("/outputs") else int(path.split("/")[2]) + n = int(path.split("/")[-1]) spec = cast(str, ro_parser.get_value(path)) has_constraints = version_constraints_missing_whitespace._CONSTRAINTS_RE.search(spec) if not has_constraints: @@ -49,7 +50,7 @@ def check_recipe(self, recipe) -> None: dependency, version = has_constraints.groups() new_dependency: Final[str] = f"{dependency} {version}" self.message( - section=path, + section=f"{path}/{n}", data={"path": path, "new_dependency": new_dependency}, output=output, ) @@ -58,7 +59,7 @@ def fix(self, message, data) -> bool: path = cast(str, data["path"]) new_dependency = cast(str, data["new_dependency"]) - def _add_whitespace(parser: RecipeParser): + def _rm_whitespace(parser: RecipeParser): selector: Final[str] = ( "" if not parser.contains_selector_at_path(path) else parser.get_selector_at_path(path) ) @@ -66,4 +67,4 @@ def _add_whitespace(parser: RecipeParser): if selector: parser.add_selector(path, selector) - return self.recipe.patch_with_parser(_add_whitespace) + return self.recipe.patch_with_parser(_rm_whitespace) From c01f7b64cf3c5eb0abeedd406d2f867126b920fa Mon Sep 17 00:00:00 2001 From: Charles Bousseau Date: Fri, 13 Sep 2024 15:09:57 -0400 Subject: [PATCH 5/6] Revert "Fixes `version_constraints_missing_whitespace` rule" This reverts commit 7ffb77e6df2594cf5da1708f555099a01ea9630c. --- anaconda_linter/lint/__init__.py | 5 +- anaconda_linter/lint/check_syntax.py | 78 +++++++++++++--------------- tests/conftest.py | 17 ++---- tests/lint/test_auto_fix_rules.py | 25 +++------ 4 files changed, 48 insertions(+), 77 deletions(-) diff --git a/anaconda_linter/lint/__init__.py b/anaconda_linter/lint/__init__.py index e9b8ea3..07b4e60 100644 --- a/anaconda_linter/lint/__init__.py +++ b/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 diff --git a/anaconda_linter/lint/check_syntax.py b/anaconda_linter/lint/check_syntax.py index 5dbc689..5ce2415 100644 --- a/anaconda_linter/lint/check_syntax.py +++ b/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 catch all accepted formats - _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]) - n = int(path.split("/")[-1]) - 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=f"{path}/{n}", - 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 _rm_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(_rm_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 diff --git a/tests/conftest.py b/tests/conftest.py index ec3b103..dafac41 100644 --- a/tests/conftest.py +++ b/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: `_.yaml` and `__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) diff --git a/tests/lint/test_auto_fix_rules.py b/tests/lint/test_auto_fix_rules.py index 957fbae..0a1d4f2 100644 --- a/tests/lint/test_auto_fix_rules.py +++ b/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) From 3685dbb2ed62ac2b1df5cf1ca968c8b8c628d1f3 Mon Sep 17 00:00:00 2001 From: Charles Bousseau Date: Fri, 13 Sep 2024 15:26:37 -0400 Subject: [PATCH 6/6] increment to 0.1.5 --- CHANGELOG.md | 3 +++ anaconda_linter/__init__.py | 2 +- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 24c07ac..4001428 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/anaconda_linter/__init__.py b/anaconda_linter/__init__.py index d0d7514..0ab4866 100644 --- a/anaconda_linter/__init__.py +++ b/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"