Fix importing Spack packages as Python modules (#29221)

fixes #29203

This PR fixes a subtle bug we have when importing
Spack packages as Python modules that can lead to
multiple module objects being created for the same
package.

It also fixes all the places in unit-tests where
"relying" on the old bug was crucial to have a new
"clean" state of the package class.

lib/spack/spack/dependency.py

@@ -124,7 +124,10 @@ def merge(self, other):
         # concatenate patch lists, or just copy them in
         for cond, p in other.patches.items():
             if cond in self.patches:
-                self.patches[cond].extend(other.patches[cond])
+                current_list = self.patches[cond]
+                current_list.extend(
+                    p for p in other.patches[cond] if p not in current_list
+                )
             else:
                 self.patches[cond] = other.patches[cond]
 

lib/spack/spack/patch.py

@@ -97,6 +97,12 @@ def to_dict(self):
             'working_dir': self.working_dir,
         }
 
+    def __eq__(self, other):
+        return self.sha256 == other.sha256
+
+    def __hash__(self):
+        return hash(self.sha256)
+
 
 class FilePatch(Patch):
     """Describes a patch that is retrieved from a file in the repository.

lib/spack/spack/repo.py

@@ -7,6 +7,7 @@
 import contextlib
 import errno
 import functools
+import importlib
 import inspect
 import itertools
 import os
@@ -1099,7 +1100,12 @@ def get_pkg_class(self, pkg_name):
                                         % (self.namespace, namespace))
 
         class_name = nm.mod_to_class(pkg_name)
-        module = self._get_pkg_module(pkg_name)
+
+        fullname = "{0}.{1}".format(self.full_namespace, pkg_name)
+        try:
+            module = importlib.import_module(fullname)
+        except ImportError:
+            raise UnknownPackageError(pkg_name)
 
         cls = getattr(module, class_name)
         if not inspect.isclass(cls):

lib/spack/spack/test/git_fetch.py

@@ -89,7 +89,8 @@ def test_fetch(type_of_test,
                mock_git_repository,
                config,
                mutable_mock_repo,
-               git_version):
+               git_version,
+               monkeypatch):
     """Tries to:
 
     1. Fetch the repo using a fetch strategy constructed with
