Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add v2 recipe editing capabilities for bumping version & build number #2920

Open
wants to merge 14 commits into
base: main
Choose a base branch
from
17 changes: 14 additions & 3 deletions conda_forge_tick/hashing.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,18 @@
import hashlib
import math
import time
from multiprocessing import Pipe, Process
from multiprocessing import Pipe, Process, connection

import requests


def _hash_url(url, hash_type, progress=False, conn=None, timeout=None):
def _hash_url(
url: str,
hash_type: str,
progress: bool = False,
conn: connection.Connection | None = None,
timeout: int | None = None,
) -> str | None:
_hash = None
try:
ha = getattr(hashlib, hash_type)()
Expand Down Expand Up @@ -68,7 +74,12 @@ def _hash_url(url, hash_type, progress=False, conn=None, timeout=None):


@functools.lru_cache(maxsize=1024)
def hash_url(url, timeout=None, progress=False, hash_type="sha256"):
def hash_url(
url: str,
timeout: int | None = None,
progress: bool = False,
hash_type: str = "sha256",
) -> str | None:
"""Hash a url with a timeout.

Parameters
Expand Down
25 changes: 14 additions & 11 deletions conda_forge_tick/migrators/core.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
import logging
import re
import typing
from pathlib import Path
from typing import Any, List, Sequence, Set

import dateutil.parser
Expand All @@ -14,7 +15,7 @@
from conda_forge_tick.lazy_json_backends import LazyJson
from conda_forge_tick.make_graph import make_outputs_lut_from_graph
from conda_forge_tick.path_lengths import cyclic_topological_sort
from conda_forge_tick.update_recipe import update_build_number
from conda_forge_tick.update_recipe import update_build_number, v2
from conda_forge_tick.utils import (
frozen_to_json_friendly,
get_bot_run_url,
Expand Down Expand Up @@ -560,25 +561,27 @@
}
return cyclic_topological_sort(graph, top_level)

def set_build_number(self, filename: str) -> None:
def set_build_number(self, filename: str | Path) -> None:
"""Bump the build number of the specified recipe.

Parameters
----------
filename : str
Path the the meta.yaml
"""
with open(filename) as f:
raw = f.read()
filename = Path(filename)
if filename.name == "recipe.yaml":
filename.write_text(v2.update_build_number(filename, self.new_build_number))

Check warning on line 574 in conda_forge_tick/migrators/core.py

View check run for this annotation

Codecov / codecov/patch

conda_forge_tick/migrators/core.py#L574

Added line #L574 was not covered by tests
else:
raw = filename.read_text()

new_myaml = update_build_number(
raw,
self.new_build_number,
build_patterns=self.build_patterns,
)
new_myaml = update_build_number(
raw,
self.new_build_number,
build_patterns=self.build_patterns,
)

with open(filename, "w") as f:
f.write(new_myaml)
filename.write_text(new_myaml)

def new_build_number(self, old_number: int) -> int:
"""Determine the new build number to use.
Expand Down
40 changes: 24 additions & 16 deletions conda_forge_tick/migrators/version.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
import random
import typing
import warnings
from pathlib import Path
from typing import Any, List, Sequence

import conda.exceptions
Expand All @@ -14,9 +15,8 @@
from conda_forge_tick.contexts import FeedstockContext
from conda_forge_tick.migrators.core import Migrator
from conda_forge_tick.models.pr_info import MigratorName
from conda_forge_tick.os_utils import pushd
from conda_forge_tick.update_deps import get_dep_updates_and_hints
from conda_forge_tick.update_recipe import update_version
from conda_forge_tick.update_recipe import update_version, v2
from conda_forge_tick.utils import get_keys_default, sanitize_string

if typing.TYPE_CHECKING:
Expand Down Expand Up @@ -195,29 +195,37 @@
) -> "MigrationUidTypedDict":
version = attrs["new_version"]

with open(os.path.join(recipe_dir, "meta.yaml")) as fp:
raw_meta_yaml = fp.read()
recipe_dir = Path(recipe_dir)
meta_yaml = recipe_dir / "meta.yaml"
recipe_yaml = recipe_dir / "recipe.yaml"
if meta_yaml.exists():
raw_meta_yaml = meta_yaml.read_text()

