From a41c9a875ed0f7a4d3f23bb61f7cacf93eabf254 Mon Sep 17 00:00:00 2001 From: Julien Jerphanion Date: Fri, 6 Dec 2024 11:50:15 +0100 Subject: [PATCH 1/5] fix: Skip empty lines in environment spec files Signed-off-by: Julien Jerphanion --- dev/environment-dev.yml | 1 + libmamba/src/api/install.cpp | 10 +++++- micromamba/tests/test_create.py | 54 +++++++++++++++++++++++++++++++++ 3 files changed, 64 insertions(+), 1 deletion(-) diff --git a/dev/environment-dev.yml b/dev/environment-dev.yml index 7a6eaf17a2..a579eb7991 100644 --- a/dev/environment-dev.yml +++ b/dev/environment-dev.yml @@ -31,6 +31,7 @@ dependencies: - pytest-asyncio - pytest-timeout - pytest-xprocess + - memory_profiler - requests - sel(win): pywin32 - sel(win): menuinst diff --git a/libmamba/src/api/install.cpp b/libmamba/src/api/install.cpp index d6ebca5033..f83d057398 100644 --- a/libmamba/src/api/install.cpp +++ b/libmamba/src/api/install.cpp @@ -933,7 +933,15 @@ namespace mamba std::vector f_specs; for (auto& line : file_contents) { - if (line[0] != '#' && line[0] != '@') + auto lstrip_line = util::lstrip(line); + + // Skip comment lines and empty lines + // Comment lines start with '#' or '@' preceded by whitespaces or tabs + auto is_comment = util::starts_with(lstrip_line, "#") + || util::starts_with(lstrip_line, "@"); + auto is_empty = lstrip_line.empty(); + + if (!is_comment && !is_empty) { f_specs.push_back(line); } diff --git a/micromamba/tests/test_create.py b/micromamba/tests/test_create.py index 76d698ce82..18b92c3b1b 100644 --- a/micromamba/tests/test_create.py +++ b/micromamba/tests/test_create.py @@ -9,6 +9,8 @@ from . import helpers +from memory_profiler import memory_usage + __this_dir__ = Path(__file__).parent.resolve() env_file_requires_pip_install_path = __this_dir__ / "env-requires-pip-install.yaml" @@ -1441,3 +1443,55 @@ def test_create_empty_or_absent_dependencies(tmp_path): "-p", env_prefix, "-f", tmp_path / "env_spec_absent_dependencies.yaml", "--json" ) assert res["success"] + + +env_spec_empty_lines_and_comments = """ +# The line below are empty (various number of spaces) +""" + +env_spec_empty_lines_and_comments += "\n" +env_spec_empty_lines_and_comments += " \n" +env_spec_empty_lines_and_comments += " \n" +env_spec_empty_lines_and_comments += " \n" +env_spec_empty_lines_and_comments += "# One comment \n" +env_spec_empty_lines_and_comments += " @ yet another one with a prefixed by a tab\n" +env_spec_empty_lines_and_comments += "wheel\n" + +env_repro_1 = """ +wheel + +setuptools +""" + +env_repro_2 = """ +wheel +setuptools + +# comment +""" + + +@pytest.mark.parametrize("env_spec", [env_spec_empty_lines_and_comments, env_repro_1, env_repro_2]) +def test_create_with_empty_lines_and_comments(tmp_path, env_spec): + # Non-regression test for: + # - https://github.com/mamba-org/mamba/issues/3289 + # - https://github.com/mamba-org/mamba/issues/3659 + memory_limit = 100 # in MB + + def memory_intensive_operation(): + env_prefix = tmp_path / "env-one_empty_line" + + env_spec_file = tmp_path / "env_spec.txt" + + with open(env_spec_file, "w") as f: + f.write(env_spec) + + res = helpers.create("-p", env_prefix, "-f", env_spec_file, "--json") + assert res["success"] + + max_memory = max(memory_usage(proc=memory_intensive_operation)) + + if max_memory > memory_limit: + pytest.fail( + f"test_create_with_empty_lines_and_comments exceeded memory limit of {memory_limit} MB (used {max_memory:.2f} MB)" + ) From c377e1f3cbb0b868fef655c6377d56d3677bdb4c Mon Sep 17 00:00:00 2001 From: Julien Jerphanion Date: Fri, 6 Dec 2024 16:05:27 +0100 Subject: [PATCH 2/5] Filter at readtime Signed-off-by: Julien Jerphanion Co-authored-by: Klaim --- libmamba/src/api/install.cpp | 31 +++++++++++-------------------- libmamba/src/core/util.cpp | 27 +++++++++++++++++++++++++++ 2 files changed, 38 insertions(+), 20 deletions(-) diff --git a/libmamba/src/api/install.cpp b/libmamba/src/api/install.cpp index f83d057398..8844c14bb3 100644 --- a/libmamba/src/api/install.cpp +++ b/libmamba/src/api/install.cpp @@ -888,6 +888,8 @@ namespace mamba { throw std::runtime_error(util::concat("Got an empty file: ", file)); } + + // Inferring potential explicit environment specification for (std::size_t i = 0; i < file_contents.size(); ++i) { auto& line = file_contents[i]; @@ -930,34 +932,23 @@ namespace mamba } } - std::vector f_specs; - for (auto& line : file_contents) - { - auto lstrip_line = util::lstrip(line); - - // Skip comment lines and empty lines - // Comment lines start with '#' or '@' preceded by whitespaces or tabs - auto is_comment = util::starts_with(lstrip_line, "#") - || util::starts_with(lstrip_line, "@"); - auto is_empty = lstrip_line.empty(); - - if (!is_comment && !is_empty) - { - f_specs.push_back(line); - } - } - + // If we reach here, we have a file with no explicit env, and the content of the + // file just lists MatchSpecs. if (specs.cli_configured()) { auto current_specs = specs.cli_value>(); - current_specs.insert(current_specs.end(), f_specs.cbegin(), f_specs.cend()); + current_specs.insert( + current_specs.end(), + file_contents.cbegin(), + file_contents.cend() + ); specs.set_cli_value(current_specs); } else { - if (!f_specs.empty()) + if (!file_contents.empty()) { - specs.set_cli_value(f_specs); + specs.set_cli_value(file_contents); } } } diff --git a/libmamba/src/core/util.cpp b/libmamba/src/core/util.cpp index 2f8c7c5197..a66c497382 100644 --- a/libmamba/src/core/util.cpp +++ b/libmamba/src/core/util.cpp @@ -301,6 +301,33 @@ namespace mamba line.pop_back(); } + const auto lstrip_line = util::lstrip(line); + + // Skipping empty lines + if (lstrip_line.empty()) + { + continue; + } + + // Skipping comment lines starting with # + if (util::starts_with(lstrip_line, "#")) + { + continue; + } + + // Skipping comment lines starting with @ BUT headers of explicit environment specs + if (util::starts_with(lstrip_line, "@")) + { + auto is_explicit_header = util::starts_with(lstrip_line, "@EXPLICIT"); + + if (is_explicit_header) + { + output.push_back(line); + } + continue; + } + + // By default, add the line to the output (MatchSpecs, etc.) output.push_back(line); } file_stream.close(); From 63348f914b2bacfc90990bfcc18884dc3c60fc15 Mon Sep 17 00:00:00 2001 From: Julien Jerphanion Date: Fri, 6 Dec 2024 16:57:51 +0100 Subject: [PATCH 3/5] Strip input strings Signed-off-by: Julien Jerphanion Co-authored-by: Klaim --- libmamba/src/core/util.cpp | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/libmamba/src/core/util.cpp b/libmamba/src/core/util.cpp index a66c497382..f82d46f66f 100644 --- a/libmamba/src/core/util.cpp +++ b/libmamba/src/core/util.cpp @@ -301,24 +301,24 @@ namespace mamba line.pop_back(); } - const auto lstrip_line = util::lstrip(line); + line = util::strip(line); // Skipping empty lines - if (lstrip_line.empty()) + if (line.empty()) { continue; } // Skipping comment lines starting with # - if (util::starts_with(lstrip_line, "#")) + if (util::starts_with(line, "#")) { continue; } // Skipping comment lines starting with @ BUT headers of explicit environment specs - if (util::starts_with(lstrip_line, "@")) + if (util::starts_with(line, "@")) { - auto is_explicit_header = util::starts_with(lstrip_line, "@EXPLICIT"); + auto is_explicit_header = util::starts_with(line, "@EXPLICIT"); if (is_explicit_header) { From 02ffa79dfee247cc35432d9898b419e2f7990b9b Mon Sep 17 00:00:00 2001 From: Julien Jerphanion Date: Fri, 6 Dec 2024 17:06:46 +0100 Subject: [PATCH 4/5] Inplace strip input strings Signed-off-by: Julien Jerphanion Co-authored-by: Klaim --- libmamba/include/mamba/util/string.hpp | 1 + libmamba/src/core/util.cpp | 3 ++- libmamba/src/util/string.cpp | 21 +++++++++++++++++++++ 3 files changed, 24 insertions(+), 1 deletion(-) diff --git a/libmamba/include/mamba/util/string.hpp b/libmamba/include/mamba/util/string.hpp index 17e94cc992..d76e34bb02 100644 --- a/libmamba/include/mamba/util/string.hpp +++ b/libmamba/include/mamba/util/string.hpp @@ -196,6 +196,7 @@ namespace mamba::util [[nodiscard]] auto strip(std::wstring_view input, std::wstring_view chars) -> std::wstring_view; [[nodiscard]] auto strip(std::string_view input) -> std::string_view; [[nodiscard]] auto strip(std::wstring_view input) -> std::wstring_view; + void inplace_strip(std::string& input); [[nodiscard]] auto strip_parts(std::string_view input, char c) -> std::array; [[nodiscard]] auto strip_parts(std::wstring_view input, wchar_t c) diff --git a/libmamba/src/core/util.cpp b/libmamba/src/core/util.cpp index f82d46f66f..23f6af6906 100644 --- a/libmamba/src/core/util.cpp +++ b/libmamba/src/core/util.cpp @@ -301,7 +301,8 @@ namespace mamba line.pop_back(); } - line = util::strip(line); + // Remove leading and trailing whitespace in place not to create a new string. + util::inplace_strip(line); // Skipping empty lines if (line.empty()) diff --git a/libmamba/src/util/string.cpp b/libmamba/src/util/string.cpp index d6d78ab920..70967c540b 100644 --- a/libmamba/src/util/string.cpp +++ b/libmamba/src/util/string.cpp @@ -447,6 +447,27 @@ namespace mamba::util return strip_if(input, [](Char c) { return !is_graphic(c); }); } + void inplace_strip(std::string& input) + { + // Dedicated implementation for std::string to avoid copies + if (input.empty()) + { + return; + } + + const auto start = input.find_first_not_of(" \t\n\v\f\r"); + + if (start == std::string::npos) + { + input.clear(); + return; + } + + const auto end = input.find_last_not_of(" \t\n\v\f\r"); + + input = input.substr(start, end - start + 1); + } + /********************************************* * Implementation of strip_parts functions * *********************************************/ From 4626ba267383a9cc0993b50aafd4389cb2efac1b Mon Sep 17 00:00:00 2001 From: Julien Jerphanion Date: Fri, 6 Dec 2024 17:19:48 +0100 Subject: [PATCH 5/5] docs: Move comment in header Signed-off-by: Julien Jerphanion Co-authored-by: Klaim --- libmamba/include/mamba/util/string.hpp | 4 ++++ libmamba/src/util/string.cpp | 1 - 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/libmamba/include/mamba/util/string.hpp b/libmamba/include/mamba/util/string.hpp index d76e34bb02..66ead50dd9 100644 --- a/libmamba/include/mamba/util/string.hpp +++ b/libmamba/include/mamba/util/string.hpp @@ -196,6 +196,10 @@ namespace mamba::util [[nodiscard]] auto strip(std::wstring_view input, std::wstring_view chars) -> std::wstring_view; [[nodiscard]] auto strip(std::string_view input) -> std::string_view; [[nodiscard]] auto strip(std::wstring_view input) -> std::wstring_view; + + /** + * Dedicated implementation for inplace stripping of `std::string` to avoid copies + */ void inplace_strip(std::string& input); [[nodiscard]] auto strip_parts(std::string_view input, char c) -> std::array; diff --git a/libmamba/src/util/string.cpp b/libmamba/src/util/string.cpp index 70967c540b..28c9cf14c2 100644 --- a/libmamba/src/util/string.cpp +++ b/libmamba/src/util/string.cpp @@ -449,7 +449,6 @@ namespace mamba::util void inplace_strip(std::string& input) { - // Dedicated implementation for std::string to avoid copies if (input.empty()) { return;