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: Support globs in MatchSpec build strings #3735

Merged
merged 7 commits into from
Jan 27, 2025
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion libmamba/include/mamba/specs/regex_spec.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ namespace mamba::specs
[[nodiscard]] static auto parse(std::string pattern) -> expected_parse_t<RegexSpec>;

RegexSpec();
RegexSpec(std::regex pattern, std::string raw_pattern);
explicit RegexSpec(std::string raw_pattern);

[[nodiscard]] auto contains(std::string_view str) const -> bool;

Expand Down
62 changes: 48 additions & 14 deletions libmamba/src/specs/regex_spec.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

#include <algorithm>
#include <cassert>
#include <sstream>

#include <fmt/format.h>

Expand All @@ -25,12 +26,10 @@ namespace mamba::specs

auto RegexSpec::parse(std::string pattern) -> expected_parse_t<RegexSpec>
{
// No other mean of getting parse result with ``std::regex``, but parse error need
// to be handled by ``tl::expected`` to be managed down the road.
// Parse error need to be handled by ``tl::expected`` to be managed down the road.
try
{
auto regex = std::regex(pattern);
return { { std::move(regex), std::move(pattern) } };
return RegexSpec{ std::move(pattern) };
}
catch (const std::regex_error& e)
{
Expand All @@ -39,25 +38,60 @@ namespace mamba::specs
}

RegexSpec::RegexSpec()
: RegexSpec(std::regex(free_pattern.data(), free_pattern.size()), std::string(free_pattern))
: RegexSpec(std::string(free_pattern))
{
}

RegexSpec::RegexSpec(std::regex pattern, std::string raw_pattern)
: m_pattern(std::move(pattern))
, m_raw_pattern(std::move(raw_pattern))
RegexSpec::RegexSpec(std::string raw_pattern)
{
// If the string is wrapped in `^` and `$`, `conda.model.MatchSpec` considers it a regex.
// See:
// https://github.com/conda/conda/blob/52b6393d6331e8aa36b2e23ab65766a980f381d2/conda/models/match_spec.py#L134-L139.
// See:
// https://github.com/conda/conda/blob/52b6393d6331e8aa36b2e23ab65766a980f381d2/conda/models/match_spec.py#L889-L894
if (util::starts_with(raw_pattern, pattern_start) && util::ends_with(raw_pattern, pattern_end))
{
m_raw_pattern = raw_pattern;
m_pattern = std::regex(m_raw_pattern);
return;
}
jjerphan marked this conversation as resolved.
Show resolved Hide resolved

// Construct ss from raw_pattern, in particular make sure to replace all `*` by `.*`
// in the pattern if they are not preceded by a `.`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't this mean that a glob string py3.* would match py310 when it shouldn't have? i.e. we should first escape the existing . to \., and then replace * by .*?

Copy link
Member Author

@jjerphan jjerphan Jan 9, 2025

Choose a reason for hiding this comment

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

Yes, I am trying to think of other cases convertions to regular expression might have changed (e.g. pattern with ?).

I will add more test cases.

Copy link
Member Author

Choose a reason for hiding this comment

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

Arf, I support your remark, but this clashes with behavior:

TEST_CASE("ChimeraStringSpec py.*")
{
auto spec = ChimeraStringSpec::parse("py.*").value();
REQUIRE(spec.contains("python"));
REQUIRE(spec.contains("py"));
REQUIRE(spec.contains("pypy"));
REQUIRE_FALSE(spec.contains(""));
REQUIRE_FALSE(spec.contains("cpython"));
REQUIRE(spec.str() == "^py.*$");
REQUIRE_FALSE(spec.is_explicitly_free());
REQUIRE_FALSE(spec.is_exact());
REQUIRE_FALSE(spec.is_glob());
}

Copy link
Contributor

@jaimergp jaimergp Jan 9, 2025

Choose a reason for hiding this comment

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

In conda, the "full regex mode" only kicks in if the build string is wrapped in ^...$. Otherwise, it's understood as a normal glob string. See docstring and implementation.

// We force regex to start with `^` and end with `$` to simplify the multiple
// possible representations, and because this is the safest way we can make sure it is
// not a glob when serializing it.
if (!util::starts_with(m_raw_pattern, pattern_start))
{
m_raw_pattern.insert(m_raw_pattern.begin(), pattern_start);
}
if (!util::ends_with(m_raw_pattern, pattern_end))
std::ostringstream ss;
ss << pattern_start;

auto first_character_it = raw_pattern.cbegin();
auto last_character_it = raw_pattern.cend() - 1;

for (auto it = first_character_it; it != raw_pattern.cend(); ++it)
{
m_raw_pattern.push_back(pattern_end);
if (it == first_character_it && *it == pattern_start)
Klaim marked this conversation as resolved.
Show resolved Hide resolved
{
continue;
}
if (it == last_character_it && *it == pattern_end)
{
continue;
}
if (*it == '*' && (it == first_character_it || *(it - 1) != '.'))
{
ss << ".*";
}
else
{
ss << *it;
}
}

ss << pattern_end;

m_raw_pattern = ss.str();

m_pattern = std::regex(m_raw_pattern);
}

auto RegexSpec::contains(std::string_view str) const -> bool
Expand Down
19 changes: 19 additions & 0 deletions libmamba/tests/src/specs/test_match_spec.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -981,6 +981,25 @@ namespace
/* .track_features =*/{ "openssl", "mkl" },
}));
}

