core: differentiate package-level fetch URLs by args to `version()`

- packagers can specify two top-level fetch URLs if one is `url`
  - e.g., `url` and `git` or `url` and `svn`

- allow only one VCS fetcher so we can differentiate between URL and VCS.

- also clean up fetcher logic and class structure

2 files changed, 186 insertions, 84 deletions

diff --git a/lib/spack/spack/fetch_strategy.py b/lib/spack/spack/fetch_strategy.py
index 6a3ea6bfbc..3245f56482 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, comma_and, quote
+from spack.util.string import comma_and, quote
 from spack.version import Version, ver
 from spack.util.compression import decompressor_for, extension
 
@@ -89,7 +89,15 @@ class FSMeta(type):
 class FetchStrategy(with_metaclass(FSMeta, object)):
     """Superclass of all fetch strategies."""
     enabled = False  # Non-abstract subclasses should be enabled.
-    required_attributes = None  # Attributes required in version() args.
+
+    #: The URL attribute must be specified either at the package class
+    #: level, or as a keyword argument to ``version()``.  It is used to
+    #: distinguish fetchers for different versions in the package DSL.
+    url_attr = None
+
+    #: Optional attributes can be used to distinguish fetchers when :
+    #: classes have multiple ``url_attrs`` at the top-level.
+    optional_attrs = []  # optional attributes in version() args.
 
     def __init__(self):
         # The stage is initialized late, so that fetch strategies can be
@@ -156,7 +164,7 @@ class FetchStrategy(with_metaclass(FSMeta, object)):
     # arguments in packages.
     @classmethod
     def matches(cls, args):
-        return any(k in args for k in cls.required_attributes)
+        return cls.url_attr in args
 
 
 @pattern.composite(interface=FetchStrategy)
@@ -179,7 +187,11 @@ class URLFetchStrategy(FetchStrategy):
        checks the archive against a checksum,and decompresses the archive.
     """
     enabled = True
-    required_attributes = ['url']
+    url_attr = 'url'
+
+    # these are checksum types. The generic 'checksum' is deprecated for
+    # specific hash names, but we need it for backward compatibility
+    optional_attrs = list(crypto.hashes.keys()) + ['checksum']
 
     def __init__(self, url=None, checksum=None, **kwargs):
         super(URLFetchStrategy, self).__init__()
@@ -190,7 +202,7 @@ class URLFetchStrategy(FetchStrategy):
         # digest can be set as the first argument, or from an explicit
         # kwarg by the hash name.
         self.digest = kwargs.get('checksum', checksum)
-        for h in crypto.hashes:
+        for h in self.optional_attrs:
             if h in kwargs:
                 self.digest = kwargs[h]
 
@@ -419,9 +431,6 @@ class URLFetchStrategy(FetchStrategy):
 class CacheURLFetchStrategy(URLFetchStrategy):
     """The resource associated with a cache URL may be out of date."""
 
-    def __init__(self, *args, **kwargs):
-        super(CacheURLFetchStrategy, self).__init__(*args, **kwargs)
-
     @_needs_stage
     def fetch(self):
         path = re.sub('^file://', '', self.url)
@@ -452,35 +461,38 @@ class CacheURLFetchStrategy(URLFetchStrategy):
 
 
 class VCSFetchStrategy(FetchStrategy):
+    """Superclass for version control system fetch strategies.
+
+    Like all fetchers, VCS fetchers are identified by the attributes
+    passed to the ``version`` directive.  The optional_attrs for a VCS
+    fetch strategy represent types of revisions, e.g. tags, branches,
+    commits, etc.
+
+    The required attributes (git, svn, etc.) are used to specify the URL
+    and to distinguish a VCS fetch strategy from a URL fetch strategy.
 