@@ -107,7 +108,7 @@ def test_fetch(type_of_test,
     spec = Spec('git-test')
     spec.concretize()
     pkg = spack.repo.get(spec)
-    pkg.versions[ver('git')] = t.args
+    monkeypatch.setitem(pkg.versions, ver('git'), t.args)
 
     # Enter the stage directory and check some properties
     with pkg.stage:
@@ -137,7 +138,9 @@ def test_fetch(type_of_test,
 
 
 @pytest.mark.parametrize("type_of_test", ['branch', 'commit'])
-def test_debug_fetch(mock_packages, type_of_test, mock_git_repository, config):
+def test_debug_fetch(
+        mock_packages, type_of_test, mock_git_repository, config, monkeypatch
+):
     """Fetch the repo with debug enabled."""
     # Retrieve the right test parameters
     t = mock_git_repository.checks[type_of_test]
@@ -146,7 +149,7 @@ def test_debug_fetch(mock_packages, type_of_test, mock_git_repository, config):
     spec = Spec('git-test')
     spec.concretize()
     pkg = spack.repo.get(spec)
-    pkg.versions[ver('git')] = t.args
+    monkeypatch.setitem(pkg.versions, ver('git'), t.args)
 
     # Fetch then ensure source path exists
     with pkg.stage:
@@ -176,7 +179,7 @@ def test_needs_stage():
 
 @pytest.mark.parametrize("get_full_repo", [True, False])
 def test_get_full_repo(get_full_repo, git_version, mock_git_repository,
-                       config, mutable_mock_repo):
+                       config, mutable_mock_repo, monkeypatch):
     """Ensure that we can clone a full repository."""
 
     if git_version < ver('1.7.1'):
@@ -193,7 +196,7 @@ def test_get_full_repo(get_full_repo, git_version, mock_git_repository,
     pkg = spack.repo.get(spec)
     args = copy.copy(t.args)
     args['get_full_repo'] = get_full_repo
-    pkg.versions[ver('git')] = args
+    monkeypatch.setitem(pkg.versions, ver('git'), args)
 
     with pkg.stage:
         with spack.config.override('config:verify_ssl', secure):
@@ -222,7 +225,7 @@ def test_get_full_repo(get_full_repo, git_version, mock_git_repository,
 @pytest.mark.disable_clean_stage_check
 @pytest.mark.parametrize("submodules", [True, False])
 def test_gitsubmodule(submodules, mock_git_repository, config,
-                      mutable_mock_repo):
+                      mutable_mock_repo, monkeypatch):
     """
     Test GitFetchStrategy behavior with submodules
     """
@@ -235,7 +238,7 @@ def test_gitsubmodule(submodules, mock_git_repository, config,
     pkg = spack.repo.get(spec)
     args = copy.copy(t.args)
     args['submodules'] = submodules
-    pkg.versions[ver('git')] = args
+    monkeypatch.setitem(pkg.versions, ver('git'), args)
     pkg.do_stage()
     with working_dir(pkg.stage.source_path):
         for submodule_count in range(2):
@@ -249,7 +252,9 @@ def test_gitsubmodule(submodules, mock_git_repository, config,
 
 
 @pytest.mark.disable_clean_stage_check
-def test_gitsubmodules_delete(mock_git_repository, config, mutable_mock_repo):
+def test_gitsubmodules_delete(
+        mock_git_repository, config, mutable_mock_repo, monkeypatch
+):
     """
     Test GitFetchStrategy behavior with submodules_delete
     """
@@ -264,7 +269,7 @@ def test_gitsubmodules_delete(mock_git_repository, config, mutable_mock_repo):
     args['submodules'] = True
     args['submodules_delete'] = ['third_party/submodule0',
                                  'third_party/submodule1']
-    pkg.versions[ver('git')] = args
+    monkeypatch.setitem(pkg.versions, ver('git'), args)
     pkg.do_stage()
     with working_dir(pkg.stage.source_path):
         file_path = os.path.join(pkg.stage.source_path,

lib/spack/spack/test/hg_fetch.py

@@ -28,7 +28,8 @@ def test_fetch(
         secure,
         mock_hg_repository,
         config,
-        mutable_mock_repo
+        mutable_mock_repo,
+        monkeypatch
 ):
     """Tries to:
 
@@ -47,7 +48,7 @@ def test_fetch(
     spec = Spec('hg-test')
     spec.concretize()
     pkg = spack.repo.get(spec)
-    pkg.versions[ver('hg')] = t.args
+    monkeypatch.setitem(pkg.versions, ver('hg'), t.args)
 
     # Enter the stage directory and check some properties
     with pkg.stage:

lib/spack/spack/test/install.py

@@ -151,7 +151,9 @@ def test_failing_overwrite_install_should_keep_previous_installation(
     assert os.path.exists(spec.prefix)
 
 
-def test_dont_add_patches_to_installed_package(install_mockery, mock_fetch):
+def test_dont_add_patches_to_installed_package(
+        install_mockery, mock_fetch, monkeypatch
+):
     dependency = Spec('dependency-install')
     dependency.concretize()
     dependency.package.do_install()
@@ -160,9 +162,11 @@ def test_dont_add_patches_to_installed_package(install_mockery, mock_fetch):
     dependent = Spec('dependent-install ^/' + dependency_hash)
     dependent.concretize()
 
-    dependency.package.patches['dependency-install'] = [
+    monkeypatch.setitem(dependency.package.patches, 'dependency-install', [
         spack.patch.UrlPatch(
-            dependent.package, 'file://fake.patch', sha256='unused-hash')]
+            dependent.package, 'file://fake.patch', sha256='unused-hash'
+        )
+    ])
 
     assert dependent['dependency-install'] == dependency
 
@@ -308,18 +312,17 @@ def test_installed_upstream(install_upstream, mock_fetch):
 
 
 @pytest.mark.disable_clean_stage_check
-def test_partial_install_keep_prefix(install_mockery, mock_fetch):
+def test_partial_install_keep_prefix(install_mockery, mock_fetch, monkeypatch):
     spec = Spec('canfail').concretized()
     pkg = spack.repo.get(spec)
 
     # Normally the stage should start unset, but other tests set it
     pkg._stage = None
-    remove_prefix = spack.package.Package.remove_prefix
+
-    try:
     # If remove_prefix is called at any point in this test, that is an
     # error
     pkg.succeed = False  # make the build fail
-        spack.package.Package.remove_prefix = mock_remove_prefix
+    monkeypatch.setattr(spack.package.Package, 'remove_prefix', mock_remove_prefix)
     with pytest.raises(spack.build_environment.ChildError):
         pkg.do_install(keep_prefix=True)
     assert os.path.exists(pkg.prefix)
@@ -333,16 +336,11 @@ def test_partial_install_keep_prefix(install_mockery, mock_fetch):
     assert pkg.installed
     assert not pkg.stage.test_destroyed
 
-    finally:
-        spack.package.Package.remove_prefix = remove_prefix
 
-
+def test_second_install_no_overwrite_first(install_mockery, mock_fetch, monkeypatch):
-def test_second_install_no_overwrite_first(install_mockery, mock_fetch):
     spec = Spec('canfail').concretized()
     pkg = spack.repo.get(spec)
-    remove_prefix = spack.package.Package.remove_prefix
+    monkeypatch.setattr(spack.package.Package, 'remove_prefix', mock_remove_prefix)
-    try:
-        spack.package.Package.remove_prefix = mock_remove_prefix
 
     pkg.succeed = True
     pkg.do_install()
@@ -352,9 +350,6 @@ def test_second_install_no_overwrite_first(install_mockery, mock_fetch):
     pkg.succeed = False
     pkg.do_install()
 
-    finally:
-        spack.package.Package.remove_prefix = remove_prefix
-
 
 def test_install_prefix_collision_fails(config, mock_fetch, mock_packages, tmpdir):
     """

lib/spack/spack/test/repo.py

@@ -86,3 +86,15 @@ def test_namespace_hasattr(attr_name, exists, mutable_mock_repo):
 def test_all_package_names_is_cached_correctly():
     assert 'mpi' in spack.repo.all_package_names(include_virtuals=True)
     assert 'mpi' not in spack.repo.all_package_names(include_virtuals=False)
+
+
+@pytest.mark.regression('29203')
+def test_use_repositories_doesnt_change_class():
+    """Test that we don't create the same package module and class multiple times
+    when swapping repositories.
+    """
+    zlib_cls_outer = spack.repo.path.get_pkg_class('zlib')
+    current_paths = [r.root for r in spack.repo.path.repos]
+    with spack.repo.use_repositories(*current_paths):
+        zlib_cls_inner = spack.repo.path.get_pkg_class('zlib')
+    assert id(zlib_cls_inner) == id(zlib_cls_outer)

lib/spack/spack/test/spec_dag.py

@@ -31,7 +31,7 @@ def saved_deps():
 
 
 @pytest.fixture()
-def set_dependency(saved_deps):
+def set_dependency(saved_deps, monkeypatch):
     """Returns a function that alters the dependency information
     for a package in the ``saved_deps`` fixture.
     """
@@ -48,7 +48,7 @@ def _mock(pkg_name, spec, deptypes=all_deptypes):
 
         cond = Spec(pkg.name)
         dependency = Dependency(pkg, spec, type=deptypes)
-        pkg.dependencies[spec.name] = {cond: dependency}
+        monkeypatch.setitem(pkg.dependencies, spec.name, {cond: dependency})
     return _mock
 
 

lib/spack/spack/test/svn_fetch.py

@@ -29,7 +29,8 @@ def test_fetch(
         secure,
         mock_svn_repository,
         config,
-        mutable_mock_repo
+        mutable_mock_repo,
+        monkeypatch
 ):
     """Tries to:
 
@@ -48,7 +49,7 @@ def test_fetch(
     spec = Spec('svn-test')
     spec.concretize()
     pkg = spack.repo.get(spec)
-    pkg.versions[ver('svn')] = t.args
+    monkeypatch.setitem(pkg.versions, ver('svn'), t.args)
 
     # Enter the stage directory and check some properties
     with pkg.stage: