From 0cc79e05642c78932f87d06f272bad6602b3cbfe Mon Sep 17 00:00:00 2001
From: Todd Gamblin <tgamblin@llnl.gov>
Date: Fri, 5 Sep 2014 10:48:18 -0700
Subject: Implement per-version attributes for flexible fetch policies.
- Tests pass with URL fetching and new scheme.
- Lots of refactoring
- Infrastructure is there for arbitrary fetch policies and more attribtues on the version() call.
- Mirrors do not currently work properly, and they get in the way of a proper git fetch
---
lib/spack/spack/fetch_strategy.py | 163 ++++++++++++++++++++++++++++++---
lib/spack/spack/package.py | 116 +++++++++++++++--------
lib/spack/spack/patch.py | 2 +-
lib/spack/spack/relations.py | 28 +++---
lib/spack/spack/stage.py | 23 +++--
lib/spack/spack/test/install.py | 8 +-
lib/spack/spack/test/package_sanity.py | 6 +-
lib/spack/spack/test/stage.py | 14 +--
8 files changed, 273 insertions(+), 87 deletions(-)
(limited to 'lib')
diff --git a/lib/spack/spack/fetch_strategy.py b/lib/spack/spack/fetch_strategy.py
index 2811c0e92b..b0845700b6 100644
--- a/lib/spack/spack/fetch_strategy.py
+++ b/lib/spack/spack/fetch_strategy.py
@@ -47,9 +47,11 @@ import llnl.util.tty as tty
import spack
import spack.error
import spack.util.crypto as crypto
+from spack.version import Version, ver
from spack.util.compression import decompressor_for
+
class FetchStrategy(object):
def __init__(self):
# The stage is initialized late, so that fetch strategies can be constructed
@@ -63,21 +65,37 @@ class FetchStrategy(object):
self.stage = stage
- # Subclasses need to implement tehse methods
+ # Subclasses need to implement these methods
def fetch(self): pass # Return True on success, False on fail
def check(self): pass
def expand(self): pass
def reset(self): pass
- def __str__(self): pass
+ def __str__(self):
+ return "FetchStrategy.__str___"
+ # This method is used to match fetch strategies to version()
+ # arguments in packages.
+ @classmethod
+ def match(kwargs):
+ return any(k in kwargs for k in self.attributes)
class URLFetchStrategy(FetchStrategy):
+ attributes = ('url', 'md5')
- def __init__(self, url, digest=None):
+ def __init__(self, url=None, digest=None, **kwargs):
super(URLFetchStrategy, self).__init__()
- self.url = url
- self.digest = digest
+
+ # If URL or digest are provided in the kwargs, then prefer
+ # those values.
+ self.url = kwargs.get('url', None)
+ if not self.url: self.url = url
+
+ self.digest = kwargs.get('md5', None)
+ if not self.digest: self.digest = digest
+
+ if not self.url:
+ raise ValueError("URLFetchStrategy requires a url for fetching.")
def fetch(self):
@@ -142,9 +160,7 @@ class URLFetchStrategy(FetchStrategy):
self.stage.chdir()
if not self.archive_file:
raise NoArchiveFileError("URLFetchStrategy couldn't find archive file",
- "Failed on expand() for URL %s" % self.url)
-
- print self.archive_file
+ "Failed on expand() for URL %s" % self.url)
decompress = decompressor_for(self.archive_file)
decompress(self.archive_file)
@@ -175,19 +191,117 @@ class URLFetchStrategy(FetchStrategy):
def __str__(self):
- return self.url
+ if self.url:
+ return self.url
+ else:
+ return "URLFetchStrategy <no url>"
+class VCSFetchStrategy(FetchStrategy):
+ def __init__(self, name):
+ super(VCSFetchStrategy, self).__init__()
+ self.name = name
+
+
+ def check(self):
+ assert(self.stage)
+ tty.msg("No check needed when fetching with %s." % self.name)
+
+ def expand(self):
+ assert(self.stage)
+ tty.debug("Source fetched with %s is already expanded." % self.name)
+
+
+
+class GitFetchStrategy(VCSFetchStrategy):
+ attributes = ('git', 'ref', 'tag', 'branch')
+
+ def __init__(self, **kwargs):
+ super(GitFetchStrategy, self).__init__("git")
+ self.url = kwargs.get('git', None)
+ if not self.url:
+ raise ValueError("GitFetchStrategy requires git argument.")
+
+ if sum((k in kwargs for k in ('ref', 'tag', 'branch'))) > 1:
+ raise FetchStrategyError(
+ "Git requires exactly one ref, branch, or tag.")
+
+ self._git = None
+ self.ref = kwargs.get('ref', None)
+ self.branch = kwargs.get('branch', None)
+ if not self.branch:
+ self.branch = kwargs.get('tag', None)
+
+
+ @property
+ def git_version(self):
+ git = which('git', required=True)
+ vstring = git('--version', return_output=True).lstrip('git version ')
+ return Version(vstring)
+
+
+ @property
+ def git(self):
+ if not self._git:
+ self._git = which('git', required=True)
+ return self._git
+
+
+ def fetch(self):
+ assert(self.stage)
+ self.stage.chdir()
+
+ if self.stage.source_path:
+ tty.msg("Already fetched %s." % self.source_path)
+ return
+
+ tty.msg("Trying to clone git repository: %s" % self.url)
-class GitFetchStrategy(FetchStrategy):
- pass
+
+ if self.ref:
+ # Need to do a regular clone and check out everything if
+ # they asked for a particular ref.
+ git('clone', self.url)
+ self.chdir_to_source()
+ git('checkout', self.ref)
+
+ else:
+ # Can be more efficient if not checking out a specific ref.
+ args = ['clone']
+
+ # If we want a particular branch ask for it.
+ if self.branch:
+ args.extend(['--branch', self.branch])
+
+ # Try to be efficient if we're using a new enough git.
+ # This checks out only one branch's history
+ if self.git_version > ver('1.7.10'):
+ args.append('--single-branch')
+
+ args.append(self.url)
+ git(*args)
+ self.chdir_to_source()
+
+
+ def reset(self):
+ assert(self.stage)
+ git = which('git', required=True)
+
+ self.stage.chdir_to_source()
+ git('checkout', '.')
+ git('clean', '-f')
+
+
+ def __str__(self):
+ return self.url
class SvnFetchStrategy(FetchStrategy):
+ attributes = ('svn', 'rev', 'revision')
pass
-def strategy_for_url(url):
+def from_url(url):
"""Given a URL, find an appropriate fetch strategy for it.
Currently just gives you a URLFetchStrategy that uses curl.
@@ -197,6 +311,27 @@ def strategy_for_url(url):
return URLFetchStrategy(url)
+def args_are_for(args, fetcher):
+ return any(arg in args for arg in fetcher.attributes)
+
+
+def from_args(args, pkg):
+ """Determine a fetch strategy based on the arguments supplied to
+ version() in the package description."""
+ fetchers = (URLFetchStrategy, GitFetchStrategy)
+ for fetcher in fetchers:
+ if args_are_for(args, fetcher):
+ attrs = {}
+ for attr in fetcher.attributes:
+ default = getattr(pkg, attr, None)
+ if default:
+ attrs[attr] = default
+
+ attrs.update(args)
+ return fetcher(**attrs)
+
+ return None
+
class FetchStrategyError(spack.error.SpackError):
def __init__(self, msg, long_msg):
super(FetchStrategyError, self).__init__(msg, long_msg)
@@ -220,3 +355,7 @@ class NoDigestError(FetchStrategyError):
super(NoDigestError, self).__init__(msg, long_msg)
+class InvalidArgsError(FetchStrategyError):
+ def __init__(self, msg, long_msg):
+ super(InvalidArgsError, self).__init__(msg, long_msg)
+
diff --git a/lib/spack/spack/package.py b/lib/spack/spack/package.py
index f5f1c9dec6..553e0118e3 100644
--- a/lib/spack/spack/package.py
+++ b/lib/spack/spack/package.py
@@ -52,6 +52,7 @@ import spack.compilers
import spack.hooks
import spack.build_environment as build_env
import spack.url as url
+import spack.fetch_strategy as fs
from spack.version import *
from spack.stage import Stage
from spack.util.web import get_pages
@@ -300,7 +301,7 @@ class Package(object):
# These variables are defaults for the various "relations".
#
"""Map of information about Versions of this package.
- Map goes: Version -> VersionDescriptor"""
+ Map goes: Version -> dict of attributes"""
versions = {}
"""Specs of dependency packages, keyed by name."""
@@ -356,8 +357,7 @@ class Package(object):
# Check version descriptors
for v in sorted(self.versions):
- vdesc = self.versions[v]
- assert(isinstance(vdesc, spack.relations.VersionDescriptor))
+ assert(isinstance(self.versions[v], dict))
# Version-ize the keys in versions dict
try:
@@ -368,9 +368,14 @@ class Package(object):
# stage used to build this package.
self._stage = None
- # patch up self.url based on the actual version
+ # If there's no default URL provided, set this package's url to None
+ if not hasattr(self, 'url'):
+ self.url = None
+
+ # Set up a fetch strategy for this package.
+ self.fetcher = None
if self.spec.concrete:
- self.url = self.url_for_version(self.version)
+ self.fetcher = fs.from_args(self.versions[self.version], self)
# Set a default list URL (place to find available versions)
if not hasattr(self, 'list_url'):
@@ -387,6 +392,61 @@ class Package(object):
return self.spec.versions[0]
+ @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]
+ if 'url' in args:
+ version_urls[v] = args['url']
+ return version_urls
+
+
+ def nearest_url(self, version):
+ """Finds the URL for the next lowest version with a URL.
+ If there is no lower version with a URL, uses the
+ package url property. If that isn't there, uses a
+ *higher* URL, and if that isn't there raises an error.
+ """
+ version_urls = self.version_urls()
+ url = self.url
+
+ for v in version_urls:
+ if v > version and url:
+ break
+ if version_urls[v]:
+ url = version_urls[v]
+ return url
+
+
+ def has_url(self):
+ """Returns whether there is a URL available for this package.
+ If there isn't, it's probably fetched some other way (version
+ control, etc.)"""
+ return self.url or self.version_urls()
+
+
+ # TODO: move this out of here and into some URL extrapolation module.
+ def url_for_version(self, version):
+ """Returns a URL that you can download a new version of this package from."""
+ if not isinstance(version, Version):
+ version = Version(version)
+
+ if not self.has_url():
+ raise NoURLError(self.__class__)
+
+ # 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]
+ else:
+ return url.substitute_version(self.nearest_url(version),
+ self.url_version(version))
+
+
@property
def stage(self):
if not self.spec.concrete:
@@ -397,11 +457,12 @@ class Package(object):
raise PackageVersionError(self.version)
# TODO: move this logic into a mirror module.
+ # TODO: get rid of dependence on extension.
mirror_path = "%s/%s" % (self.name, "%s-%s.%s" % (
self.name, self.version, extension(self.url)))
self._stage = Stage(
- self.url, mirror_path=mirror_path, name=self.spec.short_spec)
+ self.fetcher, mirror_path=mirror_path, name=self.spec.short_spec)
return self._stage
@@ -523,36 +584,6 @@ class Package(object):
return str(version)
- def url_for_version(self, version):
- """Returns a URL that you can download a new version of this package from."""
- if not isinstance(version, Version):
- version = Version(version)
-
- def nearest_url(version):
- """Finds the URL for the next lowest version with a URL.
- If there is no lower version with a URL, uses the
- package url property. If that isn't there, uses a
- *higher* URL, and if that isn't there raises an error.
- """
- url = getattr(self, 'url', None)
- for v in sorted(self.versions):
- if v > version and url:
- break
- if self.versions[v].url:
- url = self.versions[v].url
- return url
-
- if version in self.versions:
- vdesc = self.versions[version]
- if not vdesc.url:
- base_url = nearest_url(version)
- vdesc.url = url.substitute_version(
- base_url, self.url_version(version))
- return vdesc.url
- else:
- return nearest_url(version)
-
-
@property
def default_url(self):
if self.concrete:
@@ -605,7 +636,7 @@ class Package(object):
tty.msg("Created stage directory in %s." % self.stage.path)
else:
tty.msg("Already staged %s in %s." % (self.name, self.stage.path))
- self.stage.chdir_to_archive()
+ self.stage.chdir_to_source()
def do_patch(self):
@@ -628,7 +659,7 @@ class Package(object):
tty.msg("Patching failed last time. Restaging.")
self.stage.restage()
- self.stage.chdir_to_archive()
+ self.stage.chdir_to_source()
# If this file exists, then we already applied all the patches.
if os.path.isfile(good_file):
@@ -788,7 +819,7 @@ class Package(object):
def do_clean(self):
if self.stage.expanded_archive_path:
- self.stage.chdir_to_archive()
+ self.stage.chdir_to_source()
self.clean()
@@ -944,3 +975,10 @@ class VersionFetchError(PackageError):
super(VersionFetchError, self).__init__(
"Cannot fetch version for package %s " % cls.__name__ +
"because it does not define a default url.")
+
+
+class NoURLError(PackageError):
+ """Raised when someone tries to build a URL for a package with no URLs."""
+ def __init__(self, cls):
+ super(NoURLError, self).__init__(
+ "Package %s has no version with a URL." % cls.__name__)
diff --git a/lib/spack/spack/patch.py b/lib/spack/spack/patch.py
index 42d49b15e5..b1b6e07738 100644
--- a/lib/spack/spack/patch.py
+++ b/lib/spack/spack/patch.py
@@ -64,7 +64,7 @@ class Patch(object):
"""Fetch this patch, if necessary, and apply it to the source
code in the supplied stage.
"""
- stage.chdir_to_archive()
+ stage.chdir_to_source()
patch_stage = None
try:
diff --git a/lib/spack/spack/relations.py b/lib/spack/spack/relations.py
index f7e1cfd925..b1f4348945 100644
--- a/lib/spack/spack/relations.py
+++ b/lib/spack/spack/relations.py
@@ -79,32 +79,28 @@ import spack
import spack.spec
import spack.error
import spack.url
-
from spack.version import Version
from spack.patch import Patch
from spack.spec import Spec, parse_anonymous_spec
-class VersionDescriptor(object):
- """A VersionDescriptor contains information to describe a
- particular version of a package. That currently includes a URL
- for the version along with a checksum."""
- def __init__(self, checksum, url):
- self.checksum = checksum
- self.url = url
-
-def version(ver, checksum, **kwargs):
- """Adds a version and metadata describing how to fetch it."""
+def version(ver, checksum=None, **kwargs):
+ """Adds a version and metadata describing how to fetch it.
+ Metadata is just stored as a dict in the package's versions
+ dictionary. Package must turn it into a valid fetch strategy
+ later.
+ """
pkg = caller_locals()
-
versions = pkg.setdefault('versions', {})
- patches = pkg.setdefault('patches', {})
- ver = Version(ver)
- url = kwargs.get('url', None)
+ # special case checksum for backward compatibility
+ if checksum:
+ kwargs['md5'] = checksum
- versions[ver] = VersionDescriptor(checksum, url)
+ # Store the kwargs for the package to use later when constructing
+ # a fetch strategy.
+ versions[Version(ver)] = kwargs
def depends_on(*specs):
diff --git a/lib/spack/spack/stage.py b/lib/spack/spack/stage.py
index 0fa315051f..5dc6eac488 100644
--- a/lib/spack/spack/stage.py
+++ b/lib/spack/spack/stage.py
@@ -32,11 +32,10 @@ from llnl.util.filesystem import *
import spack
import spack.config
-from spack.fetch_strategy import strategy_for_url, URLFetchStrategy
+import spack.fetch_strategy as fetch_strategy
import spack.error
-
STAGE_PREFIX = 'spack-stage-'
@@ -70,7 +69,7 @@ class Stage(object):
similar, and are intended to persist for only one run of spack.
"""
- def __init__(self, url, **kwargs):
+ def __init__(self, url_or_fetch_strategy, **kwargs):
"""Create a stage object.
Parameters:
url_or_fetch_strategy
@@ -83,10 +82,16 @@ class Stage(object):
stage object later). If name is not provided, then this
stage will be given a unique name automatically.
"""
- if isinstance(url, basestring):
- self.fetcher = strategy_for_url(url)
+ if isinstance(url_or_fetch_strategy, basestring):
+ self.fetcher = fetch_strategy.from_url(url_or_fetch_strategy)
self.fetcher.set_stage(self)
+ elif isinstance(url_or_fetch_strategy, fetch_strategy.FetchStrategy):
+ self.fetcher = url_or_fetch_strategy
+
+ else:
+ raise ValueError("Can't construct Stage without url or fetch strategy")
+
self.name = kwargs.get('name')
self.mirror_path = kwargs.get('mirror_path')
@@ -237,10 +242,12 @@ class Stage(object):
# TODO: move mirror logic out of here and clean it up!
if self.mirror_path:
urls = ["%s/%s" % (m, self.mirror_path) for m in _get_mirrors()]
+
digest = None
- if isinstance(self.fetcher, URLFetchStrategy):
+ if isinstance(self.fetcher, fetch_strategy.URLFetchStrategy):
digest = self.fetcher.digest
- fetchers = [URLFetchStrategy(url, digest) for url in urls] + fetchers
+ fetchers = [fetch_strategy.URLFetchStrategy(url, digest)
+ for url in urls] + fetchers
for f in fetchers:
f.set_stage(self)
@@ -267,7 +274,7 @@ class Stage(object):
self.fetcher.expand()
- def chdir_to_archive(self):
+ def chdir_to_source(self):
"""Changes directory to the expanded archive directory.
Dies with an error if there was no expanded archive.
"""
diff --git a/lib/spack/spack/test/install.py b/lib/spack/spack/test/install.py
index 8047ab92e3..896f19ac01 100644
--- a/lib/spack/spack/test/install.py
+++ b/lib/spack/spack/test/install.py
@@ -32,6 +32,7 @@ from llnl.util.filesystem import *
import spack
from spack.stage import Stage
+from spack.fetch_strategy import URLFetchStrategy
from spack.directory_layout import SpecHashDirectoryLayout
from spack.util.executable import which
from spack.test.mock_packages_test import *
@@ -100,11 +101,16 @@ class InstallTest(MockPackagesTest):
self.assertTrue(spec.concrete)
# Get the package
+ print
+ print "======== GETTING PACKAGE ========"
pkg = spack.db.get(spec)
+ print "======== GOT PACKAGE ========"
+ print
+
# Fake the URL for the package so it downloads from a file.
archive_path = join_path(self.stage.path, archive_name)
- pkg.url = 'file://' + archive_path
+ pkg.fetcher = URLFetchStrategy('file://' + archive_path)
try:
pkg.do_install()
diff --git a/lib/spack/spack/test/package_sanity.py b/lib/spack/spack/test/package_sanity.py
index e3de695070..6222e7b5f8 100644
--- a/lib/spack/spack/test/package_sanity.py
+++ b/lib/spack/spack/test/package_sanity.py
@@ -56,8 +56,8 @@ class PackageSanityTest(unittest.TestCase):
def test_url_versions(self):
"""Check URLs for regular packages, if they are explicitly defined."""
for pkg in spack.db.all_packages():
- for v, vdesc in pkg.versions.items():
- if vdesc.url:
+ for v, vattrs in pkg.versions.items():
+ if 'url' in vattrs:
# If there is a url for the version check it.
v_url = pkg.url_for_version(v)
- self.assertEqual(vdesc.url, v_url)
+ self.assertEqual(vattrs['url'], v_url)
diff --git a/lib/spack/spack/test/stage.py b/lib/spack/spack/test/stage.py
index 8cb7ac772e..c5a7013675 100644
--- a/lib/spack/spack/test/stage.py
+++ b/lib/spack/spack/test/stage.py
@@ -170,7 +170,7 @@ class StageTest(unittest.TestCase):
self.assertEqual(os.path.realpath(stage_path), os.getcwd())
- def check_chdir_to_archive(self, stage, stage_name):
+ def check_chdir_to_source(self, stage, stage_name):
stage_path = self.get_stage_path(stage, stage_name)
self.assertEqual(
join_path(os.path.realpath(stage_path), archive_dir),
@@ -271,9 +271,9 @@ class StageTest(unittest.TestCase):
self.check_fetch(stage, stage_name)
stage.expand_archive()
- stage.chdir_to_archive()
+ stage.chdir_to_source()
self.check_expand_archive(stage, stage_name)
- self.check_chdir_to_archive(stage, stage_name)
+ self.check_chdir_to_source(stage, stage_name)
stage.destroy()
self.check_destroy(stage, stage_name)
@@ -284,9 +284,9 @@ class StageTest(unittest.TestCase):
stage.fetch()
stage.expand_archive()
- stage.chdir_to_archive()
+ stage.chdir_to_source()
self.check_expand_archive(stage, stage_name)
- self.check_chdir_to_archive(stage, stage_name)
+ self.check_chdir_to_source(stage, stage_name)
# Try to make a file in the old archive dir
with closing(open('foobar', 'w')) as file:
@@ -299,8 +299,8 @@ class StageTest(unittest.TestCase):
self.check_chdir(stage, stage_name)
self.check_fetch(stage, stage_name)
- stage.chdir_to_archive()
- self.check_chdir_to_archive(stage, stage_name)
+ stage.chdir_to_source()
+ self.check_chdir_to_source(stage, stage_name)
self.assertFalse('foobar' in os.listdir(stage.source_path))
stage.destroy()
--
cgit v1.2.3-70-g09d2