updated_meta_yaml, errors = update_version(
raw_meta_yaml,
version,
hash_type=hash_type,
)
updated_meta_yaml, errors = update_version(
raw_meta_yaml,
version,
hash_type=hash_type,
)

if len(errors) == 0 and updated_meta_yaml is not None:
with pushd(recipe_dir):
with open("meta.yaml", "w") as fp:
fp.write(updated_meta_yaml)
self.set_build_number("meta.yaml")
if len(errors) == 0 and updated_meta_yaml is not None:
meta_yaml.write_text(updated_meta_yaml)
self.set_build_number(meta_yaml)

return super().migrate(recipe_dir, attrs)
else:
elif recipe_yaml.exists():
updated_recipe, errors = v2.update_version(recipe_yaml, version)
if len(errors) == 0 and updated_recipe is not None:
recipe_yaml.write_text(updated_recipe)
self.set_build_number(recipe_yaml)

Check warning on line 218 in conda_forge_tick/migrators/version.py

View check run for this annotation

Codecov / codecov/patch

conda_forge_tick/migrators/version.py#L214-L218

Added lines #L214 - L218 were not covered by tests

if len(errors) != 0:
raise VersionMigrationError(
_fmt_error_message(
errors,
version,
)
)
else:
return super().migrate(recipe_dir, attrs)

def pr_body(self, feedstock_ctx: FeedstockContext, add_label_text=False) -> str:
if feedstock_ctx.feedstock_name in self.effective_graph.nodes:
Expand Down
4 changes: 4 additions & 0 deletions conda_forge_tick/update_recipe/v2/__init__.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
from .build_number import update_build_number
from .version import update_version

__all__ = ["update_build_number", "update_version"]
61 changes: 61 additions & 0 deletions conda_forge_tick/update_recipe/v2/build_number.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
from __future__ import annotations

import logging
from typing import TYPE_CHECKING, Any, Literal

from conda_forge_tick.update_recipe.v2.yaml import _dump_yaml_to_str, _load_yaml

if TYPE_CHECKING:
from pathlib import Path

Check warning on line 9 in conda_forge_tick/update_recipe/v2/build_number.py

View check run for this annotation

Codecov / codecov/patch

conda_forge_tick/update_recipe/v2/build_number.py#L9

Added line #L9 was not covered by tests

logger = logging.getLogger(__name__)

HashType = Literal["md5", "sha256"]


def _update_build_number_in_context(
recipe: dict[str, Any], new_build_number: int
) -> bool:
for key in recipe.get("context", {}):
if key.startswith("build_") or key == "build":
recipe["context"][key] = new_build_number
return True
return False


def _update_build_number_in_recipe(
recipe: dict[str, Any], new_build_number: int
) -> bool:
is_modified = False
if "build" in recipe and "number" in recipe["build"]:
recipe["build"]["number"] = new_build_number
is_modified = True

if "outputs" in recipe:
for output in recipe["outputs"]:
if "build" in output and "number" in output["build"]:
output["build"]["number"] = new_build_number
is_modified = True

Check warning on line 38 in conda_forge_tick/update_recipe/v2/build_number.py

View check run for this annotation

Codecov / codecov/patch

conda_forge_tick/update_recipe/v2/build_number.py#L35-L38

Added lines #L35 - L38 were not covered by tests

return is_modified


def update_build_number(file: Path, new_build_number: int = 0) -> str:
"""
Update the build number in the recipe file.

Arguments:
----------
* `file` - The path to the recipe file.
* `new_build_number` - The new build number to use. (default: 0)

Returns:
--------
* The updated recipe as a string.
"""
data = _load_yaml(file)
build_number_modified = _update_build_number_in_context(data, new_build_number)
if not build_number_modified:
_update_build_number_in_recipe(data, new_build_number)

return _dump_yaml_to_str(data)
Comment on lines +56 to +61
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

