Merge pull request #277 from LLNL/bugfix/vcs-mirror-fetch

Fix #85 and #228: errors fetching VCS packages from a mirror.

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):