specs: remove parse_anonymous_spec(); use Spec() instead

- `parse_anonymous_spec()` is a vestige of the days when Spack didn't
  support nameless specs.  We don't need it anymore because now we can
  write Spec() for a spec that will match anything, and satisfies()
  semantics work properly for anonymous specs.

- Delete `parse_anonymous_spec()` and replace its uses with simple calls
  to the Spec() constructor.

- make then handling of when='...' specs in directives more consistent.

- clean up Spec.__contains__()

- refactor directives and tests slightly to accommodate the change.

lib/spack/spack/directives.py

@@ -54,6 +54,48 @@ class OpenMpi(Package):
 _patch_order_index = 0
 
 
+def make_when_spec(value):
+    """Create a ``Spec`` that indicates when a directive should be applied.
+
+    Directives with ``when`` specs, e.g.:
+
+        patch('foo.patch', when='@4.5.1:')
+        depends_on('mpi', when='+mpi')
+        depends_on('readline', when=sys.platform() != 'darwin')
+
+    are applied conditionally depending on the value of the ``when``
+    keyword argument.  Specifically:
+
+      1. If the ``when`` argument is ``True``, the directive is always applied
+      2. If it is ``False``, the directive is never applied
+      3. If it is a ``Spec`` string, it is applied when the package's
+         concrete spec satisfies the ``when`` spec.
+
+    The first two conditions are useful for the third example case above.
+    It allows package authors to include directives that are conditional
+    at package definition time, in additional to ones that are evaluated
+    as part of concretization.
+
+    Arguments:
+        value (Spec or bool): a conditional Spec or a constant ``bool``
+           value indicating when a directive should be applied.
+
+    """
+    # Unsatisfiable conditions are discarded by the caller, and never
+    # added to the package class
+    if value is False:
+        return False
+
+    # If there is no constraint, the directive should always apply;
+    # represent this by returning the unconstrained `Spec()`, which is
+    # always satisfied.
+    if value is None or value is True:
+        return spack.spec.Spec()
+
+    # This is conditional on the spec
+    return spack.spec.Spec(value)
+
+
 class DirectiveMeta(type):
     """Flushes the directives that were temporarily stored in the staging
     area into the package.
@@ -236,13 +278,9 @@ def _execute_version(pkg):
 
 
 def _depends_on(pkg, spec, when=None, type=default_deptype, patches=None):
-    # If when is False do nothing
+    when_spec = make_when_spec(when)
-    if when is False:
+    if not when_spec:
         return
-    # If when is None or True make sure the condition is always satisfied
-    if when is None or when is True:
-        when = pkg.name
-    when_spec = spack.spec.parse_anonymous_spec(when, pkg.name)
 
     dep_spec = spack.spec.Spec(spec)
     if pkg.name == dep_spec.name:
@@ -304,8 +342,9 @@ def conflicts(conflict_spec, when=None, msg=None):
     """
     def _execute_conflicts(pkg):
         # If when is not specified the conflict always holds
-        condition = pkg.name if when is None else when
+        when_spec = make_when_spec(when)
-        when_spec = spack.spec.parse_anonymous_spec(condition, pkg.name)
+        if not when_spec:
+            return
 
         # Save in a list the conflicts and the associated custom messages
         when_spec_list = pkg.conflicts.setdefault(conflict_spec, [])
@@ -352,12 +391,11 @@ def extends(spec, **kwargs):
 
     """
     def _execute_extends(pkg):
-        # if pkg.extendees:
+        when = kwargs.get('when')
-        #     directive = 'extends'
+        when_spec = make_when_spec(when)
-        #     msg = 'Packages can extend at most one other package.'
+        if not when_spec:
-        #     raise DirectiveError(directive, msg)
+            return
 
-        when = kwargs.get('when', pkg.name)
         _depends_on(pkg, spec, when=when)
         pkg.extendees[spec] = (spack.spec.Spec(spec), kwargs)
     return _execute_extends
@@ -370,8 +408,14 @@ def provides(*specs, **kwargs):
        can use the providing package to satisfy the dependency.
     """
     def _execute_provides(pkg):
-        spec_string = kwargs.get('when', pkg.name)
+        when = kwargs.get('when')
-        provider_spec = spack.spec.parse_anonymous_spec(spec_string, pkg.name)
+        when_spec = make_when_spec(when)
+        if not when_spec:
+            return
+
+        # ``when`` specs for ``provides()`` need a name, as they are used
+        # to build the ProviderIndex.
+        when_spec.name = pkg.name
 
         for string in specs:
             for provided_spec in spack.spec.parse(string):
