From 0ab9c8b90400677729e7536496497233807ad60d Mon Sep 17 00:00:00 2001 From: Harmen Stoppels Date: Tue, 9 Jan 2024 17:57:41 +0100 Subject: [PATCH] installer.py: don't dereference stage before installing from binaries (#41986) This fixes an issue where pkg.stage throws because a patch cannot be found, but the patch is redundant because the spec is reused from a build cache and will be installed from existing binaries. --- lib/spack/spack/installer.py | 25 ++++---- lib/spack/spack/test/install.py | 107 ++++++++++++++++++-------------- 2 files changed, 72 insertions(+), 60 deletions(-) diff --git a/lib/spack/spack/installer.py b/lib/spack/spack/installer.py index 51f7034176..7cca5efbfe 100644 --- a/lib/spack/spack/installer.py +++ b/lib/spack/spack/installer.py @@ -1335,7 +1335,6 @@ def _prepare_for_install(self, task: BuildTask) -> None: """ install_args = task.request.install_args keep_prefix = install_args.get("keep_prefix") - restage = install_args.get("restage") # Make sure the package is ready to be locally installed. self._ensure_install_ready(task.pkg) @@ -1367,10 +1366,6 @@ def _prepare_for_install(self, task: BuildTask) -> None: else: tty.debug(f"{task.pkg_id} is partially installed") - # Destroy the stage for a locally installed, non-DIYStage, package - if restage and task.pkg.stage.managed_by_spack: - task.pkg.stage.destroy() - if ( rec and installed_in_db @@ -1691,6 +1686,10 @@ def _install_task(self, task: BuildTask, install_status: InstallStatus) -> None: try: self._setup_install_dir(pkg) + # Create stage object now and let it be serialized for the child process. That + # way monkeypatch in tests works correctly. + pkg.stage + # Create a child process to do the actual installation. # Preserve verbosity settings across installs. spack.package_base.PackageBase._verbose = spack.build_environment.start_build_process( @@ -2223,11 +2222,6 @@ def install(self) -> None: if not keep_prefix and not action == InstallAction.OVERWRITE: pkg.remove_prefix() - # The subprocess *may* have removed the build stage. Mark it - # not created so that the next time pkg.stage is invoked, we - # check the filesystem for it. - pkg.stage.created = False - # Perform basic task cleanup for the installed spec to # include downgrading the write to a read lock self._cleanup_task(pkg) @@ -2297,6 +2291,9 @@ def __init__(self, pkg: "spack.package_base.PackageBase", install_args: dict): # whether to keep the build stage after installation self.keep_stage = install_args.get("keep_stage", False) + # whether to restage + self.restage = install_args.get("restage", False) + # whether to skip the patch phase self.skip_patch = install_args.get("skip_patch", False) @@ -2327,9 +2324,13 @@ def __init__(self, pkg: "spack.package_base.PackageBase", install_args: dict): def run(self) -> bool: """Main entry point from ``build_process`` to kick off install in child.""" - self.pkg.stage.keep = self.keep_stage + stage = self.pkg.stage + stage.keep = self.keep_stage - with self.pkg.stage: + if self.restage: + stage.destroy() + + with stage: self.timer.start("stage") if not self.fake: diff --git a/lib/spack/spack/test/install.py b/lib/spack/spack/test/install.py index 467577cead..1c29016d84 100644 --- a/lib/spack/spack/test/install.py +++ b/lib/spack/spack/test/install.py @@ -12,10 +12,12 @@ import llnl.util.filesystem as fs import spack.error +import spack.mirror import spack.patch import spack.repo import spack.store import spack.util.spack_json as sjson +from spack import binary_distribution from spack.package_base import ( InstallError, PackageBase, @@ -118,59 +120,25 @@ def remove_prefix(self): self.wrapped_rm_prefix() -class MockStage: - def __init__(self, wrapped_stage): - self.wrapped_stage = wrapped_stage - self.test_destroyed = False - - def __enter__(self): - self.create() - return self - - def __exit__(self, exc_type, exc_val, exc_tb): - if exc_type is None: - self.destroy() - - def destroy(self): - self.test_destroyed = True - self.wrapped_stage.destroy() - - def create(self): - self.wrapped_stage.create() - - def __getattr__(self, attr): - if attr == "wrapped_stage": - # This attribute may not be defined at some point during unpickling - raise AttributeError() - return getattr(self.wrapped_stage, attr) - - def test_partial_install_delete_prefix_and_stage(install_mockery, mock_fetch, working_env): s = Spec("canfail").concretized() instance_rm_prefix = s.package.remove_prefix - try: - s.package.remove_prefix = mock_remove_prefix - with pytest.raises(MockInstallError): - s.package.do_install() - assert os.path.isdir(s.package.prefix) - rm_prefix_checker = RemovePrefixChecker(instance_rm_prefix) - s.package.remove_prefix = rm_prefix_checker.remove_prefix + s.package.remove_prefix = mock_remove_prefix + with pytest.raises(MockInstallError): + s.package.do_install() + assert os.path.isdir(s.package.prefix) + rm_prefix_checker = RemovePrefixChecker(instance_rm_prefix) + s.package.remove_prefix = rm_prefix_checker.remove_prefix - # must clear failure markings for the package before re-installing it - spack.store.STORE.failure_tracker.clear(s, True) + # must clear failure markings for the package before re-installing it + spack.store.STORE.failure_tracker.clear(s, True) - s.package.set_install_succeed() - s.package.stage = MockStage(s.package.stage) - - s.package.do_install(restage=True) - assert rm_prefix_checker.removed - assert s.package.stage.test_destroyed - assert s.package.spec.installed - - finally: - s.package.remove_prefix = instance_rm_prefix + s.package.set_install_succeed() + s.package.do_install(restage=True) + assert rm_prefix_checker.removed + assert s.package.spec.installed @pytest.mark.disable_clean_stage_check @@ -357,10 +325,8 @@ def test_partial_install_keep_prefix(install_mockery, mock_fetch, monkeypatch, w spack.store.STORE.failure_tracker.clear(s, True) s.package.set_install_succeed() - s.package.stage = MockStage(s.package.stage) s.package.do_install(keep_prefix=True) assert s.package.spec.installed - assert not s.package.stage.test_destroyed def test_second_install_no_overwrite_first(install_mockery, mock_fetch, monkeypatch): @@ -644,3 +610,48 @@ def test_empty_install_sanity_check_prefix( spec = Spec("failing-empty-install").concretized() with pytest.raises(spack.build_environment.ChildError, match="Nothing was installed"): spec.package.do_install() + + +def test_install_from_binary_with_missing_patch_succeeds( + temporary_store: spack.store.Store, mutable_config, tmp_path, mock_packages +): + """If a patch is missing in the local package repository, but was present when building and + pushing the package to a binary cache, installation from that binary cache shouldn't error out + because of the missing patch.""" + # Create a spec s with non-existing patches + s = Spec("trivial-install-test-package").concretized() + patches = ["a" * 64] + s_dict = s.to_dict() + s_dict["spec"]["nodes"][0]["patches"] = patches + s_dict["spec"]["nodes"][0]["parameters"]["patches"] = patches + s = Spec.from_dict(s_dict) + + # Create an install dir for it + os.makedirs(os.path.join(s.prefix, ".spack")) + with open(os.path.join(s.prefix, ".spack", "spec.json"), "w") as f: + s.to_json(f) + + # And register it in the database + temporary_store.db.add(s, directory_layout=temporary_store.layout, explicit=True) + + # Push it to a binary cache + build_cache = tmp_path / "my_build_cache" + binary_distribution.push_or_raise( + s, + build_cache.as_uri(), + binary_distribution.PushOptions(unsigned=True, regenerate_index=True), + ) + + # Now re-install it. + s.package.do_uninstall() + assert not temporary_store.db.query_local_by_spec_hash(s.dag_hash()) + + # Source install: fails, we don't have the patch. + with pytest.raises(spack.error.SpecError, match="Couldn't find patch for package"): + s.package.do_install() + + # Binary install: succeeds, we don't need the patch. + spack.mirror.add(spack.mirror.Mirror.from_local_path(str(build_cache))) + s.package.do_install(package_cache_only=True, dependencies_cache_only=True, unsigned=True) + + assert temporary_store.db.query_local_by_spec_hash(s.dag_hash())