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.

9 files changed, 82 insertions, 53 deletions

diff --git a/lib/spack/spack/dependency.py b/lib/spack/spack/dependency.py
index 0eaf0fbff0..0287672254 100644
--- a/lib/spack/spack/dependency.py
+++ b/lib/spack/spack/dependency.py
@@ -124,7 +124,10 @@ class Dependency(object):
         # 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]
 
diff --git a/lib/spack/spack/patch.py b/lib/spack/spack/patch.py
index 14b00b3d3f..d248966ede 100644
--- a/lib/spack/spack/patch.py
+++ b/lib/spack/spack/patch.py
@@ -97,6 +97,12 @@ class Patch(object):
             '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.
diff --git a/lib/spack/spack/repo.py b/lib/spack/spack/repo.py
index 42a7aab5b6..ea98882cf2 100644
--- a/lib/spack/spack/repo.py
+++ b/lib/spack/spack/repo.py
@@ -7,6 +7,7 @@ import abc
 import contextlib
 import errno
 import functools
+import importlib
 import inspect
 import itertools
 import os
@@ -1099,7 +1100,12 @@ class Repo(object):
                                         % (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):
diff --git a/lib/spack/spack/test/git_fetch.py b/lib/spack/spack/test/git_fetch.py
index 4f8243651f..6e8993978c 100644
--- a/lib/spack/spack/test/git_fetch.py
+++ b/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,
diff --git a/lib/spack/spack/test/hg_fetch.py b/lib/spack/spack/test/hg_fetch.py
index d312451454..5b2a41875f 100644
--- a/lib/spack/spack/test/hg_fetch.py
+++ b/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:
diff --git a/lib/spack/spack/test/install.py b/lib/spack/spack/test/install.py
index 06ac3817d8..a100d23aab 100644
--- a/lib/spack/spack/test/install.py
+++ b/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,52 +312,43 @@ 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
-        with pytest.raises(spack.build_environment.ChildError):
-            pkg.do_install(keep_prefix=True)
-        assert os.path.exists(pkg.prefix)
 
-        # must clear failure markings for the package before re-installing it
-        spack.store.db.clear_failure(spec, True)
-
-        pkg.succeed = True   # make the build succeed
-        pkg.stage = MockStage(pkg.stage)
+    # If remove_prefix is called at any point in this test, that is an
+    # error
+    pkg.succeed = False  # make the build fail
+    monkeypatch.setattr(spack.package.Package, 'remove_prefix', mock_remove_prefix)
+    with pytest.raises(spack.build_environment.ChildError):
         pkg.do_install(keep_prefix=True)
-        assert pkg.installed
-        assert not pkg.stage.test_destroyed
+    assert os.path.exists(pkg.prefix)
 
-    finally:
-        spack.package.Package.remove_prefix = remove_prefix
+    # must clear failure markings for the package before re-installing it
+    spack.store.db.clear_failure(spec, True)
+
+    pkg.succeed = True   # make the build succeed
+    pkg.stage = MockStage(pkg.stage)
+    pkg.do_install(keep_prefix=True)
+    assert pkg.installed
+    assert not pkg.stage.test_destroyed
 
 
-def test_second_install_no_overwrite_first(install_mockery, mock_fetch):
+def test_second_install_no_overwrite_first(install_mockery, mock_fetch, monkeypatch):
     spec = Spec('canfail').concretized()
     pkg = spack.repo.get(spec)
-    remove_prefix = spack.package.Package.remove_prefix
-    try:
-        spack.package.Package.remove_prefix = mock_remove_prefix
+    monkeypatch.setattr(spack.package.Package, 'remove_prefix', mock_remove_prefix)
 
-        pkg.succeed = True
-        pkg.do_install()
-        assert pkg.installed
-
-        # If Package.install is called after this point, it will fail
-        pkg.succeed = False
-        pkg.do_install()
+    pkg.succeed = True
+    pkg.do_install()
+    assert pkg.installed
 
-    finally:
-        spack.package.Package.remove_prefix = remove_prefix
+    # If Package.install is called after this point, it will fail
+    pkg.succeed = False
+    pkg.do_install()
 
 
 def test_install_prefix_collision_fails(config, mock_fetch, mock_packages, tmpdir):
diff --git a/lib/spack/spack/test/repo.py b/lib/spack/spack/test/repo.py
index 30fb73fce9..ac3138325e 100644
--- a/lib/spack/spack/test/repo.py
+++ b/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)
diff --git a/lib/spack/spack/test/spec_dag.py b/lib/spack/spack/test/spec_dag.py
index 07b255eeb1..61ec9010d2 100644
--- a/lib/spack/spack/test/spec_dag.py
+++ b/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 set_dependency(saved_deps):
 
         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
 
 
diff --git a/lib/spack/spack/test/svn_fetch.py b/lib/spack/spack/test/svn_fetch.py
index cfbc3d486c..73d672eb4a 100644
--- a/lib/spack/spack/test/svn_fetch.py
+++ b/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: