From f37f872e5f4920f9dc22237deed5895b94700136 Mon Sep 17 00:00:00 2001 From: Todd Gamblin Date: Wed, 23 Dec 2015 16:23:58 -0800 Subject: Fix #85 and #228: errors fetching VCS packages from a mirror. - Stage and fetcher were not being set up properly when fetching using a different fetch strategy than the default one for the package. - This is fixed but fetch/stage/mirror logic is still too complicated and long-term needs a rethink. - Spack will now print a warning when fetching a checksum-less tarball from a mirror -- users should be careful to use https or local filesystem mirrors for this. --- lib/spack/spack/fetch_strategy.py | 4 +-- lib/spack/spack/stage.py | 53 ++++++++++++++++++++++++++++----------- 2 files changed, 41 insertions(+), 16 deletions(-) diff --git a/lib/spack/spack/fetch_strategy.py b/lib/spack/spack/fetch_strategy.py index a9374fb34b..0657146bf6 100644 --- a/lib/spack/spack/fetch_strategy.py +++ b/lib/spack/spack/fetch_strategy.py @@ -687,7 +687,7 @@ def for_package_version(pkg, version): class FetchError(spack.error.SpackError): - def __init__(self, msg, long_msg): + def __init__(self, msg, long_msg=None): super(FetchError, self).__init__(msg, long_msg) @@ -705,7 +705,7 @@ class NoArchiveFileError(FetchError): class NoDigestError(FetchError): - def __init__(self, msg, long_msg): + def __init__(self, msg, long_msg=None): super(NoDigestError, self).__init__(msg, long_msg) diff --git a/lib/spack/spack/stage.py b/lib/spack/spack/stage.py index 754344fc01..76ca7273cb 100644 --- a/lib/spack/spack/stage.py +++ b/lib/spack/spack/stage.py @@ -82,14 +82,18 @@ class Stage(object): stage object later). If name is not provided, then this stage will be given a unique name automatically. """ + # TODO: fetch/stage coupling needs to be reworked -- the logic + # TODO: here is convoluted and not modular enough. if isinstance(url_or_fetch_strategy, basestring): self.fetcher = fs.from_url(url_or_fetch_strategy) elif isinstance(url_or_fetch_strategy, fs.FetchStrategy): self.fetcher = url_or_fetch_strategy else: raise ValueError("Can't construct Stage without url or fetch strategy") - self.fetcher.set_stage(self) + self.default_fetcher = self.fetcher # self.fetcher can change with mirrors. + self.skip_checksum_for_mirror = True # used for mirrored archives of repositories. + self.name = kwargs.get('name') self.mirror_path = kwargs.get('mirror_path') @@ -198,17 +202,18 @@ class Stage(object): @property def archive_file(self): """Path to the source archive within this stage directory.""" - if not isinstance(self.fetcher, fs.URLFetchStrategy): - return None + paths = [] + if isinstance(self.fetcher, fs.URLFetchStrategy): + paths.append(os.path.join(self.path, os.path.basename(self.fetcher.url))) - paths = [os.path.join(self.path, os.path.basename(self.fetcher.url))] if self.mirror_path: paths.append(os.path.join(self.path, os.path.basename(self.mirror_path))) for path in paths: if os.path.exists(path): return path - return None + else: + return None @property @@ -238,23 +243,34 @@ class Stage(object): """Downloads an archive or checks out code from a repository.""" self.chdir() - fetchers = [self.fetcher] + fetchers = [self.default_fetcher] # TODO: move mirror logic out of here and clean it up! + # TODO: Or @alalazo may have some ideas about how to use a + # TODO: CompositeFetchStrategy here. + self.skip_checksum_for_mirror = True if self.mirror_path: urls = ["%s/%s" % (m, self.mirror_path) for m in _get_mirrors()] + # If this archive is normally fetched from a tarball URL, + # then use the same digest. `spack mirror` ensures that + # the checksum will be the same. digest = None - if isinstance(self.fetcher, fs.URLFetchStrategy): - digest = self.fetcher.digest - fetchers = [fs.URLFetchStrategy(url, digest) - for url in urls] + fetchers - for f in fetchers: - f.set_stage(self) + if isinstance(self.default_fetcher, fs.URLFetchStrategy): + digest = self.default_fetcher.digest + + # Have to skip the checkesum for things archived from + # repositories. How can this be made safer? + self.skip_checksum_for_mirror = not bool(digest) + + for url in urls: + fetchers.insert(0, fs.URLFetchStrategy(url, digest)) for fetcher in fetchers: try: - fetcher.fetch() + fetcher.set_stage(self) + self.fetcher = fetcher + self.fetcher.fetch() break except spack.error.SpackError, e: tty.msg("Fetching from %s failed." % fetcher) @@ -262,13 +278,22 @@ class Stage(object): continue else: errMessage = "All fetchers failed for %s" % self.name + self.fetcher = self.default_fetcher raise fs.FetchError(errMessage, None) def check(self): """Check the downloaded archive against a checksum digest. No-op if this stage checks code out of a repository.""" - self.fetcher.check() + if self.fetcher is not self.default_fetcher and self.skip_checksum_for_mirror: + tty.warn("Fetching from mirror without a checksum!", + "This package is normally checked out from a version " + "control system, but it has been archived on a spack " + "mirror. This means we cannot know a checksum for the " + "tarball in advance. Be sure that your connection to " + "this mirror is secure!.") + else: + self.fetcher.check() def expand_archive(self): -- cgit v1.2.3-70-g09d2