Merge pull request #393 from LLNL/bugfix/mirror-glitches-gh382

Bugfix/mirror glitches gh382

10 files changed, 80 insertions, 71 deletions

diff --git a/lib/spack/spack/cmd/mirror.py b/lib/spack/spack/cmd/mirror.py
index 885483a840..a71ad16b65 100644
--- a/lib/spack/spack/cmd/mirror.py
+++ b/lib/spack/spack/cmd/mirror.py
@@ -53,11 +53,13 @@ def setup_parser(subparser):
     create_parser.add_argument('-d', '--directory', default=None,
                                help="Directory in which to create mirror.")
     create_parser.add_argument(
-        'specs', nargs=argparse.REMAINDER, help="Specs of packages to put in mirror")
+        'specs', nargs=argparse.REMAINDER,
+        help="Specs of packages to put in mirror")
     create_parser.add_argument(
         '-f', '--file', help="File with specs of packages to put in mirror.")
     create_parser.add_argument(
-        '-D', '--dependencies', action='store_true', help="Also fetch all dependencies")
+        '-D', '--dependencies', action='store_true',
+        help="Also fetch all dependencies")
     create_parser.add_argument(
         '-o', '--one-version-per-spec', action='store_const', const=1, default=0,
         help="Only fetch one 'preferred' version per spec, not all known versions.")
@@ -74,7 +76,8 @@ def setup_parser(subparser):
         help="Configuration scope to modify.")
 
     # Remove
