252 Add sha256 variable support for bump-recipe (#254)

* Adds metrics logging on SHA-256 calculations

* Adds ability to update conda recipes that use common hash variables

* Fixes support for SHA-256 variables. Includes some minor logging and code cleanliness improvements. Fixes pytest-pep8 unit test accordingly

conda_recipe_manager/commands/bump_recipe.py

@@ -13,13 +13,49 @@
 
 from conda_recipe_manager.commands.utils.types import ExitCode
 from conda_recipe_manager.fetcher.artifact_fetcher import from_recipe as af_from_recipe
+from conda_recipe_manager.fetcher.base_artifact_fetcher import BaseArtifactFetcher
 from conda_recipe_manager.fetcher.http_artifact_fetcher import HttpArtifactFetcher
 from conda_recipe_manager.parser.recipe_parser import RecipeParser
 from conda_recipe_manager.parser.recipe_reader import RecipeReader
 from conda_recipe_manager.types import JsonPatchType
 
 log = logging.getLogger(__name__)
 
+## Constants ##
+
+
+class RecipePaths:
+    """
+    Namespace to store common recipe path constants.
+    """
+
+    BUILD_NUM: Final[str] = "/build/number"
+    SOURCE: Final[str] = "/source"
+    SINGLE_SHA_256: Final[str] = f"{SOURCE}/sha256"
+    VERSION: Final[str] = "/package/version"
+
+
+# Common variable names used for source artifact hashes.
+_COMMON_HASH_VAR_NAMES: Final[set[str]] = {"sha256", "hash", "hash_val", "hash_value"}
+
+## Functions ##
+
+
+def _validate_target_version(ctx: click.Context, param: str, value: str) -> str:  # pylint: disable=unused-argument
+    """
+    Provides additional input validation on the target package version.
+
+    :param ctx: Click's context object
+    :param param: Argument parameter name
+    :param value: Target value to validate
+    :raises click.BadParameter: In the event the input is not valid.
+    :returns: The value of the argument, if valid.
+    """
+    # NOTE: `None` indicates the flag is not provided.
+    if value == "":
+        raise click.BadParameter("The target version cannot be an empty string.")
+    return value
+
 
 def _exit_on_failed_patch(recipe_parser: RecipeParser, patch_blob: JsonPatchType) -> None:
     """
@@ -37,6 +73,16 @@ def _exit_on_failed_patch(recipe_parser: RecipeParser, patch_blob: JsonPatchType
     sys.exit(ExitCode.PATCH_ERROR)
 
 
+def _pre_update_cleanup(recipe_parser: RecipeParser) -> None:  # pylint: disable=unused-argument
+    """
+    Performs some recipe clean-up tasks before other upgrade operations can commence.
+
+    :param recipe_parser: Recipe file to update.
+    """
+    # TODO `hash_type` will need to be corrected
+    # TODO delete unused variables?
+
+
 def _update_build_num(recipe_parser: RecipeParser, increment_build_num: bool) -> None:
     """
     Attempts to update the build number in a recipe file.
@@ -55,20 +101,20 @@ def _update_build_num(recipe_parser: RecipeParser, increment_build_num: bool) ->
     # From the previous check, we know that `/build` exists. If `/build/number` is missing, it'll be added by
     # a patch-add operation and set to a default value of 0. Otherwise, we attempt to increment the build number, if
     # requested.
-    if increment_build_num and recipe_parser.contains_value("/build/number"):
-        build_number = recipe_parser.get_value("/build/number")
+    if increment_build_num and recipe_parser.contains_value(RecipePaths.BUILD_NUM):
+        build_number = recipe_parser.get_value(RecipePaths.BUILD_NUM)
 
         if not isinstance(build_number, int):
             log.error("Build number is not an integer.")
             sys.exit(ExitCode.ILLEGAL_OPERATION)
 
         _exit_on_failed_patch(
             recipe_parser,
-            cast(JsonPatchType, {"op": "replace", "path": "/build/number", "value": build_number + 1}),
+            cast(JsonPatchType, {"op": "replace", "path": RecipePaths.BUILD_NUM, "value": build_number + 1}),
         )
         return
 
-    _exit_on_failed_patch(recipe_parser, cast(JsonPatchType, {"op": "add", "path": "/build/number", "value": 0}))
+    _exit_on_failed_patch(recipe_parser, cast(JsonPatchType, {"op": "add", "path": RecipePaths.BUILD_NUM, "value": 0}))
 
 
 def _update_version(recipe_parser: RecipeParser, target_version: str) -> None:
@@ -87,12 +133,26 @@ def _update_version(recipe_parser: RecipeParser, target_version: str) -> None:
         recipe_parser.set_variable("version", target_version)
         # Generate a warning if `version` is not being used in the `/package/version` field. NOTE: This is a linear
         # search on a small list.
-        if "/package/version" not in recipe_parser.get_variable_references("version"):
+        if RecipePaths.VERSION not in recipe_parser.get_variable_references("version"):
             log.warning("`/package/version` does not use the defined JINJA variable `version`.")
         return
 
-    op: Final[str] = "replace" if recipe_parser.contains_value("/package/version") else "add"
-    _exit_on_failed_patch(recipe_parser, {"op": op, "path": "/package/version", "value": target_version})
+    op: Final[str] = "replace" if recipe_parser.contains_value(RecipePaths.VERSION) else "add"
+    _exit_on_failed_patch(recipe_parser, {"op": op, "path": RecipePaths.VERSION, "value": target_version})
+
+
+def _get_sha256(fetcher: HttpArtifactFetcher) -> str:
+    """
+    Wrapping function that attempts to retrieve an HTTP/HTTPS artifact with a retry mechanism.
+
+    :param fetcher: Artifact fetching instance to use.
+    :raises FetchError: If an issue occurred while downloading or extracting the archive.
+    :returns: The SHA-256 hash of the artifact, if it was able to be downloaded.
+    """
+    # TODO retry mechanism, and log attempts
+    # TODO attempt fetch in the background, especially if multiple fetch() calls are required.
+    fetcher.fetch()
+    return fetcher.get_archive_sha256()
 
 
 def _update_sha256(recipe_parser: RecipeParser) -> None:
@@ -105,32 +165,67 @@ def _update_sha256(recipe_parser: RecipeParser) -> None:
 
     :param recipe_parser: Recipe file to update.
     """
-    fetcher_lst = af_from_recipe(recipe_parser, True)
-    if not fetcher_lst:
+    fetcher_tbl = af_from_recipe(recipe_parser, True)
+    if not fetcher_tbl:
         log.warning("`/source` is missing or does not contain a supported source type.")
         return
 
-    # TODO handle case where SHA is stored in one or more variables (see cctools-ld64.yaml)
-    # TODO handle case where SHA is a variable
+    # Check to see if the SHA-256 hash might be set in a variable. In extremely rare cases, we log warnings to indicate
+    # that the "correct" action is unclear and likely requires human intervention. Otherwise, if we see a hash variable
+    # and it is used by a single source, we will edit the variable directly.
+    hash_vars_set: Final[set[str]] = _COMMON_HASH_VAR_NAMES & set(recipe_parser.list_variables())
+
+    if len(hash_vars_set) == 1 and len(fetcher_tbl) == 1:
+        hash_var: Final[str] = next(iter(hash_vars_set))
+        src_fetcher: Final[Optional[BaseArtifactFetcher]] = fetcher_tbl.get(RecipePaths.SOURCE, None)
+        # By far, this is the most commonly seen case when a hash variable name is used.
+        if (
+            src_fetcher
+            and isinstance(src_fetcher, HttpArtifactFetcher)
+            # NOTE: This is a linear search on a small list.
+            and RecipePaths.SINGLE_SHA_256 in recipe_parser.get_variable_references(hash_var)
+        ):
+            recipe_parser.set_variable(hash_var, _get_sha256(src_fetcher))
+            return
+
+        log.warning(
+            (
+                "Commonly used hash variable detected: `%s` but is not referenced by `/source/sha256`."
+                " The hash value will be changed directly at `/source/sha256`."
+            ),
+            hash_var,
+        )
+    elif len(hash_vars_set) > 1:
+        log.warning(
+            "Multiple commonly used hash variables detected. Hash values will be changed directly in `/source` keys."
+        )
 
-    # TODO Future: Figure out
     # NOTE: Each source _might_ have a different SHA-256 hash. This is the case for the `cctools-ld64` feedstock. That
     # project has a different implementation per architecture. However, in other circumstances, mirrored sources with
-    # different hashes might imply there is a security threat.
-    for src_path, fetcher in fetcher_lst.items():
+    # different hashes might imply there is a security threat. We will log some statistics so the user can best decide
+    # what to do.
+    unique_hashes: set[str] = set()
+    total_hash_cntr = 0
+    for src_path, fetcher in fetcher_tbl.items():
         if not isinstance(fetcher, HttpArtifactFetcher):
             continue
 
-        # TODO retry mechanism, and log attempts
-        # TODO attempt fetch in the background, especially if multiple fetch() calls are required.
-        fetcher.fetch()
-        sha = fetcher.get_archive_sha256()
+        sha = _get_sha256(fetcher)
+        total_hash_cntr += 1
+        unique_hashes.add(sha)
         sha_path = RecipeReader.append_to_path(src_path, "/sha256")
 
         # Guard against the unlikely scenario that the `sha256` field is missing.
         patch_op = "replace" if recipe_parser.contains_value(sha_path) else "add"
         _exit_on_failed_patch(recipe_parser, {"op": patch_op, "path": sha_path, "value": sha})
 
+    log.info(
+        "Found %d unique SHA-256 hash(es) out of a total of %d hash(es) in %d sources.",
+        len(unique_hashes),
+        total_hash_cntr,
+        len(fetcher_tbl),
+    )
+
 
 # TODO Improve. In order for `click` to play nice with `pyfakefs`, we set `path_type=str` and delay converting to a
 # `Path` instance. This is caused by how `click` uses decorators. See these links for more detail:
@@ -149,6 +244,7 @@ def _update_sha256(recipe_parser: RecipeParser) -> None:
     "--target-version",
     default=None,
     type=str,
+    callback=_validate_target_version,
     help="New project version to target. Required if `--build-num` is NOT specified.",
 )
 def bump_recipe(recipe_file_path: str, build_num: bool, target_version: Optional[str]) -> None:
@@ -181,6 +277,10 @@ def bump_recipe(recipe_file_path: str, build_num: bool, target_version: Optional
     # the `build_num` flag is invalidated if we are bumping to a new version. The build number must be reset to 0 in
     # this case.
     if target_version is not None:
+        if target_version == recipe_parser.get_value(RecipePaths.VERSION, default=None, sub_vars=True):
+            log.warning("The provided target version is the same value found in the recipe file: %s", target_version)
+
+        _pre_update_cleanup(recipe_parser)
         # Version must be updated before hash to ensure the correct artifact is hashed.
         _update_version(recipe_parser, target_version)
         _update_sha256(recipe_parser)

tests/commands/test_bump_recipe.py

@@ -90,7 +90,6 @@ def test_usage() -> None:
         ("gsm-amzn2-aarch64.yaml", "2.0.20210721.2", "bump_recipe/gsm-amzn2-aarch64_version_bump.yaml"),
         # Has a `sha256` variable
         ("pytest-pep8.yaml", None, "bump_recipe/pytest-pep8_build_num_2.yaml"),
-        # TODO: Fix expected file when we support the `sha256` variable
         ("pytest-pep8.yaml", "1.0.7", "bump_recipe/pytest-pep8_version_bump.yaml"),
         ("google-cloud-cpp.yaml", None, "bump_recipe/google-cloud-cpp_build_num_2.yaml"),
         ("google-cloud-cpp.yaml", "2.31.0", "bump_recipe/google-cloud-cpp_version_bump.yaml"),

tests/test_aux_files/bump_recipe/pytest-pep8_version_bump.yaml

@@ -1,6 +1,6 @@
 {% set name = "pytest-pep8" %}
 {% set version = "1.0.7" %}
-{% set sha256 = "032ef7e5fa3ac30f4458c73e05bb67b0f036a8a5cb418a534b3170f89f120318" %}
+{% set sha256 = "e594f5bc141acabe4b0298d05234e80195116667edad3d6a9cd610cab36bc4e1" %}
 
 package:
   name: {{ name|lower }}
@@ -9,7 +9,7 @@ package:
 source:
   fn: {{ name }}-{{ version }}.tar.gz
   url: https://pypi.io/packages/source/{{ name[0] }}/{{ name }}/{{ name }}-{{ version }}.tar.gz
-  sha256: e594f5bc141acabe4b0298d05234e80195116667edad3d6a9cd610cab36bc4e1
+  sha256: {{ sha256 }}
 
 build:
   noarch: python