From 31431f967a59c07585021cba40683c2ca6ff2c47 Mon Sep 17 00:00:00 2001 From: Peter Scheibel Date: Mon, 17 Jul 2023 04:17:32 -0700 Subject: Environment/depfile: fix bug with Git hash versions (attempt #2) (#37560) Co-authored-by: Harmen Stoppels --- lib/spack/spack/environment/depfile.py | 50 ++++++++++++++++++++++++-------- lib/spack/spack/test/cmd/env.py | 53 ++++++++++++++++++++++++++++++++++ 2 files changed, 91 insertions(+), 12 deletions(-) (limited to 'lib') diff --git a/lib/spack/spack/environment/depfile.py b/lib/spack/spack/environment/depfile.py index 9f4087d055..4854fa784f 100644 --- a/lib/spack/spack/environment/depfile.py +++ b/lib/spack/spack/environment/depfile.py @@ -8,6 +8,7 @@ depfiles from an environment. """ 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.prereqs = prereqs + self.target = MakefileSpec(target) + 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 @@ class DepfileSpecVisitor: 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 @@ class MakefileModel: 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 @@ class MakefileModel: # And here we collect a tuple of (target, prereqs, dag_hash, nice_name, buildcache_flag) self.make_adjacency_list = [ ( - self._safe_name(item.target), - " ".join(self._install_target(self._safe_name(s)) for s in item.prereqs), - item.target.dag_hash(), - item.target.format("{name}{@version}{%compiler}{variants}{arch=architecture}"), + item.target.safe_name(), + " ".join(self._install_target(s.safe_name()) for s in item.prereqs), + item.target.spec_hash(), + 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 @@ class MakefileModel: 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 @@ class MakefileModel: 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 diff --git a/lib/spack/spack/test/cmd/env.py b/lib/spack/spack/test/cmd/env.py index 5aeb93ad58..8fe919d00e 100644 --- a/lib/spack/spack/test/cmd/env.py +++ b/lib/spack/spack/test/cmd/env.py @@ -19,10 +19,12 @@ import llnl.util.link_tree 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", [ -- cgit v1.2.3-60-g2f50