@@ -381,7 +425,7 @@ def _execute_provides(pkg):
 
                 if provided_spec not in pkg.provided:
                     pkg.provided[provided_spec] = set()
-                pkg.provided[provided_spec].add(provider_spec)
+                pkg.provided[provided_spec].add(when_spec)
     return _execute_provides
 
 
@@ -407,9 +451,9 @@ def patch(url_or_filename, level=1, when=None, working_dir=".", **kwargs):
 
     """
     def _execute_patch(pkg_or_dep):
-        constraint = pkg_or_dep.name if when is None else when
+        when_spec = make_when_spec(when)
-        when_spec = spack.spec.parse_anonymous_spec(
+        if not when_spec:
-            constraint, pkg_or_dep.name)
+            return
 
         # if this spec is identical to some other, then append this
         # patch to the existing list.
@@ -551,7 +595,11 @@ def resource(**kwargs):
       resource is moved into the main package stage area.
     """
     def _execute_resource(pkg):
-        when = kwargs.get('when', pkg.name)
+        when = kwargs.get('when')
+        when_spec = make_when_spec(when)
+        if not when_spec:
+            return
+
         destination = kwargs.get('destination', "")
         placement = kwargs.get('placement', None)
 
@@ -574,7 +622,6 @@ def _execute_resource(pkg):
             message += "\tdestination : '{dest}'\n".format(dest=destination)
             raise RuntimeError(message)
 
-        when_spec = spack.spec.parse_anonymous_spec(when, pkg.name)
         resources = pkg.resources.setdefault(when_spec, [])
         name = kwargs.get('name')
         fetcher = from_kwargs(**kwargs)

lib/spack/spack/spec.py

