core: add check for conflicting top-level fetch attributes in packages

- ensure that packages can't have more than one of git, hg, svn, or url

4 files changed, 35 insertions, 6 deletions

diff --git a/lib/spack/spack/fetch_strategy.py b/lib/spack/spack/fetch_strategy.py
index a497342379..6a3ea6bfbc 100644
--- a/lib/spack/spack/fetch_strategy.py
+++ b/lib/spack/spack/fetch_strategy.py
@@ -56,7 +56,7 @@ import spack.error
 import spack.util.crypto as crypto
 import spack.util.pattern as pattern
 from spack.util.executable import which
-from spack.util.string import comma_or
+from spack.util.string import comma_or, comma_and, quote
 from spack.version import Version, ver
 from spack.util.compression import decompressor_for, extension
 
@@ -968,9 +968,26 @@ def args_are_for(args, fetcher):
     fetcher.matches(args)
 
 
+def check_attributes(pkg):
+    """Find ambiguous top-level fetch attributes in a package."""
+    # a single package cannot have required attributes from multiple
+    # fetch strategies *unless* they are the same attribute.
+    conflicts = set(
+        sum([[a for a in s.required_attributes if hasattr(pkg, a)]
+             for s in all_strategies],
+            []))
+
+    if len(conflicts) > 1:
+        raise FetcherConflict(
+            'Package %s cannot specify %s together. Must pick only one.'
+            % (pkg.name, comma_and(quote(conflicts))))
+
+
 def for_package_version(pkg, version):
     """Determine a fetch strategy based on the arguments supplied to
        version() in the package description."""
+    check_attributes(pkg)
+
     if not isinstance(version, Version):
         version = Version(version)
 
@@ -1094,6 +1111,10 @@ class ExtrapolationError(FetchError):
     """Raised when we can't extrapolate a version for a package."""
 
 
+class FetcherConflict(FetchError):
+    """Raised for packages with invalid fetch attributes."""
+
+
 class InvalidArgsError(FetchError):
     def __init__(self, pkg, version):
         msg = ("Could not construct a fetch strategy for package %s at "
diff --git a/lib/spack/spack/repo.py b/lib/spack/spack/repo.py
index f00428f9a2..ec09262541 100644
--- a/lib/spack/spack/repo.py
+++ b/lib/spack/spack/repo.py
@@ -831,7 +831,12 @@ class Repo(object):
         package_class = self.get_pkg_class(spec.name)
         try:
             return package_class(spec)
+        except spack.error.SpackError:
+            # pass these through as their error messages will be fine.
+            raise
         except Exception:
+            # make sure other errors in constructors hit the error
+            # handler by wrapping them
             if spack.config.get('config:debug'):
                 sys.excepthook(*sys.exc_info())
             raise FailedConstructorError(spec.fullname, *sys.exc_info())
diff --git a/lib/spack/spack/test/packages.py b/lib/spack/spack/test/packages.py
index da46283593..2769ceb70d 100644
--- a/lib/spack/spack/test/packages.py
+++ b/lib/spack/spack/test/packages.py
@@ -264,9 +264,8 @@ def test_no_extrapolate_without_url(mock_packages, config):
 
 
 def test_git_and_url_top_level(mock_packages, config):
-    """Verify that URL takes precedence over other top-level attributes."""
-    pkg = spack.repo.get('git-and-url-top-level')
+    """Verify conflict when url and git are specified together."""
 
-    fetcher = spack.fetch_strategy.for_package_version(pkg, '2.0')
-    assert isinstance(fetcher, spack.fetch_strategy.URLFetchStrategy)
-    assert fetcher.url == 'https://example.com/some/tarball-2.0.tar.gz'
+    pkg = spack.repo.get('git-and-url-top-level')
+    with pytest.raises(spack.fetch_strategy.FetcherConflict):
+        spack.fetch_strategy.for_package_version(pkg, '1.0')
diff --git a/lib/spack/spack/util/string.py b/lib/spack/spack/util/string.py
index 93de553c32..3bab0c652e 100644
--- a/lib/spack/spack/util/string.py
+++ b/lib/spack/spack/util/string.py
@@ -49,3 +49,7 @@ def comma_or(sequence):
 
 def comma_and(sequence):
     return comma_list(sequence, 'and')
+
+
+def quote(sequence, q="'"):
+    return ['%s%s%s' % (q, e, q) for e in sequence]