core: fixes and tests for handling of fetcher attributes in packages

- Packages can remove the top-level `url` attribute and still work

- These are now legal:
  - Packages with *only* version-specific URLs (even with gaps)

  - Packages with a top-level git/hg/svn attribute and `version`
    directives for that.

- If a package has both a top-level hg/git/svn attribute AND a top-level
  url attribute, the url attribute takes precedence.

9 files changed, 389 insertions, 40 deletions

diff --git a/lib/spack/spack/fetch_strategy.py b/lib/spack/spack/fetch_strategy.py
index 8aa38a4c01..a497342379 100644
--- a/lib/spack/spack/fetch_strategy.py
+++ b/lib/spack/spack/fetch_strategy.py
@@ -971,9 +971,18 @@ def args_are_for(args, fetcher):
 def for_package_version(pkg, version):
     """Determine a fetch strategy based on the arguments supplied to
        version() in the package description."""
-    # If it's not a known version, extrapolate one.
+    if not isinstance(version, Version):
+        version = Version(version)
+
+    # If it's not a known version, extrapolate one by URL.
     if version not in pkg.versions:
-        url = pkg.url_for_version(version)
+        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)
@@ -987,10 +996,11 @@ def for_package_version(pkg, version):
             return fetcher(**args)
 
     # If nothing matched for a *specific* version, test all strategies
-    # against
+    # against attributes in the version directives and on the package
     for fetcher in all_strategies:
-        attrs = dict((attr, getattr(pkg, attr, None))
-                     for attr in fetcher.required_attributes)
+        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)
@@ -1080,6 +1090,10 @@ class NoDigestError(FetchError):
     """Raised after attempt to checksum when URL has no digest."""
 
 
+class ExtrapolationError(FetchError):
+    """Raised when we can't extrapolate a version for a package."""
+
+
 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/package.py b/lib/spack/spack/package.py
index 8d6b2b99f5..22d986a6d5 100644
--- a/lib/spack/spack/package.py
+++ b/lib/spack/spack/package.py
@@ -45,6 +45,7 @@ import time
 from six import StringIO
 from six import string_types
 from six import with_metaclass
+from ordereddict_backport import OrderedDict
 
 import llnl.util.tty as tty
 
@@ -487,13 +488,6 @@ class PackageBase(with_metaclass(PackageMeta, PackageViewMixin, object)):
         except ValueError as e:
             raise ValueError("In package %s: %s" % (self.name, e.message))
 
-        # stage used to build this package.
-        self._stage = None
-
-        # Init fetch strategy and url to None
-        self._fetcher = None
-        self.url = getattr(self.__class__, 'url', None)
-
         # Set a default list URL (place to find available versions)
         if not hasattr(self, 'list_url'):
             self.list_url = None
@@ -501,15 +495,17 @@ class PackageBase(with_metaclass(PackageMeta, PackageViewMixin, object)):
         if not hasattr(self, 'list_depth'):
             self.list_depth = 0
 
-        # Set up some internal variables for timing.
+        # init internal variables
+        self._stage = None
+        self._fetcher = None
+
+        # Set up timing variables
         self._fetch_time = 0.0
         self._total_time = 0.0
 
         if self.is_extension:
             spack.repo.get(self.extendee_spec)._check_extendable()
 
