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

fix: make sure to catch jinja2 vars used in single-line for and set statements #5447

Merged
merged 12 commits into from
Aug 7, 2024
18 changes: 17 additions & 1 deletion conda_build/variants.py
Original file line number Diff line number Diff line change
Expand Up @@ -745,15 +745,31 @@ def find_used_variables_in_text(variant, recipe_text, selectors_only=False):
v_req_regex = "[-_]".join(map(re.escape, v.split("_")))
variant_regex = rf"\{{\s*(?:pin_[a-z]+\(\s*?['\"])?{v_regex}[^'\"]*?\}}\}}"
selector_regex = rf"^[^#\[]*?\#?\s\[[^\]]*?(?<![_\w\d]){v_regex}[=\s<>!\]]"
# NOTE: why use a regex instead of the jinja2 parser/AST?
# One can ask the jinj2 parser for undefined variables, but conda-build moves whole
jaimergp marked this conversation as resolved.
Show resolved Hide resolved
# blocks of text around when searching for variables and applies selectors to the text.
# So the text that reaches this function is not necessarily valid jinja2 syntax. :/
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jaimergp @wolfv @h-vetinari I have added a comment explaining why we have to use a regex here. Let's merge this one instead of letting the perfect be the enemy of the good.

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, I think at this point it's impossible to fix this properly, sadly. LGTM!

conditional_regex = (
r"(?:^|[^\{])\{%\s*(?:el)?if\s*.*" + v_regex + r"\s*(?:[^%]*?)?%\}"
)
# TODO: this `for` regex won't catch some common cases like lists of vars, multiline
# jinja2 blocks, if filters on the for loop, etc.
for_regex = r"(?:^|[^\{])\{%\s*for\s*.*\s*in\s*" + v_regex + r"(?:[^%]*?)?%\}"
beckermr marked this conversation as resolved.
Show resolved Hide resolved
set_regex = r"(?:^|[^\{])\{%\s*set\s*.*\s*=\s*.*" + v_regex + r"(?:[^%]*?)?%\}"
Copy link
Contributor

Choose a reason for hiding this comment

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

This regex is not very good.
The following yaml matches glib and glibc both.

{% set a = glibc %}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

any suggestions to fix?

Copy link
Contributor

Choose a reason for hiding this comment

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

set_regex = r"(?:^|[^\{])\{%\s*set\s*.*\s*=\s*.*" + v_regex + r"(?![a-zA-Z_0-9])(?:[^%]*?)?%\}"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See #5514

# plain req name, no version spec. Look for end of line after name, or comment or selector
requirement_regex = rf"^\s+\-\s+{v_req_regex}\s*(?:\s[\[#]|$)"
if selectors_only:
all_res.insert(0, selector_regex)
else:
all_res.extend([variant_regex, requirement_regex, conditional_regex])
all_res.extend(
[
variant_regex,
requirement_regex,
conditional_regex,
for_regex,
set_regex,
]
)
# consolidate all re's into one big one for speedup
all_res = r"|".join(all_res)
if any(re.search(all_res, line) for line in variant_lines):
Expand Down
20 changes: 20 additions & 0 deletions news/5447-jinja2-for-set-vars
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
### Enhancements

* <news item>

### Bug fixes

* Variables used in single-line jinja2 ``for`` and ``set`` statements are now properly included in the variant
jaimergp marked this conversation as resolved.
Show resolved Hide resolved
matrix for some edge cases. (#5447)

### Deprecations

* <news item>

### Docs

* <news item>

### Other

* <news item>
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
CLANG_VERSION:
- 16.0.6
- 17.0.6
- 18.1.8
- 19.1.0.rc1

VCVER:
- 14.3
- 14.2
CL_VERSION:
- 19.40.33808
- 19.29.30139

BLAH:
- a
- b

FOO:
- cdf

FOOBAR:
- hgf

zip_keys:
-
- VCVER
- CL_VERSION
42 changes: 42 additions & 0 deletions tests/test-recipes/variants/jinja2_used_variables/meta.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
{% if CLANG_VERSION is not defined %}
{% set CLANG_VERSION = "16.0.6" %}
{% set CL_VERSION = "19.29" %}
{% set VCVER = "" %}
{% set FOO = "" %}
{% set FOOBAR = "" %}
{% endif %}
{% set clang_major = CLANG_VERSION.split(".")[0] %}
{% set cl_minor = CL_VERSION.split(".")[1] %}
{% set vc_major = VCVER.split(".")[0] %}

package:
name: clang-win-activation
version: {{ CLANG_VERSION }}

build:
number: 0
{% if clang_major|int == 16 and cl_minor|int >= 40 %}
skip: true
{% endif %}

outputs:
- name: clang_win-64
build:
run_exports:
strong:
- vc >={{ VCVER }}
requirements:
run:
- clang {{ CLANG_VERSION }}.*

test:
commands:
{% for var in FOO.split() %}
- echo {{ var }}
{% endfor %}

test:
commands:
{% for var in FOOBAR.split() %}
- echo {{ var }}
{% endfor %}
31 changes: 31 additions & 0 deletions tests/test_variants.py
Original file line number Diff line number Diff line change
Expand Up @@ -426,6 +426,37 @@ def test_get_used_loop_vars():
}


def test_get_used_loop_vars_jinja2():
metadata = api.render(
os.path.join(variants_dir, "jinja2_used_variables"),
finalize=False,
bypass_env_check=True,
)
# 4 CLANG_VERSION values x 2 VCVER values - one skipped because of jinja2 conditionals
assert len(metadata) == 7
for m, _, _ in metadata:
assert m.get_used_loop_vars(force_top_level=False) == {"CLANG_VERSION", "VCVER"}
assert m.get_used_loop_vars(force_top_level=True) == {
"CL_VERSION",
"CLANG_VERSION",
"VCVER",
}
assert m.get_used_vars(force_top_level=False) == {
"CLANG_VERSION",
"VCVER",
"FOO",
"target_platform",
}
assert m.get_used_vars(force_top_level=True) == {
"CLANG_VERSION",
"CL_VERSION",
"VCVER",
"FOO",
"FOOBAR",
"target_platform",
}


def test_reprovisioning_source():
api.render(os.path.join(variants_dir, "20_reprovision_source"))

Expand Down
Loading