-    def __init__(self, name, *rev_types, **kwargs):
+    """
+
+    def __init__(self, **kwargs):
         super(VCSFetchStrategy, self).__init__()
-        self.name = name
 
         # Set a URL based on the type of fetch strategy.
-        self.url = kwargs.get(name, None)
+        self.url = kwargs.get(self.url_attr, None)
         if not self.url:
             raise ValueError(
-                "%s requires %s argument." % (self.__class__, name))
-
-        # Ensure that there's only one of the rev_types
-        if sum(k in kwargs for k in rev_types) > 1:
-            raise ValueError(
-                "Supply only one of %s to fetch with %s" % (
-                    comma_or(rev_types), name
-                ))
+                "%s requires %s argument." % (self.__class__, self.url_attr))
 
-        # Set attributes for each rev type.
-        for rt in rev_types:
-            setattr(self, rt, kwargs.get(rt, None))
+        for attr in self.optional_attrs:
+            setattr(self, attr, kwargs.get(attr, None))
 
     @_needs_stage
     def check(self):
-        tty.msg("No checksum needed when fetching with %s" % self.name)
+        tty.msg("No checksum needed when fetching with %s" % self.url_attr)
 
     @_needs_stage
     def expand(self):
-        tty.debug("Source fetched with %s is already expanded." % self.name)
+        tty.debug(
+            "Source fetched with %s is already expanded." % self.url_attr)
 
     @_needs_stage
     def archive(self, destination, **kwargs):
@@ -517,15 +529,15 @@ class GoFetchStrategy(VCSFetchStrategy):
     Go get does not natively support versions, they can be faked with git
     """
     enabled = True
-    required_attributes = ('go', )
+    url_attr = 'go'
 
     def __init__(self, **kwargs):
         # Discards the keywords in kwargs that may conflict with the next
         # call to __init__
         forwarded_args = copy.copy(kwargs)
         forwarded_args.pop('name', None)
+        super(GoFetchStrategy, self).__init__(**forwarded_args)
 
-        super(GoFetchStrategy, self).__init__('go', **forwarded_args)
         self._go = None
 
     @property
@@ -583,16 +595,16 @@ class GitFetchStrategy(VCSFetchStrategy):
         * ``commit``: Particular commit hash in the repo
     """
     enabled = True
-    required_attributes = ('git', )
+    url_attr = 'git'
+    optional_attrs = ['tag', 'branch', 'commit']
 
     def __init__(self, **kwargs):
         # Discards the keywords in kwargs that may conflict with the next call
         # to __init__
         forwarded_args = copy.copy(kwargs)
         forwarded_args.pop('name', None)
+        super(GitFetchStrategy, self).__init__(**forwarded_args)
 
-        super(GitFetchStrategy, self).__init__(
-            'git', 'tag', 'branch', 'commit', **forwarded_args)
         self._git = None
         self.submodules = kwargs.get('submodules', False)
 
@@ -745,16 +757,16 @@ class SvnFetchStrategy(VCSFetchStrategy):
                    revision='1641')
     """
     enabled = True
-    required_attributes = ['svn']
+    url_attr = 'svn'
+    optional_attrs = ['revision']
 
     def __init__(self, **kwargs):
         # Discards the keywords in kwargs that may conflict with the next call
         # to __init__
         forwarded_args = copy.copy(kwargs)
         forwarded_args.pop('name', None)
+        super(SvnFetchStrategy, self).__init__(**forwarded_args)
 
-        super(SvnFetchStrategy, self).__init__(
-            'svn', 'revision', **forwarded_args)
         self._svn = None
         if self.revision is not None:
             self.revision = str(self.revision)
@@ -845,16 +857,16 @@ class HgFetchStrategy(VCSFetchStrategy):
         * ``revision``: Particular revision, branch, or tag.
     """
     enabled = True
-    required_attributes = ['hg']
+    url_attr = 'hg'
+    optional_attrs = ['revision']
 
     def __init__(self, **kwargs):
         # Discards the keywords in kwargs that may conflict with the next call
         # to __init__
         forwarded_args = copy.copy(kwargs)
         forwarded_args.pop('name', None)
+        super(HgFetchStrategy, self).__init__(**forwarded_args)
 
-        super(HgFetchStrategy, self).__init__(
-            'hg', 'revision', **forwarded_args)
         self._hg = None
 
     @property
@@ -957,74 +969,88 @@ def from_kwargs(**kwargs):
     for fetcher in all_strategies:
         if fetcher.matches(kwargs):
             return fetcher(**kwargs)
-    # Raise an error in case we can't instantiate any known strategy
-    message = "Cannot instantiate any FetchStrategy"
-    long_message = message + " from the given arguments : {arguments}".format(
-        arguments=kwargs)
-    raise FetchError(message, long_message)
 
+    raise InvalidArgsError(**kwargs)
 
-def args_are_for(args, fetcher):
-    fetcher.matches(args)
 
+def check_pkg_attributes(pkg):
+    """Find ambiguous top-level fetch attributes in a package.
 
