installer.py: don't dereference stage before installing from binaries#41986
installer.py: don't dereference stage before installing from binaries#41986alalazo merged 6 commits intospack:developfrom
Conversation
| # 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 |
There was a problem hiding this comment.
This is redundant because restage is true by default, so we hit stage.destroy() in BuildProcessInstaller.run().
Necessary to remove, because this path is followed for binary installs too.
| 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: |
There was a problem hiding this comment.
This is moved into BuildProcessInstaller.run() so we don't dereference task.pkg.stage when doing binary installs
|
|
||
| s.package.do_install(restage=True) | ||
| assert rm_prefix_checker.removed | ||
| assert s.package.stage.test_destroyed |
There was a problem hiding this comment.
MockStage is copied to a sub-process and mutated there, so checking s.package.stage properties is completely broken.
I guess the intention of the test was to check whether a previous stage is destroyed before attempting a new build. However, this is broken too, since the stage is always destroyed on a successful install, so it would always pass.
With these changes, we're testing that an install after a partial install works (prefix exists, but it wasn't registered in the db), also it tests that the stage is deleted afterwards (which happens in all tests...). We're not testing whether the previous stage is destroyed before running a new install.
There was a problem hiding this comment.
(I passed through here while resolving a conflict with #41373, although I don't think there's any conflicts there, I found this comment confusing)
MockStage is copied to a sub-process and mutated there, so checking s.package.stage properties is completely broken.
Before this PR, the stage destruction operations we care about for this test were performed in prepare_for_install (in the parent process). So this PR created the situation you describe (and ideally resolves it). As far as fragility goes: subprocesses are not a major threat to default-false-and-search-for-something-that-set-it-true-style tests.
also it tests that the stage is deleted afterwards (which happens in all tests...).
Hopefully not for --keep-stage tests
We're not testing whether the previous stage is destroyed before running a new install.
That's what this check was intending to test, and what we'd still like to test - is this a current gap in our tests (i.e. to clarify, are you saying this never tested and still doesn't? or are you saying it didn't and now does?).
| 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 |
There was a problem hiding this comment.
Same here, this property can't be tested. If it's set to true, it would be in a sub-process and we would never know in the test.
haampie
force-pushed
the
fix/delay-stage
branch
from
cc8a8f0 to
9103a45
Compare
…ubprocess invisible to the parent process
haampie
force-pushed
the
fix/delay-stage
branch
from
9103a45 to
1ea0704
Compare
|
Have to check what |
haampie
force-pushed
the
fix/delay-stage
branch
from
bf7089b to
7818b98
Compare
haampie
force-pushed
the
fix/delay-stage
branch
from
7818b98 to
3171420
Compare
| # Create stage object now and let it be serialized for the child process. That | ||
| # way monkeypatch in tests works correctly. | ||
| pkg.stage |
There was a problem hiding this comment.
This screams for a later refactor (attribute access with side effects 😮)
There was a problem hiding this comment.
True, but it's effectively pkg.stage()
…#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.
…#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.
…#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.
…spack#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.
…spack#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.
…spack#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.
…spack#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.
…spack#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.
…spack#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.
…spack#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.
…spack#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.
3 participants
This fixes an issue where
pkg.stagethrows 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.