-
Notifications
You must be signed in to change notification settings - Fork 372
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
base: main
Are you sure you want to change the base?
Conversation
3ef58f7
to
c7db207
Compare
6f965bc
to
844e4bc
Compare
// Construct ss from raw_pattern, in particular make sure to replace all `*` by `.*` | ||
// in the pattern if they are not preceded by a `.`. |
There was a problem hiding this comment.
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 .*
?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
mamba/libmamba/tests/src/specs/test_chimera_string_spec.cpp
Lines 43 to 57 in 374228f
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()); | |
} |
There was a problem hiding this comment.
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.
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")); | ||
|
||
REQUIRE_FALSE(spec.contains("py3")); | ||
REQUIRE_FALSE(spec.contains("py310")); | ||
} | ||
|
||
TEST_CASE("RegexSpec ? semantic") | ||
{ | ||
auto spec = RegexSpec::parse("py3.?").value(); | ||
|
||
REQUIRE(spec.contains("py3.")); | ||
REQUIRE(spec.contains("py3.1")); | ||
REQUIRE(spec.contains("py3.0")); | ||
REQUIRE(spec.contains("py3.?")); | ||
REQUIRE(spec.contains("py3..")); | ||
|
||
REQUIRE_FALSE(spec.contains("py3.10")); | ||
REQUIRE_FALSE(spec.contains("py3")); | ||
REQUIRE_FALSE(spec.contains("py310")); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this is the semantics we want to use, yet it clashes with ChimeraString
's (see https://github.com/mamba-org/mamba/pull/3735/files#r1908971307).
b8104d2
to
2d1ce12
Compare
2d1ce12
to
1dacead
Compare
Signed-off-by: Julien Jerphanion <[email protected]>
Signed-off-by: Julien Jerphanion <[email protected]> Co-authored-by: Johan Mabille <[email protected]>
Signed-off-by: Julien Jerphanion <[email protected]> Co-authored-by: jaimergp <[email protected]>
Signed-off-by: Julien Jerphanion <[email protected]>
Signed-off-by: Julien Jerphanion <[email protected]> Co-authored-by: jaimergp <[email protected]>
1dacead
to
5a33edd
Compare
m_raw_pattern = raw_pattern; | ||
m_pattern = std::regex(m_raw_pattern); | ||
return; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would find it clearer to have something like:
RegexSpec::RegexSpec(std::string raw_pattern)
: m_raw_pattern(format_raw_pattern(std::move(raw_pattern))
, m_pattern(std::regex(m_raw_pattern))
{
}
And have all the logic in format_raw_pattern
(or any better name you can come with) rather than in the constructor, with a return statement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought of this, but does this guarantee that m_raw_pattern
is strictly initialized before m_pattern
?
I am not sure it is safe as m_pattern
is declared before m_raw_pattern
:
mamba/libmamba/include/mamba/specs/regex_spec.hpp
Lines 64 to 65 in ad9b2d6
std::regex m_pattern; | |
std::string m_raw_pattern; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah good catch, you have to invert them in the declaration. The intialization order is guanranteed to be the same as the declaration order.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this change the ABI?
Fix #3699.