-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],
-            []))
+    Currently this only ensures that two or more VCS fetch strategies are
+    not specified at once.
+    """
+    # a single package cannot have URL attributes for multiple VCS fetch
+    # strategies *unless* they are the same attribute.
+    conflicts = set([s.url_attr for s in all_strategies
+                     if hasattr(pkg, s.url_attr)])
+
+    # URL isn't a VCS fetch method. We can use it with a VCS method.
+    conflicts -= set(['url'])
 
     if len(conflicts) > 1:
         raise FetcherConflict(
-            'Package %s cannot specify %s together. Must pick only one.'
+            'Package %s cannot specify %s together. Pick at most one.'
             % (pkg.name, comma_and(quote(conflicts))))
 
 
+def _extrapolate(pkg, version):
+    """Create a fetcher from an extrapolated URL for this version."""
+    try:
+        return URLFetchStrategy(pkg.url_for_version(version))
+    except spack.package.NoURLError:
+        msg = ("Can't extrapolate a URL for version %s "
+               "because package %s defines no URLs")
+        raise ExtrapolationError(msg % (version, pkg.name))
+
+
+def _from_merged_attrs(fetcher, pkg, version):
+    """Create a fetcher from merged package and version attributes."""
+    if fetcher.url_attr == 'url':
+        url = pkg.url_for_version(version)
+    else:
+        url = getattr(pkg, fetcher.url_attr)
+
+    attrs = {fetcher.url_attr: url}
+    attrs.update(pkg.versions[version])
+    return fetcher(**attrs)
+
+
 def for_package_version(pkg, version):
     """Determine a fetch strategy based on the arguments supplied to
        version() in the package description."""
-    check_attributes(pkg)
+    check_pkg_attributes(pkg)
 
     if not isinstance(version, Version):
         version = Version(version)
 
-    # If it's not a known version, extrapolate one by URL.
+    # If it's not a known version, try to extrapolate one by URL
     if version not in pkg.versions:
-        try:
-            url = pkg.url_for_version(version)
-        except spack.package.NoURLError:
-            msg = ("Can't extrapolate a URL for version %s "
-                   "because package %s defines no URLs")
-            raise ExtrapolationError(msg % (version, pkg.name))
-
-        if not url:
-            raise InvalidArgsError(pkg, version)
-        return URLFetchStrategy(url)
+        return _extrapolate(pkg, version)
 
     # Grab a dict of args out of the package version dict
     args = pkg.versions[version]
 
-    # Test all strategies against per-version arguments.
+    # If the version specifies a `url_attr` directly, use that.
     for fetcher in all_strategies:
-        if fetcher.matches(args):
+        if fetcher.url_attr in args:
             return fetcher(**args)
 
-    # If nothing matched for a *specific* version, test all strategies
-    # against attributes in the version directives and on the package
+    # if a version's optional attributes imply a particular fetch
+    # strategy, and we have the `url_attr`, then use that strategy.
+    for fetcher in all_strategies:
+        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):
+                return _from_merged_attrs(fetcher, pkg, version)
+
+    # if the optional attributes tell us nothing, then use any `url_attr`
+    # on the package.  This prefers URL vs. VCS, b/c URLFetchStrategy is
+    # defined first in this file.
     for fetcher in all_strategies:
-        attrs = dict((attr, getattr(pkg, attr))
-                     for attr in fetcher.required_attributes
-                     if hasattr(pkg, attr))
-        if 'url' in attrs:
-            attrs['url'] = pkg.url_for_version(version)
-        attrs.update(args)
-        if fetcher.matches(attrs):
-            return fetcher(**attrs)
+        if hasattr(pkg, fetcher.url_attr):
+            return _from_merged_attrs(fetcher, pkg, version)
 
-    raise InvalidArgsError(pkg, version)
+    raise InvalidArgsError(pkg, version, **args)
 
 
 def from_list_url(pkg):
@@ -1116,11 +1142,15 @@ class FetcherConflict(FetchError):
 
 
 class InvalidArgsError(FetchError):
-    def __init__(self, pkg, version):
-        msg = ("Could not construct a fetch strategy for package %s at "
-               "version %s")
-        msg %= (pkg.name, version)
-        super(InvalidArgsError, self).__init__(msg)
+    """Raised when a version can't be deduced from a set of arguments."""
+    def __init__(self, pkg=None, version=None, **args):
+        msg = "Could not guess a fetch strategy"
+        if pkg:
+            msg += ' for {pkg}'.format(pkg=pkg)
+            if version:
+                msg += '@{version}'.format(version=version)
+        long_msg = 'with arguments: {args}'.format(args=args)
+        super(InvalidArgsError, self).__init__(msg, long_msg)
 
 
 class ChecksumError(FetchError):