People can and have mixed setting build numbers in both the jinja2 vars / context and explicitly in the recipe. I know it is awful. We should update the code to do both and check if the build number in the non-context part is templated or not before updating.

69 changes: 69 additions & 0 deletions conda_forge_tick/update_recipe/v2/conditional_list.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
from __future__ import annotations

from typing import TYPE_CHECKING, Any, Generic, List, TypeVar, Union, cast

if TYPE_CHECKING:
from collections.abc import Callable, Generator

Check warning on line 6 in conda_forge_tick/update_recipe/v2/conditional_list.py

View check run for this annotation

Codecov / codecov/patch

conda_forge_tick/update_recipe/v2/conditional_list.py#L6

Added line #L6 was not covered by tests

T = TypeVar("T")
K = TypeVar("K")


class IfStatement(Generic[T]):
if_: Any
then: T | list[T]
else_: T | list[T] | None


ConditionalList = Union[T, IfStatement[T], List[Union[T, IfStatement[T]]]]


def visit_conditional_list( # noqa: C901
value: T | IfStatement[T] | list[T | IfStatement[T]],
evaluator: Callable[[Any], bool] | None = None,
) -> Generator[T]:
"""
A function that yields individual branches of a conditional list.

Arguments
---------
* `value` - The value to evaluate
* `evaluator` - An optional evaluator to evaluate the `if` expression.

Returns
-------
A generator that yields the individual branches.
"""

def yield_from_list(value: list[K] | K) -> Generator[K]:
if isinstance(value, list):
yield from value

Check warning on line 40 in conda_forge_tick/update_recipe/v2/conditional_list.py

View check run for this annotation

Codecov / codecov/patch

conda_forge_tick/update_recipe/v2/conditional_list.py#L40

Added line #L40 was not covered by tests
else:
yield value

if not isinstance(value, list):
value = [value]

for element in value:
if isinstance(element, dict):
if (expr := element.get("if", None)) is not None:
then = element.get("then")
otherwise = element.get("else")
# Evaluate the if expression if the evaluator is provided
if evaluator:
if evaluator(expr):
yield from yield_from_list(then)
elif otherwise:
yield from yield_from_list(otherwise)

Check warning on line 57 in conda_forge_tick/update_recipe/v2/conditional_list.py

View check run for this annotation

Codecov / codecov/patch

conda_forge_tick/update_recipe/v2/conditional_list.py#L54-L57

Added lines #L54 - L57 were not covered by tests
# Otherwise, just yield the branches
else:
yield from yield_from_list(then)
if otherwise:
yield from yield_from_list(otherwise)

Check warning on line 62 in conda_forge_tick/update_recipe/v2/conditional_list.py

View check run for this annotation

Codecov / codecov/patch

conda_forge_tick/update_recipe/v2/conditional_list.py#L62

Added line #L62 was not covered by tests
else:
# In this case its not an if statement
yield cast(T, element)
# If the element is not a dictionary, just yield it
else:
# (tim) I get a pyright error here, but I don't know how to fix it
yield cast(T, element)

Check warning on line 69 in conda_forge_tick/update_recipe/v2/conditional_list.py

View check run for this annotation

Codecov / codecov/patch

conda_forge_tick/update_recipe/v2/conditional_list.py#L69

Added line #L69 was not covered by tests
19 changes: 19 additions & 0 deletions conda_forge_tick/update_recipe/v2/context.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
import jinja2


