From b369ff461a9a7c175632d12fe2b05e1d2a6e3a70 Mon Sep 17 00:00:00 2001 From: Danny McClanahan <1305167+cosmicexplorer@users.noreply.github.com> Date: Wed, 2 Jun 2021 09:56:51 -0700 Subject: ensure the staging dir exists for `spack stage -p ` (#23963) * ensure that the stage root exists for `spack stage -p ` * add test to verify `spack stage -p ` works! * move out shared tmp staging path setup to a fixture to fix the test --- lib/spack/spack/cmd/stage.py | 15 ++++++++++++--- lib/spack/spack/stage.py | 14 ++++++++++++-- lib/spack/spack/test/cmd/stage.py | 24 ++++++++++++++---------- lib/spack/spack/test/stage.py | 10 +++++----- 4 files changed, 43 insertions(+), 20 deletions(-) diff --git a/lib/spack/spack/cmd/stage.py b/lib/spack/spack/cmd/stage.py index 639f32f28a..bd3b984606 100644 --- a/lib/spack/spack/cmd/stage.py +++ b/lib/spack/spack/cmd/stage.py @@ -3,12 +3,15 @@ # # SPDX-License-Identifier: (Apache-2.0 OR MIT) +import os + import llnl.util.tty as tty import spack.environment as ev import spack.repo import spack.cmd import spack.cmd.common.arguments as arguments +import spack.stage description = "expand downloaded archive in preparation for install" section = "build" @@ -24,6 +27,12 @@ def setup_parser(subparser): def stage(parser, args): + # We temporarily modify the working directory when setting up a stage, so we need to + # convert this to an absolute path here in order for it to remain valid later. + custom_path = os.path.abspath(args.path) if args.path else None + if custom_path: + spack.stage.create_stage_root(custom_path) + if not args.specs: env = ev.get_env(args, 'stage') if env: @@ -44,12 +53,12 @@ def stage(parser, args): specs = spack.cmd.parse_specs(args.specs, concretize=False) # prevent multiple specs from extracting in the same folder - if len(specs) > 1 and args.path: + if len(specs) > 1 and custom_path: tty.die("`--path` requires a single spec, but multiple were provided") for spec in specs: spec = spack.cmd.matching_spec_from_env(spec) package = spack.repo.get(spec) - if args.path: - package.path = args.path + if custom_path: + package.path = custom_path package.do_stage() diff --git a/lib/spack/spack/stage.py b/lib/spack/spack/stage.py index c39b4d2f33..1bcf644eb5 100644 --- a/lib/spack/spack/stage.py +++ b/lib/spack/spack/stage.py @@ -44,7 +44,8 @@ _source_path_subdir = 'spack-src' stage_prefix = 'spack-stage-' -def _create_stage_root(path): +def create_stage_root(path): + # type: (str) -> None """Create the stage root directory and ensure appropriate access perms.""" assert path.startswith(os.path.sep) and len(path.strip()) > 1 @@ -99,6 +100,15 @@ def _create_stage_root(path): tty.warn("Expected user {0} to own {1}, but it is owned by {2}" .format(user_uid, p, p_stat.st_uid)) + spack_src_subdir = os.path.join(path, _source_path_subdir) + # When staging into a user-specified directory with `spack stage -p `, we need + # to ensure the `spack-src` subdirectory exists, as we can't rely on it being + # created automatically by spack. It's not clear why this is the case for `spack + # stage -p`, but since `mkdirp()` is idempotent, this should not change the behavior + # for any other code paths. + if not os.path.isdir(spack_src_subdir): + mkdirp(spack_src_subdir, mode=stat.S_IRWXU) + def _first_accessible_path(paths): """Find the first path that is accessible, creating it if necessary.""" @@ -110,7 +120,7 @@ def _first_accessible_path(paths): return path else: # Now create the stage root with the proper group/perms. - _create_stage_root(path) + create_stage_root(path) return path except OSError as e: diff --git a/lib/spack/spack/test/cmd/stage.py b/lib/spack/spack/test/cmd/stage.py index 481694dc6e..28ffc71451 100644 --- a/lib/spack/spack/test/cmd/stage.py +++ b/lib/spack/spack/test/cmd/stage.py @@ -3,6 +3,7 @@ # # SPDX-License-Identifier: (Apache-2.0 OR MIT) +import os import pytest from spack.main import SpackCommand import spack.environment as ev @@ -31,27 +32,30 @@ def test_stage_spec(monkeypatch): assert len(expected) == 0 -def test_stage_path(monkeypatch): - """Verify that --path only works with single specs.""" +@pytest.fixture(scope='function') +def check_stage_path(monkeypatch, tmpdir): + expected_path = os.path.join(str(tmpdir), 'x') def fake_stage(pkg, mirror_only=False): - assert pkg.path == 'x' + assert pkg.path == expected_path + assert os.path.isdir(expected_path), expected_path monkeypatch.setattr(spack.package.PackageBase, 'do_stage', fake_stage) - stage('--path=x', 'trivial-install-test-package') + return expected_path -def test_stage_path_errors_multiple_specs(monkeypatch): +def test_stage_path(check_stage_path): """Verify that --path only works with single specs.""" + stage('--path={0}'.format(check_stage_path), 'trivial-install-test-package') - def fake_stage(pkg, mirror_only=False): - pass - - monkeypatch.setattr(spack.package.PackageBase, 'do_stage', fake_stage) +def test_stage_path_errors_multiple_specs(check_stage_path): + """Verify that --path only works with single specs.""" with pytest.raises(spack.main.SpackCommandError): - stage('--path=x', 'trivial-install-test-package', 'mpileaks') + stage('--path={0}'.format(check_stage_path), + 'trivial-install-test-package', + 'mpileaks') def test_stage_with_env_outside_env(mutable_mock_env_path, monkeypatch): diff --git a/lib/spack/spack/test/stage.py b/lib/spack/spack/test/stage.py index 3e3ceeaaa0..3ebfcdf407 100644 --- a/lib/spack/spack/test/stage.py +++ b/lib/spack/spack/test/stage.py @@ -691,14 +691,14 @@ class TestStage(object): shutil.rmtree(str(name)) def test_create_stage_root(self, tmpdir, no_path_access): - """Test _create_stage_root permissions.""" + """Test create_stage_root permissions.""" test_dir = tmpdir.join('path') test_path = str(test_dir) try: if getpass.getuser() in str(test_path).split(os.sep): # Simply ensure directory created if tmpdir includes user - spack.stage._create_stage_root(test_path) + spack.stage.create_stage_root(test_path) assert os.path.exists(test_path) p_stat = os.stat(test_path) @@ -706,7 +706,7 @@ class TestStage(object): else: # Ensure an OS Error is raised on created, non-user directory with pytest.raises(OSError) as exc_info: - spack.stage._create_stage_root(test_path) + spack.stage.create_stage_root(test_path) assert exc_info.value.errno == errno.EACCES finally: @@ -748,10 +748,10 @@ class TestStage(object): # # with monkeypatch.context() as m: # m.setattr(os, 'stat', _stat) - # spack.stage._create_stage_root(user_path) + # spack.stage.create_stage_root(user_path) # assert os.stat(user_path).st_uid != os.getuid() monkeypatch.setattr(os, 'stat', _stat) - spack.stage._create_stage_root(user_path) + spack.stage.create_stage_root(user_path) # The following check depends on the patched os.stat as a poor # substitute for confirming the generated warnings. -- cgit v1.2.3-70-g09d2