From 336191c25145823be104011fda23651e6e6aa723 Mon Sep 17 00:00:00 2001 From: Greg Becker Date: Tue, 31 Mar 2020 16:09:08 -0700 Subject: packages.yaml: allow virtuals to specify buildable: false (#14934) Currently, to force Spack to use an external MPI, you have to specify `buildable: False` for every MPI provider in Spack in your packages.yaml file. This is both tedious and fragile, as new MPI providers can be added and break your workflow when you do a git pull. This PR allows you to specify an entire virtual dependency as non-buildable, and specify particular implementations to be built: ``` packages: all: providers: mpi: [mpich] mpi: buildable: false paths: mpich@3.2 %gcc@7.3.0: /usr/packages/mpich-3.2-gcc-7.3.0 ``` will force all Spack builds to use the specified `mpich` install. --- lib/spack/docs/build_settings.rst | 33 ++++++++++ lib/spack/spack/package.py | 8 +++ lib/spack/spack/package_prefs.py | 78 ++++++++-------------- lib/spack/spack/test/concretize_preferences.py | 51 ++++++++------- lib/spack/spack/test/conftest.py | 7 ++ lib/spack/spack/test/mirror.py | 90 +++++++++++++------------- lib/spack/spack/test/module_parsing.py | 30 ++------- lib/spack/spack/test/spec_dag.py | 2 - lib/spack/spack/util/module_cmd.py | 9 +-- 9 files changed, 155 insertions(+), 153 deletions(-) diff --git a/lib/spack/docs/build_settings.rst b/lib/spack/docs/build_settings.rst index b141f2b717..cfd850af28 100644 --- a/lib/spack/docs/build_settings.rst +++ b/lib/spack/docs/build_settings.rst @@ -124,6 +124,39 @@ The ``buildable`` does not need to be paired with external packages. It could also be used alone to forbid packages that may be buggy or otherwise undesirable. +Virtual packages in Spack can also be specified as not buildable, and +external implementations can be provided. In the example above, +OpenMPI is configured as not buildable, but Spack will often prefer +other MPI implementations over the externally available OpenMPI. Spack +can be configured with every MPI provider not buildable individually, +but more conveniently: + +.. code-block:: yaml + + packages: + mpi: + buildable: False + openmpi: + paths: + openmpi@1.4.3%gcc@4.4.7 arch=linux-debian7-x86_64: /opt/openmpi-1.4.3 + openmpi@1.4.3%gcc@4.4.7 arch=linux-debian7-x86_64+debug: /opt/openmpi-1.4.3-debug + openmpi@1.6.5%intel@10.1 arch=linux-debian7-x86_64: /opt/openmpi-1.6.5-intel + +Implementations can also be listed immediately under the virtual they provide: + +.. code-block:: yaml + + packages: + mpi: + buildable: False + openmpi@1.4.3%gcc@4.4.7 arch=linux-debian7-x86_64: /opt/openmpi-1.4.3 + openmpi@1.4.3%gcc@4.4.7 arch=linux-debian7-x86_64+debug: /opt/openmpi-1.4.3-debug + openmpi@1.6.5%intel@10.1 arch=linux-debian7-x86_64: /opt/openmpi-1.6.5-intel + mpich@3.3 %clang@9.0.0 arch=linux-debian7-x86_64: /opt/mpich-3.3-intel + +Spack can then use any of the listed external implementations of MPI +to satisfy a dependency, and will choose depending on the compiler and +architecture. .. _concretization-preferences: diff --git a/lib/spack/spack/package.py b/lib/spack/spack/package.py index 9156d7d0dd..b8ded0364b 100644 --- a/lib/spack/spack/package.py +++ b/lib/spack/spack/package.py @@ -1035,6 +1035,14 @@ class PackageBase(with_metaclass(PackageMeta, PackageViewMixin, object)): for s, constraints in self.provided.items() if s.name == vpkg_name ) + @property + def virtuals_provided(self): + """ + virtual packages provided by this package with its spec + """ + return [vspec for vspec, constraints in self.provided.items() + if any(self.spec.satisfies(c) for c in constraints)] + @property def installed(self): """Installation status of a package. diff --git a/lib/spack/spack/package_prefs.py b/lib/spack/spack/package_prefs.py index 2801a6d123..0158c7063a 100644 --- a/lib/spack/spack/package_prefs.py +++ b/lib/spack/spack/package_prefs.py @@ -6,7 +6,6 @@ import stat from six import string_types -from six import iteritems import spack.repo import spack.error @@ -23,27 +22,6 @@ def _spec_type(component): return _lesser_spec_types.get(component, spack.spec.Spec) -def get_packages_config(): - """Wrapper around get_packages_config() to validate semantics.""" - config = spack.config.get('packages') - - # Get a list of virtuals from packages.yaml. Note that because we - # check spack.repo, this collects virtuals that are actually provided - # by sometihng, not just packages/names that don't exist. - # So, this won't include, e.g., 'all'. - virtuals = [(pkg_name, pkg_name._start_mark) for pkg_name in config - if spack.repo.path.is_virtual(pkg_name)] - - # die if there are virtuals in `packages.py` - if virtuals: - errors = ["%s: %s" % (line_info, name) for name, line_info in virtuals] - raise VirtualInPackagesYAMLError( - "packages.yaml entries cannot be virtual packages:", - '\n'.join(errors)) - - return config - - class PackagePrefs(object): """Defines the sort order for a set of specs. @@ -116,7 +94,7 @@ class PackagePrefs(object): pkglist.append('all') for pkg in pkglist: - pkg_entry = get_packages_config().get(pkg) + pkg_entry = spack.config.get('packages').get(pkg) if not pkg_entry: continue @@ -160,7 +138,8 @@ class PackagePrefs(object): def preferred_variants(cls, pkg_name): """Return a VariantMap of preferred variants/values for a spec.""" for pkg in (pkg_name, 'all'): - variants = get_packages_config().get(pkg, {}).get('variants', '') + variants = spack.config.get('packages').get(pkg, {}).get( + 'variants', '') if variants: break @@ -181,33 +160,29 @@ def spec_externals(spec): # break circular import. from spack.util.module_cmd import get_path_from_module # NOQA: ignore=F401 - allpkgs = get_packages_config() - name = spec.name + allpkgs = spack.config.get('packages') + names = set([spec.name]) + names |= set(vspec.name for vspec in spec.package.virtuals_provided) external_specs = [] - pkg_paths = allpkgs.get(name, {}).get('paths', None) - pkg_modules = allpkgs.get(name, {}).get('modules', None) - if (not pkg_paths) and (not pkg_modules): - return [] - - for external_spec, path in iteritems(pkg_paths): - if not path: - # skip entries without paths (avoid creating extra Specs) + for name in names: + pkg_config = allpkgs.get(name, {}) + pkg_paths = pkg_config.get('paths', {}) + pkg_modules = pkg_config.get('modules', {}) + if (not pkg_paths) and (not pkg_modules): continue - external_spec = spack.spec.Spec(external_spec, - external_path=canonicalize_path(path)) - if external_spec.satisfies(spec): - external_specs.append(external_spec) - - for external_spec, module in iteritems(pkg_modules): - if not module: - continue + for external_spec, path in pkg_paths.items(): + external_spec = spack.spec.Spec( + external_spec, external_path=canonicalize_path(path)) + if external_spec.satisfies(spec): + external_specs.append(external_spec) - external_spec = spack.spec.Spec( - external_spec, external_module=module) - if external_spec.satisfies(spec): - external_specs.append(external_spec) + for external_spec, module in pkg_modules.items(): + external_spec = spack.spec.Spec( + external_spec, external_module=module) + if external_spec.satisfies(spec): + external_specs.append(external_spec) # defensively copy returned specs return [s.copy() for s in external_specs] @@ -215,12 +190,11 @@ def spec_externals(spec): def is_spec_buildable(spec): """Return true if the spec pkgspec is configured as buildable""" - allpkgs = get_packages_config() - if spec.name not in allpkgs: - return True - if 'buildable' not in allpkgs[spec.name]: - return True - return allpkgs[spec.name]['buildable'] + allpkgs = spack.config.get('packages') + do_not_build = [name for name, entry in allpkgs.items() + if not entry.get('buildable', True)] + return not (spec.name in do_not_build or + any(spec.package.provides(name) for name in do_not_build)) def get_package_dir_permissions(spec): diff --git a/lib/spack/spack/test/concretize_preferences.py b/lib/spack/spack/test/concretize_preferences.py index 81b5360869..922d5a11d8 100644 --- a/lib/spack/spack/test/concretize_preferences.py +++ b/lib/spack/spack/test/concretize_preferences.py @@ -185,36 +185,39 @@ class TestConcretizePreferences(object): spec.concretize() assert spec.version == Version('0.2.15.develop') - def test_no_virtuals_in_packages_yaml(self): - """Verify that virtuals are not allowed in packages.yaml.""" + def test_external_mpi(self): + # make sure this doesn't give us an external first. + spec = Spec('mpi') + spec.concretize() + assert not spec['mpi'].external - # set up a packages.yaml file with a vdep as a key. We use - # syaml.load_config here to make sure source lines in the config are - # attached to parsed strings, as the error message uses them. + # load config conf = syaml.load_config("""\ -mpi: +all: + providers: + mpi: [mpich] +mpich: + buildable: false paths: - mpi-with-lapack@2.1: /path/to/lapack + mpich@3.0.4: /dummy/path """) spack.config.set('packages', conf, scope='concretize') - # now when we get the packages.yaml config, there should be an error - with pytest.raises(spack.package_prefs.VirtualInPackagesYAMLError): - spack.package_prefs.get_packages_config() - - def test_all_is_not_a_virtual(self): - """Verify that `all` is allowed in packages.yaml.""" - conf = syaml.load_config("""\ -all: - variants: [+mpi] -""") - spack.config.set('packages', conf, scope='concretize') + # ensure that once config is in place, external is used + spec = Spec('mpi') + spec.concretize() + assert spec['mpich'].external_path == '/dummy/path' - # should be no error for 'all': - spack.package_prefs.get_packages_config() + def test_external_module(self, monkeypatch): + """Test that packages can find externals specified by module - def test_external_mpi(self): + The specific code for parsing the module is tested elsewhere. + This just tests that the preference is accounted for""" # make sure this doesn't give us an external first. + def mock_module(cmd, module): + return 'prepend-path PATH /dummy/path' + monkeypatch.setattr(spack.util.module_cmd, 'module', mock_module) + spec = Spec('mpi') spec.concretize() assert not spec['mpi'].external @@ -224,10 +227,10 @@ all: all: providers: mpi: [mpich] -mpich: +mpi: buildable: false - paths: - mpich@3.0.4: /dummy/path + modules: + mpich@3.0.4: dummy """) spack.config.set('packages', conf, scope='concretize') diff --git a/lib/spack/spack/test/conftest.py b/lib/spack/spack/test/conftest.py index a186463907..8912c0219b 100644 --- a/lib/spack/spack/test/conftest.py +++ b/lib/spack/spack/test/conftest.py @@ -1040,6 +1040,13 @@ class MockPackage(object): self.conflicts = {} self.patches = {} + def provides(self, vname): + return vname in self.provided + + @property + def virtuals_provided(self): + return [v.name for v, c in self.provided] + class MockPackageMultiRepo(object): def __init__(self, packages): diff --git a/lib/spack/spack/test/mirror.py b/lib/spack/spack/test/mirror.py index 570c329b71..05cf46dc20 100644 --- a/lib/spack/spack/test/mirror.py +++ b/lib/spack/spack/test/mirror.py @@ -52,52 +52,52 @@ def check_mirror(): mirror_root = os.path.join(stage.path, 'test-mirror') # register mirror with spack config mirrors = {'spack-mirror-test': 'file://' + mirror_root} - spack.config.set('mirrors', mirrors) - with spack.config.override('config:checksum', False): - specs = [Spec(x).concretized() for x in repos] - spack.mirror.create(mirror_root, specs) - - # Stage directory exists - assert os.path.isdir(mirror_root) - - for spec in specs: - fetcher = spec.package.fetcher[0] - per_package_ref = os.path.join( - spec.name, '-'.join([spec.name, str(spec.version)])) - mirror_paths = spack.mirror.mirror_archive_paths( - fetcher, - per_package_ref) - expected_path = os.path.join( - mirror_root, mirror_paths.storage_path) - assert os.path.exists(expected_path) - - # Now try to fetch each package. - for name, mock_repo in repos.items(): - spec = Spec(name).concretized() - pkg = spec.package - + with spack.config.override('mirrors', mirrors): with spack.config.override('config:checksum', False): - with pkg.stage: - pkg.do_stage(mirror_only=True) - - # Compare the original repo with the expanded archive - original_path = mock_repo.path - if 'svn' in name: - # have to check out the svn repo to compare. - original_path = os.path.join( - mock_repo.path, 'checked_out') - - svn = which('svn', required=True) - svn('checkout', mock_repo.url, original_path) - - dcmp = filecmp.dircmp( - original_path, pkg.stage.source_path) - - # make sure there are no new files in the expanded - # tarball - assert not dcmp.right_only - # and that all original files are present. - assert all(l in exclude for l in dcmp.left_only) + specs = [Spec(x).concretized() for x in repos] + spack.mirror.create(mirror_root, specs) + + # Stage directory exists + assert os.path.isdir(mirror_root) + + for spec in specs: + fetcher = spec.package.fetcher[0] + per_package_ref = os.path.join( + spec.name, '-'.join([spec.name, str(spec.version)])) + mirror_paths = spack.mirror.mirror_archive_paths( + fetcher, + per_package_ref) + expected_path = os.path.join( + mirror_root, mirror_paths.storage_path) + assert os.path.exists(expected_path) + + # Now try to fetch each package. + for name, mock_repo in repos.items(): + spec = Spec(name).concretized() + pkg = spec.package + + with spack.config.override('config:checksum', False): + with pkg.stage: + pkg.do_stage(mirror_only=True) + + # Compare the original repo with the expanded archive + original_path = mock_repo.path + if 'svn' in name: + # have to check out the svn repo to compare. + original_path = os.path.join( + mock_repo.path, 'checked_out') + + svn = which('svn', required=True) + svn('checkout', mock_repo.url, original_path) + + dcmp = filecmp.dircmp( + original_path, pkg.stage.source_path) + + # make sure there are no new files in the expanded + # tarball + assert not dcmp.right_only + # and that all original files are present. + assert all(l in exclude for l in dcmp.left_only) def test_url_mirror(mock_archive): diff --git a/lib/spack/spack/test/module_parsing.py b/lib/spack/spack/test/module_parsing.py index bbe18b1ad0..0bf485913f 100644 --- a/lib/spack/spack/test/module_parsing.py +++ b/lib/spack/spack/test/module_parsing.py @@ -20,28 +20,11 @@ test_module_lines = ['prepend-path LD_LIBRARY_PATH /path/to/lib', 'setenv LDFLAGS -L/path/to/lib', 'prepend-path PATH /path/to/bin'] +_test_template = "'. %s 2>&1' % args[1]" -@pytest.fixture -def module_function_test_mode(): - old_mode = spack.util.module_cmd._test_mode - spack.util.module_cmd._test_mode = True - yield - - spack.util.module_cmd._test_mode = old_mode - - -@pytest.fixture -def save_module_func(): - old_func = spack.util.module_cmd.module - - yield - - spack.util.module_cmd.module = old_func - - -def test_module_function_change_env(tmpdir, working_env, - module_function_test_mode): +def test_module_function_change_env(tmpdir, working_env, monkeypatch): + monkeypatch.setattr(spack.util.module_cmd, '_cmd_template', _test_template) src_file = str(tmpdir.join('src_me')) with open(src_file, 'w') as f: f.write('export TEST_MODULE_ENV_VAR=TEST_SUCCESS\n') @@ -53,7 +36,8 @@ def test_module_function_change_env(tmpdir, working_env, assert os.environ['NOT_AFFECTED'] == "NOT_AFFECTED" -def test_module_function_no_change(tmpdir, module_function_test_mode): +def test_module_function_no_change(tmpdir, monkeypatch): + monkeypatch.setattr(spack.util.module_cmd, '_cmd_template', _test_template) src_file = str(tmpdir.join('src_me')) with open(src_file, 'w') as f: f.write('echo TEST_MODULE_FUNCTION_PRINT') @@ -65,11 +49,11 @@ def test_module_function_no_change(tmpdir, module_function_test_mode): assert os.environ == old_env -def test_get_path_from_module_faked(save_module_func): +def test_get_path_from_module_faked(monkeypatch): for line in test_module_lines: def fake_module(*args): return line - spack.util.module_cmd.module = fake_module + monkeypatch.setattr(spack.util.module_cmd, 'module', fake_module) path = get_path_from_module('mod') assert path == '/path/to' diff --git a/lib/spack/spack/test/spec_dag.py b/lib/spack/spack/test/spec_dag.py index 25917f9424..e031f02c25 100644 --- a/lib/spack/spack/test/spec_dag.py +++ b/lib/spack/spack/test/spec_dag.py @@ -387,7 +387,6 @@ class TestSpecDag(object): with pytest.raises(spack.spec.UnsatisfiableArchitectureSpecError): spec.normalize() - @pytest.mark.usefixtures('config') def test_invalid_dep(self): spec = Spec('libelf ^mpich') with pytest.raises(spack.spec.InvalidDependencyError): @@ -602,7 +601,6 @@ class TestSpecDag(object): copy_ids = set(id(s) for s in copy.traverse()) assert not orig_ids.intersection(copy_ids) - @pytest.mark.usefixtures('config') def test_copy_concretized(self): orig = Spec('mpileaks') orig.concretize() diff --git a/lib/spack/spack/util/module_cmd.py b/lib/spack/spack/util/module_cmd.py index 0edf7e6102..74790156ae 100644 --- a/lib/spack/spack/util/module_cmd.py +++ b/lib/spack/spack/util/module_cmd.py @@ -19,16 +19,11 @@ import llnl.util.tty as tty # If we need another option that changes the environment, add it here. module_change_commands = ['load', 'swap', 'unload', 'purge', 'use', 'unuse'] py_cmd = "'import os;import json;print(json.dumps(dict(os.environ)))'" - -# This is just to enable testing. I hate it but we can't find a better way -_test_mode = False +_cmd_template = "'module ' + ' '.join(args) + ' 2>&1'" def module(*args): - module_cmd = 'module ' + ' '.join(args) + ' 2>&1' - if _test_mode: - tty.warn('module function operating in test mode') - module_cmd = ". %s 2>&1" % args[1] + module_cmd = eval(_cmd_template) # So we can monkeypatch for testing if args[0] in module_change_commands: # Do the module manipulation, then output the environment in JSON # and read the JSON back in the parent process to update os.environ -- cgit v1.2.3-70-g09d2