fix: Handle extra white-space in MatchSpec
#3456
Conversation
Signed-off-by: Julien Jerphanion <git@jjerphan.xyz>
Signed-off-by: Julien Jerphanion <git@jjerphan.xyz>
Signed-off-by: Julien Jerphanion <git@jjerphan.xyz>
jjerphan
force-pushed
the
fix/handle-extra-white-space
branch
from
caf12de
to
60658f7
Compare
Signed-off-by: Julien Jerphanion <git@jjerphan.xyz>
Signed-off-by: Julien Jerphanion <git@jjerphan.xyz>
Signed-off-by: Julien Jerphanion <git@jjerphan.xyz>
Signed-off-by: Julien Jerphanion <git@jjerphan.xyz>
|
|
||
|
|
||
| @pytest.mark.parametrize("shared_pkgs_dirs", [True], indirect=True) | ||
| def test_env_create_whitespace(tmp_home, tmp_root_prefix, tmp_path): |
There was a problem hiding this comment.
I don't think we need this test, tests in test_match_spec are enough to make sure the parsing is done correctly.
Adding this one is redundant and is not relevant in my opinion.
There was a problem hiding this comment.
I agree with this, I just wanted to have a non-regression test for the reported issue.
| std::string raw_match_spec_str = std::string(str); | ||
| raw_match_spec_str = util::strip(raw_match_spec_str); | ||
|
|
||
| // Remove any with space after binary operators, such as: | ||
| // - `openmpi-4.1.4-ha1ae619_102`'s improperly encoded `constrains`: "cudatoolkit >= 10.2" | ||
| // - `pytorch-1.13.0-cpu_py310h02c325b_0.conda`'s improperly encoded | ||
| // `constrains`: "pytorch-cpu = 1.13.0", "pytorch-gpu = 99999999" | ||
| // - `fipy-3.4.2.1-py310hff52083_3.tar.bz2`'s improperly encoded `constrains` or | ||
| // `dep`: ">=4.5.2" | ||
| // - `infokonoha-4.6.3-pyhd8ed1ab_0.tar.bz2`'s `kytea >=0.1.4, 0.2.0` -> `kytea | ||
| // >=0.1.4,0.2.0` | ||
| // TODO: this solution reallocates memory several times potentially, but the | ||
| // number of operators is small and the strings are short, so it must be fine. | ||
| // If needed it can be optimized so that the string is only copied once. | ||
| for (const std::string& op : { ">=", "<=", "==", ">", "<", "!=", "=", "==", "," }) | ||
| { | ||
| const std::string& bad_op = op + " "; | ||
| while (raw_match_spec_str.find(bad_op) != std::string::npos) | ||
| { | ||
| raw_match_spec_str = raw_match_spec_str.substr(0, raw_match_spec_str.find(bad_op)) + op | ||
| + raw_match_spec_str.substr( | ||
| raw_match_spec_str.find(bad_op) + bad_op.size() | ||
| ); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Maybe we won't have a choice and we will need to fix this this way, but I really think that we should do this properly and stop postponing everything to later, because it will just increase the complexity (making it harder to change things afterwards), especially regarding the MatchSpec...
IIRC this should be rather handled here, so we need to adapt the logic accordingly (and try to keep the string_view).
There was a problem hiding this comment.
I agree with your proposal although there is some complexity with the low level parser: if we want to handle all the current cases while excluding the PEP 508 environment markers, I am afraid that the amount of complexity to manage will be far more complex that this horrible yet working and short solution.
In my opinion, the long-term robust solution (as discussed in the past) is to define a grammar for MatchSpec and use lexers to generate parsers in applications. But this goes far beyond the scope of this PR or the time we have at hand.
Signed-off-by: Julien Jerphanion <git@jjerphan.xyz>
Signed-off-by: Julien Jerphanion <git@jjerphan.xyz> Co-authored-by: Hind Montassif <hind.montassif@gmail.com>
Signed-off-by: Julien Jerphanion <git@jjerphan.xyz> Co-authored-by: Hind Montassif <hind.montassif@gmail.com>
Co-authored-by: Hind-M <70631848+Hind-M@users.noreply.github.com>
Co-authored-by: Hind-M <70631848+Hind-M@users.noreply.github.com>
Signed-off-by: Julien Jerphanion <git@jjerphan.xyz>
Signed-off-by: Julien Jerphanion <git@jjerphan.xyz>
Fix #3453.