From 4755b28398da08a424ab159fa425a25e3966dab3 Mon Sep 17 00:00:00 2001
From: Harmen Stoppels <me@harmenstoppels.nl>
Date: Sun, 5 Nov 2023 08:56:11 +0100
Subject: Hidden modules: always append hash (#40868)

---
 lib/spack/spack/hooks/module_file_generation.py    | 13 ++--
 lib/spack/spack/modules/__init__.py                |  9 ++-
 lib/spack/spack/modules/common.py                  | 34 ++++++----
 lib/spack/spack/modules/lmod.py                    | 59 ++++++++---------
 lib/spack/spack/modules/tcl.py                     | 36 +++++-----
 .../test/data/modules/lmod/hide_implicits.yaml     |  1 +
 .../test/data/modules/tcl/exclude_implicits.yaml   |  1 +
 .../test/data/modules/tcl/hide_implicits.yaml      |  1 +
 lib/spack/spack/test/modules/lmod.py               | 77 +++++++++++-----------
 lib/spack/spack/test/modules/tcl.py                | 72 +++++++++-----------
 10 files changed, 153 insertions(+), 150 deletions(-)

diff --git a/lib/spack/spack/hooks/module_file_generation.py b/lib/spack/spack/hooks/module_file_generation.py
index 0c6428ebd4..1a2bbfdfe4 100644
--- a/lib/spack/spack/hooks/module_file_generation.py
+++ b/lib/spack/spack/hooks/module_file_generation.py
@@ -3,17 +3,22 @@
 #
 # SPDX-License-Identifier: (Apache-2.0 OR MIT)
 
+from typing import Optional, Set
+
 from llnl.util import tty
 
 import spack.config
 import spack.modules
+import spack.spec
 
 
-def _for_each_enabled(spec, method_name, explicit=None):
+def _for_each_enabled(
+    spec: spack.spec.Spec, method_name: str, explicit: Optional[bool] = None
+) -> None:
     """Calls a method for each enabled module"""
-    set_names = set(spack.config.get("modules", {}).keys())
+    set_names: Set[str] = set(spack.config.get("modules", {}).keys())
     for name in set_names:
-        enabled = spack.config.get("modules:%s:enable" % name)
+        enabled = spack.config.get(f"modules:{name}:enable")
         if not enabled:
             tty.debug("NO MODULE WRITTEN: list of enabled module files is empty")
             continue
@@ -28,7 +33,7 @@ def _for_each_enabled(spec, method_name, explicit=None):
                 tty.warn(msg.format(method_name, str(e)))
 
 
-def post_install(spec, explicit):
+def post_install(spec, explicit: bool):
     import spack.environment as ev  # break import cycle
 
     if ev.active_environment():
diff --git a/lib/spack/spack/modules/__init__.py b/lib/spack/spack/modules/__init__.py
index 13b8a95bed..dde8b74a5c 100644
--- a/lib/spack/spack/modules/__init__.py
+++ b/lib/spack/spack/modules/__init__.py
@@ -7,10 +7,15 @@
 include Tcl non-hierarchical modules, Lua hierarchical modules, and others.
 """
 
-from .common import disable_modules
+from typing import Dict, Type
+
+from .common import BaseModuleFileWriter, disable_modules
 from .lmod import LmodModulefileWriter
 from .tcl import TclModulefileWriter
 
 __all__ = ["TclModulefileWriter", "LmodModulefileWriter", "disable_modules"]
 
-module_types = {"tcl": TclModulefileWriter, "lmod": LmodModulefileWriter}
+module_types: Dict[str, Type[BaseModuleFileWriter]] = {
+    "tcl": TclModulefileWriter,
+    "lmod": LmodModulefileWriter,
+}
diff --git a/lib/spack/spack/modules/common.py b/lib/spack/spack/modules/common.py
index 49040e5ba3..465fed0324 100644
--- a/lib/spack/spack/modules/common.py
+++ b/lib/spack/spack/modules/common.py
@@ -35,7 +35,7 @@ import inspect
 import os.path
 import re
 import string
-from typing import Optional
+from typing import List, Optional
 
 import llnl.util.filesystem
 import llnl.util.tty as tty
@@ -50,6 +50,7 @@ import spack.paths
 import spack.projections as proj
 import spack.repo
 import spack.schema.environment
+import spack.spec
 import spack.store
 import spack.tengine as tengine
 import spack.util.environment
@@ -395,16 +396,14 @@ class BaseConfiguration:
 
     default_projections = {"all": "{name}/{version}-{compiler.name}-{compiler.version}"}
 
-    def __init__(self, spec, module_set_name, explicit=None):
+    def __init__(self, spec: spack.spec.Spec, module_set_name: str, explicit: bool) -> None:
         # Module where type(self) is defined
-        self.module = inspect.getmodule(self)
+        m = inspect.getmodule(self)
+        assert m is not None  # make mypy happy
+        self.module = m
         # Spec for which we want to generate a module file
         self.spec = spec
         self.name = module_set_name
-        # Software installation has been explicitly asked (get this information from
-        # db when querying an existing module, like during a refresh or rm operations)
-        if explicit is None:
-            explicit = spec._installed_explicitly()
         self.explicit = explicit
         # Dictionary of configuration options that should be applied
         # to the spec
@@ -458,7 +457,11 @@ class BaseConfiguration:
             if constraint in self.spec:
                 suffixes.append(suffix)
         suffixes = list(dedupe(suffixes))
-        if self.hash:
+        # For hidden modules we can always add a fixed length hash as suffix, since it guards
+        # against file name clashes, and the module is not exposed to the user anyways.
+        if self.hidden:
+            suffixes.append(self.spec.dag_hash(length=7))
+        elif self.hash:
             suffixes.append(self.hash)
         return suffixes
 
@@ -551,8 +554,7 @@ class BaseConfiguration:
     def _create_list_for(self, what):
         include = []
         for item in self.conf[what]:
-            conf = type(self)(item, self.name)
-            if not conf.excluded:
+            if not self.module.make_configuration(item, self.name).excluded:
                 include.append(item)
         return include
 
@@ -826,8 +828,7 @@ class BaseContext(tengine.Context):
     def _create_module_list_of(self, what):
         m = self.conf.module
         name = self.conf.name
-        explicit = self.conf.explicit
-        return [m.make_layout(x, name, explicit).use_name for x in getattr(self.conf, what)]
+        return [m.make_layout(x, name).use_name for x in getattr(self.conf, what)]
 
     @tengine.context_property
     def verbose(self):
@@ -836,12 +837,19 @@ class BaseContext(tengine.Context):
 
 
 class BaseModuleFileWriter:
-    def __init__(self, spec, module_set_name, explicit=None):
+    default_template: str
+    hide_cmd_format: str
+    modulerc_header: List[str]
+
+    def __init__(
+        self, spec: spack.spec.Spec, module_set_name: str, explicit: Optional[bool] = None
+    ) -> None:
         self.spec = spec
 
         # This class is meant to be derived. Get the module of the
         # actual writer.
         self.module = inspect.getmodule(self)
+        assert self.module is not None  # make mypy happy
         m = self.module
 
         # Create the triplet of configuration/layout/context
diff --git a/lib/spack/spack/modules/lmod.py b/lib/spack/spack/modules/lmod.py
index e2bcfa2973..8f529ba21c 100644
--- a/lib/spack/spack/modules/lmod.py
+++ b/lib/spack/spack/modules/lmod.py
@@ -6,8 +6,7 @@
 import collections
 import itertools
 import os.path
-import posixpath
-from typing import Any, Dict, List
+from typing import Dict, List, Optional, Tuple
 
 import llnl.util.filesystem as fs
 import llnl.util.lang as lang
@@ -24,18 +23,19 @@ from .common import BaseConfiguration, BaseContext, BaseFileLayout, BaseModuleFi
 
 
 #: lmod specific part of the configuration
-def configuration(module_set_name):
-    config_path = "modules:%s:lmod" % module_set_name
-    config = spack.config.get(config_path, {})
-    return config
+def configuration(module_set_name: str) -> dict:
+    return spack.config.get(f"modules:{module_set_name}:lmod", {})
 
 
 # Caches the configuration {spec_hash: configuration}
-configuration_registry: Dict[str, Any] = {}
+configuration_registry: Dict[Tuple[str, str, bool], BaseConfiguration] = {}
 
 
-def make_configuration(spec, module_set_name, explicit):
+def make_configuration(
+    spec: spack.spec.Spec, module_set_name: str, explicit: Optional[bool] = None
+) -> BaseConfiguration:
     """Returns the lmod configuration for spec"""
+    explicit = bool(spec._installed_explicitly()) if explicit is None else explicit
     key = (spec.dag_hash(), module_set_name, explicit)
     try:
         return configuration_registry[key]
@@ -45,16 +45,18 @@ def make_configuration(spec, module_set_name, explicit):
         )
 
 
-def make_layout(spec, module_set_name, explicit):
+def make_layout(
+    spec: spack.spec.Spec, module_set_name: str, explicit: Optional[bool] = None
+) -> BaseFileLayout:
     """Returns the layout information for spec"""
-    conf = make_configuration(spec, module_set_name, explicit)
-    return LmodFileLayout(conf)
+    return LmodFileLayout(make_configuration(spec, module_set_name, explicit))
 
 
-def make_context(spec, module_set_name, explicit):
+def make_context(
+    spec: spack.spec.Spec, module_set_name: str, explicit: Optional[bool] = None
+) -> BaseContext:
     """Returns the context information for spec"""
-    conf = make_configuration(spec, module_set_name, explicit)
-    return LmodContext(conf)
+    return LmodContext(make_configuration(spec, module_set_name, explicit))
 
 
 def guess_core_compilers(name, store=False) -> List[spack.spec.CompilerSpec]:
@@ -97,10 +99,7 @@ def guess_core_compilers(name, store=False) -> List[spack.spec.CompilerSpec]:
 class LmodConfiguration(BaseConfiguration):
     """Configuration class for lmod module files."""
 
-    # Note: Posixpath is used here as well as below as opposed to
-    # os.path.join due to spack.spec.Spec.format
-    # requiring forward slash path seperators at this stage
-    default_projections = {"all": posixpath.join("{name}", "{version}")}
+    default_projections = {"all": "{name}/{version}"}
 
     @property
     def core_compilers(self) -> List[spack.spec.CompilerSpec]:
@@ -274,19 +273,16 @@ class LmodFileLayout(BaseFileLayout):
         hierarchy_name = os.path.join(*parts)
 
         # Compute the absolute path
-        fullname = os.path.join(
+        return os.path.join(
             self.arch_dirname,  # root for lmod files on this architecture
             hierarchy_name,  # relative path
-            ".".join([self.use_name, self.extension]),  # file name
+            f"{self.use_name}.{self.extension}",  # file name
         )
-        return fullname
 
     @property
     def modulerc(self):
         """Returns the modulerc file associated with current module file"""
-        return os.path.join(
-            os.path.dirname(self.filename), ".".join([".modulerc", self.extension])
-        )
+        return os.path.join(os.path.dirname(self.filename), f".modulerc.{self.extension}")
 
     def token_to_path(self, name, value):
         """Transforms a hierarchy token into the corresponding path part.
@@ -319,9 +315,7 @@ class LmodFileLayout(BaseFileLayout):
         # we need to append a hash to the version to distinguish
         # among flavors of the same library (e.g. openblas~openmp vs.
         # openblas+openmp)
-        path = path_part_fmt(token=value)
-        path = "-".join([path, value.dag_hash(length=7)])
-        return path
+        return f"{path_part_fmt(token=value)}-{value.dag_hash(length=7)}"
 
     @property
     def available_path_parts(self):
@@ -333,8 +327,7 @@ class LmodFileLayout(BaseFileLayout):
         # List of services that are part of the hierarchy
         hierarchy = self.conf.hierarchy_tokens
         # Tokenize each part that is both in the hierarchy and available
-        parts = [self.token_to_path(x, available[x]) for x in hierarchy if x in available]
-        return parts
+        return [self.token_to_path(x, available[x]) for x in hierarchy if x in available]
 
     @property
     @lang.memoized
@@ -452,7 +445,7 @@ class LmodContext(BaseContext):
     @lang.memoized
     def unlocked_paths(self):
         """Returns the list of paths that are unlocked unconditionally."""
-        layout = make_layout(self.spec, self.conf.name, self.conf.explicit)
+        layout = make_layout(self.spec, self.conf.name)
         return [os.path.join(*parts) for parts in layout.unlocked_paths[None]]
 
     @tengine.context_property
@@ -460,7 +453,7 @@ class LmodContext(BaseContext):
         """Returns the list of paths that are unlocked conditionally.
         Each item in the list is a tuple with the structure (condition, path).
         """
-        layout = make_layout(self.spec, self.conf.name, self.conf.explicit)
+        layout = make_layout(self.spec, self.conf.name)
         value = []
         conditional_paths = layout.unlocked_paths
         conditional_paths.pop(None)
@@ -482,9 +475,9 @@ class LmodContext(BaseContext):
 class LmodModulefileWriter(BaseModuleFileWriter):
     """Writer class for lmod module files."""
 
-    default_template = posixpath.join("modules", "modulefile.lua")
+    default_template = "modules/modulefile.lua"
 
-    modulerc_header: list = []
+    modulerc_header = []
 
     hide_cmd_format = 'hide_version("%s")'
 
diff --git a/lib/spack/spack/modules/tcl.py b/lib/spack/spack/modules/tcl.py
index ed12827c33..6d7f49b330 100644
--- a/lib/spack/spack/modules/tcl.py
+++ b/lib/spack/spack/modules/tcl.py
@@ -7,28 +7,29 @@
 non-hierarchical modules.
 """
 import os.path
-import posixpath
-from typing import Any, Dict
+from typing import Dict, Optional, Tuple
 
 import spack.config
+import spack.spec
 import spack.tengine as tengine
 
 from .common import BaseConfiguration, BaseContext, BaseFileLayout, BaseModuleFileWriter
 
 
 #: Tcl specific part of the configuration
-def configuration(module_set_name):
-    config_path = "modules:%s:tcl" % module_set_name
-    config = spack.config.get(config_path, {})
-    return config
+def configuration(module_set_name: str) -> dict:
+    return spack.config.get(f"modules:{module_set_name}:tcl", {})
 
 
 # Caches the configuration {spec_hash: configuration}
-configuration_registry: Dict[str, Any] = {}
+configuration_registry: Dict[Tuple[str, str, bool], BaseConfiguration] = {}
 
 
-def make_configuration(spec, module_set_name, explicit):
+def make_configuration(
+    spec: spack.spec.Spec, module_set_name: str, explicit: Optional[bool] = None
+) -> BaseConfiguration:
     """Returns the tcl configuration for spec"""
+    explicit = bool(spec._installed_explicitly()) if explicit is None else explicit
     key = (spec.dag_hash(), module_set_name, explicit)
     try:
         return configuration_registry[key]
@@ -38,16 +39,18 @@ def make_configuration(spec, module_set_name, explicit):
         )
 
 
-def make_layout(spec, module_set_name, explicit):
+def make_layout(
+    spec: spack.spec.Spec, module_set_name: str, explicit: Optional[bool] = None
+) -> BaseFileLayout:
     """Returns the layout information for spec"""
-    conf = make_configuration(spec, module_set_name, explicit)
-    return TclFileLayout(conf)
+    return TclFileLayout(make_configuration(spec, module_set_name, explicit))
 
 
-def make_context(spec, module_set_name, explicit):
+def make_context(
+    spec: spack.spec.Spec, module_set_name: str, explicit: Optional[bool] = None
+) -> BaseContext:
     """Returns the context information for spec"""
-    conf = make_configuration(spec, module_set_name, explicit)
-    return TclContext(conf)
+    return TclContext(make_configuration(spec, module_set_name, explicit))
 
 
 class TclConfiguration(BaseConfiguration):
@@ -75,10 +78,7 @@ class TclContext(BaseContext):
 class TclModulefileWriter(BaseModuleFileWriter):
     """Writer class for tcl module files."""
 
-    # Note: Posixpath is used here as opposed to
-    # os.path.join due to spack.spec.Spec.format
-    # requiring forward slash path seperators at this stage
-    default_template = posixpath.join("modules", "modulefile.tcl")
+    default_template = "modules/modulefile.tcl"
 
     modulerc_header = ["#%Module4.7"]
 
diff --git a/lib/spack/spack/test/data/modules/lmod/hide_implicits.yaml b/lib/spack/spack/test/data/modules/lmod/hide_implicits.yaml
index d13c1a7b97..e9326ab42c 100644
--- a/lib/spack/spack/test/data/modules/lmod/hide_implicits.yaml
+++ b/lib/spack/spack/test/data/modules/lmod/hide_implicits.yaml
@@ -2,6 +2,7 @@ enable:
   - lmod
 lmod:
   hide_implicits: true
+  hash_length: 0
   core_compilers:
     - 'clang@3.3'
   hierarchy:
diff --git a/lib/spack/spack/test/data/modules/tcl/exclude_implicits.yaml b/lib/spack/spack/test/data/modules/tcl/exclude_implicits.yaml
index 5af22e6e40..4835b4ecd9 100644
--- a/lib/spack/spack/test/data/modules/tcl/exclude_implicits.yaml
+++ b/lib/spack/spack/test/data/modules/tcl/exclude_implicits.yaml
@@ -4,5 +4,6 @@ enable:
   - tcl
 tcl:
   exclude_implicits: true
+  hash_length: 0
   all:
     autoload: direct
diff --git a/lib/spack/spack/test/data/modules/tcl/hide_implicits.yaml b/lib/spack/spack/test/data/modules/tcl/hide_implicits.yaml
index 3ae7517b8f..136c42f3c7 100644
--- a/lib/spack/spack/test/data/modules/tcl/hide_implicits.yaml
+++ b/lib/spack/spack/test/data/modules/tcl/hide_implicits.yaml
@@ -2,5 +2,6 @@ enable:
   - tcl
 tcl:
   hide_implicits: true
+  hash_length: 0
   all:
     autoload: direct
diff --git a/lib/spack/spack/test/modules/lmod.py b/lib/spack/spack/test/modules/lmod.py
index 510006f0a9..acaae90f69 100644
--- a/lib/spack/spack/test/modules/lmod.py
+++ b/lib/spack/spack/test/modules/lmod.py
@@ -435,7 +435,7 @@ class TestLmod:
 
         assert str(spec.os) not in path
 
-    def test_hide_implicits(self, module_configuration):
+    def test_hide_implicits(self, module_configuration, temporary_store):
         """Tests the addition and removal of hide command in modulerc."""
         module_configuration("hide_implicits")
 
@@ -446,29 +446,42 @@ class TestLmod:
         writer.write()
         assert os.path.exists(writer.layout.modulerc)
         with open(writer.layout.modulerc) as f:
-            content = f.readlines()
-            content = "".join(content).split("\n")
-        hide_cmd = 'hide_version("%s")' % writer.layout.use_name
-        assert len([x for x in content if hide_cmd == x]) == 1
-
-        # mpileaks becomes explicit, thus modulerc is removed
-        writer = writer_cls(spec, "default", True)
-        writer.write(overwrite=True)
-        assert not os.path.exists(writer.layout.modulerc)
-
-        # mpileaks is defined as explicit, no modulerc file should exist
+            content = [line.strip() for line in f.readlines()]
+        hide_implicit_mpileaks = f'hide_version("{writer.layout.use_name}")'
+        assert len([x for x in content if hide_implicit_mpileaks == x]) == 1
+
+        # The direct dependencies are all implicitly installed, and they should all be hidden,
+        # except for mpich, which is provider for mpi, which is in the hierarchy, and therefore
+        # can't be hidden. All other hidden modules should have a 7 character hash (the config
+        # hash_length = 0 only applies to exposed modules).
+        with open(writer.layout.filename) as f:
+            depends_statements = [line.strip() for line in f.readlines() if "depends_on" in line]
+            for dep in spec.dependencies(deptype=("link", "run")):
+                if dep.satisfies("mpi"):
+                    assert not any(dep.dag_hash(7) in line for line in depends_statements)
+                else:
+                    assert any(dep.dag_hash(7) in line for line in depends_statements)
+
+        # when mpileaks becomes explicit, its file name changes (hash_length = 0), meaning an
+        # extra module file is created; the old one still exists and remains hidden.
         writer = writer_cls(spec, "default", True)
         writer.write()
-        assert not os.path.exists(writer.layout.modulerc)
-
-        # explicit module is removed
-        writer.remove()
+        assert os.path.exists(writer.layout.modulerc)
+        with open(writer.layout.modulerc) as f:
+            content = [line.strip() for line in f.readlines()]
+        assert hide_implicit_mpileaks in content  # old, implicit mpileaks is still hidden
+        assert f'hide_version("{writer.layout.use_name}")' not in content
+
+        # after removing both the implicit and explicit module, the modulerc file would be empty
+        # and should be removed.
+        writer_cls(spec, "default", False).remove()
+        writer_cls(spec, "default", True).remove()
         assert not os.path.exists(writer.layout.modulerc)
         assert not os.path.exists(writer.layout.filename)
 
         # implicit module is removed
         writer = writer_cls(spec, "default", False)
-        writer.write(overwrite=True)
+        writer.write()
         assert os.path.exists(writer.layout.filename)
         assert os.path.exists(writer.layout.modulerc)
         writer.remove()
@@ -486,35 +499,19 @@ class TestLmod:
         writer_alt2.write(overwrite=True)
         assert os.path.exists(writer.layout.modulerc)
         with open(writer.layout.modulerc) as f:
-            content = f.readlines()
-            content = "".join(content).split("\n")
-        hide_cmd = 'hide_version("%s")' % writer.layout.use_name
-        hide_cmd_alt1 = 'hide_version("%s")' % writer_alt1.layout.use_name
-        hide_cmd_alt2 = 'hide_version("%s")' % writer_alt2.layout.use_name
+            content = [line.strip() for line in f.readlines()]
+        hide_cmd = f'hide_version("{writer.layout.use_name}")'
+        hide_cmd_alt1 = f'hide_version("{writer_alt1.layout.use_name}")'
+        hide_cmd_alt2 = f'hide_version("{writer_alt2.layout.use_name}")'
         assert len([x for x in content if hide_cmd == x]) == 1
         assert len([x for x in content if hide_cmd_alt1 == x]) == 1
         assert len([x for x in content if hide_cmd_alt2 == x]) == 1
 
-        # one version is removed, a second becomes explicit
+        # one version is removed
         writer_alt1.remove()
-        writer_alt2 = writer_cls(spec_alt2, "default", True)
-        writer_alt2.write(overwrite=True)
         assert os.path.exists(writer.layout.modulerc)
         with open(writer.layout.modulerc) as f:
-            content = f.readlines()
-            content = "".join(content).split("\n")
+            content = [line.strip() for line in f.readlines()]
         assert len([x for x in content if hide_cmd == x]) == 1
         assert len([x for x in content if hide_cmd_alt1 == x]) == 0
-        assert len([x for x in content if hide_cmd_alt2 == x]) == 0
-
-        # disable hide_implicits configuration option
-        module_configuration("autoload_direct")
-        writer = writer_cls(spec, "default")
-        writer.write(overwrite=True)
-        assert not os.path.exists(writer.layout.modulerc)
-
-        # reenable hide_implicits configuration option
-        module_configuration("hide_implicits")
-        writer = writer_cls(spec, "default")
-        writer.write(overwrite=True)
-        assert os.path.exists(writer.layout.modulerc)
+        assert len([x for x in content if hide_cmd_alt2 == x]) == 1
diff --git a/lib/spack/spack/test/modules/tcl.py b/lib/spack/spack/test/modules/tcl.py
index 4a8d9e10a2..00460b6796 100644
--- a/lib/spack/spack/test/modules/tcl.py
+++ b/lib/spack/spack/test/modules/tcl.py
@@ -488,7 +488,7 @@ class TestTcl:
 
         assert str(spec.os) not in path
 
-    def test_hide_implicits(self, module_configuration):
+    def test_hide_implicits(self, module_configuration, temporary_store):
         """Tests the addition and removal of hide command in modulerc."""
         module_configuration("hide_implicits")
 
@@ -499,29 +499,37 @@ class TestTcl:
         writer.write()
         assert os.path.exists(writer.layout.modulerc)
         with open(writer.layout.modulerc) as f:
-            content = f.readlines()
-            content = "".join(content).split("\n")
-        hide_cmd = "module-hide --soft --hidden-loaded %s" % writer.layout.use_name
-        assert len([x for x in content if hide_cmd == x]) == 1
-
-        # mpileaks becomes explicit, thus modulerc is removed
-        writer = writer_cls(spec, "default", True)
-        writer.write(overwrite=True)
-        assert not os.path.exists(writer.layout.modulerc)
-
-        # mpileaks is defined as explicit, no modulerc file should exist
+            content = [line.strip() for line in f.readlines()]
+        hide_implicit_mpileaks = f"module-hide --soft --hidden-loaded {writer.layout.use_name}"
+        assert len([x for x in content if hide_implicit_mpileaks == x]) == 1
+
+        # The direct dependencies are all implicit, and they should have depends-on with fixed
+        # 7 character hash, even though the config is set to hash_length = 0.
+        with open(writer.layout.filename) as f:
+            depends_statements = [line.strip() for line in f.readlines() if "depends-on" in line]
+            for dep in spec.dependencies(deptype=("link", "run")):
+                assert any(dep.dag_hash(7) in line for line in depends_statements)
+
+        # when mpileaks becomes explicit, its file name changes (hash_length = 0), meaning an
+        # extra module file is created; the old one still exists and remains hidden.
         writer = writer_cls(spec, "default", True)
         writer.write()
-        assert not os.path.exists(writer.layout.modulerc)
-
-        # explicit module is removed
-        writer.remove()
+        assert os.path.exists(writer.layout.modulerc)
+        with open(writer.layout.modulerc) as f:
+            content = [line.strip() for line in f.readlines()]
+        assert hide_implicit_mpileaks in content  # old, implicit mpileaks is still hidden
+        assert f"module-hide --soft --hidden-loaded {writer.layout.use_name}" not in content
+
+        # after removing both the implicit and explicit module, the modulerc file would be empty
+        # and should be removed.
+        writer_cls(spec, "default", False).remove()
+        writer_cls(spec, "default", True).remove()
         assert not os.path.exists(writer.layout.modulerc)
         assert not os.path.exists(writer.layout.filename)
 
         # implicit module is removed
         writer = writer_cls(spec, "default", False)
-        writer.write(overwrite=True)
+        writer.write()
         assert os.path.exists(writer.layout.filename)
         assert os.path.exists(writer.layout.modulerc)
         writer.remove()
@@ -539,35 +547,19 @@ class TestTcl:
         writer_alt2.write(overwrite=True)
         assert os.path.exists(writer.layout.modulerc)
         with open(writer.layout.modulerc) as f:
-            content = f.readlines()
-            content = "".join(content).split("\n")
-        hide_cmd = "module-hide --soft --hidden-loaded %s" % writer.layout.use_name
-        hide_cmd_alt1 = "module-hide --soft --hidden-loaded %s" % writer_alt1.layout.use_name
-        hide_cmd_alt2 = "module-hide --soft --hidden-loaded %s" % writer_alt2.layout.use_name
+            content = [line.strip() for line in f.readlines()]
+        hide_cmd = f"module-hide --soft --hidden-loaded {writer.layout.use_name}"
+        hide_cmd_alt1 = f"module-hide --soft --hidden-loaded {writer_alt1.layout.use_name}"
+        hide_cmd_alt2 = f"module-hide --soft --hidden-loaded {writer_alt2.layout.use_name}"
         assert len([x for x in content if hide_cmd == x]) == 1
         assert len([x for x in content if hide_cmd_alt1 == x]) == 1
         assert len([x for x in content if hide_cmd_alt2 == x]) == 1
 
-        # one version is removed, a second becomes explicit
+        # one version is removed
         writer_alt1.remove()
-        writer_alt2 = writer_cls(spec_alt2, "default", True)
-        writer_alt2.write(overwrite=True)
         assert os.path.exists(writer.layout.modulerc)
         with open(writer.layout.modulerc) as f:
-            content = f.readlines()
-            content = "".join(content).split("\n")
+            content = [line.strip() for line in f.readlines()]
         assert len([x for x in content if hide_cmd == x]) == 1
         assert len([x for x in content if hide_cmd_alt1 == x]) == 0
-        assert len([x for x in content if hide_cmd_alt2 == x]) == 0
-
-        # disable hide_implicits configuration option
-        module_configuration("autoload_direct")
-        writer = writer_cls(spec, "default")
-        writer.write(overwrite=True)
-        assert not os.path.exists(writer.layout.modulerc)
-
-        # reenable hide_implicits configuration option
-        module_configuration("hide_implicits")
-        writer = writer_cls(spec, "default")
-        writer.write(overwrite=True)
-        assert os.path.exists(writer.layout.modulerc)
+        assert len([x for x in content if hide_cmd_alt2 == x]) == 1
-- 
cgit v1.2.3-60-g2f50