-        self.extra_args = {}
-
         super(PackageBase, self).__init__()
 
     def possible_dependencies(
@@ -577,17 +573,46 @@ class PackageBase(with_metaclass(PackageMeta, PackageViewMixin, object)):
 
     @memoized
     def version_urls(self):
-        """Return a list of URLs for different versions of this
-           package, sorted by version.  A version's URL only appears
-           in this list if it has an explicitly defined URL."""
-        version_urls = {}
-        for v in sorted(self.versions):
-            args = self.versions[v]
+        """OrderedDict of explicitly defined URLs for versions of this package.
+
+        Return:
+           An OrderedDict (version -> URL) different versions of this
+           package, sorted by version.
+
+        A version's URL only appears in the result if it has an an
+        explicitly defined ``url`` argument. So, this list may be empty
+        if a package only defines ``url`` at the top level.
+        """
+        version_urls = OrderedDict()
+        for v, args in sorted(self.versions.items()):
             if 'url' in args:
                 version_urls[v] = args['url']
         return version_urls
 
-    # TODO: move this out of here and into some URL extrapolation module?
+    def nearest_url(self, version):
+        """Finds the URL with the "closest" version to ``version``.
+
+        This uses the following precedence order:
+
+          1. Find the next lowest or equal version with a URL.
+          2. If no lower URL, return the next *higher* URL.
+          3. If no higher URL, return None.
+
+        """
+        version_urls = self.version_urls()
+
+        if version in version_urls:
+            return version_urls[version]
+
+        last_url = None
+        for v, u in self.version_urls().items():
+            if v > version:
+                if last_url:
+                    return last_url
+            last_url = u
+
+        return last_url
+
     def url_for_version(self, version):
         """Returns a URL from which the specified version of this package
         may be downloaded.
@@ -600,17 +625,22 @@ class PackageBase(with_metaclass(PackageMeta, PackageViewMixin, object)):
         if not isinstance(version, Version):
             version = Version(version)
 
-        cls = self.__class__
-        if not (hasattr(cls, 'url') or self.version_urls()):
-            raise NoURLError(cls)
-
         # If we have a specific URL for this version, don't extrapolate.
         version_urls = self.version_urls()
         if version in version_urls:
             return version_urls[version]
 
-        # If we have no idea, substitute the version into the default URL.
-        default_url = getattr(self.__class__, 'url', None)
+        # If no specific URL, use the default, class-level URL
+        default_url = getattr(self, 'url', None)
+
+        # if no exact match AND no class-level default, use the nearest URL
+        if not default_url:
+            default_url = self.nearest_url(version)
+
+            # if there are NO URLs to go by, then we can't do anything
+            if not default_url:
+                raise NoURLError(self.__class__)
+
         return spack.url.substitute_version(
             default_url, self.url_version(version))
 
diff --git a/lib/spack/spack/test/packages.py b/lib/spack/spack/test/packages.py
index 8f9838ce78..da46283593 100644
--- a/lib/spack/spack/test/packages.py
+++ b/lib/spack/spack/test/packages.py
@@ -26,6 +26,7 @@ import os.path
 import pytest
 
 import spack.repo
+import spack.fetch_strategy
 from spack.paths import mock_packages_path
 from spack.util.naming import mod_to_class
 from spack.spec import Spec
@@ -166,17 +167,106 @@ class TestPackage(object):
         import spack.pkg.builtin.mock as m              # noqa
         from spack.pkg.builtin import mock              # noqa
 
-    @pytest.mark.regression('2737')
-    def test_urls_for_versions(self):
-        # Checks that a version directive without a 'url' argument
-        # specified uses the default url
-        for spec_str in ('url_override@0.9.0', 'url_override@1.0.0'):
-            s = Spec(spec_str).concretized()
-            url = s.package.url_for_version('0.9.0')
-            assert url == 'http://www.anothersite.org/uo-0.9.0.tgz'
 
-            url = s.package.url_for_version('1.0.0')
-            assert url == 'http://www.doesnotexist.org/url_override-1.0.0.tar.gz'
+@pytest.mark.regression('2737')
+def test_urls_for_versions(mock_packages, config):
+    """Version directive without a 'url' argument should use default url."""
+    for spec_str in ('url_override@0.9.0', 'url_override@1.0.0'):
+        s = Spec(spec_str).concretized()
+        url = s.package.url_for_version('0.9.0')
+        assert url == 'http://www.anothersite.org/uo-0.9.0.tgz'
 
-            url = s.package.url_for_version('0.8.1')
-            assert url == 'http://www.doesnotexist.org/url_override-0.8.1.tar.gz'
+        url = s.package.url_for_version('1.0.0')
+        assert url == 'http://www.doesnotexist.org/url_override-1.0.0.tar.gz'
+
+        url = s.package.url_for_version('0.8.1')
+        assert url == 'http://www.doesnotexist.org/url_override-0.8.1.tar.gz'
+
+
+def test_url_for_version_with_no_urls():
+    pkg = spack.repo.get('git-test')
+    with pytest.raises(spack.package.NoURLError):
+        pkg.url_for_version('1.0')
+
+    with pytest.raises(spack.package.NoURLError):
+        pkg.url_for_version('1.1')
+
+
+def test_url_for_version_with_only_overrides(mock_packages, config):
+    spec = Spec('url-only-override')
+    spec.concretize()
+
+    pkg = spack.repo.get(spec)
+
+    # these exist and should just take the URL provided in the package
+    assert pkg.url_for_version('1.0.0') == 'http://a.example.com/url_override-1.0.0.tar.gz'
+    assert pkg.url_for_version('0.9.0') == 'http://b.example.com/url_override-0.9.0.tar.gz'
+    assert pkg.url_for_version('0.8.1') == 'http://c.example.com/url_override-0.8.1.tar.gz'
+
+    # these don't exist but should still work, even if there are only overrides
+    assert pkg.url_for_version('1.0.5') == 'http://a.example.com/url_override-1.0.5.tar.gz'
+    assert pkg.url_for_version('0.9.5') == 'http://b.example.com/url_override-0.9.5.tar.gz'
+    assert pkg.url_for_version('0.8.5') == 'http://c.example.com/url_override-0.8.5.tar.gz'
+    assert pkg.url_for_version('0.7.0') == 'http://c.example.com/url_override-0.7.0.tar.gz'
+
+
+def test_url_for_version_with_only_overrides_with_gaps(mock_packages, config):
+    spec = Spec('url-only-override-with-gaps')
+    spec.concretize()
+
+    pkg = spack.repo.get(spec)
+
+    # same as for url-only-override -- these are specific
+    assert pkg.url_for_version('1.0.0') == 'http://a.example.com/url_override-1.0.0.tar.gz'
+    assert pkg.url_for_version('0.9.0') == 'http://b.example.com/url_override-0.9.0.tar.gz'
+    assert pkg.url_for_version('0.8.1') == 'http://c.example.com/url_override-0.8.1.tar.gz'
+
+    # these don't have specific URLs, but should still work by extrapolation
+    assert pkg.url_for_version('1.0.5') == 'http://a.example.com/url_override-1.0.5.tar.gz'
+    assert pkg.url_for_version('0.9.5') == 'http://b.example.com/url_override-0.9.5.tar.gz'
+    assert pkg.url_for_version('0.8.5') == 'http://c.example.com/url_override-0.8.5.tar.gz'
+    assert pkg.url_for_version('0.7.0') == 'http://c.example.com/url_override-0.7.0.tar.gz'
+
+
+def test_git_top_level(mock_packages, config):
+    """Ensure that top-level git attribute can be used as a default."""
+    pkg = spack.repo.get('git-top-level')
+
+    fetcher = spack.fetch_strategy.for_package_version(pkg, '1.0')
+    assert isinstance(fetcher, spack.fetch_strategy.GitFetchStrategy)
+    assert fetcher.url == 'https://example.com/some/git/repo'
+
+
+def test_svn_top_level(mock_packages, config):
+    """Ensure that top-level svn attribute can be used as a default."""
+    pkg = spack.repo.get('svn-top-level')
+
+    fetcher = spack.fetch_strategy.for_package_version(pkg, '1.0')
+    assert isinstance(fetcher, spack.fetch_strategy.SvnFetchStrategy)
+    assert fetcher.url == 'https://example.com/some/svn/repo'
+
+
+def test_hg_top_level(mock_packages, config):
+    """Ensure that top-level hg attribute can be used as a default."""
+    pkg = spack.repo.get('hg-top-level')
+
+    fetcher = spack.fetch_strategy.for_package_version(pkg, '1.0')
+    assert isinstance(fetcher, spack.fetch_strategy.HgFetchStrategy)
+    assert fetcher.url == 'https://example.com/some/hg/repo'
+
+
+def test_no_extrapolate_without_url(mock_packages, config):
+    """Verify that we can't extrapolate versions for non-URL packages."""
+    pkg = spack.repo.get('git-top-level')
+
+    with pytest.raises(spack.fetch_strategy.ExtrapolationError):
+        spack.fetch_strategy.for_package_version(pkg, '1.1')
+
+
+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')
+
+    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'
diff --git a/var/spack/repos/builtin.mock/packages/git-and-url-top-level/package.py b/var/spack/repos/builtin.mock/packages/git-and-url-top-level/package.py
new file mode 100644
index 0000000000..d34c9d9b06
--- /dev/null
+++ b/var/spack/repos/builtin.mock/packages/git-and-url-top-level/package.py
@@ -0,0 +1,38 @@
+##############################################################################
+# Copyright (c) 2013-2018, Lawrence Livermore National Security, LLC.
+# Produced at the Lawrence Livermore National Laboratory.
+#
+# This file is part of Spack.
+# Created by Todd Gamblin, tgamblin@llnl.gov, All rights reserved.
+# LLNL-CODE-647188
+#
+# For details, see https://github.com/spack/spack
+# Please also see the NOTICE and LICENSE files for our notice and the LGPL.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU Lesser General Public License (as
+# published by the Free Software Foundation) version 2.1, February 1999.
+#
+# This program is distributed in the hope that it will be useful, but
+# WITHOUT ANY WARRANTY; without even the IMPLIED WARRANTY OF
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the terms and
+# conditions of the GNU Lesser General Public License for more details.
+#
+# You should have received a copy of the GNU Lesser General Public
+# License along with this program; if not, write to the Free Software
+# Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
+##############################################################################
+from spack import *
+
+
+class GitAndUrlTopLevel(Package):
+    """Mock package that uses git for fetching."""
+    homepage = "http://www.git-fetch-example.com"
+
+    git = 'https://example.com/some/git/repo'
+    url = 'https://example.com/some/tarball-1.0.tar.gz'
+
+    version('2.0')
+
+    def install(self, spec, prefix):
+        pass
diff --git a/var/spack/repos/builtin.mock/packages/git-top-level/package.py b/var/spack/repos/builtin.mock/packages/git-top-level/package.py
new file mode 100644
index 0000000000..5c0be81101
--- /dev/null
+++ b/var/spack/repos/builtin.mock/packages/git-top-level/package.py
@@ -0,0 +1,36 @@
+##############################################################################
+# Copyright (c) 2013-2018, Lawrence Livermore National Security, LLC.
+# Produced at the Lawrence Livermore National Laboratory.
+#
+# This file is part of Spack.
+# Created by Todd Gamblin, tgamblin@llnl.gov, All rights reserved.
+# LLNL-CODE-647188
+#
+# For details, see https://github.com/spack/spack
+# Please also see the NOTICE and LICENSE files for our notice and the LGPL.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU Lesser General Public License (as
+# published by the Free Software Foundation) version 2.1, February 1999.
+#
+# This program is distributed in the hope that it will be useful, but
+# WITHOUT ANY WARRANTY; without even the IMPLIED WARRANTY OF
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the terms and
+# conditions of the GNU Lesser General Public License for more details.
+#
+# You should have received a copy of the GNU Lesser General Public
+# License along with this program; if not, write to the Free Software
+# Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
+##############################################################################
+from spack import *
+
+
+class GitTopLevel(Package):
+    """Mock package that uses git for fetching."""
+    homepage = "http://www.git-fetch-example.com"
+
+    git = 'https://example.com/some/git/repo'
+    version('1.0')
+
+    def install(self, spec, prefix):
+        pass
diff --git a/var/spack/repos/builtin.mock/packages/hg-top-level/package.py b/var/spack/repos/builtin.mock/packages/hg-top-level/package.py
new file mode 100644
index 0000000000..94a06cd38e
--- /dev/null
+++ b/var/spack/repos/builtin.mock/packages/hg-top-level/package.py
@@ -0,0 +1,36 @@
+##############################################################################
+# Copyright (c) 2013-2018, Lawrence Livermore National Security, LLC.
+# Produced at the Lawrence Livermore National Laboratory.
+#
+# This file is part of Spack.
+# Created by Todd Gamblin, tgamblin@llnl.gov, All rights reserved.
+# LLNL-CODE-647188
+#
+# For details, see https://github.com/spack/spack
+# Please also see the NOTICE and LICENSE files for our notice and the LGPL.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU Lesser General Public License (as
+# published by the Free Software Foundation) version 2.1, February 1999.
+#
+# This program is distributed in the hope that it will be useful, but
+# WITHOUT ANY WARRANTY; without even the IMPLIED WARRANTY OF
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the terms and
+# conditions of the GNU Lesser General Public License for more details.
+#
+# You should have received a copy of the GNU Lesser General Public
+# License along with this program; if not, write to the Free Software
+# Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
+##############################################################################
+from spack import *
+
+
+class HgTopLevel(Package):
+    """Test package that does fetching with mercurial."""
+    homepage = "http://www.hg-fetch-example.com"
+
+    hg = 'https://example.com/some/hg/repo'
+    version('1.0')
+
+    def install(self, spec, prefix):
+        pass
diff --git a/var/spack/repos/builtin.mock/packages/svn-top-level/package.py b/var/spack/repos/builtin.mock/packages/svn-top-level/package.py
new file mode 100644
index 0000000000..3198b16435
--- /dev/null
+++ b/var/spack/repos/builtin.mock/packages/svn-top-level/package.py
@@ -0,0 +1,35 @@
+##############################################################################
+# Copyright (c) 2013-2018, Lawrence Livermore National Security, LLC.
+# Produced at the Lawrence Livermore National Laboratory.
+#
+# This file is part of Spack.
+# Created by Todd Gamblin, tgamblin@llnl.gov, All rights reserved.
+# LLNL-CODE-647188
+#
+# For details, see https://github.com/spack/spack
+# Please also see the NOTICE and LICENSE files for our notice and the LGPL.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU Lesser General Public License (as
+# published by the Free Software Foundation) version 2.1, February 1999.
+#
+# This program is distributed in the hope that it will be useful, but
+# WITHOUT ANY WARRANTY; without even the IMPLIED WARRANTY OF
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the terms and
+# conditions of the GNU Lesser General Public License for more details.
+#
+# You should have received a copy of the GNU Lesser General Public
+# License along with this program; if not, write to the Free Software
+# Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
+##############################################################################
+from spack import *
+
+
+class SvnTopLevel(Package):
+    """Mock package that uses svn for fetching."""
+
+    svn = 'https://example.com/some/svn/repo'
+    version('1.0')
+
+    def install(self, spec, prefix):
+        pass
diff --git a/var/spack/repos/builtin.mock/packages/url-only-override-with-gaps/package.py b/var/spack/repos/builtin.mock/packages/url-only-override-with-gaps/package.py
new file mode 100644
index 0000000000..8bcbb386bd
--- /dev/null
+++ b/var/spack/repos/builtin.mock/packages/url-only-override-with-gaps/package.py
@@ -0,0 +1,37 @@
+##############################################################################
+# Copyright (c) 2013-2018, Lawrence Livermore National Security, LLC.
+# Produced at the Lawrence Livermore National Laboratory.
+#
+# This file is part of Spack.
+# Created by Todd Gamblin, tgamblin@llnl.gov, All rights reserved.
+# LLNL-CODE-647188
+#
+# For details, see https://github.com/spack/spack
+# Please also see the NOTICE and LICENSE files for our notice and the LGPL.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU Lesser General Public License (as
+# published by the Free Software Foundation) version 2.1, February 1999.
+#
+# This program is distributed in the hope that it will be useful, but
+# WITHOUT ANY WARRANTY; without even the IMPLIED WARRANTY OF
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the terms and
+# conditions of the GNU Lesser General Public License for more details.
+#
+# You should have received a copy of the GNU Lesser General Public
+# License along with this program; if not, write to the Free Software
+# Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
+##############################################################################
+from spack import *
+
+
+class UrlOnlyOverrideWithGaps(Package):
+    homepage = 'http://www.example.com'
+
+    version('1.0.5', 'abcdef0')
+    version('1.0.0', 'bcdef0a', url='http://a.example.com/url_override-1.0.0.tar.gz')
+    version('0.9.5', 'cdef0ab')
+    version('0.9.0', 'def0abc', url='http://b.example.com/url_override-0.9.0.tar.gz')
+    version('0.8.5', 'ef0abcd')
+    version('0.8.1', 'f0abcde', url='http://c.example.com/url_override-0.8.1.tar.gz')
+    version('0.7.0', '0abcdef')
diff --git a/var/spack/repos/builtin.mock/packages/url-only-override/package.py b/var/spack/repos/builtin.mock/packages/url-only-override/package.py
new file mode 100644
index 0000000000..76999fb21e
--- /dev/null
+++ b/var/spack/repos/builtin.mock/packages/url-only-override/package.py
@@ -0,0 +1,33 @@
+##############################################################################
+# Copyright (c) 2013-2018, Lawrence Livermore National Security, LLC.
+# Produced at the Lawrence Livermore National Laboratory.
+#
+# This file is part of Spack.
+# Created by Todd Gamblin, tgamblin@llnl.gov, All rights reserved.
+# LLNL-CODE-647188
+#
+# For details, see https://github.com/spack/spack
+# Please also see the NOTICE and LICENSE files for our notice and the LGPL.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU Lesser General Public License (as
+# published by the Free Software Foundation) version 2.1, February 1999.
+#
+# This program is distributed in the hope that it will be useful, but
+# WITHOUT ANY WARRANTY; without even the IMPLIED WARRANTY OF
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the terms and
+# conditions of the GNU Lesser General Public License for more details.
+#
+# You should have received a copy of the GNU Lesser General Public
+# License along with this program; if not, write to the Free Software
+# Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
+##############################################################################
+from spack import *
+
+
+class UrlOnlyOverride(Package):
+    homepage = 'http://www.example.com'
+
+    version('1.0.0', 'cxyzab', url='http://a.example.com/url_override-1.0.0.tar.gz')
+    version('0.9.0', 'bcxyza', url='http://b.example.com/url_override-0.9.0.tar.gz')
+    version('0.8.1', 'cxyzab', url='http://c.example.com/url_override-0.8.1.tar.gz')