@@ -128,7 +128,6 @@
 __all__ = [
     'Spec',
     'parse',
-    'parse_anonymous_spec',
     'SpecError',
     'SpecParseError',
     'DuplicateDependencyError',
@@ -2554,17 +2553,9 @@ def _autospec(self, spec_like):
         it.  If it's a string, tries to parse a string.  If that fails, tries
         to parse a local spec from it (i.e. name is assumed to be self's name).
         """
-        if isinstance(spec_like, spack.spec.Spec):
+        if isinstance(spec_like, Spec):
             return spec_like
-
+        return Spec(spec_like)
-        try:
-            spec = spack.spec.Spec(spec_like)
-            if not spec.name:
-                raise SpecError(
-                    "anonymous package -- this will always be handled")
-            return spec
-        except SpecError:
-            return parse_anonymous_spec(spec_like, self.name)
 
     def satisfies(self, other, deps=True, strict=False, strict_deps=False):
         """Determine if this spec satisfies all constraints of another.
@@ -2933,15 +2924,21 @@ def __getitem__(self, name):
         return value
 
     def __contains__(self, spec):
-        """True if this spec satisfies the provided spec, or if any dependency
+        """True if this spec or some dependency satisfies the spec.
-           does.  If the spec has no name, then we parse this one first.
+
+        Note: If ``spec`` is anonymous, we ONLY check whether the root
+        satisfies it, NOT dependencies.  This is because most anonymous
+        specs (e.g., ``@1.2``) don't make sense when applied across an
+        entire DAG -- we limit them to the root.
+
         """
         spec = self._autospec(spec)
-        for s in self.traverse():
-            if s.satisfies(spec, strict=True):
-                return True
 
-        return False
+        # if anonymous or same name, we only have to look at the root
+        if not spec.name or spec.name == self.name:
+            return self.satisfies(spec)
+        else:
+            return any(s.satisfies(spec) for s in self.traverse(root=False))
 
     def sorted_deps(self):
         """Return a list of all dependencies sorted by name."""
@@ -3945,41 +3942,6 @@ def parse(string):
     return SpecParser().parse(string)
 
 
-def parse_anonymous_spec(spec_like, pkg_name):
-    """Allow the user to omit the package name part of a spec if they
-       know what it has to be already.
-
-       e.g., provides('mpi@2', when='@1.9:') says that this package
-       provides MPI-3 when its version is higher than 1.9.
-    """
-    if not isinstance(spec_like, (str, Spec)):
-        raise TypeError('spec must be Spec or spec string.  Found %s'
-                        % type(spec_like))
-
-    if isinstance(spec_like, str):
-        try:
-            anon_spec = Spec(spec_like)
-            if anon_spec.name != pkg_name:
-                raise SpecParseError(spack.parse.ParseError(
-                    "",
-                    "",
-                    "Expected anonymous spec for package %s but found spec for"
-                    "package %s" % (pkg_name, anon_spec.name)))
-        except SpecParseError:
-            anon_spec = Spec(pkg_name + ' ' + spec_like)
-            if anon_spec.name != pkg_name:
-                raise ValueError(
-                    "Invalid spec for package %s: %s" % (pkg_name, spec_like))
-    else:
-        anon_spec = spec_like.copy()
-
-    if anon_spec.name != pkg_name:
-        raise ValueError("Spec name '%s' must match package name '%s'"
-                         % (anon_spec.name, pkg_name))
-
-    return anon_spec
-
-
 def save_dependency_spec_yamls(
         root_spec_as_yaml, output_directory, dependencies=None):
     """Given a root spec (represented as a yaml object), index it with a subset

lib/spack/spack/test/directives.py

@@ -0,0 +1,34 @@
+# Copyright 2013-2019 Lawrence Livermore National Security, LLC and other
+# Spack Project Developers. See the top-level COPYRIGHT file for details.
+#
+# SPDX-License-Identifier: (Apache-2.0 OR MIT)
+
+import spack.repo
+from spack.spec import Spec
+
+
+def test_false_directives_do_not_exist(mock_packages):
+    """Ensure directives that evaluate to False at import time are added to
+       dicts on packages.
+    """
+    cls = spack.repo.path.get_pkg_class('when-directives-false')
+    assert not cls.dependencies
+    assert not cls.resources
+    assert not cls.patches
+
+
+def test_true_directives_exist(mock_packages):
+    """Ensure directives that evaluate to True at import time are added to
+       dicts on packages.
+    """
+    cls = spack.repo.path.get_pkg_class('when-directives-true')
+
+    assert cls.dependencies
+    assert Spec() in cls.dependencies['extendee']
+    assert Spec() in cls.dependencies['b']
+
+    assert cls.resources
+    assert Spec() in cls.resources
+
+    assert cls.patches
+    assert Spec() in cls.patches

lib/spack/spack/test/patch.py

@@ -143,16 +143,16 @@ def test_nested_directives(mock_packages):
     # to Dependency objects.
     libelf_dep = next(iter(patcher.dependencies['libelf'].values()))
     assert len(libelf_dep.patches) == 1
-    assert len(libelf_dep.patches[Spec('libelf')]) == 1
+    assert len(libelf_dep.patches[Spec()]) == 1
 
     libdwarf_dep = next(iter(patcher.dependencies['libdwarf'].values()))
     assert len(libdwarf_dep.patches) == 2
-    assert len(libdwarf_dep.patches[Spec('libdwarf')]) == 1
+    assert len(libdwarf_dep.patches[Spec()]) == 1
-    assert len(libdwarf_dep.patches[Spec('libdwarf@20111030')]) == 1
+    assert len(libdwarf_dep.patches[Spec('@20111030')]) == 1
 
     fake_dep = next(iter(patcher.dependencies['fake'].values()))
     assert len(fake_dep.patches) == 1
-    assert len(fake_dep.patches[Spec('fake')]) == 2
+    assert len(fake_dep.patches[Spec()]) == 2
 
 
 def test_patched_dependency(

lib/spack/spack/test/spec_semantics.py

@@ -7,7 +7,7 @@
 import pytest
 
 from spack.spec import Spec, UnsatisfiableSpecError, SpecError
-from spack.spec import substitute_abstract_variants, parse_anonymous_spec
+from spack.spec import substitute_abstract_variants
 from spack.spec import SpecFormatSigilError, SpecFormatStringError
 from spack.variant import InvalidVariantValueError
 from spack.variant import MultipleValuesInExclusiveVariantError
@@ -17,50 +17,46 @@
 import spack.error
 
 
-def target_factory(spec_string, target_concrete):
+def make_spec(spec_like, concrete):
-    spec = Spec(spec_string) if spec_string else Spec()
+    if isinstance(spec_like, Spec):
+        return spec_like
 
-    if target_concrete:
+    spec = Spec(spec_like)
+    if concrete:
         spec._mark_concrete()
         substitute_abstract_variants(spec)
-
     return spec
 
 
-def argument_factory(argument_spec, left):
+def _specify(spec_like):
-    try:
+    if isinstance(spec_like, Spec):
-        # If it's not anonymous, allow it
+        return spec_like
-        right = target_factory(argument_spec, False)
+
-    except Exception:
+    return Spec(spec_like)
-        right = parse_anonymous_spec(argument_spec, left.name)
-    return right
 
 
-def check_satisfies(target_spec, argument_spec, target_concrete=False):
+def check_satisfies(target_spec, constraint_spec, target_concrete=False):
 
-    left = target_factory(target_spec, target_concrete)
+    target = make_spec(target_spec, target_concrete)
-    right = argument_factory(argument_spec, left)
+    constraint = _specify(constraint_spec)
 
     # Satisfies is one-directional.
-    assert left.satisfies(right)
+    assert target.satisfies(constraint)
-    if argument_spec:
-        assert left.satisfies(argument_spec)
 
-    # If left satisfies right, then we should be able to constrain
+    # If target satisfies constraint, then we should be able to constrain
-    # right by left.  Reverse is not always true.
+    # constraint by target.  Reverse is not always true.
-    right.copy().constrain(left)
+    constraint.copy().constrain(target)
 
 
-def check_unsatisfiable(target_spec, argument_spec, target_concrete=False):
+def check_unsatisfiable(target_spec, constraint_spec, target_concrete=False):
 
-    left = target_factory(target_spec, target_concrete)
+    target = make_spec(target_spec, target_concrete)
-    right = argument_factory(argument_spec, left)
+    constraint = _specify(constraint_spec)
 
-    assert not left.satisfies(right)
+    assert not target.satisfies(constraint)
-    assert not left.satisfies(argument_spec)
 
     with pytest.raises(UnsatisfiableSpecError):
-        right.copy().constrain(left)
+        constraint.copy().constrain(target)
 
 
 def check_constrain(expected, spec, constraint):
@@ -99,31 +95,31 @@ def test_satisfies(self):
 
     def test_empty_satisfies(self):
         # Basic satisfaction
-        check_satisfies('libelf', '')
+        check_satisfies('libelf', Spec())
-        check_satisfies('libdwarf', '')
+        check_satisfies('libdwarf', Spec())
-        check_satisfies('%intel', '')
+        check_satisfies('%intel', Spec())
-        check_satisfies('^mpi', '')
+        check_satisfies('^mpi', Spec())
-        check_satisfies('+debug', '')
+        check_satisfies('+debug', Spec())
-        check_satisfies('@3:', '')
+        check_satisfies('@3:', Spec())
 
         # Concrete (strict) satisfaction
-        check_satisfies('libelf', '', True)
+        check_satisfies('libelf', Spec(), True)
-        check_satisfies('libdwarf', '', True)
+        check_satisfies('libdwarf', Spec(), True)
-        check_satisfies('%intel', '', True)
+        check_satisfies('%intel', Spec(), True)
-        check_satisfies('^mpi', '', True)
+        check_satisfies('^mpi', Spec(), True)
         # TODO: Variants can't be called concrete while anonymous
-        # check_satisfies('+debug', '', True)
+        # check_satisfies('+debug', Spec(), True)
-        check_satisfies('@3:', '', True)
+        check_satisfies('@3:', Spec(), True)
 
         # Reverse (non-strict) satisfaction
-        check_satisfies('', 'libelf')
+        check_satisfies(Spec(), 'libelf')
-        check_satisfies('', 'libdwarf')
+        check_satisfies(Spec(), 'libdwarf')
-        check_satisfies('', '%intel')
+        check_satisfies(Spec(), '%intel')
-        check_satisfies('', '^mpi')
+        check_satisfies(Spec(), '^mpi')
         # TODO: Variant matching is auto-strict
         # we should rethink this
-        # check_satisfies('', '+debug')
+        # check_satisfies(Spec(), '+debug')
-        check_satisfies('', '@3:')
+        check_satisfies(Spec(), '@3:')
 
     def test_satisfies_namespace(self):
         check_satisfies('builtin.mpich', 'mpich')
@@ -343,8 +339,8 @@ def test_unsatisfiable_multi_value_variant(self):
         # Semantics for a multi-valued variant is different
         # Depending on whether the spec is concrete or not
 
-        a = target_factory(
+        a = make_spec(
-            'multivalue_variant foo="bar"', target_concrete=True
+            'multivalue_variant foo="bar"', concrete=True
         )
         spec_str = 'multivalue_variant foo="bar,baz"'
         b = Spec(spec_str)
@@ -363,8 +359,8 @@ def test_unsatisfiable_multi_value_variant(self):
         # An abstract spec can instead be constrained
         assert a.constrain(b)
 
-        a = target_factory(
+        a = make_spec(
-            'multivalue_variant foo="bar,baz"', target_concrete=True
+            'multivalue_variant foo="bar,baz"', concrete=True
         )
         spec_str = 'multivalue_variant foo="bar,baz,quux"'
         b = Spec(spec_str)
@@ -416,13 +412,13 @@ def test_unsatisfiable_variant_types(self):
 
         check_unsatisfiable(
             target_spec='multivalue_variant foo="bar"',
-            argument_spec='multivalue_variant +foo',
+            constraint_spec='multivalue_variant +foo',
             target_concrete=True
         )
 
         check_unsatisfiable(
             target_spec='multivalue_variant foo="bar"',
-            argument_spec='multivalue_variant ~foo',
+            constraint_spec='multivalue_variant ~foo',
             target_concrete=True
         )
 

lib/spack/spack/test/spec_syntax.py

@@ -9,7 +9,7 @@
 import spack.store
 import spack.spec as sp
 from spack.parse import Token
-from spack.spec import Spec, parse, parse_anonymous_spec
+from spack.spec import Spec
 from spack.spec import SpecParseError, RedundantSpecError
 from spack.spec import AmbiguousHashError, InvalidHashError, NoSuchHashError
 from spack.spec import DuplicateArchitectureError, DuplicateVariantError
@@ -532,18 +532,3 @@ def test_kv_with_spaces(self):
             "mvapich_foo debug= 4 "
             "^ _openmpi @1.2 : 1.4 , 1.6 % intel @ 12.1 : 12.6 + debug - qt_4 "
             "^ stackwalker @ 8.1_1e")
-
-
-@pytest.mark.parametrize('spec,anon_spec,spec_name', [
-    ('openmpi languages=go', 'languages=go', 'openmpi'),
-    ('openmpi @4.6:', '@4.6:', 'openmpi'),
-    ('openmpi languages=go @4.6:', 'languages=go @4.6:', 'openmpi'),
-    ('openmpi @4.6: languages=go', '@4.6: languages=go', 'openmpi'),
-])
-def test_parse_anonymous_specs(spec, anon_spec, spec_name):
-
-    expected = parse(spec)
-    spec = parse_anonymous_spec(anon_spec, spec_name)
-
-    assert len(expected) == 1
-    assert spec in expected

var/spack/repos/builtin.mock/packages/when-directives-false/package.py

@@ -0,0 +1,28 @@
+# Copyright 2013-2019 Lawrence Livermore National Security, LLC and other
+# Spack Project Developers. See the top-level COPYRIGHT file for details.
+#
+# SPDX-License-Identifier: (Apache-2.0 OR MIT)
+
+from spack import *
+
+
+class WhenDirectivesFalse(Package):
+    """Package that tests False when specs on directives."""
+
+    homepage = "http://www.example.com"
+    url = "http://www.example.com/example-1.0.tar.gz"
+
+    version('1.0', '0123456789abcdef0123456789abcdef')
+
+    patch('https://example.com/foo.patch',
+          sha256='abcd1234abcd1234abcd1234abcd1234abcd1234abcd1234abcd1234abcd1234',
+          when=False)
+    extends('extendee', when=False)
+    depends_on('b', when=False)
+    conflicts('@1.0', when=False)
+    resource(url="http://www.example.com/example-1.0-resource.tar.gz",
+             md5='0123456789abcdef0123456789abcdef',
+             when=False)
+
+    def install(self, spec, prefix):
+        pass

var/spack/repos/builtin.mock/packages/when-directives-true/package.py

@@ -0,0 +1,28 @@
+# Copyright 2013-2019 Lawrence Livermore National Security, LLC and other
+# Spack Project Developers. See the top-level COPYRIGHT file for details.
+#
+# SPDX-License-Identifier: (Apache-2.0 OR MIT)
+
+from spack import *
+
+
+class WhenDirectivesTrue(Package):
+    """Package that tests True when specs on directives."""
+
+    homepage = "http://www.example.com"
+    url = "http://www.example.com/example-1.0.tar.gz"
+
+    version('1.0', '0123456789abcdef0123456789abcdef')
+
+    patch('https://example.com/foo.patch',
+          sha256='abcd1234abcd1234abcd1234abcd1234abcd1234abcd1234abcd1234abcd1234',
+          when=True)
+    extends('extendee', when=True)
+    depends_on('b', when=True)
+    conflicts('@1.0', when=True)
+    resource(url="http://www.example.com/example-1.0-resource.tar.gz",
+             md5='0123456789abcdef0123456789abcdef',
+             when=True)
+
+    def install(self, spec, prefix):
+        pass