diff --git a/lib/spack/spack/test/packages.py b/lib/spack/spack/test/packages.py
index 2769ceb70d..d50de07a57 100644
--- a/lib/spack/spack/test/packages.py
+++ b/lib/spack/spack/test/packages.py
@@ -263,9 +263,81 @@ def test_no_extrapolate_without_url(mock_packages, config):
         spack.fetch_strategy.for_package_version(pkg, '1.1')
 
 
-def test_git_and_url_top_level(mock_packages, config):
-    """Verify conflict when url and git are specified together."""
+def test_two_vcs_fetchers_top_level(mock_packages, config):
+    """Verify conflict when two VCS strategies are specified together."""
 
-    pkg = spack.repo.get('git-and-url-top-level')
+    pkg = spack.repo.get('git-url-svn-top-level')
     with pytest.raises(spack.fetch_strategy.FetcherConflict):
         spack.fetch_strategy.for_package_version(pkg, '1.0')
+
+    pkg = spack.repo.get('git-svn-top-level')
+    with pytest.raises(spack.fetch_strategy.FetcherConflict):
+        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."""
+
+    pkg = spack.repo.get('git-url-top-level')
+
+    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'
+    assert fetcher.digest == 'abc20'
+
+    fetcher = spack.fetch_strategy.for_package_version(pkg, '2.1')
+    assert isinstance(fetcher, spack.fetch_strategy.URLFetchStrategy)
+    assert fetcher.url == 'https://example.com/some/tarball-2.1.tar.gz'
+    assert fetcher.digest == 'abc21'
+
+    fetcher = spack.fetch_strategy.for_package_version(pkg, '2.2')
+    assert isinstance(fetcher, spack.fetch_strategy.URLFetchStrategy)
+    assert fetcher.url == 'https://www.example.com/foo2.2.tar.gz'
+    assert fetcher.digest == 'abc22'
+
+    fetcher = spack.fetch_strategy.for_package_version(pkg, '2.3')
+    assert isinstance(fetcher, spack.fetch_strategy.URLFetchStrategy)
+    assert fetcher.url == 'https://www.example.com/foo2.3.tar.gz'
+    assert fetcher.digest == 'abc23'
+
+    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'
+    assert fetcher.tag == 'v3.0'
+    assert fetcher.commit is None
+    assert fetcher.branch is None
+
+    fetcher = spack.fetch_strategy.for_package_version(pkg, '3.1')
+    assert isinstance(fetcher, spack.fetch_strategy.GitFetchStrategy)
+    assert fetcher.url == 'https://example.com/some/git/repo'
+    assert fetcher.tag == 'v3.1'
+    assert fetcher.commit == 'abc31'
+    assert fetcher.branch is None
+
+    fetcher = spack.fetch_strategy.for_package_version(pkg, '3.2')
+    assert isinstance(fetcher, spack.fetch_strategy.GitFetchStrategy)
+    assert fetcher.url == 'https://example.com/some/git/repo'
+    assert fetcher.tag is None
+    assert fetcher.commit is None
+    assert fetcher.branch == 'releases/v3.2'
+
+    fetcher = spack.fetch_strategy.for_package_version(pkg, '3.3')
+    assert isinstance(fetcher, spack.fetch_strategy.GitFetchStrategy)
+    assert fetcher.url == 'https://example.com/some/git/repo'
+    assert fetcher.tag is None
+    assert fetcher.commit == 'abc33'
+    assert fetcher.branch == 'releases/v3.3'
+
+    fetcher = spack.fetch_strategy.for_package_version(pkg, '3.4')
+    assert isinstance(fetcher, spack.fetch_strategy.GitFetchStrategy)
+    assert fetcher.url == 'https://example.com/some/git/repo'
+    assert fetcher.tag is None
+    assert fetcher.commit == 'abc34'
+    assert fetcher.branch is None
+
+    fetcher = spack.fetch_strategy.for_package_version(pkg, 'develop')
+    assert isinstance(fetcher, spack.fetch_strategy.GitFetchStrategy)
+    assert fetcher.url == 'https://example.com/some/git/repo'
+    assert fetcher.tag is None
+    assert fetcher.commit is None
+    assert fetcher.branch == 'develop'