tests: add checks and tests for consistent version() arguments

- Previously, Spack didn't check the arguments you put in version()
  directives.

- So, you could do something like this, where there are arguments for a
  URL fetcher AND for a git fetcher:

  version('1.0', md5='abc123', git='https://foo.bar', commit='feda2343')

- Now, we check the arguments before constructing a fetcher, to ensure
  that each package has *only* arguments for a single type of fetcher.

- Also added `test_package_version_consistency()` to the `package_sanity`
  test, so that all builtin packages are required to have valid
  `version()` directives.

3 files changed, 60 insertions, 2 deletions

diff --git a/lib/spack/spack/fetch_strategy.py b/lib/spack/spack/fetch_strategy.py
index 3245f56482..38b837b659 100644
--- a/lib/spack/spack/fetch_strategy.py
+++ b/lib/spack/spack/fetch_strategy.py
@@ -993,6 +993,27 @@ def check_pkg_attributes(pkg):
             % (pkg.name, comma_and(quote(conflicts))))
 
 
+def _check_version_attributes(fetcher, pkg, version):
+    """Ensure that the fetcher for a version is not ambiguous.
+
+    This assumes that we have already determined the fetcher for the
+    specific version using ``for_package_version()``
+    """
+    all_optionals = set(a for s in all_strategies for a in s.optional_attrs)
+
+    args = pkg.versions[version]
+    extra = set(args) - set(fetcher.optional_attrs) - set([fetcher.url_attr])
+    extra.intersection_update(all_optionals)
+
+    if extra:
+        legal_attrs = [fetcher.url_attr] + list(fetcher.optional_attrs)
+        raise FetcherConflict(
+            "%s version '%s' has extra arguments: %s"
+            % (pkg.name, version, comma_and(quote(extra))),
+            "Valid arguments for a %s fetcher are: \n    %s"
+            % (fetcher.url_attr, comma_and(quote(legal_attrs))))
+
+
 def _extrapolate(pkg, version):
     """Create a fetcher from an extrapolated URL for this version."""
     try:
@@ -1033,6 +1054,7 @@ def for_package_version(pkg, version):
     # If the version specifies a `url_attr` directly, use that.
     for fetcher in all_strategies:
         if fetcher.url_attr in args:
+            _check_version_attributes(fetcher, pkg, version)
             return fetcher(**args)
 
     # if a version's optional attributes imply a particular fetch
@@ -1041,6 +1063,7 @@ def for_package_version(pkg, version):
         if hasattr(pkg, fetcher.url_attr) or fetcher.url_attr == 'url':
             optionals = fetcher.optional_attrs
             if optionals and any(a in args for a in optionals):
+                _check_version_attributes(fetcher, pkg, version)
                 return _from_merged_attrs(fetcher, pkg, version)
 
     # if the optional attributes tell us nothing, then use any `url_attr`
@@ -1048,6 +1071,7 @@ def for_package_version(pkg, version):
     # defined first in this file.
     for fetcher in all_strategies:
         if hasattr(pkg, fetcher.url_attr):
+            _check_version_attributes(fetcher, pkg, version)
             return _from_merged_attrs(fetcher, pkg, version)
 
     raise InvalidArgsError(pkg, version, **args)
diff --git a/lib/spack/spack/test/package_sanity.py b/lib/spack/spack/test/package_sanity.py
index 0c26fd7d95..74890664a6 100644
--- a/lib/spack/spack/test/package_sanity.py
+++ b/lib/spack/spack/test/package_sanity.py
@@ -29,6 +29,7 @@ import pytest
 
 import spack.paths
 import spack.repo
+import spack.fetch_strategy
 
 
 def check_repo():
@@ -68,3 +69,12 @@ def test_all_virtual_packages_have_default_providers():
 
     for provider in providers:
         assert provider in default_providers
+
+
+def test_package_version_consistency():
+    """Make sure all versions on builtin packages can produce a fetcher."""
+    for name in spack.repo.all_package_names():
+        pkg = spack.repo.get(name)
+        spack.fetch_strategy.check_pkg_attributes(pkg)
+        for version in pkg.versions:
+            assert spack.fetch_strategy.for_package_version(pkg, version)
diff --git a/lib/spack/spack/test/packages.py b/lib/spack/spack/test/packages.py
index d50de07a57..464e37a8d2 100644
--- a/lib/spack/spack/test/packages.py
+++ b/lib/spack/spack/test/packages.py
@@ -275,8 +275,8 @@ def test_two_vcs_fetchers_top_level(mock_packages, config):
         spack.fetch_strategy.for_package_version(pkg, '1.0')
 
 
-def test_git_url_top_level(mock_packages, config):
-    """Test fetch strategy inference when url is specified with a VCS."""
+def test_git_url_top_level_url_versions(mock_packages, config):
+    """Test URL fetch strategy inference when url is specified with git."""
 
     pkg = spack.repo.get('git-url-top-level')
 
@@ -300,6 +300,12 @@ def test_git_url_top_level(mock_packages, config):
     assert fetcher.url == 'https://www.example.com/foo2.3.tar.gz'
     assert fetcher.digest == 'abc23'
 
+
+def test_git_url_top_level_git_versions(mock_packages, config):
+    """Test git fetch strategy inference when url is specified with git."""
+
+    pkg = spack.repo.get('git-url-top-level')
+
     fetcher = spack.fetch_strategy.for_package_version(pkg, '3.0')
     assert isinstance(fetcher, spack.fetch_strategy.GitFetchStrategy)
     assert fetcher.url == 'https://example.com/some/git/repo'
@@ -341,3 +347,21 @@ def test_git_url_top_level(mock_packages, config):
     assert fetcher.tag is None
     assert fetcher.commit is None
     assert fetcher.branch == 'develop'
+
+
+def test_git_url_top_level_conflicts(mock_packages, config):
+    """Test git fetch strategy inference when url is specified with git."""
+
+    pkg = spack.repo.get('git-url-top-level')
+
+    with pytest.raises(spack.fetch_strategy.FetcherConflict):
+        spack.fetch_strategy.for_package_version(pkg, '1.0')
+
+    with pytest.raises(spack.fetch_strategy.FetcherConflict):
+        spack.fetch_strategy.for_package_version(pkg, '1.1')
+
+    with pytest.raises(spack.fetch_strategy.FetcherConflict):
+        spack.fetch_strategy.for_package_version(pkg, '1.2')
+
+    with pytest.raises(spack.fetch_strategy.FetcherConflict):
+        spack.fetch_strategy.for_package_version(pkg, '1.3')