Environment/depfile: fix bug with Git hash versions (attempt #2) (#37560)

Co-authored-by: Harmen Stoppels <me@harmenstoppels.nl>

lib/spack/spack/environment/depfile.py

@@ -8,6 +8,7 @@
 """
 
 import os
+import re
 from enum import Enum
 from typing import List, Optional
 
@@ -45,8 +46,8 @@ class DepfileNode:
     def __init__(
         self, target: spack.spec.Spec, prereqs: List[spack.spec.Spec], buildcache: UseBuildCache
     ):
-        self.target = target
+        self.target = MakefileSpec(target)
-        self.prereqs = prereqs
+        self.prereqs = list(MakefileSpec(x) for x in prereqs)
         if buildcache == UseBuildCache.ONLY:
             self.buildcache_flag = "--use-buildcache=only"
         elif buildcache == UseBuildCache.NEVER:
@@ -89,6 +90,32 @@ def accept(self, node):
         return True
 
 
+class MakefileSpec(object):
+    """Limited interface to spec to help generate targets etc. without
+    introducing unwanted special characters.
+    """
+
+    _pattern = None
+
+    def __init__(self, spec):
+        self.spec = spec
+
+    def safe_name(self):
+        return self.safe_format("{name}-{version}-{hash}")
+
+    def spec_hash(self):
+        return self.spec.dag_hash()
+
+    def safe_format(self, format_str):
+        unsafe_result = self.spec.format(format_str)
+        if not MakefileSpec._pattern:
+            MakefileSpec._pattern = re.compile(r"[^A-Za-z0-9_.-]")
+        return MakefileSpec._pattern.sub("_", unsafe_result)
+
+    def unsafe_format(self, format_str):
+        return self.spec.format(format_str)
+
+
 class MakefileModel:
     """This class produces all data to render a makefile for specs of an environment."""
 
@@ -114,7 +141,7 @@ def __init__(
         self.env_path = env.path
 
         # These specs are built in the default target.
-        self.roots = roots
+        self.roots = list(MakefileSpec(x) for x in roots)
 
         # The SPACK_PACKAGE_IDS variable is "exported", which can be used when including
         # generated makefiles to add post-install hooks, like pushing to a buildcache,
@@ -131,17 +158,19 @@ def __init__(
         # And here we collect a tuple of (target, prereqs, dag_hash, nice_name, buildcache_flag)
         self.make_adjacency_list = [
             (
-                self._safe_name(item.target),
+                item.target.safe_name(),
-                " ".join(self._install_target(self._safe_name(s)) for s in item.prereqs),
+                " ".join(self._install_target(s.safe_name()) for s in item.prereqs),
-                item.target.dag_hash(),
+                item.target.spec_hash(),
-                item.target.format("{name}{@version}{%compiler}{variants}{arch=architecture}"),
+                item.target.unsafe_format(
+                    "{name}{@version}{%compiler}{variants}{arch=architecture}"
+                ),
                 item.buildcache_flag,
             )
             for item in adjacency_list
         ]
 
         # Root specs without deps are the prereqs for the environment target
-        self.root_install_targets = [self._install_target(self._safe_name(s)) for s in roots]
+        self.root_install_targets = [self._install_target(s.safe_name()) for s in self.roots]
 
         self.jobserver_support = "+" if jobserver else ""
 
@@ -157,7 +186,7 @@ def __init__(
         self.phony_convenience_targets: List[str] = []
 
         for node in adjacency_list:
-            tgt = self._safe_name(node.target)
+            tgt = node.target.safe_name()
             self.all_pkg_identifiers.append(tgt)
             self.all_install_related_targets.append(self._install_target(tgt))
             self.all_install_related_targets.append(self._install_deps_target(tgt))
@@ -165,9 +194,6 @@ def __init__(
                 self.phony_convenience_targets.append(os.path.join("install", tgt))
                 self.phony_convenience_targets.append(os.path.join("install-deps", tgt))
 
-    def _safe_name(self, spec: spack.spec.Spec) -> str:
-        return spec.format("{name}-{version}-{hash}")
-
     def _target(self, name: str) -> str:
         # The `all` and `clean` targets are phony. It doesn't make sense to
         # have /abs/path/to/env/metadir/{all,clean} targets. But it *does* make

lib/spack/spack/test/cmd/env.py

@@ -19,10 +19,12 @@
 import spack.cmd.env
 import spack.config
 import spack.environment as ev
+import spack.environment.depfile as depfile
 import spack.environment.environment
 import spack.environment.shell
 import spack.error
 import spack.modules
+import spack.package_base
 import spack.paths
 import spack.repo
 import spack.util.spack_json as sjson
@@ -3136,6 +3138,57 @@ def test_environment_depfile_makefile(depfile_flags, expected_installs, tmpdir,
     assert len(specs_that_make_would_install) == len(set(specs_that_make_would_install))
 
 
+def test_depfile_safe_format():
+    """Test that depfile.MakefileSpec.safe_format() escapes target names."""
+
+    class SpecLike:
+        def format(self, _):
+            return "abc@def=ghi"
+
+    spec = depfile.MakefileSpec(SpecLike())
+    assert spec.safe_format("{name}") == "abc_def_ghi"
+    assert spec.unsafe_format("{name}") == "abc@def=ghi"
+
+
+def test_depfile_works_with_gitversions(tmpdir, mock_packages, monkeypatch):
+    """Git versions may contain = chars, which should be escaped in targets,
+    otherwise they're interpreted as makefile variable assignments."""
+    monkeypatch.setattr(spack.package_base.PackageBase, "git", "repo.git", raising=False)
+    env("create", "test")
+
+    make = Executable("make")
+    makefile = str(tmpdir.join("Makefile"))
+
+    # Create an environment with dttop and dtlink1 both at a git version,
+    # and generate a depfile
+    with ev.read("test"):
+        add(f"dttop@{'a' * 40}=1.0 ^dtlink1@{'b' * 40}=1.0")
+        concretize()
+        env("depfile", "-o", makefile, "--make-disable-jobserver", "--make-prefix=prefix")
+
+    # Do a dry run on the generated depfile
+    out = make("-n", "-f", makefile, output=str)
+
+    # Check that all specs are there (without duplicates)
+    specs_that_make_would_install = _parse_dry_run_package_installs(out)
+    expected_installs = [
+        "dtbuild1",
+        "dtbuild2",
+        "dtbuild3",
+        "dtlink1",
+        "dtlink2",
+        "dtlink3",
+        "dtlink4",
+        "dtlink5",
+        "dtrun1",
+        "dtrun2",
+        "dtrun3",
+        "dttop",
+    ]
+    assert set(specs_that_make_would_install) == set(expected_installs)
+    assert len(specs_that_make_would_install) == len(set(specs_that_make_would_install))
+
+
 @pytest.mark.parametrize(
     "picked_package,expected_installs",
     [