`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

4 files changed, 98 insertions, 0 deletions

diff --git a/lib/spack/spack/cmd/external.py b/lib/spack/spack/cmd/external.py
index e9060d5d17..ad475e54f5 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)