From 773cfe088fab98f7ca4451bb5cfcacbab48529a5 Mon Sep 17 00:00:00 2001 From: Todd Gamblin Date: Sun, 22 Jul 2018 13:49:01 -0700 Subject: 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 --- lib/spack/spack/fetch_strategy.py | 192 ++++++++++++++++++++++---------------- lib/spack/spack/test/packages.py | 78 +++++++++++++++- 2 files changed, 186 insertions(+), 84 deletions(-) (limited to 'lib') 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' -- cgit v1.2.3-60-g2f50