fix: ensure variable finding works properly and add unit tests for variable finding

[beckermr]

Description

Currently, conda-build will say the variant variable python is used for a statement like

requirements:
  host:
    - python {{ python_min }}

This is incorrect and only python_min is used in this case AFAIK.

It also says this statement does NOT use python when it does

requirements:
  run:
    - {{ pin_compatible('python') }}

This bug was introduced in #2838.

The issue appears to be a buggy regex for searching for variants.

This PR proposes a new regex and adds unit tests that assert the correct behavior.

Checklist - did you ...

• Add a file to the news directory (using the template) for the next release's release notes?
• Add / update necessary tests?
• Add / update outdated documentation?

[codspeed-hq]

CodSpeed Performance Report

Merging #5535 will not alter performance

_{Comparing beckermr:find-vars-unit-tests (6e833ea) with main (98ece01)}

Summary

✅ 5 untouched benchmarks

[beckermr]

@conda/builds-tools @jezdez @kenodegard @beeankha @jaimergp This request is unusual, but can we consider merging this PR before the next conda-build release?

This PR fixes a very non-trivial bug that has been present in the code base for at least five years. conda-build is misdetecting which variants are being used, which is not great. :/

[jaimergp]

Hm, I wonder if this is behind conda-forge/conda-smithy#2100 and conda-forge/conda-smithy#2071

[beckermr]

Looks like it very likely could be. Currently conda-build will confuse the pin_compatible statement for napari-base with using napari.

[jaimergp]

And side note, I think these regexes (which have received several contributions lately) would benefit from re.VERBOSE. Maybe that helps with the review too.

[beckermr]

That is a good idea, but I'd like to defer to another PR. Also, I am not sure I could actually describe what it is doing. I use regex101.com to build and test regex strings because they are so confusing. :/

[jaimergp]

I use regex101.com to build and test regex strings

Heh, same here but with regexr.com 😂

That is a good idea, but I'd like to defer to another PR.

Let me give it a try, but yes, non-blocking.

[jaimergp]

Hm, let's see:

variant_regex = re.compile(
    # This regex matches stuff in Jinja expressions and functions; e.g. {{ python }} and {{ pin_compatible('python') }}
    rf"""
    \{{\{{              # Double curly brace opens Ninja expression
        \s*             # Zero or more spaces
        (?:             # Opens non-capturing group
            pin_[a-z]+  # Matches any lowercase pin_* function name (e.g. pin_compatible)
            \(          # Matches the opening parenthesis of the pin_* function
                \s*?    # Non-greedy glob for any number of spaces (JRG: not sure why we need the non greedy version)
                ['"]    # Single or double quote, required once
        )?              # Closes non-capturing group and makes it optional; i.e. the whole pin_compatible(' part can be ommitted 
        {v_regex}       # The regex from above, should match a variant name
        [^_0-9a-zA-Z]   # One instance of any character that is NOT a number, a letter or an underscore; i.e. not part of the variant name
        .*?             # Any number of any character, in a non-greedy way; aka "whatever comes before the closing curly braces"
    \}}\}}              # Double curly brace closes Ninja expression
    """,
    re.VERBOSE
)

selector_regex = re.compile(
    # This regex matches selectors; e.g. # [py>38]
    rf"""
    ^                   # Starts with... (one thing, I don't know why 🎵)
    [^#\[]*?            # Any character that is not a # or [, any number of times, non-greedy
    \#?                 # A hashtag, optionally
    \s                  # A space
    \[                  # An opening square bracket
        [^\]]*?         # Any character that is not a ], any number of times, non-greedy
        (?<!            # Negative lookbehind assertion ✨ aka "NOT preceded by..."
            [_\w\d]     # ... underscores, letters or digits
        )               # Closes the negative lookbehind assertion
        {v_regex}       # The variant name regex
    [                   # One of
        =\s<>!\]        # equals, spaces, angle brackets, exclamation mark or closing square bracket
                        # ... These are all characters that "terminate" a variant name, like operators 
                        # ... or the end of the selector
    ] 
    """,
    re.VERBOSE
)

[beckermr]

@jaimergp my concern is that the conda-build test suite is not robust enough to catch bugs that might be introduced when we change the regex strings to the more verbose style. It'd be better to defer to future PRs that change the strings and introduce more robust tests to ensure nothing breaks.

[jaimergp]

It'd be better to defer to future PRs that change the strings and introduce more robust tests to ensure nothing breaks.

That's ok with me. Just updated my comment because I was already "parsing" the regexes for the review, so might as well write it down.

[jaimergp]

My understanding of the change:

• The previous regex was non-greedy-matching any characters after the variant name that were NOT a single or double quote. That meant that partial matches were possible (e.g. it found pari in pin_compatible('napari').
• The new regex tries to non-greedy-match any character after the variant name that is NOT a valid variant name character (underscores, numbers and letters).

[beckermr]

That's correct. We assert the variable name is terminated by a non-allowed character for variables. Then we get the rest of the characters up until the jinja2 statement is closed, asserting it is closed.

[beckermr]

shall we merge @jaimergp? no pressure, but I am contemplating adding a patch to the conda-build feedstock if this PR is going to sit for a while.

[jaimergp]

Let's give @beeankha and @ForgottenProgramme a few hours to take a look. The release branch has not been created yet so there's no rush to merge just yet. It does LGTM though.