Support yaml paths anywhere specs are handled on CLI (#12561)

Update command-line (CLI) parsing to understand references to yaml
files that store Spack specs. Where a file reference is encountered,
the full Spec in the file will be read in. A file reference may
appear anywhere that a spec could appear before. For example, if you
write "spack spec -y openmpi > openmpi.yaml" you may then install the
spec using the yaml file by running "spack install ./openmpi.yaml";
you can also refer to dependencies in this way (e.g.
"spack install foo^./openmpi.yaml").

There are two requirements for file references:

* A file path entered on the CLI must include a "/" even if the file
  exists in your current working directory. For example, if you
  create an openmpi.yaml file as above and run
  "spack install openmpi.yaml" from the same directory, it will
  report an error.
* A file path entered on the CLI must end with ".yaml"

This commit adds error messages to clearly inform the user of both
violations.

4 files changed, 318 insertions, 19 deletions

diff --git a/lib/spack/spack/parse.py b/lib/spack/spack/parse.py
index 836b40c3a6..6bfdec7b43 100644
--- a/lib/spack/spack/parse.py
+++ b/lib/spack/spack/parse.py
@@ -12,7 +12,7 @@ from six import string_types
 import spack.error
 
 
-class Token:
+class Token(object):
     """Represents tokens; generated from input by lexer and fed to parse()."""
 
     def __init__(self, type, value='', start=0, end=0):
@@ -25,7 +25,7 @@ class Token:
         return str(self)
 
     def __str__(self):
-        return "'%s'" % self.value
+        return "<%d: '%s'>" % (self.type, self.value)
 
     def is_a(self, type):
         return self.type == type
@@ -128,7 +128,7 @@ class Parser(object):
         raise ParseError(message, self.text, self.token.start)
 
     def unexpected_token(self):
-        self.next_token_error("Unexpected token")
+        self.next_token_error("Unexpected token: '%s'" % self.next.value)
 
     def expect(self, id):
         """Like accept(), but fails if we don't like the next token."""
diff --git a/lib/spack/spack/repo.py b/lib/spack/spack/repo.py
index d726fafacc..3ea196dd4d 100644
--- a/lib/spack/spack/repo.py
+++ b/lib/spack/spack/repo.py
@@ -564,7 +564,7 @@ class RepoPath(object):
     def providers_for(self, vpkg_spec):
         providers = self.provider_index.providers_for(vpkg_spec)
         if not providers:
-            raise UnknownPackageError(vpkg_spec.name)
+            raise UnknownPackageError(vpkg_spec.fullname)
         return providers
 
     @autospec
@@ -967,7 +967,7 @@ class Repo(object):
     def providers_for(self, vpkg_spec):
         providers = self.provider_index.providers_for(vpkg_spec)
         if not providers:
-            raise UnknownPackageError(vpkg_spec.name)
+            raise UnknownPackageError(vpkg_spec.fullname)
         return providers
 
     @autospec
@@ -1283,10 +1283,17 @@ class UnknownPackageError(UnknownEntityError):
     def __init__(self, name, repo=None):
         msg = None
         if repo:
-            msg = "Package %s not found in repository %s" % (name, repo)
+            msg = "Package '%s' not found in repository '%s'" % (name, repo)
         else:
-            msg = "Package %s not found." % name
-        super(UnknownPackageError, self).__init__(msg)
+            msg = "Package '%s' not found." % name
+
+        # special handling for specs that may have been intended as filenames
+        # prompt the user to ask whether they intended to write './<name>'
+        long_msg = None
+        if name.endswith(".yaml"):
+            long_msg = "Did you mean to specify a filename with './%s'?" % name
+
+        super(UnknownPackageError, self).__init__(msg, long_msg)
         self.name = name
 
 
diff --git a/lib/spack/spack/spec.py b/lib/spack/spack/spec.py
index c7926ad3a3..dca06101b8 100644
--- a/lib/spack/spack/spec.py
+++ b/lib/spack/spack/spec.py
@@ -3804,10 +3804,11 @@ class LazySpecCache(collections.defaultdict):
         return value
 
 
-#
-# These are possible token types in the spec grammar.
-#
-HASH, DEP, AT, COLON, COMMA, ON, OFF, PCT, EQ, ID, VAL = range(11)
+#: These are possible token types in the spec grammar.
+HASH, DEP, AT, COLON, COMMA, ON, OFF, PCT, EQ, ID, VAL, FILE = range(12)
+
+#: Regex for fully qualified spec names. (e.g., builtin.hdf5)
+spec_id_re = r'\w[\w.-]*'
 
 
 class SpecLexer(spack.parse.Lexer):
@@ -3816,7 +3817,6 @@ class SpecLexer(spack.parse.Lexer):
 
     def __init__(self):
         super(SpecLexer, self).__init__([
-            (r'/', lambda scanner, val: self.token(HASH,  val)),
             (r'\^', lambda scanner, val: self.token(DEP,   val)),
             (r'\@', lambda scanner, val: self.token(AT,    val)),
             (r'\:', lambda scanner, val: self.token(COLON, val)),
@@ -3826,9 +3826,18 @@ class SpecLexer(spack.parse.Lexer):
             (r'\~', lambda scanner, val: self.token(OFF,   val)),
             (r'\%', lambda scanner, val: self.token(PCT,   val)),
             (r'\=', lambda scanner, val: self.token(EQ,    val)),
-            # This is more liberal than identifier_re (see above).
-            # Checked by check_identifier() for better error messages.
-            (r'\w[\w.-]*', lambda scanner, val: self.token(ID,    val)),
+
+            # Filenames match before identifiers, so no initial filename
+            # component is parsed as a spec (e.g., in subdir/spec.yaml)
+            (r'[/\w.-]+\.yaml[^\b]*', lambda scanner, v: self.token(FILE, v)),
+
+            # Hash match after filename. No valid filename can be a hash
+            # (files end w/.yaml), but a hash can match a filename prefix.
+            (r'/', lambda scanner, val: self.token(HASH, val)),
+
+            # Identifiers match after filenames and hashes.
+            (spec_id_re, lambda scanner, val: self.token(ID, val)),
+
             (r'\s+', lambda scanner, val: None)],
             [EQ],
             [(r'[\S].*', lambda scanner, val: self.token(VAL,    val)),
@@ -3859,7 +3868,14 @@ class SpecParser(spack.parse.Parser):
 
         try:
             while self.next:
-                # TODO: clean this parsing up a bit
+                # Try a file first, but if it doesn't succeed, keep parsing
+                # as from_file may backtrack and try an id.
+                if self.accept(FILE):
+                    spec = self.spec_from_file()
+                    if spec:
+                        specs.append(spec)
+                        continue
+
                 if self.accept(ID):
                     self.previous = self.token
                     if self.accept(EQ):
@@ -3899,12 +3915,18 @@ class SpecParser(spack.parse.Parser):
                         self.push_tokens([self.token])
                         specs.append(self.spec(None))
                     else:
-                        if self.accept(HASH):
+                        dep = None
+                        if self.accept(FILE):
+                            # this may return None, in which case we backtrack
+                            dep = self.spec_from_file()
+
+                        if not dep and self.accept(HASH):
                             # We're finding a dependency by hash for an
                             # anonymous spec
                             dep = self.spec_by_hash()
                             dep = dep.copy(deps=('link', 'run'))
-                        else:
+
+                        if not dep:
                             # We're adding a dependency to the last spec
                             self.expect(ID)
                             dep = self.spec(self.token.value)
@@ -3944,6 +3966,51 @@ class SpecParser(spack.parse.Parser):
                     s._set_architecture(platform=platform_default)
         return specs
 
+    def spec_from_file(self):
+        """Read a spec from a filename parsed on the input stream.
+
+        There is some care taken here to ensure that filenames are a last
+        resort, and that any valid package name is parsed as a name
+        before we consider it as a file. Specs are used in lots of places;
+        we don't want the parser touching the filesystem unnecessarily.
+
+        The parse logic is as follows:
+
+        1. We require that filenames end in .yaml, which means that no valid
+           filename can be interpreted as a hash (hashes can't have '.')
+
+        2. We avoid treating paths like /path/to/spec.yaml as hashes, or paths
+           like subdir/spec.yaml as ids by lexing filenames before hashes.
+
+        3. For spec names that match file and id regexes, like 'builtin.yaml',
+           we backtrack from spec_from_file() and treat them as spec names.
+
+        """
+        path = self.token.value
+
+        # don't treat builtin.yaml, builtin.yaml-cpp, etc. as filenames
+        if re.match(spec_id_re + '$', path):
+            self.push_tokens([spack.parse.Token(ID, self.token.value)])
+            return None
+
+        # Special case where someone omits a space after a filename. Consider:
+        #
+        #     libdwarf^/some/path/to/libelf.yamllibdwarf ^../../libelf.yaml
+        #
+        # The error is clearly an omitted space. To handle this, the FILE
+        # regex admits text *beyond* .yaml, and we raise a nice error for
+        # file names that don't end in .yaml.
+        if not path.endswith(".yaml"):
+            raise SpecFilenameError(
+                "Spec filename must end in .yaml: '{0}'".format(path))
+
+        # if we get here, we're *finally* interpreting path as a filename
+        if not os.path.exists(path):
+            raise NoSuchSpecFileError("No such spec file: '{0}'".format(path))
+
+        with open(path) as f:
+            return Spec.from_yaml(f)
+
     def parse_compiler(self, text):
         self.setup(text)
         return self.compiler()
@@ -4281,6 +4348,14 @@ class NoSuchHashError(SpecError):
             % hash)
 
 
+class SpecFilenameError(SpecError):
+    """Raised when a spec file name is invalid."""
+
+
+class NoSuchSpecFileError(SpecFilenameError):
+    """Raised when a spec file doesn't exist."""
+
+
 class RedundantSpecError(SpecError):
     def __init__(self, spec, addition):
         super(RedundantSpecError, self).__init__(
diff --git a/lib/spack/spack/test/spec_syntax.py b/lib/spack/spack/test/spec_syntax.py
index 699ecc987c..238b21f535 100644
--- a/lib/spack/spack/test/spec_syntax.py
+++ b/lib/spack/spack/test/spec_syntax.py
@@ -3,9 +3,14 @@
 #
 # SPDX-License-Identifier: (Apache-2.0 OR MIT)
 
+import os
 import pytest
 import shlex
 
+import llnl.util.filesystem as fs
+
+import spack.hash_types as ht
+import spack.repo
 import spack.store
 import spack.spec as sp
 from spack.parse import Token
@@ -14,6 +19,7 @@ from spack.spec import SpecParseError, RedundantSpecError
 from spack.spec import AmbiguousHashError, InvalidHashError, NoSuchHashError
 from spack.spec import DuplicateArchitectureError, DuplicateVariantError
 from spack.spec import DuplicateDependencyError, DuplicateCompilerSpecError
+from spack.spec import SpecFilenameError, NoSuchSpecFileError
 
 
 # Sample output for a complex lexing.
@@ -109,6 +115,7 @@ class TestSpecSyntax(object):
     def _check_raises(self, exc_type, items):
         for item in items:
             with pytest.raises(exc_type):
+                print("CHECKING: ", item, "=======================")
                 Spec(item)
 
     # ========================================================================
@@ -433,6 +440,216 @@ class TestSpecSyntax(object):
         ]
         self._check_raises(DuplicateArchitectureError, duplicates)
 
+    @pytest.mark.usefixtures('config')
+    def test_parse_yaml_simple(self, mock_packages, tmpdir):
+        s = Spec('libdwarf')
+        s.concretize()
+
+        specfile = tmpdir.join('libdwarf.yaml')
+
+        with specfile.open('w') as f:
+            f.write(s.to_yaml(hash=ht.build_hash))
+
+        # Check an absolute path to spec.yaml by itself:
+        #     "spack spec /path/to/libdwarf.yaml"
+        specs = sp.parse(specfile.strpath)
+        assert len(specs) == 1
+
+        # Check absolute path to spec.yaml mixed with a clispec, e.g.:
+        #     "spack spec mvapich_foo /path/to/libdwarf.yaml"
+        specs = sp.parse('mvapich_foo {0}'.format(specfile.strpath))
+        assert len(specs) == 2
+
+    @pytest.mark.usefixtures('config')
+    def test_parse_filename_missing_slash_as_spec(self, mock_packages, tmpdir):
+        """Ensure that libelf.yaml parses as a spec, NOT a file."""
+        s = Spec('libelf')
+        s.concretize()
+
+        specfile = tmpdir.join('libelf.yaml')
+
+        # write the file to the current directory to make sure it exists,
+        # and that we still do not parse the spec as a file.
+        with specfile.open('w') as f:
+            f.write(s.to_yaml(hash=ht.build_hash))
+
+        # Check the spec `libelf.yaml` in the working directory, which
+        # should evaluate to a spec called `yaml` in the `libelf`
+        # namespace, NOT a spec for `libelf`.
+        with tmpdir.as_cwd():
+            specs = sp.parse("libelf.yaml")
+        assert len(specs) == 1
+
+        spec = specs[0]
+        assert spec.name == "yaml"
+        assert spec.namespace == "libelf"
+        assert spec.fullname == "libelf.yaml"
+
+        # check that if we concretize this spec, we get a good error
+        # message that mentions we might've meant a file.
+        with pytest.raises(spack.repo.UnknownPackageError) as exc_info:
+            spec.concretize()
+        assert exc_info.value.long_message
+        assert ("Did you mean to specify a filename with './libelf.yaml'?"
+                in exc_info.value.long_message)
+
+        # make sure that only happens when the spec ends in yaml
+        with pytest.raises(spack.repo.UnknownPackageError) as exc_info:
+            Spec('builtin.mock.doesnotexist').concretize()
+        assert (
+            not exc_info.value.long_message or (
+                "Did you mean to specify a filename with" not in
+                exc_info.value.long_message
+            )
+        )
+
+    @pytest.mark.usefixtures('config')
+    def test_parse_yaml_dependency(self, mock_packages, tmpdir):
+        s = Spec('libdwarf')
+        s.concretize()
+
+        specfile = tmpdir.join('libelf.yaml')
+
+        with specfile.open('w') as f:
+            f.write(s['libelf'].to_yaml(hash=ht.build_hash))
+
+        print("")
+        print("")
+        print("PARSING HERE")
+
+        # Make sure we can use yaml path as dependency, e.g.:
+        #     "spack spec libdwarf ^ /path/to/libelf.yaml"
+        specs = sp.parse('libdwarf ^ {0}'.format(specfile.strpath))
+        assert len(specs) == 1
+
+    @pytest.mark.usefixtures('config')
+    def test_parse_yaml_relative_paths(self, mock_packages, tmpdir):
+        s = Spec('libdwarf')
+        s.concretize()
+
+        specfile = tmpdir.join('libdwarf.yaml')
+
+        with specfile.open('w') as f:
+            f.write(s.to_yaml(hash=ht.build_hash))
+
+        file_name = specfile.basename
+        parent_dir = os.path.basename(specfile.dirname)
+
+        # Relative path to specfile
+        with fs.working_dir(specfile.dirname):
+            # Test for command like: "spack spec libelf.yaml"
+            # This should parse a single spec, but should not concretize.
+            # See test_parse_filename_missing_slash_as_spec()
+            specs = sp.parse('{0}'.format(file_name))
+            assert len(specs) == 1
+
+            # Make sure this also works: "spack spec ./libelf.yaml"
+            specs = sp.parse('./{0}'.format(file_name))
+            assert len(specs) == 1
+
+            # Should also be accepted: "spack spec ../<cur-dir>/libelf.yaml"
+            specs = sp.parse('../{0}/{1}'.format(parent_dir, file_name))
+            assert len(specs) == 1
+
+            # Should also handle mixed clispecs and relative paths, e.g.:
+            #     "spack spec mvapich_foo ../<cur-dir>/libelf.yaml"
+            specs = sp.parse('mvapich_foo ../{0}/{1}'.format(
+                parent_dir, file_name))
+            assert len(specs) == 2
+
+    @pytest.mark.usefixtures('config')
+    def test_parse_yaml_relative_subdir_path(self, mock_packages, tmpdir):
+        s = Spec('libdwarf')
+        s.concretize()
+
+        specfile = tmpdir.mkdir('subdir').join('libdwarf.yaml')
+
+        with specfile.open('w') as f:
+            f.write(s.to_yaml(hash=ht.build_hash))
+
+        file_name = specfile.basename
+
+        # Relative path to specfile
+        with tmpdir.as_cwd():
+            assert os.path.exists('subdir/{0}'.format(file_name))
+
+            # Test for command like: "spack spec libelf.yaml"
+            specs = sp.parse('subdir/{0}'.format(file_name))
+            assert len(specs) == 1
+
+    @pytest.mark.usefixtures('config')
+    def test_parse_yaml_dependency_relative_paths(self, mock_packages, tmpdir):
+        s = Spec('libdwarf')
+        s.concretize()
+
+        specfile = tmpdir.join('libelf.yaml')
+
+        with specfile.open('w') as f:
+            f.write(s['libelf'].to_yaml(hash=ht.build_hash))
+
+        file_name = specfile.basename
+        parent_dir = os.path.basename(specfile.dirname)
+
+        # Relative path to specfile
+        with fs.working_dir(specfile.dirname):
+            # Test for command like: "spack spec libelf.yaml"
+            specs = sp.parse('libdwarf^{0}'.format(file_name))
+            assert len(specs) == 1
+
+            # Make sure this also works: "spack spec ./libelf.yaml"
+            specs = sp.parse('libdwarf^./{0}'.format(file_name))
+            assert len(specs) == 1
+
+            # Should also be accepted: "spack spec ../<cur-dir>/libelf.yaml"
+            specs = sp.parse('libdwarf^../{0}/{1}'.format(
+                parent_dir, file_name))
+            assert len(specs) == 1
+
+    def test_parse_yaml_error_handling(self):
+        self._check_raises(NoSuchSpecFileError, [
+            # Single spec that looks like a yaml path
+            '/bogus/path/libdwarf.yaml',
+            '../../libdwarf.yaml',
+            './libdwarf.yaml',
+            # Dependency spec that looks like a yaml path
+            'libdwarf^/bogus/path/libelf.yaml',
+            'libdwarf ^../../libelf.yaml',
+            'libdwarf^ ./libelf.yaml',
+            # Multiple specs, one looks like a yaml path
+            'mvapich_foo /bogus/path/libelf.yaml',
+            'mvapich_foo ../../libelf.yaml',
+            'mvapich_foo ./libelf.yaml',
+        ])
+
+    def test_nice_error_for_no_space_after_spec_filename(self):
+        """Ensure that omitted spaces don't give weird errors about hashes."""
+        self._check_raises(SpecFilenameError, [
+            '/bogus/path/libdwarf.yamlfoobar',
+            'libdwarf^/bogus/path/libelf.yamlfoobar ^/path/to/bogus.yaml',
+        ])
+
+    @pytest.mark.usefixtures('config')
+    def test_yaml_spec_not_filename(self, mock_packages, tmpdir):
+        with pytest.raises(spack.repo.UnknownPackageError):
+            Spec('builtin.mock.yaml').concretize()
+
+        with pytest.raises(spack.repo.UnknownPackageError):
+            Spec('builtin.mock.yamlfoobar').concretize()
+
+    @pytest.mark.usefixtures('config')
+    def test_parse_yaml_variant_error(self, mock_packages, tmpdir):
+        s = Spec('a')
+        s.concretize()
+
+        specfile = tmpdir.join('a.yaml')
+
+        with specfile.open('w') as f:
+            f.write(s.to_yaml(hash=ht.build_hash))
+
+        with pytest.raises(RedundantSpecError):
+            # Trying to change a variant on a concrete spec is an error
+            sp.parse('{0} ~bvv'.format(specfile.strpath))
+
     # ========================================================================
     # Lex checks
     # ========================================================================