From be354e85c9d168b5764f8fec6cd1da53a5b4a7fe Mon Sep 17 00:00:00 2001 From: Todd Gamblin Date: Sun, 24 Jan 2016 16:16:43 -0800 Subject: Better errors for mkdirp failure in mirror. --- lib/spack/spack/mirror.py | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) (limited to 'lib') 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("$_$@")) -- cgit v1.2.3-70-g09d2 From fe50593c66d8cf7ee527fa13928990595d8b0a29 Mon Sep 17 00:00:00 2001 From: Todd Gamblin Date: Sun, 24 Jan 2016 20:14:21 -0800 Subject: Minor line width reductions. --- lib/spack/spack/cmd/mirror.py | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) (limited to 'lib') diff --git a/lib/spack/spack/cmd/mirror.py b/lib/spack/spack/cmd/mirror.py index 885483a840..92b92e07af 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, -- cgit v1.2.3-70-g09d2 From d3ff8ca00f7f47d365ea88ed717fa7940a1346c6 Mon Sep 17 00:00:00 2001 From: Todd Gamblin Date: Sun, 24 Jan 2016 20:14:40 -0800 Subject: Fixes #382: Issues with spack fetch. - urljoin() was compliant with RFC 1808 but not with my understanding of how paths should be joined. - updated path joining logic to comply. --- lib/spack/spack/stage.py | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) (limited to 'lib') diff --git a/lib/spack/spack/stage.py b/lib/spack/spack/stage.py index 31635a854a..19b7d03664 100644 --- a/lib/spack/spack/stage.py +++ b/lib/spack/spack/stage.py @@ -252,7 +252,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 -- cgit v1.2.3-70-g09d2 From a48d0a494f8524c789e94a09c182ff1b6e465b8c Mon Sep 17 00:00:00 2001 From: Todd Gamblin Date: Mon, 25 Jan 2016 00:32:14 -0800 Subject: Refactor mock_repo: add a destroy() method. - classes using mock_repo need not know about its stage now. --- lib/spack/spack/test/git_fetch.py | 5 +---- lib/spack/spack/test/hg_fetch.py | 5 +---- lib/spack/spack/test/install.py | 4 +--- lib/spack/spack/test/mirror.py | 23 ++++++++++------------- lib/spack/spack/test/mock_repo.py | 6 ++++++ lib/spack/spack/test/svn_fetch.py | 5 +---- 6 files changed, 20 insertions(+), 28 deletions(-) (limited to 'lib') 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..0093b5d82f 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,17 +73,6 @@ 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') @@ -129,7 +126,7 @@ class MirrorTest(MockPackagesTest): self.assertTrue(all(l in exclude for l in dcmp.left_only)) 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() -- cgit v1.2.3-70-g09d2 From 5502fd1054e665d5deb028217f554640bedf8514 Mon Sep 17 00:00:00 2001 From: Todd Gamblin Date: Mon, 25 Jan 2016 01:59:02 -0800 Subject: More thorough mirror test: test full round-trip - Old test: did not attempt to actually fetch mirrored packages. - New test: 1. Creates a temporary mirror, 2. Registers it with spack, 3. Fetches from it, and 4. Verifies that the fetched archive matches the original - This test will hopefully mean that `spack mirror` is less brittle from now on. --- lib/spack/spack/cmd/mirror.py | 1 + lib/spack/spack/package.py | 14 +++++------- lib/spack/spack/stage.py | 10 +++++---- lib/spack/spack/test/mirror.py | 49 +++++++++++++++++++++--------------------- 4 files changed, 37 insertions(+), 37 deletions(-) (limited to 'lib') diff --git a/lib/spack/spack/cmd/mirror.py b/lib/spack/spack/cmd/mirror.py index 92b92e07af..a71ad16b65 100644 --- a/lib/spack/spack/cmd/mirror.py +++ b/lib/spack/spack/cmd/mirror.py @@ -172,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/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 19b7d03664..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 @@ -267,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/mirror.py b/lib/spack/spack/test/mirror.py index 0093b5d82f..046ec56604 100644 --- a/lib/spack/spack/test/mirror.py +++ b/lib/spack/spack/test/mirror.py @@ -77,6 +77,10 @@ class MirrorTest(MockPackagesTest): 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( @@ -85,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)) @@ -93,38 +97,35 @@ 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: stage.destroy() -- cgit v1.2.3-70-g09d2