SECTION("pytorch=2.3.1=py3.10_cuda11.8*")
{
// Check that it contains `pytorch=2.3.1=py3.10_cuda11.8_cudnn8.7.0_0`

const auto ms = "pytorch=2.3.1=py3.10_cuda11.8*"_ms;

REQUIRE(ms.contains_except_channel(Pkg{
/* .name= */ "pytorch",
/* .version= */ "2.3.1"_v,
/* .build_string= */ "py3.10_cuda11.8_cudnn8.7.0_0",
/* .build_number= */ 0,
/* .md5= */ "lemd5",
/* .sha256= */ "somesha256",
/* .license= */ "GPL",
/* .platform= */ "linux-64",
/* .track_features =*/{},
}));
}
}

TEST_CASE("MatchSpec comparability and hashability")
Expand Down
16 changes: 16 additions & 0 deletions libmamba/tests/src/specs/test_regex_spec.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -80,4 +80,20 @@ namespace
REQUIRE(hash_fn(spec1) == hash_fn(spec2));
REQUIRE(hash_fn(spec1) != hash_fn(spec3));
}

TEST_CASE("RegexSpec py3.10_cuda11.8*")
{
auto spec = RegexSpec::parse("py3.10_cuda11.8*").value();
REQUIRE(spec.contains("py3.10_cuda11.8_cudnn8.7.0_0"));
}

TEST_CASE("RegexSpec * semantic")
{
auto spec = RegexSpec::parse("py3.*").value();

REQUIRE(spec.contains("py3."));
REQUIRE(spec.contains("py3.10"));
REQUIRE(spec.contains("py3.10_cuda11.8_cudnn8.7.0_0"));
}

}
37 changes: 37 additions & 0 deletions micromamba/tests/test_create.py
Original file line number Diff line number Diff line change
Expand Up @@ -1638,3 +1638,40 @@ def test_ca_certificates(tmp_path):
)

assert root_prefix_ca_certificates_used or fall_back_certificates_used


def test_glob_in_build_string(monkeypatch, tmp_path):
# Non-regression test for https://github.com/mamba-org/mamba/issues/3699
env_prefix = tmp_path / "test_glob_in_build_string"

pytorch_match_spec = "pytorch=2.3.1=py3.10_cuda11.8*"

# Export CONDA_OVERRIDE_GLIBC=2.17 to force the solver to use the glibc 2.17 package
monkeypatch.setenv("CONDA_OVERRIDE_GLIBC", "2.17")

# Should run without error
out = helpers.create(
"-p",
env_prefix,
pytorch_match_spec,
"-c",
"pytorch",
"-c",
"nvidia/label/cuda-11.8.0",
"-c",
"nvidia",
"-c",
"conda-forge",
"--platform",
"linux-64",
"--dry-run",
"--json",
)

# Check that a build of pytorch 2.3.1 with `py3.10_cuda11.8_cudnn8.7.0_0` as a build string is found
assert any(
package["name"] == "pytorch"
and package["version"] == "2.3.1"
and package["build_string"] == "py3.10_cuda11.8_cudnn8.7.0_0"
for package in out["actions"]["FETCH"]
)
Loading