From 87194f681b8c50a2502bdf5a60c5ffe8e08281ab Mon Sep 17 00:00:00 2001 From: Bianca Henderson Date: Mon, 8 Jul 2024 12:17:19 -0400 Subject: [PATCH] Replace `sys.exit` calls in `conda_build/metadata.py` (#5371) * Update exceptions imports/usage in metadata.py * Replace sys.exit call with CondaBuildUserError for select_lines() function * Add unit test for CondaBuildUserError exception in select_lines test * Replace sys.exit call in _git_clean() and add unit test * Replace sys.exit call in check_bad_chrs() and add unit test * Update import statements and remove sys.exit call from yamlize() * Mark _get_env_path() for deprecation * Remove sys.exit call from parse_until_resolved() and add unit test * Remove sys.exit call from _get_contents() * Ignore deprecation warnings in metadata.py tests, point to correct exception in test_jinja_typo in test_api_build.py * Relax match string in error assertion for Windows * Revert 'cleanup' code changes from #5255 --- conda_build/metadata.py | 122 +++++++++++++++++++--------------------- tests/test_api_build.py | 2 +- tests/test_metadata.py | 74 ++++++++++++++++++++++++ 3 files changed, 134 insertions(+), 64 deletions(-) diff --git a/conda_build/metadata.py b/conda_build/metadata.py index d3ee86f214..527fc99e6a 100644 --- a/conda_build/metadata.py +++ b/conda_build/metadata.py @@ -12,18 +12,27 @@ import warnings from collections import OrderedDict from functools import lru_cache -from os.path import isfile, join +from os.path import isdir, isfile, join from typing import TYPE_CHECKING, NamedTuple, overload +import yaml from bs4 import UnicodeDammit -from conda.base.context import context +from conda.base.context import locate_prefix_by_name from conda.gateways.disk.read import compute_sum from conda.models.match_spec import MatchSpec from frozendict import deepfreeze -from . import exceptions, utils +from . import utils from .config import Config, get_or_merge_config from .deprecations import deprecated +from .exceptions import ( + CondaBuildException, + CondaBuildUserError, + DependencyNeedsBuildingError, + RecipeError, + UnableToParse, + UnableToParseMissingJinja2, +) from .features import feature_list from .license_family import ensure_valid_license_family from .utils import ( @@ -46,6 +55,7 @@ ) if TYPE_CHECKING: + from pathlib import Path from typing import Any, Literal, Self OutputDict = dict[str, Any] @@ -336,12 +346,12 @@ def select_lines(text: str, namespace: dict[str, Any], variants_in_place: bool) if value: lines.append(line) except Exception as e: - sys.exit( - f"Error: Invalid selector in meta.yaml line {i + 1}:\n" - f"offending line:\n" - f"{line}\n" + raise CondaBuildUserError( + f"Invalid selector in meta.yaml line {i + 1}:\n" + f"offending selector:\n" + f" [{selector}]\n" f"exception:\n" - f"{e.__class__.__name__}: {e}\n" + f" {e.__class__.__name__}: {e}\n" ) return "\n".join(lines) + "\n" @@ -356,10 +366,10 @@ def yamlize(data): jinja2 # Avoid pyflakes failure: 'jinja2' imported but unused except ImportError: - raise exceptions.UnableToParseMissingJinja2(original=e) + raise UnableToParseMissingJinja2(original=e) print("Problematic recipe:", file=sys.stderr) print(data, file=sys.stderr) - raise exceptions.UnableToParse(original=e) + raise UnableToParse(original=e) def ensure_valid_fields(meta): @@ -400,9 +410,7 @@ def _trim_None_strings(meta_dict): def ensure_valid_noarch_value(meta): build_noarch = meta.get("build", {}).get("noarch") if build_noarch and build_noarch not in NOARCH_TYPES: - raise exceptions.CondaBuildException( - f"Invalid value for noarch: {build_noarch}" - ) + raise CondaBuildException(f"Invalid value for noarch: {build_noarch}") def _get_all_dependencies(metadata, envs=("host", "build", "run")): @@ -444,7 +452,7 @@ def check_circular_dependencies( error = "Circular dependencies in recipe: \n" for pair in pairs: error += " {} <-> {}\n".format(*pair) - raise exceptions.RecipeError(error) + raise RecipeError(error) def _check_circular_dependencies( @@ -477,7 +485,7 @@ def _check_circular_dependencies( error = "Circular dependencies in recipe: \n" for pair in pairs: error += " {} <-> {}\n".format(*pair) - raise exceptions.RecipeError(error) + raise RecipeError(error) def _check_run_constrained(metadata_tuples): @@ -495,7 +503,7 @@ def _check_run_constrained(metadata_tuples): f"Reason: {exc}" ) if errors: - raise exceptions.RecipeError("\n".join(["", *errors])) + raise RecipeError("\n".join(["", *errors])) def _variants_equal(metadata, output_metadata): @@ -539,7 +547,7 @@ def ensure_matching_hashes(output_metadata): error += "Mismatching package: {} (id {}); dep: {}; consumer package: {}\n".format( *prob ) - raise exceptions.RecipeError( + raise RecipeError( "Mismatching hashes in recipe. Exact pins in dependencies " "that contribute to the hash often cause this. Can you " "change one or more exact pins to version bound constraints?\n" @@ -766,19 +774,18 @@ def _git_clean(source_meta): If more than one field is used to specified, exit and complain. """ - git_rev_tags_old = ("git_branch", "git_tag") git_rev = "git_rev" git_rev_tags = (git_rev,) + git_rev_tags_old - has_rev_tags = tuple(bool(source_meta.get(tag, "")) for tag in git_rev_tags) - if sum(has_rev_tags) > 1: - msg = "Error: multiple git_revs:" - msg += ", ".join( - f"{key}" for key, has in zip(git_rev_tags, has_rev_tags) if has - ) - sys.exit(msg) + + keys = [key for key in (git_rev, "git_branch", "git_tag") if key in source_meta] + if not keys: + # git_branch, git_tag, nor git_rev specified, return as-is + return source_meta + elif len(keys) > 1: + raise CondaBuildUserError(f"Multiple git_revs: {', '.join(keys)}") # make a copy of the input so we have no side-effects ret_meta = source_meta.copy() @@ -801,15 +808,16 @@ def _str_version(package_meta): return package_meta -def check_bad_chrs(s, field): - bad_chrs = "=@#$%^&*:;\"'\\|<>?/ " +def check_bad_chrs(value: str, field: str) -> None: + bad_chrs = set("=@#$%^&*:;\"'\\|<>?/ ") if field in ("package/version", "build/string"): - bad_chrs += "-" + bad_chrs.add("-") if field != "package/version": - bad_chrs += "!" - for c in bad_chrs: - if c in s: - sys.exit(f"Error: bad character '{c}' in {field}: {s}") + bad_chrs.add("!") + if invalid := bad_chrs.intersection(value): + raise CondaBuildUserError( + f"Bad character(s) ({''.join(sorted(invalid))}) in {field}: {value}." + ) def get_package_version_pin(build_reqs, name): @@ -882,20 +890,17 @@ def build_string_from_metadata(metadata): return build_str -# This really belongs in conda, and it is int conda.cli.common, -# but we don't presently have an API there. -def _get_env_path(env_name_or_path): - if not os.path.isdir(env_name_or_path): - for envs_dir in list(context.envs_dirs) + [os.getcwd()]: - path = os.path.join(envs_dir, env_name_or_path) - if os.path.isdir(path): - env_name_or_path = path - break - bootstrap_metadir = os.path.join(env_name_or_path, "conda-meta") - if not os.path.isdir(bootstrap_metadir): - print(f"Bootstrap environment '{env_name_or_path}' not found") - sys.exit(1) - return env_name_or_path +@deprecated( + "24.7", "24.9", addendum="Use `conda.base.context.locate_prefix_by_name` instead." +) +def _get_env_path( + env_name_or_path: str | os.PathLike | Path, +) -> str | os.PathLike | Path: + return ( + env_name_or_path + if isdir(env_name_or_path) + else locate_prefix_by_name(env_name_or_path) + ) def _get_dependencies_from_environment(env_name_or_path): @@ -1124,7 +1129,7 @@ def finalize_outputs_pass( fm.name(), deepfreeze({k: fm.config.variant[k] for k in fm.get_used_vars()}), ] = (output_d, fm) - except exceptions.DependencyNeedsBuildingError as e: + except DependencyNeedsBuildingError as e: if not permit_unsatisfiable_variants: raise else: @@ -1427,12 +1432,11 @@ def parse_until_resolved( ): """variant contains key-value mapping for additional functions and values for jinja2 variables""" - # undefined_jinja_vars is refreshed by self.parse again - undefined_jinja_vars = () # store the "final" state that we think we're in. reloading the meta.yaml file # can reset it (to True) final = self.final - # always parse again at least once. + + # always parse again at least once self.parse_again( permit_undefined_jinja=True, allow_no_other_outputs=allow_no_other_outputs, @@ -1440,6 +1444,8 @@ def parse_until_resolved( ) self.final = final + # recursively parse again so long as each iteration has fewer undefined jinja variables + undefined_jinja_vars = () while set(undefined_jinja_vars) != set(self.undefined_jinja_vars): undefined_jinja_vars = self.undefined_jinja_vars self.parse_again( @@ -1448,18 +1454,8 @@ def parse_until_resolved( bypass_env_check=bypass_env_check, ) self.final = final - if undefined_jinja_vars: - self.parse_again( - permit_undefined_jinja=False, - allow_no_other_outputs=allow_no_other_outputs, - bypass_env_check=bypass_env_check, - ) - sys.exit( - f"Undefined Jinja2 variables remain ({self.undefined_jinja_vars}). Please enable " - "source downloading and try again." - ) - # always parse again at the end, too. + # always parse again at the end without permit_undefined_jinja self.parse_again( permit_undefined_jinja=False, allow_no_other_outputs=allow_no_other_outputs, @@ -2116,8 +2112,8 @@ def _get_contents( except jinja2.TemplateError as ex: if "'None' has not attribute" in str(ex): ex = "Failed to run jinja context function" - sys.exit( - f"Error: Failed to render jinja template in {self.meta_path}:\n{str(ex)}" + raise CondaBuildUserError( + f"Failed to render jinja template in {self.meta_path}:\n{str(ex)}" ) finally: if "CONDA_BUILD_STATE" in os.environ: diff --git a/tests/test_api_build.py b/tests/test_api_build.py index 514db4f223..5f5e3f2263 100644 --- a/tests/test_api_build.py +++ b/tests/test_api_build.py @@ -504,7 +504,7 @@ def test_recursive_fail(testing_config): @pytest.mark.sanity def test_jinja_typo(testing_config): - with pytest.raises(SystemExit, match="GIT_DSECRIBE_TAG"): + with pytest.raises(CondaBuildUserError, match="GIT_DSECRIBE_TAG"): api.build( os.path.join(fail_dir, "source_git_jinja2_oops"), config=testing_config ) diff --git a/tests/test_metadata.py b/tests/test_metadata.py index 1b9fc34258..b8dc9df8e4 100644 --- a/tests/test_metadata.py +++ b/tests/test_metadata.py @@ -5,6 +5,7 @@ import os import subprocess import sys +from contextlib import nullcontext from itertools import product from typing import TYPE_CHECKING @@ -15,12 +16,15 @@ from conda_build import api from conda_build.config import Config +from conda_build.exceptions import CondaBuildUserError from conda_build.metadata import ( FIELDS, OPTIONALLY_ITERABLE_FIELDS, MetaData, _hash_dependencies, + check_bad_chrs, get_selectors, + sanitize, select_lines, yamlize, ) @@ -30,6 +34,8 @@ from .utils import metadata_dir, metadata_path, thisdir if TYPE_CHECKING: + from pathlib import Path + from pytest import MonkeyPatch @@ -200,6 +206,7 @@ def test_clobber_section_data(testing_metadata): @pytest.mark.serial +@pytest.mark.filterwarnings("ignore", category=PendingDeprecationWarning) def test_build_bootstrap_env_by_name(testing_metadata): assert not any( "git" in pkg for pkg in testing_metadata.meta["requirements"].get("build", []) @@ -218,6 +225,7 @@ def test_build_bootstrap_env_by_name(testing_metadata): subprocess.check_call(cmd.split()) +@pytest.mark.filterwarnings("ignore", category=PendingDeprecationWarning) def test_build_bootstrap_env_by_path(testing_metadata): assert not any( "git" in pkg for pkg in testing_metadata.meta["requirements"].get("build", []) @@ -549,3 +557,69 @@ def test_get_section(testing_metadata: MetaData): assert isinstance(section, list) else: assert isinstance(section, dict) + + +def test_select_lines_invalid(): + with pytest.raises( + CondaBuildUserError, + match=r"Invalid selector in meta\.yaml", + ): + select_lines("text # [{bad]", {}, variants_in_place=True) + + +@pytest.mark.parametrize( + "keys,expected", + [ + pytest.param([], {}, id="git_tag"), + pytest.param(["git_tag"], {"git_rev": "rev"}, id="git_tag"), + pytest.param(["git_branch"], {"git_rev": "rev"}, id="git_branch"), + pytest.param(["git_rev"], {"git_rev": "rev"}, id="git_rev"), + pytest.param(["git_tag", "git_branch"], None, id="git_tag + git_branch"), + pytest.param(["git_tag", "git_rev"], None, id="git_tag + git_rev"), + pytest.param(["git_branch", "git_rev"], None, id="git_branch + git_rev"), + pytest.param( + ["git_tag", "git_branch", "git_rev"], + None, + id="git_tag + git_branch + git_rev", + ), + ], +) +def test_sanitize_source(keys: list[str], expected: dict[str, str] | None) -> None: + with pytest.raises( + CondaBuildUserError, + match=r"Multiple git_revs:", + ) if expected is None else nullcontext(): + assert sanitize({"source": {key: "rev" for key in keys}}) == { + "source": expected + } + + +@pytest.mark.parametrize( + "value,field,invalid", + [ + pytest.param("good", "field", None, id="valid field"), + pytest.param("!@d&;-", "field", "!&;@", id="invalid field"), + pytest.param("good", "package/version", None, id="valid package/version"), + pytest.param("!@d&;-", "package/version", "&-;@", id="invalid package/version"), + pytest.param("good", "build/string", None, id="valid build/string"), + pytest.param("!@d&;-", "build/string", "!&-;@", id="invalid build/string"), + ], +) +def test_check_bad_chrs(value: str, field: str, invalid: str) -> None: + with pytest.raises( + CondaBuildUserError, + match=rf"Bad character\(s\) \({invalid}\) in {field}: {value}\.", + ) if invalid else nullcontext(): + check_bad_chrs(value, field) + + +def test_parse_until_resolved(testing_metadata: MetaData, tmp_path: Path) -> None: + (recipe := tmp_path / (name := "meta.yaml")).write_text("{{ UNDEFINED[:2] }}") + testing_metadata._meta_path = recipe + testing_metadata._meta_name = name + + with pytest.raises( + CondaBuildUserError, + match=("Failed to render jinja template"), + ): + testing_metadata.parse_until_resolved()