From 3969653f1b423a7e6a20329e61c27649caad8b3c Mon Sep 17 00:00:00 2001 From: Peter Scheibel Date: Fri, 29 Sep 2023 09:47:30 -0700 Subject: Cray manifest: compiler handling fixes (#40061) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Make use of `prefix` in the Cray manifest schema (prepend it to the relative CC etc.) - this was a Spack error. * Warn people when wrong-looking compilers are found in the manifest (i.e. non-existent CC path). * Bypass compilers that we fail to add (don't allow a single bad compiler to terminate the entire read-cray-manifest action). * Refactor Cray manifest tests: module-level variables have been replaced with fixtures, specifically using the `test_platform` fixture, which allows the unit tests to run with the new concretizer. * Add unit test to check case where adding a compiler raises an exception (check that this doesn't prevent processing the rest of the manifest). --- lib/spack/spack/compilers/aocc.py | 1 + lib/spack/spack/cray_manifest.py | 54 ++++++- lib/spack/spack/test/cmd/external.py | 13 -- lib/spack/spack/test/conftest.py | 11 -- lib/spack/spack/test/cray_manifest.py | 267 +++++++++++++++++++++------------- 5 files changed, 215 insertions(+), 131 deletions(-) diff --git a/lib/spack/spack/compilers/aocc.py b/lib/spack/spack/compilers/aocc.py index 51f7b02e2b..a642960b7d 100644 --- a/lib/spack/spack/compilers/aocc.py +++ b/lib/spack/spack/compilers/aocc.py @@ -112,6 +112,7 @@ class Aocc(Compiler): match = re.search(r"AOCC_(\d+)[._](\d+)[._](\d+)", output) if match: return ".".join(match.groups()) + return "unknown" @classmethod def fc_version(cls, fortran_compiler): diff --git a/lib/spack/spack/cray_manifest.py b/lib/spack/spack/cray_manifest.py index ac40191d0f..48ec52d782 100644 --- a/lib/spack/spack/cray_manifest.py +++ b/lib/spack/spack/cray_manifest.py @@ -4,6 +4,9 @@ # SPDX-License-Identifier: (Apache-2.0 OR MIT) import json +import os +import traceback +import warnings import jsonschema import jsonschema.exceptions @@ -46,9 +49,29 @@ def translated_compiler_name(manifest_compiler_name): ) -def compiler_from_entry(entry): +def compiler_from_entry(entry: dict, manifest_path: str): + # Note that manifest_path is only passed here to compose a + # useful warning message when paths appear to be missing. compiler_name = translated_compiler_name(entry["name"]) - paths = entry["executables"] + + if "prefix" in entry: + prefix = entry["prefix"] + paths = dict( + (lang, os.path.join(prefix, relpath)) + for (lang, relpath) in entry["executables"].items() + ) + else: + paths = entry["executables"] + + # Do a check for missing paths. Note that this isn't possible for + # all compiler entries, since their "paths" might actually be + # exe names like "cc" that depend on modules being loaded. Cray + # manifest entries are always paths though. + missing_paths = [] + for path in paths.values(): + if not os.path.exists(path): + missing_paths.append(path) + # to instantiate a compiler class we may need a concrete version: version = "={}".format(entry["version"]) arch = entry["arch"] @@ -57,8 +80,18 @@ def compiler_from_entry(entry): compiler_cls = spack.compilers.class_for_compiler_name(compiler_name) spec = spack.spec.CompilerSpec(compiler_cls.name, version) - paths = [paths.get(x, None) for x in ("cc", "cxx", "f77", "fc")] - return compiler_cls(spec, operating_system, target, paths) + path_list = [paths.get(x, None) for x in ("cc", "cxx", "f77", "fc")] + + if missing_paths: + warnings.warn( + "Manifest entry refers to nonexistent paths:\n\t" + + "\n\t".join(missing_paths) + + f"\nfor {str(spec)}" + + f"\nin {manifest_path}" + + "\nPlease report this issue" + ) + + return compiler_cls(spec, operating_system, target, path_list) def spec_from_entry(entry): @@ -187,12 +220,21 @@ def read(path, apply_updates): tty.debug("{0}: {1} specs read from manifest".format(path, str(len(specs)))) compilers = list() if "compilers" in json_data: - compilers.extend(compiler_from_entry(x) for x in json_data["compilers"]) + compilers.extend(compiler_from_entry(x, path) for x in json_data["compilers"]) tty.debug("{0}: {1} compilers read from manifest".format(path, str(len(compilers)))) # Filter out the compilers that already appear in the configuration compilers = spack.compilers.select_new_compilers(compilers) if apply_updates and compilers: - spack.compilers.add_compilers_to_config(compilers, init_config=False) + for compiler in compilers: + try: + spack.compilers.add_compilers_to_config([compiler], init_config=False) + except Exception: + warnings.warn( + f"Could not add compiler {str(compiler.spec)}: " + f"\n\tfrom manifest: {path}" + "\nPlease reexecute with 'spack -d' and include the stack trace" + ) + tty.debug(f"Include this\n{traceback.format_exc()}") if apply_updates: for spec in specs.values(): spack.store.STORE.db.add(spec, directory_layout=None) diff --git a/lib/spack/spack/test/cmd/external.py b/lib/spack/spack/test/cmd/external.py index 0b4eca124d..e94d6efe5c 100644 --- a/lib/spack/spack/test/cmd/external.py +++ b/lib/spack/spack/test/cmd/external.py @@ -203,19 +203,6 @@ def test_find_external_manifest_failure(mutable_config, mutable_mock_repo, tmpdi assert "Skipping manifest and continuing" in output -def test_find_external_nonempty_default_manifest_dir( - mutable_database, mutable_mock_repo, tmpdir, monkeypatch, directory_with_manifest -): - """The user runs 'spack external find'; the default manifest directory - contains a manifest file. Ensure that the specs are read. - """ - monkeypatch.setenv("PATH", "") - monkeypatch.setattr(spack.cray_manifest, "default_path", str(directory_with_manifest)) - external("find") - specs = spack.store.STORE.db.query("hwloc") - assert any(x.dag_hash() == "hwlocfakehashaaa" for x in specs) - - def test_find_external_merge(mutable_config, mutable_mock_repo): """Check that 'spack find external' doesn't overwrite an existing spec entry in packages.yaml. diff --git a/lib/spack/spack/test/conftest.py b/lib/spack/spack/test/conftest.py index 25417de6f4..c4b3df92ed 100644 --- a/lib/spack/spack/test/conftest.py +++ b/lib/spack/spack/test/conftest.py @@ -1714,17 +1714,6 @@ def brand_new_binary_cache(): ) -@pytest.fixture -def directory_with_manifest(tmpdir): - """Create a manifest file in a directory. Used by 'spack external'.""" - with tmpdir.as_cwd(): - test_db_fname = "external-db.json" - with open(test_db_fname, "w") as db_file: - json.dump(spack.test.cray_manifest.create_manifest_content(), db_file) - - yield str(tmpdir) - - @pytest.fixture() def noncyclical_dir_structure(tmpdir): """ diff --git a/lib/spack/spack/test/cray_manifest.py b/lib/spack/spack/test/cray_manifest.py index f9e7ae8594..123e2ac3f1 100644 --- a/lib/spack/spack/test/cray_manifest.py +++ b/lib/spack/spack/test/cray_manifest.py @@ -23,53 +23,6 @@ import spack.spec import spack.store from spack.cray_manifest import compiler_from_entry, entries_to_specs -example_x_json_str = """\ -{ - "name": "packagex", - "hash": "hash-of-x", - "prefix": "/path/to/packagex-install/", - "version": "1.0", - "arch": { - "platform": "linux", - "platform_os": "centos8", - "target": { - "name": "haswell" - } - }, - "compiler": { - "name": "gcc", - "version": "10.2.0.cray" - }, - "dependencies": { - "packagey": { - "hash": "hash-of-y", - "type": ["link"] - } - }, - "parameters": { - "precision": ["double", "float"] - } -} -""" - - -example_compiler_entry = """\ -{ - "name": "gcc", - "prefix": "/path/to/compiler/", - "version": "7.5.0", - "arch": { - "os": "centos8", - "target": "x86_64" - }, - "executables": { - "cc": "/path/to/compiler/cc", - "cxx": "/path/to/compiler/cxx", - "fc": "/path/to/compiler/fc" - } -} -""" - class JsonSpecEntry: def __init__(self, name, hash, prefix, version, arch, compiler, dependencies, parameters): @@ -104,16 +57,19 @@ class JsonArchEntry: self.os = os self.target = target - def to_dict(self): + def spec_json(self): return {"platform": self.platform, "platform_os": self.os, "target": {"name": self.target}} + def compiler_json(self): + return {"os": self.os, "target": self.target} + class JsonCompilerEntry: def __init__(self, name, version, arch=None, executables=None): self.name = name self.version = version if not arch: - arch = {"os": "centos8", "target": "x86_64"} + arch = JsonArchEntry("anyplatform", "anyos", "anytarget") if not executables: executables = { "cc": "/path/to/compiler/cc", @@ -127,7 +83,7 @@ class JsonCompilerEntry: return { "name": self.name, "version": self.version, - "arch": self.arch, + "arch": self.arch.compiler_json(), "executables": self.executables, } @@ -138,22 +94,58 @@ class JsonCompilerEntry: return {"name": self.name, "version": self.version} -_common_arch = JsonArchEntry(platform="linux", os="centos8", target="haswell").to_dict() +@pytest.fixture +def _common_arch(test_platform): + return JsonArchEntry( + platform=test_platform.name, + os=test_platform.front_os, + target=test_platform.target("fe").name, + ) + + +@pytest.fixture +def _common_compiler(_common_arch): + return JsonCompilerEntry( + name="gcc", + version="10.2.0.2112", + arch=_common_arch, + executables={ + "cc": "/path/to/compiler/cc", + "cxx": "/path/to/compiler/cxx", + "fc": "/path/to/compiler/fc", + }, + ) + + +@pytest.fixture +def _other_compiler(_common_arch): + return JsonCompilerEntry( + name="clang", + version="3.0.0", + arch=_common_arch, + executables={ + "cc": "/path/to/compiler/clang", + "cxx": "/path/to/compiler/clang++", + "fc": "/path/to/compiler/flang", + }, + ) + -# Intended to match example_compiler_entry above -_common_compiler = JsonCompilerEntry( - name="gcc", - version="10.2.0.cray", - arch={"os": "centos8", "target": "x86_64"}, - executables={ - "cc": "/path/to/compiler/cc", - "cxx": "/path/to/compiler/cxx", - "fc": "/path/to/compiler/fc", - }, -) +@pytest.fixture +def _raw_json_x(_common_arch): + return { + "name": "packagex", + "hash": "hash-of-x", + "prefix": "/path/to/packagex-install/", + "version": "1.0", + "arch": _common_arch.spec_json(), + "compiler": {"name": "gcc", "version": "10.2.0.2112"}, + "dependencies": {"packagey": {"hash": "hash-of-y", "type": ["link"]}}, + "parameters": {"precision": ["double", "float"]}, + } -def test_compatibility(): +def test_manifest_compatibility(_common_arch, _common_compiler, _raw_json_x): """Make sure that JsonSpecEntry outputs the expected JSON structure by comparing it with JSON parsed from an example string. This ensures that the testing objects like JsonSpecEntry produce the @@ -164,7 +156,7 @@ def test_compatibility(): hash="hash-of-y", prefix="/path/to/packagey-install/", version="1.0", - arch=_common_arch, + arch=_common_arch.spec_json(), compiler=_common_compiler.spec_json(), dependencies={}, parameters={}, @@ -175,23 +167,44 @@ def test_compatibility(): hash="hash-of-x", prefix="/path/to/packagex-install/", version="1.0", - arch=_common_arch, + arch=_common_arch.spec_json(), compiler=_common_compiler.spec_json(), dependencies=dict([y.as_dependency(deptypes=["link"])]), parameters={"precision": ["double", "float"]}, ) x_from_entry = x.to_dict() - x_from_str = json.loads(example_x_json_str) - assert x_from_entry == x_from_str + assert x_from_entry == _raw_json_x def test_compiler_from_entry(): - compiler_data = json.loads(example_compiler_entry) - compiler_from_entry(compiler_data) + compiler_data = json.loads( + """\ +{ + "name": "gcc", + "prefix": "/path/to/compiler/", + "version": "7.5.0", + "arch": { + "os": "centos8", + "target": "x86_64" + }, + "executables": { + "cc": "/path/to/compiler/cc", + "cxx": "/path/to/compiler/cxx", + "fc": "/path/to/compiler/fc" + } +} +""" + ) + compiler = compiler_from_entry(compiler_data, "/example/file") + assert compiler.cc == "/path/to/compiler/cc" + assert compiler.cxx == "/path/to/compiler/cxx" + assert compiler.fc == "/path/to/compiler/fc" + assert compiler.operating_system == "centos8" -def generate_openmpi_entries(): +@pytest.fixture +def generate_openmpi_entries(_common_arch, _common_compiler): """Generate two example JSON entries that refer to an OpenMPI installation and a hwloc dependency. """ @@ -202,7 +215,7 @@ def generate_openmpi_entries(): hash="hwlocfakehashaaa", prefix="/path/to/hwloc-install/", version="2.0.3", - arch=_common_arch, + arch=_common_arch.spec_json(), compiler=_common_compiler.spec_json(), dependencies={}, parameters={}, @@ -216,26 +229,25 @@ def generate_openmpi_entries(): hash="openmpifakehasha", prefix="/path/to/openmpi-install/", version="4.1.0", - arch=_common_arch, + arch=_common_arch.spec_json(), compiler=_common_compiler.spec_json(), dependencies=dict([hwloc.as_dependency(deptypes=["link"])]), parameters={"internal-hwloc": False, "fabrics": ["psm"], "missing_variant": True}, ) - return [openmpi, hwloc] + return list(x.to_dict() for x in [openmpi, hwloc]) -def test_generate_specs_from_manifest(): +def test_generate_specs_from_manifest(generate_openmpi_entries): """Given JSON entries, check that we can form a set of Specs including dependency references. """ - entries = list(x.to_dict() for x in generate_openmpi_entries()) - specs = entries_to_specs(entries) + specs = entries_to_specs(generate_openmpi_entries) (openmpi_spec,) = list(x for x in specs.values() if x.name == "openmpi") assert openmpi_spec["hwloc"] -def test_translate_cray_platform_to_linux(monkeypatch): +def test_translate_cray_platform_to_linux(monkeypatch, _common_compiler): """Manifests might list specs on newer Cray platforms as being "cray", but Spack identifies such platforms as "linux". Make sure we automaticaly transform these entries. @@ -247,13 +259,13 @@ def test_translate_cray_platform_to_linux(monkeypatch): monkeypatch.setattr(spack.platforms, "host", the_host_is_linux) - cray_arch = JsonArchEntry(platform="cray", os="rhel8", target="x86_64").to_dict() + cray_arch = JsonArchEntry(platform="cray", os="rhel8", target="x86_64") spec_json = JsonSpecEntry( name="cray-mpich", hash="craympichfakehashaaa", prefix="/path/to/cray-mpich/", version="1.0.0", - arch=cray_arch, + arch=cray_arch.spec_json(), compiler=_common_compiler.spec_json(), dependencies={}, parameters={}, @@ -263,14 +275,15 @@ def test_translate_cray_platform_to_linux(monkeypatch): assert spec.architecture.platform == "linux" -def test_translate_compiler_name(): +def test_translate_compiler_name(_common_arch): nvidia_compiler = JsonCompilerEntry( name="nvidia", version="19.1", + arch=_common_arch, executables={"cc": "/path/to/compiler/nvc", "cxx": "/path/to/compiler/nvc++"}, ) - compiler = compiler_from_entry(nvidia_compiler.compiler_json()) + compiler = compiler_from_entry(nvidia_compiler.compiler_json(), "/example/file") assert compiler.name == "nvhpc" spec_json = JsonSpecEntry( @@ -278,7 +291,7 @@ def test_translate_compiler_name(): hash="hwlocfakehashaaa", prefix="/path/to/hwloc-install/", version="2.0.3", - arch=_common_arch, + arch=_common_arch.spec_json(), compiler=nvidia_compiler.spec_json(), dependencies={}, parameters={}, @@ -288,18 +301,18 @@ def test_translate_compiler_name(): assert spec.compiler.name == "nvhpc" -def test_failed_translate_compiler_name(): +def test_failed_translate_compiler_name(_common_arch): unknown_compiler = JsonCompilerEntry(name="unknown", version="1.0") with pytest.raises(spack.compilers.UnknownCompilerError): - compiler_from_entry(unknown_compiler.compiler_json()) + compiler_from_entry(unknown_compiler.compiler_json(), "/example/file") spec_json = JsonSpecEntry( name="packagey", hash="hash-of-y", prefix="/path/to/packagey-install/", version="1.0", - arch=_common_arch, + arch=_common_arch.spec_json(), compiler=unknown_compiler.spec_json(), dependencies={}, parameters={}, @@ -309,7 +322,8 @@ def test_failed_translate_compiler_name(): entries_to_specs([spec_json]) -def create_manifest_content(): +@pytest.fixture +def manifest_content(generate_openmpi_entries, _common_compiler, _other_compiler): return { # Note: the cray_manifest module doesn't use the _meta section right # now, but it is anticipated to be useful @@ -319,43 +333,70 @@ def create_manifest_content(): "schema-version": "1.3", "cpe-version": "22.06", }, - "specs": list(x.to_dict() for x in generate_openmpi_entries()), - "compilers": [_common_compiler.compiler_json()], + "specs": generate_openmpi_entries, + "compilers": [_common_compiler.compiler_json(), _other_compiler.compiler_json()], } -@pytest.mark.only_original( - "The ASP-based concretizer is currently picky about OS matching and will fail." -) -def test_read_cray_manifest(tmpdir, mutable_config, mock_packages, mutable_database): +def test_read_cray_manifest( + tmpdir, mutable_config, mock_packages, mutable_database, manifest_content +): """Check that (a) we can read the cray manifest and add it to the Spack Database and (b) we can concretize specs based on that. """ with tmpdir.as_cwd(): test_db_fname = "external-db.json" with open(test_db_fname, "w") as db_file: - json.dump(create_manifest_content(), db_file) + json.dump(manifest_content, db_file) cray_manifest.read(test_db_fname, True) query_specs = spack.store.STORE.db.query("openmpi") assert any(x.dag_hash() == "openmpifakehasha" for x in query_specs) concretized_specs = spack.cmd.parse_specs( - "depends-on-openmpi %gcc@4.5.0 arch=test-redhat6-x86_64" " ^/openmpifakehasha".split(), - concretize=True, + "depends-on-openmpi ^/openmpifakehasha".split(), concretize=True ) assert concretized_specs[0]["hwloc"].dag_hash() == "hwlocfakehashaaa" -@pytest.mark.only_original( - "The ASP-based concretizer is currently picky about OS matching and will fail." -) +def test_read_cray_manifest_add_compiler_failure( + tmpdir, mutable_config, mock_packages, mutable_database, manifest_content, monkeypatch +): + """Check that cray manifest can be read even if some compilers cannot + be added. + """ + orig_add_compilers_to_config = spack.compilers.add_compilers_to_config + + class fail_for_clang: + def __init__(self): + self.called_with_clang = False + + def __call__(self, compilers, **kwargs): + if any(x.name == "clang" for x in compilers): + self.called_with_clang = True + raise Exception() + return orig_add_compilers_to_config(compilers, **kwargs) + + checker = fail_for_clang() + monkeypatch.setattr(spack.compilers, "add_compilers_to_config", checker) + + with tmpdir.as_cwd(): + test_db_fname = "external-db.json" + with open(test_db_fname, "w") as db_file: + json.dump(manifest_content, db_file) + cray_manifest.read(test_db_fname, True) + query_specs = spack.store.STORE.db.query("openmpi") + assert any(x.dag_hash() == "openmpifakehasha" for x in query_specs) + + assert checker.called_with_clang + + def test_read_cray_manifest_twice_no_compiler_duplicates( - tmpdir, mutable_config, mock_packages, mutable_database + tmpdir, mutable_config, mock_packages, mutable_database, manifest_content ): with tmpdir.as_cwd(): test_db_fname = "external-db.json" with open(test_db_fname, "w") as db_file: - json.dump(create_manifest_content(), db_file) + json.dump(manifest_content, db_file) # Read the manifest twice cray_manifest.read(test_db_fname, True) @@ -363,7 +404,7 @@ def test_read_cray_manifest_twice_no_compiler_duplicates( compilers = spack.compilers.all_compilers() filtered = list( - c for c in compilers if c.spec == spack.spec.CompilerSpec("gcc@=10.2.0.cray") + c for c in compilers if c.spec == spack.spec.CompilerSpec("gcc@=10.2.0.2112") ) assert len(filtered) == 1 @@ -423,3 +464,27 @@ def test_convert_validation_error(tmpdir, mutable_config, mock_packages, mutable with pytest.raises(cray_manifest.ManifestValidationError) as e: cray_manifest.read(invalid_schema_path, True) str(e) + + +@pytest.fixture +def directory_with_manifest(tmpdir, manifest_content): + """Create a manifest file in a directory. Used by 'spack external'.""" + with tmpdir.as_cwd(): + test_db_fname = "external-db.json" + with open(test_db_fname, "w") as db_file: + json.dump(manifest_content, db_file) + + yield str(tmpdir) + + +def test_find_external_nonempty_default_manifest_dir( + mutable_database, mutable_mock_repo, tmpdir, monkeypatch, directory_with_manifest +): + """The user runs 'spack external find'; the default manifest directory + contains a manifest file. Ensure that the specs are read. + """ + monkeypatch.setenv("PATH", "") + monkeypatch.setattr(spack.cray_manifest, "default_path", str(directory_with_manifest)) + spack.cmd.external._collect_and_consume_cray_manifest_files(ignore_default_dir=False) + specs = spack.store.STORE.db.query("hwloc") + assert any(x.dag_hash() == "hwlocfakehashaaa" for x in specs) -- cgit v1.2.3-70-g09d2