def load_recipe_context(
context: dict[str, str], jinja_env: jinja2.Environment
) -> dict[str, str]:
"""
Load all string values from the context dictionary as Jinja2 templates.
Use linux-64 as default target_platform, build_platform, and mpi.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm concerned that assuming linux as the default will result in invalid updates for some recipes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand where you are coming from but we do ignore the selectors in the version updating code (e.g. we go down both branches, then and else.

However, we do use the value for templating, so people could theoretically do something like

source:
  url: https://foo.${{ "linux" if linux else "bla" }}/...
  sha256: ${{ "foo" ...

But that doesn't seem very likely and also buggy so I don't think we'll see that a lot in the wild.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are folks allowed to use if..then in the context section to change the values of variables?

Copy link
Contributor Author

@wolfv wolfv Aug 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe that doesn't work in the context section. However you can use templating and inline Jinja expressions, such as:

context:
  version: "1.2.3"
  version_string: "v${{ version if linux else "foo" }}"

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right so we need to make this correct.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That won't change other items that might be platform dependent in the context section.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But the SHA hash will make it impossible, thankfully :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope. Folks could define parts of urls in the context section that are platform dependent that are not hashes. So when the bot comes through to render those urls and parts, it will compute the wrong hash since it might have a valid, but incorrect url for that platform.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@beckermr - i sorta get where you come from but is it really necessary to care about all of these edge cases on day one?

It is also not a very clever way to write the recipe. You would have to "double-if" the source.

E.g.

context:
  foo: ${{ "bla" if linux else "foo" }}

source:
  - if: linux
    then:
      url: ${{ foo }}
      sha256: 1abcd
    else:
      url: ${{ foo }}
      sha256: 2bcde

So I don't think many people would wanna write their sources like this (doesn't make much logical sense).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes we should care. My suggested procedure for this work was to abstract the current version update algorithm to be independent of the recipe format backend and then use that. It should handle most, if not all, of the cases I am bringing up.

"""
# Process each key-value pair in the dictionary
for key, value in context.items():
# If the value is a string, render it as a template
if isinstance(value, str):
template = jinja_env.from_string(value)
rendered_value = template.render(context)
context[key] = rendered_value

return context
3 changes: 3 additions & 0 deletions conda_forge_tick/update_recipe/v2/jinja/__init__.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
from .jinja import jinja_env

__all__ = ["jinja_env"]
28 changes: 28 additions & 0 deletions conda_forge_tick/update_recipe/v2/jinja/filters.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
from __future__ import annotations

from conda_forge_tick.update_recipe.v2.jinja.utils import _MissingUndefined


def _version_to_build_string(some_string: str | _MissingUndefined) -> str:
"""
Converts some version by removing the . character and returning only the first two elements of the version.
If piped value is undefined, it returns the undefined value as is.
"""
if isinstance(some_string, _MissingUndefined):
return f"{some_string._undefined_name}_version_to_build_string" # noqa: SLF001

Check warning on line 12 in conda_forge_tick/update_recipe/v2/jinja/filters.py

View check run for this annotation

Codecov / codecov/patch

conda_forge_tick/update_recipe/v2/jinja/filters.py#L11-L12

Added lines #L11 - L12 were not covered by tests
# We first split the string by whitespace and take the first part
split = some_string.split()[0] if some_string.split() else some_string

Check warning on line 14 in conda_forge_tick/update_recipe/v2/jinja/filters.py

View check run for this annotation

Codecov / codecov/patch

conda_forge_tick/update_recipe/v2/jinja/filters.py#L14

Added line #L14 was not covered by tests
# We then split the string by . and take the first two parts
parts = split.split(".")
major = parts[0] if len(parts) > 0 else ""
minor = parts[1] if len(parts) > 1 else ""
return f"{major}{minor}"

Check warning on line 19 in conda_forge_tick/update_recipe/v2/jinja/filters.py

View check run for this annotation

Codecov / codecov/patch

conda_forge_tick/update_recipe/v2/jinja/filters.py#L16-L19

Added lines #L16 - L19 were not covered by tests


def _bool(value: str) -> bool:
return bool(value)

Check warning on line 23 in conda_forge_tick/update_recipe/v2/jinja/filters.py

View check run for this annotation

Codecov / codecov/patch

conda_forge_tick/update_recipe/v2/jinja/filters.py#L23

Added line #L23 was not covered by tests


def _split(s: str, sep: str | None = None) -> list[str]:
"""Filter that split a string by a separator"""
return s.split(sep)

Check warning on line 28 in conda_forge_tick/update_recipe/v2/jinja/filters.py

View check run for this annotation

Codecov / codecov/patch

conda_forge_tick/update_recipe/v2/jinja/filters.py#L28

Added line #L28 was not covered by tests
Loading