-    remove_parser = sp.add_parser('remove', aliases=['rm'], help=mirror_remove.__doc__)
+    remove_parser = sp.add_parser('remove', aliases=['rm'],
+                                  help=mirror_remove.__doc__)
     remove_parser.add_argument('name')
     remove_parser.add_argument(
         '--scope', choices=scopes, default=spack.cmd.default_modify_scope,
@@ -169,6 +172,7 @@ def mirror_create(args):
         specs = [Spec(n) for n in spack.repo.all_package_names()]
         specs.sort(key=lambda s: s.format("$_$@").lower())
 
+    # If the user asked for dependencies, traverse spec DAG get them.
     if args.dependencies:
         new_specs = set()
         for spec in specs:
diff --git a/lib/spack/spack/mirror.py b/lib/spack/spack/mirror.py
index 1d9b0e7ef2..d4f1bb89fe 100644
--- a/lib/spack/spack/mirror.py
+++ b/lib/spack/spack/mirror.py
@@ -147,7 +147,11 @@ def create(path, specs, **kwargs):
     # Get the absolute path of the root before we start jumping around.
     mirror_root = os.path.abspath(path)
     if not os.path.isdir(mirror_root):
-        mkdirp(mirror_root)
+        try:
+            mkdirp(mirror_root)
+        except OSError as e:
+            raise MirrorError(
+                "Cannot create directory '%s':" % mirror_root, str(e))
 
     # Things to keep track of while parsing specs.
     present  = []
@@ -164,7 +168,11 @@ def create(path, specs, **kwargs):
             # create a subdirectory for the current package@version
             archive_path = os.path.abspath(join_path(mirror_root, mirror_archive_path(spec)))
             subdir = os.path.dirname(archive_path)
-            mkdirp(subdir)
+            try:
+                mkdirp(subdir)
+            except OSError as e:
+                raise MirrorError(
+                    "Cannot create directory '%s':" % subdir, str(e))
 
             if os.path.exists(archive_path):
                 tty.msg("Already added %s" % spec.format("$_$@"))
diff --git a/lib/spack/spack/package.py b/lib/spack/spack/package.py
index fba2912b75..8cb947c276 100644
--- a/lib/spack/spack/package.py
+++ b/lib/spack/spack/package.py
@@ -631,7 +631,7 @@ class Package(object):
         spack.install_layout.remove_install_directory(self.spec)
 
 
-    def do_fetch(self):
+    def do_fetch(self, mirror_only=False):
         """Creates a stage directory and downloads the taball for this package.
            Working directory will be set to the stage directory.
         """
@@ -656,7 +656,7 @@ class Package(object):
                 raise FetchError(
                     "Will not fetch %s." % self.spec.format('$_$@'), checksum_msg)
 
-        self.stage.fetch()
+        self.stage.fetch(mirror_only)
 
         ##########
         # Fetch resources
@@ -677,7 +677,8 @@ class Package(object):
         if spack.do_checksum and self.version in self.versions:
             self.stage.check()
 
-    def do_stage(self):
+
+    def do_stage(self, mirror_only=False):
         """Unpacks the fetched tarball, then changes into the expanded tarball
            directory."""
         if not self.spec.concrete:
@@ -691,8 +692,7 @@ class Package(object):
             else:
                 tty.msg("Already staged %s in %s." % (name, stage.path))
 
-
-        self.do_fetch()
+        self.do_fetch(mirror_only)
         _expand_archive(self.stage)
 
         ##########
@@ -835,10 +835,6 @@ class Package(object):
         resource_stage_folder = '-'.join(pieces)
         return resource_stage_folder
 
-    def _build_logger(self, log_path):
-        """Create a context manager to log build output."""
-
-
 
     def do_install(self,
                    keep_prefix=False,  keep_stage=False, ignore_deps=False,
diff --git a/lib/spack/spack/stage.py b/lib/spack/spack/stage.py
index 31635a854a..79c9030e20 100644
--- a/lib/spack/spack/stage.py
+++ b/lib/spack/spack/stage.py
@@ -97,7 +97,6 @@ class Stage(object):
 
         self.name = kwargs.get('name')
         self.mirror_path = kwargs.get('mirror_path')
-
         self.tmp_root = find_tmp_root()
 
         self.path = None
@@ -240,11 +239,13 @@ class Stage(object):
             tty.die("Setup failed: no such directory: " + self.path)
 
 
-    def fetch(self):
+    def fetch(self, mirror_only=False):
         """Downloads an archive or checks out code from a repository."""
         self.chdir()
 
-        fetchers = [self.default_fetcher]
+        fetchers = []
+        if not mirror_only:
+            fetchers.append(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
@@ -252,7 +253,13 @@ class Stage(object):
         self.skip_checksum_for_mirror = True
         if self.mirror_path:
             mirrors = spack.config.get_config('mirrors')
-            urls = [urljoin(u, self.mirror_path) for name, u in mirrors.items()]
+
+            # Join URLs of mirror roots with mirror paths. Because
+            # urljoin() will strip everything past the final '/' in
+            # the root, so we add a '/' if it is not present.
+            mirror_roots = [root if root.endswith('/') else root + '/'
+                            for root in mirrors.values()]
+            urls = [urljoin(root, self.mirror_path) for root in mirror_roots]
 
             # If this archive is normally fetched from a tarball URL,
             # then use the same digest.  `spack mirror` ensures that
@@ -261,10 +268,11 @@ class Stage(object):
             if isinstance(self.default_fetcher, fs.URLFetchStrategy):
                 digest = self.default_fetcher.digest
 
-            # Have to skip the checkesum for things archived from
+            # Have to skip the checksum for things archived from
             # repositories.  How can this be made safer?
             self.skip_checksum_for_mirror = not bool(digest)
 
+            # Add URL strategies for all the mirrors with the digest
             for url in urls:
                 fetchers.insert(0, fs.URLFetchStrategy(url, digest))
 
diff --git a/lib/spack/spack/test/git_fetch.py b/lib/spack/spack/test/git_fetch.py
index 3813079065..d84433176a 100644
--- a/lib/spack/spack/test/git_fetch.py
+++ b/lib/spack/spack/test/git_fetch.py
@@ -56,10 +56,7 @@ class GitFetchTest(MockPackagesTest):
     def tearDown(self):
         """Destroy the stage space used by this test."""
         super(GitFetchTest, self).tearDown()
-
-        if self.repo.stage is not None:
-            self.repo.stage.destroy()
-
+        self.repo.destroy()
         self.pkg.do_clean()
 
 
diff --git a/lib/spack/spack/test/hg_fetch.py b/lib/spack/spack/test/hg_fetch.py
index ee8327aec8..bbcb64e4c1 100644
--- a/lib/spack/spack/test/hg_fetch.py
+++ b/lib/spack/spack/test/hg_fetch.py
@@ -53,10 +53,7 @@ class HgFetchTest(MockPackagesTest):
     def tearDown(self):
         """Destroy the stage space used by this test."""
         super(HgFetchTest, self).tearDown()
-
-        if self.repo.stage is not None:
-            self.repo.stage.destroy()
-
+        self.repo.destroy()
         self.pkg.do_clean()
 
 
diff --git a/lib/spack/spack/test/install.py b/lib/spack/spack/test/install.py
index 628329a423..c09bd24c8e 100644
--- a/lib/spack/spack/test/install.py
+++ b/lib/spack/spack/test/install.py
@@ -59,9 +59,7 @@ class InstallTest(MockPackagesTest):
 
     def tearDown(self):
         super(InstallTest, self).tearDown()
-
-        if self.repo.stage is not None:
-            self.repo.stage.destroy()
+        self.repo.destroy()
 
         # Turn checksumming back on
         spack.do_checksum = True
diff --git a/lib/spack/spack/test/mirror.py b/lib/spack/spack/test/mirror.py
index 04e9e3db2e..046ec56604 100644
--- a/lib/spack/spack/test/mirror.py
+++ b/lib/spack/spack/test/mirror.py
@@ -44,8 +44,16 @@ class MirrorTest(MockPackagesTest):
         self.repos = {}
 
 
+    def tearDown(self):
+        """Destroy all the stages created by the repos in setup."""
+        super(MirrorTest, self).tearDown()
+        for repo in self.repos.values():
+            repo.destroy()
+        self.repos.clear()
+
+
     def set_up_package(self, name, MockRepoClass, url_attr):
-        """Use this to set up a mock package to be mirrored.
+        """Set up a mock package to be mirrored.
            Each package needs us to:
              1. Set up a mock repo/archive to fetch from.
              2. Point the package's version args at that repo.
@@ -65,21 +73,14 @@ class MirrorTest(MockPackagesTest):
         pkg.versions[v][url_attr] = repo.url
 
 
-    def tearDown(self):
-        """Destroy all the stages created by the repos in setup."""
-        super(MirrorTest, self).tearDown()
-
-        for name, repo in self.repos.items():
-            if repo.stage:
-                pass #repo.stage.destroy()
-
-        self.repos.clear()
-
-
     def check_mirror(self):
         stage = Stage('spack-mirror-test')
         mirror_root = join_path(stage.path, 'test-mirror')
 
+        # register mirror with spack config
+        mirrors = { 'spack-mirror-test' : 'file://' + mirror_root }
+        spack.config.update_config('mirrors', mirrors)
+
         try:
             os.chdir(stage.path)
             spack.mirror.create(
@@ -88,7 +89,7 @@ class MirrorTest(MockPackagesTest):
             # Stage directory exists
             self.assertTrue(os.path.isdir(mirror_root))
 
-            # subdirs for each package
+            # check that there are subdirs for each package
             for name in self.repos:
                 subdir = join_path(mirror_root, name)
                 self.assertTrue(os.path.isdir(subdir))
@@ -96,40 +97,37 @@ class MirrorTest(MockPackagesTest):
                 files = os.listdir(subdir)
                 self.assertEqual(len(files), 1)
 
-                # Decompress archive in the mirror
-                archive = files[0]
-                archive_path = join_path(subdir, archive)
-                decomp = decompressor_for(archive_path)
-
-                with working_dir(subdir):
-                    decomp(archive_path)
+            # Now try to fetch each package.
+            for name, mock_repo in self.repos.items():
+                spec = Spec(name).concretized()
+                pkg = spec.package
 
-                    # Find the untarred archive directory.
-                    files = os.listdir(subdir)
-                    self.assertEqual(len(files), 2)
-                    self.assertTrue(archive in files)
-                    files.remove(archive)
-
-                    expanded_archive = join_path(subdir, files[0])
-                    self.assertTrue(os.path.isdir(expanded_archive))
+                saved_checksum_setting = spack.do_checksum
+                try:
+                    # Stage the archive from the mirror and cd to it.
+                    spack.do_checksum = False
+                    pkg.do_stage(mirror_only=True)
 
                     # Compare the original repo with the expanded archive
-                    repo = self.repos[name]
-                    if not 'svn' in name:
-                        original_path = repo.path
-                    else:
-                        co = 'checked_out'
-                        svn('checkout', repo.url, co)
-                        original_path = join_path(subdir, co)
+                    original_path = mock_repo.path
+                    if 'svn' in name:
+                        # have to check out the svn repo to compare.
+                        original_path = join_path(mock_repo.path, 'checked_out')
+                        svn('checkout', mock_repo.url, original_path)
 
-                    dcmp = dircmp(original_path, expanded_archive)
+                    dcmp = dircmp(original_path, pkg.stage.source_path)
 
                     # make sure there are no new files in the expanded tarball
                     self.assertFalse(dcmp.right_only)
+
+                    # and that all original files are present.
                     self.assertTrue(all(l in exclude for l in dcmp.left_only))
 
+                finally:
+                    spack.do_checksum = saved_checksum_setting
+                    pkg.do_clean()
         finally:
-            pass #stage.destroy()
+            stage.destroy()
 
 
     def test_git_mirror(self):
diff --git a/lib/spack/spack/test/mock_repo.py b/lib/spack/spack/test/mock_repo.py
index 9738ba4e72..ed94023b0e 100644
--- a/lib/spack/spack/test/mock_repo.py
+++ b/lib/spack/spack/test/mock_repo.py
@@ -55,6 +55,12 @@ class MockRepo(object):
         mkdirp(self.path)
 
 
+    def destroy(self):
+        """Destroy resources associated with this mock repo."""
+        if self.stage:
+            self.stage.destroy()
+
+
 class MockArchive(MockRepo):
     """Creates a very simple archive directory with a configure script and a
        makefile that installs to a prefix.  Tars it up into an archive."""
diff --git a/lib/spack/spack/test/svn_fetch.py b/lib/spack/spack/test/svn_fetch.py
index 7d150b42f4..454a7f1d1f 100644
--- a/lib/spack/spack/test/svn_fetch.py
+++ b/lib/spack/spack/test/svn_fetch.py
@@ -55,10 +55,7 @@ class SvnFetchTest(MockPackagesTest):
     def tearDown(self):
         """Destroy the stage space used by this test."""
         super(SvnFetchTest, self).tearDown()
-
-        if self.repo.stage is not None:
-            self.repo.stage.destroy()
-
+        self.repo.destroy()
         self.pkg.do_clean()