From f7ef064bdc91a07f7e0f3f784a6a1856f1be9f41 Mon Sep 17 00:00:00 2001 From: Peter Scheibel Date: Mon, 27 Jun 2022 12:26:12 -0700 Subject: `spack external find`: handle manifest with bad permissions (#31201) Allow `spack external find` (with no extra args) to proceed if the manifest file exists but without sufficient permissions; in that case, print a warning. Also add a test for that behavior. TODOs: - [x] continue past any exception raised during manifest parsing as part of `spack external find`, except for CTRL-C (and other errors that indicate immediate program termination) - [x] Semi-unrelated but came up when discussing this with the user who reported this issue to me: the manifest parser now accepts older schemas See: https://github.com/spack/spack/issues/31191 --- lib/spack/spack/cmd/external.py | 16 +++++++++++ lib/spack/spack/schema/cray_manifest.py | 3 ++ lib/spack/spack/test/cmd/external.py | 49 +++++++++++++++++++++++++++++++++ lib/spack/spack/test/cray_manifest.py | 30 ++++++++++++++++++++ 4 files changed, 98 insertions(+) diff --git a/lib/spack/spack/cmd/external.py b/lib/spack/spack/cmd/external.py index e08bd13876..20f4744a1e 100644 --- a/lib/spack/spack/cmd/external.py +++ b/lib/spack/spack/cmd/external.py @@ -5,6 +5,7 @@ from __future__ import print_function import argparse +import errno import os import sys @@ -93,6 +94,21 @@ def external_find(args): # It's fine to not find any manifest file if we are doing the # search implicitly (i.e. as part of 'spack external find') pass + except Exception as e: + # For most exceptions, just print a warning and continue. + # Note that KeyboardInterrupt does not subclass Exception + # (so CTRL-C will terminate the program as expected). + skip_msg = ("Skipping manifest and continuing with other external " + "checks") + if ((isinstance(e, IOError) or isinstance(e, OSError)) and + e.errno in [errno.EPERM, errno.EACCES]): + # The manifest file does not have sufficient permissions enabled: + # print a warning and keep going + tty.warn("Unable to read manifest due to insufficient " + "permissions.", skip_msg) + else: + tty.warn("Unable to read manifest, unexpected error: {0}" + .format(str(e)), skip_msg) # If the user didn't specify anything, search for build tools by default if not args.tags and not args.all and not args.packages: diff --git a/lib/spack/spack/schema/cray_manifest.py b/lib/spack/spack/schema/cray_manifest.py index 9613bbf1dc..89619a6aaa 100644 --- a/lib/spack/spack/schema/cray_manifest.py +++ b/lib/spack/spack/schema/cray_manifest.py @@ -26,6 +26,9 @@ schema = { "cpe-version": {"type": "string", "minLength": 1}, "system-type": {"type": "string", "minLength": 1}, "schema-version": {"type": "string", "minLength": 1}, + # Older schemas use did not have "cpe-version", just the + # schema version; in that case it was just called "version" + "version": {"type": "string", "minLength": 1}, } }, "compilers": { diff --git a/lib/spack/spack/test/cmd/external.py b/lib/spack/spack/test/cmd/external.py index abcdec680a..0074bee2a9 100644 --- a/lib/spack/spack/test/cmd/external.py +++ b/lib/spack/spack/test/cmd/external.py @@ -8,6 +8,8 @@ import sys import pytest +from llnl.util.filesystem import getuid, touch + import spack import spack.detection import spack.detection.path @@ -194,6 +196,53 @@ def test_find_external_empty_default_manifest_dir( external('find') +@pytest.mark.skipif(sys.platform == 'win32', + reason="Can't chmod on Windows") +@pytest.mark.skipif(getuid() == 0, reason='user is root') +def test_find_external_manifest_with_bad_permissions( + mutable_config, working_env, mock_executable, mutable_mock_repo, + _platform_executables, tmpdir, monkeypatch): + """The user runs 'spack external find'; the default path for storing + manifest files exists but with insufficient permissions. Check that + the command does not fail. + """ + test_manifest_dir = str(tmpdir.mkdir('manifest_dir')) + test_manifest_file_path = os.path.join(test_manifest_dir, 'badperms.json') + touch(test_manifest_file_path) + monkeypatch.setenv('PATH', '') + monkeypatch.setattr(spack.cray_manifest, 'default_path', + test_manifest_dir) + try: + os.chmod(test_manifest_file_path, 0) + output = external('find') + assert 'insufficient permissions' in output + assert 'Skipping manifest and continuing' in output + finally: + os.chmod(test_manifest_file_path, 0o700) + + +def test_find_external_manifest_failure( + mutable_config, mutable_mock_repo, tmpdir, monkeypatch): + """The user runs 'spack external find'; the manifest parsing fails with + some exception. Ensure that the command still succeeds (i.e. moves on + to other external detection mechanisms). + """ + # First, create an empty manifest file (without a file to read, the + # manifest parsing is skipped) + test_manifest_dir = str(tmpdir.mkdir('manifest_dir')) + test_manifest_file_path = os.path.join(test_manifest_dir, 'test.json') + touch(test_manifest_file_path) + + def fail(): + raise Exception() + + monkeypatch.setattr( + spack.cmd.external, '_collect_and_consume_cray_manifest_files', fail) + monkeypatch.setenv('PATH', '') + output = external('find') + assert 'Skipping manifest and continuing' in output + + def test_find_external_nonempty_default_manifest_dir( mutable_database, mutable_mock_repo, _platform_executables, tmpdir, monkeypatch, diff --git a/lib/spack/spack/test/cray_manifest.py b/lib/spack/spack/test/cray_manifest.py index 97bd7e1d14..6a92848539 100644 --- a/lib/spack/spack/test/cray_manifest.py +++ b/lib/spack/spack/test/cray_manifest.py @@ -10,6 +10,7 @@ establish dependency relationships (and in general the manifest-parsing logic needs to consume all related specs in a single pass). """ import json +import os import pytest @@ -309,6 +310,14 @@ def test_failed_translate_compiler_name(): def create_manifest_content(): return { + # Note: the cray_manifest module doesn't use the _meta section right + # now, but it is anticipated to be useful + '_meta': { + "file-type": "cray-pe-json", + "system-type": "test", + "schema-version": "1.3", + "cpe-version": "22.06" + }, 'specs': list(x.to_dict() for x in generate_openmpi_entries()), 'compilers': [] } @@ -336,3 +345,24 @@ def test_read_cray_manifest( ' ^/openmpifakehasha'.split(), concretize=True) assert concretized_specs[0]['hwloc'].dag_hash() == 'hwlocfakehashaaa' + + +def test_read_old_manifest_v1_2( + tmpdir, mutable_config, mock_packages, mutable_database): + """Test reading a file using the older format + ('version' instead of 'schema-version'). + """ + manifest_dir = str(tmpdir.mkdir('manifest_dir')) + manifest_file_path = os.path.join(manifest_dir, 'test.json') + with open(manifest_file_path, 'w') as manifest_file: + manifest_file.write("""\ +{ + "_meta": { + "file-type": "cray-pe-json", + "system-type": "EX", + "version": "1.3" + }, + "specs": [] +} +""") + cray_manifest.read(manifest_file_path, True) -- cgit v1.2.3-60-g2f50