fix: Skip empty lines in environment spec files (#3662)

Signed-off-by: Julien Jerphanion <git@jjerphan.xyz>
Co-authored-by: Klaim <Klaim@users.noreply.github.com>

dev/environment-dev.yml

@@ -31,6 +31,7 @@ dependencies:
   - pytest-asyncio
   - pytest-timeout
   - pytest-xprocess
+  - memory_profiler
   - requests
   - sel(win): pywin32
   - sel(win): menuinst

libmamba/include/mamba/util/string.hpp

@@ -197,6 +197,11 @@ namespace mamba::util
     [[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<std::string_view, 3>;
     [[nodiscard]] auto strip_parts(std::wstring_view input, wchar_t c)
         -> std::array<std::wstring_view, 3>;

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,26 +932,23 @@ namespace mamba
                         }
                     }
 
-                    std::vector<std::string> f_specs;
-                    for (auto& line : file_contents)
-                    {
-                        if (line[0] != '#' && line[0] != '@')
-                        {
-                            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<std::vector<std::string>>();
-                        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);
                         }
                     }
                 }

libmamba/src/core/util.cpp

@@ -301,6 +301,34 @@ namespace mamba
                 line.pop_back();
             }
 
+            // Remove leading and trailing whitespace in place not to create a new string.
+            util::inplace_strip(line);
+
+            // Skipping empty lines
+            if (line.empty())
+            {
+                continue;
+            }
+
+            // Skipping comment lines starting with #
+            if (util::starts_with(line, "#"))
+            {
+                continue;
+            }
+
+            // Skipping comment lines starting with @ BUT headers of explicit environment specs
+            if (util::starts_with(line, "@"))
+            {
+                auto is_explicit_header = util::starts_with(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();

libmamba/src/util/string.cpp

@@ -447,6 +447,26 @@ namespace mamba::util
         return strip_if(input, [](Char c) { return !is_graphic(c); });
     }
 
+    void inplace_strip(std::string& input)
+    {
+        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  *
      *********************************************/

micromamba/tests/test_create.py

@@ -9,6 +9,8 @@ import yaml
 
 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)"
+        )