From 16fa40b893054d8bd7f13625f204bf02d74a27b5 Mon Sep 17 00:00:00 2001 From: Peter Scheibel Date: Fri, 18 Mar 2016 15:50:24 -0700 Subject: [PATCH 01/25] (1) add a var/cache directory under spack. (2) downloads from URLFetchStrategy check the cache and skip the download if the source is available there. --- lib/spack/spack/__init__.py | 2 ++ lib/spack/spack/fetch_strategy.py | 19 +++++++++++++++++++ 2 files changed, 21 insertions(+) diff --git a/lib/spack/spack/__init__.py b/lib/spack/spack/__init__.py index 0ba42bbbfc..22436e46c4 100644 --- a/lib/spack/spack/__init__.py +++ b/lib/spack/spack/__init__.py @@ -46,6 +46,8 @@ stage_path = join_path(var_path, "stage") repos_path = join_path(var_path, "repos") share_path = join_path(spack_root, "share", "spack") +cache_path = join_path(spack_root, "var", "cache") +mkdirp(cache_path) prefix = spack_root opt_path = join_path(prefix, "opt") diff --git a/lib/spack/spack/fetch_strategy.py b/lib/spack/spack/fetch_strategy.py index 0d0a7db8a9..3a774a35cd 100644 --- a/lib/spack/spack/fetch_strategy.py +++ b/lib/spack/spack/fetch_strategy.py @@ -156,6 +156,11 @@ def fetch(self): if self.archive_file: tty.msg("Already downloaded %s" % self.archive_file) return + cached = self.check_cache() + if cached: + tty.msg("Cached %s." % cached) + shutil.copy(cached, "./") + return tty.msg("Trying to fetch from %s" % self.url) @@ -211,6 +216,9 @@ def fetch(self): if not self.archive_file: raise FailedDownloadError(self.url) + else: + shutil.copy(self.archive_file, spack.cache_path) + @property def archive_file(self): @@ -283,6 +291,17 @@ def check(self): "%s checksum failed for %s" % (checker.hash_name, self.archive_file), "Expected %s but got %s" % (self.digest, checker.sum)) + + def check_cache(self): + if not self.digest: + return + checker = crypto.Checker(self.digest) + paths = (join_path(spack.cache_path, f) for f in os.listdir(spack.cache_path)) + for p in paths: + if checker.check(p): + return p + + @_needs_stage def reset(self): """Removes the source path if it exists, then re-expands the archive.""" From ac7323118e9c3ddfc7b27992e545e59de2f32c7f Mon Sep 17 00:00:00 2001 From: Peter Scheibel Date: Fri, 18 Mar 2016 16:34:45 -0700 Subject: [PATCH 02/25] rename for clarity --- lib/spack/spack/fetch_strategy.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/spack/spack/fetch_strategy.py b/lib/spack/spack/fetch_strategy.py index 3a774a35cd..73083a0f5a 100644 --- a/lib/spack/spack/fetch_strategy.py +++ b/lib/spack/spack/fetch_strategy.py @@ -156,7 +156,7 @@ def fetch(self): if self.archive_file: tty.msg("Already downloaded %s" % self.archive_file) return - cached = self.check_cache() + cached = self.search_cache() if cached: tty.msg("Cached %s." % cached) shutil.copy(cached, "./") @@ -292,7 +292,7 @@ def check(self): "Expected %s but got %s" % (self.digest, checker.sum)) - def check_cache(self): + def search_cache(self): if not self.digest: return checker = crypto.Checker(self.digest) From fd067dd8b888ac4c2cc6cacec44d2ba978b04e8a Mon Sep 17 00:00:00 2001 From: Peter Scheibel Date: Fri, 18 Mar 2016 17:00:13 -0700 Subject: [PATCH 03/25] since only archives with checksums can be retrieved from the cache, make sure that an archive without a checksum isnt placed there (this wouldn't cause an error but does waste space and might be confusing) --- lib/spack/spack/fetch_strategy.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/spack/spack/fetch_strategy.py b/lib/spack/spack/fetch_strategy.py index 73083a0f5a..27fb38b7a6 100644 --- a/lib/spack/spack/fetch_strategy.py +++ b/lib/spack/spack/fetch_strategy.py @@ -216,7 +216,7 @@ def fetch(self): if not self.archive_file: raise FailedDownloadError(self.url) - else: + elif self.digest: shutil.copy(self.archive_file, spack.cache_path) From d632266a40e8d472892991144391b1862231fec0 Mon Sep 17 00:00:00 2001 From: Peter Scheibel Date: Fri, 18 Mar 2016 17:15:45 -0700 Subject: [PATCH 04/25] move cache to var/spack/cache --- lib/spack/spack/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/spack/spack/__init__.py b/lib/spack/spack/__init__.py index 22436e46c4..6fb472b15d 100644 --- a/lib/spack/spack/__init__.py +++ b/lib/spack/spack/__init__.py @@ -46,7 +46,7 @@ stage_path = join_path(var_path, "stage") repos_path = join_path(var_path, "repos") share_path = join_path(spack_root, "share", "spack") -cache_path = join_path(spack_root, "var", "cache") +cache_path = join_path(var_path, "cache") mkdirp(cache_path) prefix = spack_root From ee5e507ff60e56f9d83de9b7e2c19e929dcd3481 Mon Sep 17 00:00:00 2001 From: Peter Scheibel Date: Mon, 21 Mar 2016 20:48:12 -0700 Subject: [PATCH 05/25] pursuing a strategy using fetch.archive and treating var/spack/cache as a mirror. this should support both URLFetchStrategy as well as VCSFetchStrategy (the previous strategy supported only the former). this won't work until URLFetchStrategy.archive is updated --- lib/spack/spack/fetch_strategy.py | 17 ----------------- lib/spack/spack/package.py | 2 ++ lib/spack/spack/stage.py | 13 ++++++++++++- 3 files changed, 14 insertions(+), 18 deletions(-) diff --git a/lib/spack/spack/fetch_strategy.py b/lib/spack/spack/fetch_strategy.py index 27fb38b7a6..d1b3ce9291 100644 --- a/lib/spack/spack/fetch_strategy.py +++ b/lib/spack/spack/fetch_strategy.py @@ -156,11 +156,6 @@ def fetch(self): if self.archive_file: tty.msg("Already downloaded %s" % self.archive_file) return - cached = self.search_cache() - if cached: - tty.msg("Cached %s." % cached) - shutil.copy(cached, "./") - return tty.msg("Trying to fetch from %s" % self.url) @@ -216,8 +211,6 @@ def fetch(self): if not self.archive_file: raise FailedDownloadError(self.url) - elif self.digest: - shutil.copy(self.archive_file, spack.cache_path) @property @@ -292,16 +285,6 @@ def check(self): "Expected %s but got %s" % (self.digest, checker.sum)) - def search_cache(self): - if not self.digest: - return - checker = crypto.Checker(self.digest) - paths = (join_path(spack.cache_path, f) for f in os.listdir(spack.cache_path)) - for p in paths: - if checker.check(p): - return p - - @_needs_stage def reset(self): """Removes the source path if it exists, then re-expands the archive.""" diff --git a/lib/spack/spack/package.py b/lib/spack/spack/package.py index b488e4c49d..d007f37aeb 100644 --- a/lib/spack/spack/package.py +++ b/lib/spack/spack/package.py @@ -715,6 +715,8 @@ def do_fetch(self, mirror_only=False): if spack.do_checksum and self.version in self.versions: self.stage.check() + self.stage.cache_local() + def do_stage(self, mirror_only=False): """Unpacks the fetched tarball, then changes into the expanded tarball diff --git a/lib/spack/spack/stage.py b/lib/spack/spack/stage.py index f88f82fc2d..8933ad6da2 100644 --- a/lib/spack/spack/stage.py +++ b/lib/spack/spack/stage.py @@ -273,6 +273,7 @@ def fetch(self, mirror_only=False): # the root, so we add a '/' if it is not present. mirror_roots = [root if root.endswith('/') else root + '/' for root in mirrors.values()] + mirror_roots.append("file://" + os.path.abspath(spack.cache_path) + os.sep) urls = [urljoin(root, self.mirror_path) for root in mirror_roots] # If this archive is normally fetched from a tarball URL, @@ -305,6 +306,7 @@ def fetch(self, mirror_only=False): 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.""" @@ -318,6 +320,15 @@ def check(self): else: self.fetcher.check() + + def cache_local(self): + archiveDst = join_path(os.path.abspath(spack.cache_path), self.mirror_path) + mkdirp(os.path.dirname(archiveDst)) + # TODO: this moves the archive for URLFetchStrategy vs. a copy - edit + # to do a move? + self.fetcher.archive(archiveDst) + + def expand_archive(self): """Changes to the stage directory and attempt to expand the downloaded archive. Fail if the stage is not set up or if the archive is not yet @@ -421,7 +432,7 @@ def expand_archive(self): shutil.move(source_path, destination_path) -@pattern.composite(method_list=['fetch', 'create', 'check', 'expand_archive', 'restage', 'destroy']) +@pattern.composite(method_list=['fetch', 'create', 'check', 'expand_archive', 'restage', 'destroy', 'cache_local']) class StageComposite: """ Composite for Stage type objects. The first item in this composite is considered to be the root package, and From b255f02762375cee064b062837581e5466fdb908 Mon Sep 17 00:00:00 2001 From: Peter Scheibel Date: Mon, 21 Mar 2016 20:50:26 -0700 Subject: [PATCH 06/25] undoing whitespace-only diff --- lib/spack/spack/fetch_strategy.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/lib/spack/spack/fetch_strategy.py b/lib/spack/spack/fetch_strategy.py index d1b3ce9291..0d0a7db8a9 100644 --- a/lib/spack/spack/fetch_strategy.py +++ b/lib/spack/spack/fetch_strategy.py @@ -212,7 +212,6 @@ def fetch(self): if not self.archive_file: raise FailedDownloadError(self.url) - @property def archive_file(self): """Path to the source archive within this stage directory.""" @@ -284,7 +283,6 @@ def check(self): "%s checksum failed for %s" % (checker.hash_name, self.archive_file), "Expected %s but got %s" % (self.digest, checker.sum)) - @_needs_stage def reset(self): """Removes the source path if it exists, then re-expands the archive.""" From 41a97c8f80b8e93a7b180bd4d4a5a4286ce6f311 Mon Sep 17 00:00:00 2001 From: Peter Scheibel Date: Mon, 21 Mar 2016 20:55:23 -0700 Subject: [PATCH 07/25] temporarily wrap archiving with conditional to avoid moving (this still causes a failure on the initial download) --- lib/spack/spack/stage.py | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/lib/spack/spack/stage.py b/lib/spack/spack/stage.py index 8933ad6da2..61faec6de9 100644 --- a/lib/spack/spack/stage.py +++ b/lib/spack/spack/stage.py @@ -323,10 +323,11 @@ def check(self): def cache_local(self): archiveDst = join_path(os.path.abspath(spack.cache_path), self.mirror_path) - mkdirp(os.path.dirname(archiveDst)) - # TODO: this moves the archive for URLFetchStrategy vs. a copy - edit - # to do a move? - self.fetcher.archive(archiveDst) + if not os.path.exists(archiveDst): #tmp conditional + mkdirp(os.path.dirname(archiveDst)) + # TODO: this moves the archive for URLFetchStrategy vs. a copy - + # edit to do a move? + self.fetcher.archive(archiveDst) def expand_archive(self): From 75460d8586bf62dcedaf4eb5acdb2de75f1d662a Mon Sep 17 00:00:00 2001 From: Peter Scheibel Date: Tue, 22 Mar 2016 10:43:43 -0700 Subject: [PATCH 08/25] URLFetchStrategy.archive does a copy vs. a move now --- lib/spack/spack/fetch_strategy.py | 2 +- lib/spack/spack/stage.py | 7 ++----- 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/lib/spack/spack/fetch_strategy.py b/lib/spack/spack/fetch_strategy.py index 0d0a7db8a9..fc5d7e231c 100644 --- a/lib/spack/spack/fetch_strategy.py +++ b/lib/spack/spack/fetch_strategy.py @@ -268,7 +268,7 @@ def archive(self, destination): if not extension(destination) == extension(self.archive_file): raise ValueError("Cannot archive without matching extensions.") - shutil.move(self.archive_file, destination) + shutil.copy(self.archive_file, destination) @_needs_stage def check(self): diff --git a/lib/spack/spack/stage.py b/lib/spack/spack/stage.py index 61faec6de9..54359bddce 100644 --- a/lib/spack/spack/stage.py +++ b/lib/spack/spack/stage.py @@ -323,11 +323,8 @@ def check(self): def cache_local(self): archiveDst = join_path(os.path.abspath(spack.cache_path), self.mirror_path) - if not os.path.exists(archiveDst): #tmp conditional - mkdirp(os.path.dirname(archiveDst)) - # TODO: this moves the archive for URLFetchStrategy vs. a copy - - # edit to do a move? - self.fetcher.archive(archiveDst) + mkdirp(os.path.dirname(archiveDst)) + self.fetcher.archive(archiveDst) def expand_archive(self): From cb9fba98d8d0c389f3295d5d30cdb650e6ea79b7 Mon Sep 17 00:00:00 2001 From: Peter Scheibel Date: Tue, 22 Mar 2016 19:37:47 -0700 Subject: [PATCH 09/25] (1) relocate cache for tests (2) initial approach for restoring unit tests (just for git tests although the same concept applies to the other unit tests which are failing - namely those for svn and hg) --- lib/spack/spack/cmd/test.py | 4 ++++ lib/spack/spack/test/git_fetch.py | 4 ++++ 2 files changed, 8 insertions(+) diff --git a/lib/spack/spack/cmd/test.py b/lib/spack/spack/cmd/test.py index ddc6cb4fce..9a85c7d270 100644 --- a/lib/spack/spack/cmd/test.py +++ b/lib/spack/spack/cmd/test.py @@ -23,6 +23,7 @@ # Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA ############################################################################## import os +import shutil from pprint import pprint from llnl.util.filesystem import join_path, mkdirp @@ -66,4 +67,7 @@ def test(parser, args): if not os.path.exists(outputDir): mkdirp(outputDir) + spack.cache_path = join_path(spack.var_path, "test-cache") + mkdirp(spack.cache_path) spack.test.run(args.names, outputDir, args.verbose) + shutil.rmtree(spack.cache_path) diff --git a/lib/spack/spack/test/git_fetch.py b/lib/spack/spack/test/git_fetch.py index 3578044116..76814f089a 100644 --- a/lib/spack/spack/test/git_fetch.py +++ b/lib/spack/spack/test/git_fetch.py @@ -30,6 +30,8 @@ from spack.test.mock_repo import MockGitRepo from spack.version import ver +import shutil +import os class GitFetchTest(MockPackagesTest): """Tests fetching from a dummy git repository.""" @@ -49,6 +51,8 @@ def tearDown(self): """Destroy the stage space used by this test.""" super(GitFetchTest, self).tearDown() self.repo.destroy() + for d in os.listdir(spack.cache_path): + shutil.rmtree(os.path.join(spack.cache_path, d)) def assert_rev(self, rev): """Check that the current git revision is equal to the supplied rev.""" From ed0f6f75a7215c959431377ffe7e4faf09dc88d6 Mon Sep 17 00:00:00 2001 From: Peter Scheibel Date: Wed, 23 Mar 2016 19:49:28 -0700 Subject: [PATCH 10/25] clear test cache before and after each MockPackagesTest (I think Ive got a better way to avoid test fragility but Ill add this for now) --- lib/spack/spack/cmd/test.py | 2 +- lib/spack/spack/test/git_fetch.py | 5 ----- lib/spack/spack/test/mock_packages_test.py | 7 +++++++ 3 files changed, 8 insertions(+), 6 deletions(-) diff --git a/lib/spack/spack/cmd/test.py b/lib/spack/spack/cmd/test.py index 9a85c7d270..233cd186f0 100644 --- a/lib/spack/spack/cmd/test.py +++ b/lib/spack/spack/cmd/test.py @@ -70,4 +70,4 @@ def test(parser, args): spack.cache_path = join_path(spack.var_path, "test-cache") mkdirp(spack.cache_path) spack.test.run(args.names, outputDir, args.verbose) - shutil.rmtree(spack.cache_path) + shutil.rmtree(spack.cache_path, ignore_errors=True) diff --git a/lib/spack/spack/test/git_fetch.py b/lib/spack/spack/test/git_fetch.py index 76814f089a..d3e1206c13 100644 --- a/lib/spack/spack/test/git_fetch.py +++ b/lib/spack/spack/test/git_fetch.py @@ -30,9 +30,6 @@ from spack.test.mock_repo import MockGitRepo from spack.version import ver -import shutil -import os - class GitFetchTest(MockPackagesTest): """Tests fetching from a dummy git repository.""" @@ -51,8 +48,6 @@ def tearDown(self): """Destroy the stage space used by this test.""" super(GitFetchTest, self).tearDown() self.repo.destroy() - for d in os.listdir(spack.cache_path): - shutil.rmtree(os.path.join(spack.cache_path, d)) def assert_rev(self, rev): """Check that the current git revision is equal to the supplied rev.""" diff --git a/lib/spack/spack/test/mock_packages_test.py b/lib/spack/spack/test/mock_packages_test.py index 6d24a84150..85c15d4a6a 100644 --- a/lib/spack/spack/test/mock_packages_test.py +++ b/lib/spack/spack/test/mock_packages_test.py @@ -125,9 +125,16 @@ def cleanmock(self): pkg.dependencies.update(deps) + def rm_cache(self): + shutil.rmtree(spack.cache_path, ignore_errors=True) + + def setUp(self): + self.rm_cache() + mkdirp(spack.cache_path) self.initmock() def tearDown(self): + self.rm_cache() self.cleanmock() From dbfa6c925ec60c89004f6ccd88e985f71f650a69 Mon Sep 17 00:00:00 2001 From: Peter Scheibel Date: Wed, 23 Mar 2016 20:18:58 -0700 Subject: [PATCH 11/25] replace references to cache directory with references to new cache object. tests may assign a mock cache but by default it is None (this will avoid any implicit caching behavior confusing unit tests) --- lib/spack/llnl/util/filesystem.py | 9 +++++++++ lib/spack/spack/__init__.py | 5 ++++- lib/spack/spack/cmd/test.py | 4 +--- lib/spack/spack/stage.py | 4 +--- lib/spack/spack/test/mock_packages_test.py | 12 +++++++++--- 5 files changed, 24 insertions(+), 10 deletions(-) diff --git a/lib/spack/llnl/util/filesystem.py b/lib/spack/llnl/util/filesystem.py index c4665c284c..7586d514d1 100644 --- a/lib/spack/llnl/util/filesystem.py +++ b/lib/spack/llnl/util/filesystem.py @@ -392,3 +392,12 @@ def remove_linked_tree(path): os.unlink(path) else: shutil.rmtree(path, True) + +class FsCache(object): + def __init__(self, root): + self.root = os.path.abspath(root) + + def store(self, copyCmd, relativeDst): + dst = join_path(self.root, relativeDst) + mkdirp(os.path.dirname(dst)) + copyCmd(dst) diff --git a/lib/spack/spack/__init__.py b/lib/spack/spack/__init__.py index 6fb472b15d..0041d50624 100644 --- a/lib/spack/spack/__init__.py +++ b/lib/spack/spack/__init__.py @@ -47,7 +47,10 @@ repos_path = join_path(var_path, "repos") share_path = join_path(spack_root, "share", "spack") cache_path = join_path(var_path, "cache") -mkdirp(cache_path) + +# TODO: i get a complaint if i dont qualify this, fix that +import llnl.util.filesystem +cache = llnl.util.filesystem.FsCache(cache_path) prefix = spack_root opt_path = join_path(prefix, "opt") diff --git a/lib/spack/spack/cmd/test.py b/lib/spack/spack/cmd/test.py index 233cd186f0..1a02521814 100644 --- a/lib/spack/spack/cmd/test.py +++ b/lib/spack/spack/cmd/test.py @@ -67,7 +67,5 @@ def test(parser, args): if not os.path.exists(outputDir): mkdirp(outputDir) - spack.cache_path = join_path(spack.var_path, "test-cache") - mkdirp(spack.cache_path) + spack.cache = None spack.test.run(args.names, outputDir, args.verbose) - shutil.rmtree(spack.cache_path, ignore_errors=True) diff --git a/lib/spack/spack/stage.py b/lib/spack/spack/stage.py index 54359bddce..780f391603 100644 --- a/lib/spack/spack/stage.py +++ b/lib/spack/spack/stage.py @@ -322,9 +322,7 @@ def check(self): def cache_local(self): - archiveDst = join_path(os.path.abspath(spack.cache_path), self.mirror_path) - mkdirp(os.path.dirname(archiveDst)) - self.fetcher.archive(archiveDst) + spack.cache.store(self.fetcher.archive, self.mirror_path) def expand_archive(self): diff --git a/lib/spack/spack/test/mock_packages_test.py b/lib/spack/spack/test/mock_packages_test.py index 85c15d4a6a..06d7e7d4e1 100644 --- a/lib/spack/spack/test/mock_packages_test.py +++ b/lib/spack/spack/test/mock_packages_test.py @@ -34,6 +34,8 @@ from spack.repository import RepoPath from spack.spec import Spec +import llnl.util.tty as tty + mock_compiler_config = """\ compilers: all: @@ -130,11 +132,15 @@ def rm_cache(self): def setUp(self): - self.rm_cache() - mkdirp(spack.cache_path) + spack.cache = MockCache() self.initmock() def tearDown(self): - self.rm_cache() + spack.cache = None self.cleanmock() + +class MockCache(object): + def store(self, copyCmd, relativeDst): + tty.warn("Copying " + str(relativeDst)) + pass From 13bf7d4ff11e6897d4b688c16564f924e40f657e Mon Sep 17 00:00:00 2001 From: Peter Scheibel Date: Thu, 24 Mar 2016 12:02:39 -0700 Subject: [PATCH 12/25] (1) move definition of MockCache to test command (no definitions or extra work is required in MockPackagesTest) (2) removing outdated logic (which originated in this branch) and minor cleanup --- lib/spack/spack/cmd/test.py | 7 ++++++- lib/spack/spack/test/git_fetch.py | 1 + lib/spack/spack/test/mock_packages_test.py | 13 ------------- 3 files changed, 7 insertions(+), 14 deletions(-) diff --git a/lib/spack/spack/cmd/test.py b/lib/spack/spack/cmd/test.py index 1a02521814..3c405c5c6b 100644 --- a/lib/spack/spack/cmd/test.py +++ b/lib/spack/spack/cmd/test.py @@ -51,6 +51,11 @@ def setup_parser(subparser): help="verbose output") +class MockCache(object): + def store(self, copyCmd, relativeDst): + pass + + def test(parser, args): if args.list: print "Available tests:" @@ -67,5 +72,5 @@ def test(parser, args): if not os.path.exists(outputDir): mkdirp(outputDir) - spack.cache = None + spack.cache = MockCache() spack.test.run(args.names, outputDir, args.verbose) diff --git a/lib/spack/spack/test/git_fetch.py b/lib/spack/spack/test/git_fetch.py index d3e1206c13..3578044116 100644 --- a/lib/spack/spack/test/git_fetch.py +++ b/lib/spack/spack/test/git_fetch.py @@ -30,6 +30,7 @@ from spack.test.mock_repo import MockGitRepo from spack.version import ver + class GitFetchTest(MockPackagesTest): """Tests fetching from a dummy git repository.""" diff --git a/lib/spack/spack/test/mock_packages_test.py b/lib/spack/spack/test/mock_packages_test.py index 06d7e7d4e1..6d24a84150 100644 --- a/lib/spack/spack/test/mock_packages_test.py +++ b/lib/spack/spack/test/mock_packages_test.py @@ -34,8 +34,6 @@ from spack.repository import RepoPath from spack.spec import Spec -import llnl.util.tty as tty - mock_compiler_config = """\ compilers: all: @@ -127,20 +125,9 @@ def cleanmock(self): pkg.dependencies.update(deps) - def rm_cache(self): - shutil.rmtree(spack.cache_path, ignore_errors=True) - - def setUp(self): - spack.cache = MockCache() self.initmock() def tearDown(self): - spack.cache = None self.cleanmock() - -class MockCache(object): - def store(self, copyCmd, relativeDst): - tty.warn("Copying " + str(relativeDst)) - pass From fe71ba992d26b66a4a9492d9b0fbf17b1410b1e1 Mon Sep 17 00:00:00 2001 From: Peter Scheibel Date: Thu, 24 Mar 2016 12:16:50 -0700 Subject: [PATCH 13/25] remove unused import --- lib/spack/spack/cmd/test.py | 1 - 1 file changed, 1 deletion(-) diff --git a/lib/spack/spack/cmd/test.py b/lib/spack/spack/cmd/test.py index 3c405c5c6b..82bf3928c3 100644 --- a/lib/spack/spack/cmd/test.py +++ b/lib/spack/spack/cmd/test.py @@ -23,7 +23,6 @@ # Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA ############################################################################## import os -import shutil from pprint import pprint from llnl.util.filesystem import join_path, mkdirp From 142d1f5cbc098a4e8a0046148400bb2f40e839bc Mon Sep 17 00:00:00 2001 From: Peter Scheibel Date: Thu, 24 Mar 2016 19:28:21 -0700 Subject: [PATCH 14/25] stage creates cache fetcher with cache object (so it can be mocked for tests) --- lib/spack/llnl/util/filesystem.py | 9 --------- lib/spack/spack/__init__.py | 4 ++-- lib/spack/spack/cmd/test.py | 14 ++++++++++++++ lib/spack/spack/fetch_strategy.py | 14 ++++++++++++++ lib/spack/spack/stage.py | 2 +- 5 files changed, 31 insertions(+), 12 deletions(-) diff --git a/lib/spack/llnl/util/filesystem.py b/lib/spack/llnl/util/filesystem.py index 7586d514d1..c4665c284c 100644 --- a/lib/spack/llnl/util/filesystem.py +++ b/lib/spack/llnl/util/filesystem.py @@ -392,12 +392,3 @@ def remove_linked_tree(path): os.unlink(path) else: shutil.rmtree(path, True) - -class FsCache(object): - def __init__(self, root): - self.root = os.path.abspath(root) - - def store(self, copyCmd, relativeDst): - dst = join_path(self.root, relativeDst) - mkdirp(os.path.dirname(dst)) - copyCmd(dst) diff --git a/lib/spack/spack/__init__.py b/lib/spack/spack/__init__.py index 0041d50624..6e7cb217c8 100644 --- a/lib/spack/spack/__init__.py +++ b/lib/spack/spack/__init__.py @@ -49,8 +49,8 @@ cache_path = join_path(var_path, "cache") # TODO: i get a complaint if i dont qualify this, fix that -import llnl.util.filesystem -cache = llnl.util.filesystem.FsCache(cache_path) +import spack.fetch_strategy +cache = fetch_strategy.FsCache(cache_path) prefix = spack_root opt_path = join_path(prefix, "opt") diff --git a/lib/spack/spack/cmd/test.py b/lib/spack/spack/cmd/test.py index 82bf3928c3..8c9dea3c66 100644 --- a/lib/spack/spack/cmd/test.py +++ b/lib/spack/spack/cmd/test.py @@ -31,6 +31,7 @@ import spack import spack.test +from spack.fetch_strategy import FetchError description ="Run unit tests" @@ -54,6 +55,19 @@ class MockCache(object): def store(self, copyCmd, relativeDst): pass + def fetcher(self, targetPath, digest): + return MockCacheFetcher() + + +class MockCacheFetcher(object): + def set_stage(self, stage): + pass + + def fetch(self): + raise FetchError("Mock cache always fails for tests") + + def __str__(self): + return "[mock fetcher]" def test(parser, args): if args.list: diff --git a/lib/spack/spack/fetch_strategy.py b/lib/spack/spack/fetch_strategy.py index fc5d7e231c..b696a12e7a 100644 --- a/lib/spack/spack/fetch_strategy.py +++ b/lib/spack/spack/fetch_strategy.py @@ -689,6 +689,20 @@ def for_package_version(pkg, version): raise InvalidArgsError(pkg, version) +class FsCache(object): + def __init__(self, root): + self.root = os.path.abspath(root) + + def store(self, copyCmd, relativeDst): + dst = join_path(self.root, relativeDst) + mkdirp(os.path.dirname(dst)) + copyCmd(dst) + + def fetcher(self, targetPath, digest): + url = "file://" + join_path(self.root, targetPath) + return URLFetchStrategy(url, digest) + + class FetchError(spack.error.SpackError): def __init__(self, msg, long_msg=None): super(FetchError, self).__init__(msg, long_msg) diff --git a/lib/spack/spack/stage.py b/lib/spack/spack/stage.py index 780f391603..9f2619f43e 100644 --- a/lib/spack/spack/stage.py +++ b/lib/spack/spack/stage.py @@ -273,7 +273,6 @@ def fetch(self, mirror_only=False): # the root, so we add a '/' if it is not present. mirror_roots = [root if root.endswith('/') else root + '/' for root in mirrors.values()] - mirror_roots.append("file://" + os.path.abspath(spack.cache_path) + os.sep) urls = [urljoin(root, self.mirror_path) for root in mirror_roots] # If this archive is normally fetched from a tarball URL, @@ -290,6 +289,7 @@ def fetch(self, mirror_only=False): # Add URL strategies for all the mirrors with the digest for url in urls: fetchers.insert(0, fs.URLFetchStrategy(url, digest)) + fetchers.insert(0, spack.cache.fetcher(self.mirror_path, digest)) for fetcher in fetchers: try: From 6423eab917f6914cfce253315c9063762e6ae749 Mon Sep 17 00:00:00 2001 From: Peter Scheibel Date: Thu, 24 Mar 2016 19:45:10 -0700 Subject: [PATCH 15/25] implemented cache_local method for DIY stage (as a noop) --- lib/spack/spack/stage.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/lib/spack/spack/stage.py b/lib/spack/spack/stage.py index 9f2619f43e..e8239d27be 100644 --- a/lib/spack/spack/stage.py +++ b/lib/spack/spack/stage.py @@ -503,6 +503,8 @@ def destroy(self): # No need to destroy DIY stage. pass + def cache_local(self): + tty.msg("Sources for DIY stages are not cached") def _get_mirrors(): """Get mirrors from spack configuration.""" From bd5abb292254b40140d1b61849cb86851718b335 Mon Sep 17 00:00:00 2001 From: Peter Scheibel Date: Thu, 24 Mar 2016 19:48:15 -0700 Subject: [PATCH 16/25] spacing issue --- lib/spack/spack/stage.py | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/spack/spack/stage.py b/lib/spack/spack/stage.py index e8239d27be..d5ee231ef7 100644 --- a/lib/spack/spack/stage.py +++ b/lib/spack/spack/stage.py @@ -506,6 +506,7 @@ def destroy(self): def cache_local(self): tty.msg("Sources for DIY stages are not cached") + def _get_mirrors(): """Get mirrors from spack configuration.""" config = spack.config.get_config('mirrors') From 06c98320a88924234a86b038ca7d3a60a8361f8c Mon Sep 17 00:00:00 2001 From: Peter Scheibel Date: Fri, 25 Mar 2016 18:38:26 -0700 Subject: [PATCH 17/25] handle case where file contents change but resource name does not (e.g. if resource maintainer uses same name for each new version of a package) --- lib/spack/spack/fetch_strategy.py | 10 ++++++++++ lib/spack/spack/mirror.py | 5 ++++- 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/lib/spack/spack/fetch_strategy.py b/lib/spack/spack/fetch_strategy.py index b696a12e7a..9938f011d0 100644 --- a/lib/spack/spack/fetch_strategy.py +++ b/lib/spack/spack/fetch_strategy.py @@ -107,6 +107,8 @@ def reset(self): pass # Revert to freshly downloaded state. def archive(self, destination): pass # Used to create tarball for mirror. + def file_hash(self): pass # Identifies the resource to be retrieved + def __str__(self): # Should be human readable URL. return "FetchStrategy.__str___" @@ -217,6 +219,10 @@ def archive_file(self): """Path to the source archive within this stage directory.""" return self.stage.archive_file + @property + def file_hash(self): + return self.digest + @_needs_stage def expand(self): if not self.expand_archive: @@ -349,6 +355,10 @@ def archive(self, destination, **kwargs): self.stage.chdir() tar('-czf', destination, os.path.basename(self.stage.source_path)) + @property + def file_hash(self): + return None + def __str__(self): return "VCS: %s" % self.url diff --git a/lib/spack/spack/mirror.py b/lib/spack/spack/mirror.py index 6981f69ac0..cb2588fb29 100644 --- a/lib/spack/spack/mirror.py +++ b/lib/spack/spack/mirror.py @@ -61,7 +61,10 @@ def mirror_archive_filename(spec, fetcher): # Otherwise we'll make a .tar.gz ourselves ext = 'tar.gz' - filename = "%s-%s" % (spec.package.name, spec.version) + tokens = [spec.package.name, spec.version] + if fetcher.file_hash: + tokens.append(fetcher.file_hash) + filename = '-'.join(str(t) for t in tokens) if ext: filename += ".%s" % ext return filename From bee224c567ee735547bb183cd6a1d6e04309c81a Mon Sep 17 00:00:00 2001 From: Peter Scheibel Date: Tue, 29 Mar 2016 18:25:22 -0700 Subject: [PATCH 18/25] mirror archive filename now includes the digest type as well as the digest --- lib/spack/spack/fetch_strategy.py | 10 ---------- lib/spack/spack/mirror.py | 7 +++++-- lib/spack/spack/package.py | 7 +++++++ 3 files changed, 12 insertions(+), 12 deletions(-) diff --git a/lib/spack/spack/fetch_strategy.py b/lib/spack/spack/fetch_strategy.py index 9938f011d0..b696a12e7a 100644 --- a/lib/spack/spack/fetch_strategy.py +++ b/lib/spack/spack/fetch_strategy.py @@ -107,8 +107,6 @@ def reset(self): pass # Revert to freshly downloaded state. def archive(self, destination): pass # Used to create tarball for mirror. - def file_hash(self): pass # Identifies the resource to be retrieved - def __str__(self): # Should be human readable URL. return "FetchStrategy.__str___" @@ -219,10 +217,6 @@ def archive_file(self): """Path to the source archive within this stage directory.""" return self.stage.archive_file - @property - def file_hash(self): - return self.digest - @_needs_stage def expand(self): if not self.expand_archive: @@ -355,10 +349,6 @@ def archive(self, destination, **kwargs): self.stage.chdir() tar('-czf', destination, os.path.basename(self.stage.source_path)) - @property - def file_hash(self): - return None - def __str__(self): return "VCS: %s" % self.url diff --git a/lib/spack/spack/mirror.py b/lib/spack/spack/mirror.py index cb2588fb29..c929a092b4 100644 --- a/lib/spack/spack/mirror.py +++ b/lib/spack/spack/mirror.py @@ -62,8 +62,11 @@ def mirror_archive_filename(spec, fetcher): ext = 'tar.gz' tokens = [spec.package.name, spec.version] - if fetcher.file_hash: - tokens.append(fetcher.file_hash) + package = spack.repo.get(spec) + digests = package.digests + if digests: + if 'md5' in digests: + tokens.extend(['md5', digests['md5']]) filename = '-'.join(str(t) for t in tokens) if ext: filename += ".%s" % ext diff --git a/lib/spack/spack/package.py b/lib/spack/spack/package.py index d007f37aeb..2feea51fa5 100644 --- a/lib/spack/spack/package.py +++ b/lib/spack/spack/package.py @@ -397,6 +397,13 @@ def version(self): raise ValueError("Can only get of package with concrete version.") return self.spec.versions[0] + @property + def digests(self): + versionInfo = self.versions[self.version] + digests = {} + if 'md5' in versionInfo: + digests['md5'] = versionInfo['md5'] + return digests @memoized def version_urls(self): From ce4de6227e339bab98f8a73013bce1898f275b6f Mon Sep 17 00:00:00 2001 From: Peter Scheibel Date: Tue, 29 Mar 2016 18:45:58 -0700 Subject: [PATCH 19/25] (1) access package via spec property (2) use any digest to form archive filename --- lib/spack/spack/mirror.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/spack/spack/mirror.py b/lib/spack/spack/mirror.py index c929a092b4..78db22f73b 100644 --- a/lib/spack/spack/mirror.py +++ b/lib/spack/spack/mirror.py @@ -62,11 +62,11 @@ def mirror_archive_filename(spec, fetcher): ext = 'tar.gz' tokens = [spec.package.name, spec.version] - package = spack.repo.get(spec) - digests = package.digests + digests = spec.package.digests if digests: - if 'md5' in digests: - tokens.extend(['md5', digests['md5']]) + # If a package has multiple digests, any one is sufficient to identify it + digestType, digest = digests.iteritems().next() + tokens.extend([digestType, digest]) filename = '-'.join(str(t) for t in tokens) if ext: filename += ".%s" % ext From 03d907e1e5bfc01f7cdb457dbcd58bb903683e34 Mon Sep 17 00:00:00 2001 From: Peter Scheibel Date: Tue, 29 Mar 2016 18:48:25 -0700 Subject: [PATCH 20/25] in the case of multiple digests, avoid creating different mirror filenames from run to run (as long as the available digests do not change) --- lib/spack/spack/mirror.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/spack/spack/mirror.py b/lib/spack/spack/mirror.py index 78db22f73b..64f589ad8b 100644 --- a/lib/spack/spack/mirror.py +++ b/lib/spack/spack/mirror.py @@ -65,7 +65,7 @@ def mirror_archive_filename(spec, fetcher): digests = spec.package.digests if digests: # If a package has multiple digests, any one is sufficient to identify it - digestType, digest = digests.iteritems().next() + digestType, digest = sorted(digests.iteritems())[0] tokens.extend([digestType, digest]) filename = '-'.join(str(t) for t in tokens) if ext: From c40559433bdfadb6059070060114880c9042d799 Mon Sep 17 00:00:00 2001 From: Peter Scheibel Date: Tue, 29 Mar 2016 18:54:24 -0700 Subject: [PATCH 21/25] added docstring --- lib/spack/spack/package.py | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/spack/spack/package.py b/lib/spack/spack/package.py index 2feea51fa5..d1479ec9de 100644 --- a/lib/spack/spack/package.py +++ b/lib/spack/spack/package.py @@ -399,6 +399,7 @@ def version(self): @property def digests(self): + """All digests for the concretized package version.""" versionInfo = self.versions[self.version] digests = {} if 'md5' in versionInfo: From a0c42a3fd1a8c5319ff8e02313b7f12e8c59349d Mon Sep 17 00:00:00 2001 From: Peter Scheibel Date: Tue, 29 Mar 2016 18:58:18 -0700 Subject: [PATCH 22/25] removed stale TODO --- lib/spack/spack/__init__.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/lib/spack/spack/__init__.py b/lib/spack/spack/__init__.py index 6e7cb217c8..a2d61fa2f7 100644 --- a/lib/spack/spack/__init__.py +++ b/lib/spack/spack/__init__.py @@ -48,9 +48,8 @@ share_path = join_path(spack_root, "share", "spack") cache_path = join_path(var_path, "cache") -# TODO: i get a complaint if i dont qualify this, fix that import spack.fetch_strategy -cache = fetch_strategy.FsCache(cache_path) +cache = spack.fetch_strategy.FsCache(cache_path) prefix = spack_root opt_path = join_path(prefix, "opt") From de1ec4be8b0c7bc8c36e88a460e64b3be38cff35 Mon Sep 17 00:00:00 2001 From: Peter Scheibel Date: Mon, 6 Jun 2016 12:26:13 -0700 Subject: [PATCH 23/25] change source archive caching to omit digest from name and instead calculate and compare the checksum. This achieves the original goal of discarding stale cache files without preserving multiple files for the same version. --- lib/spack/spack/fetch_strategy.py | 23 +++++++++++++++++++++-- lib/spack/spack/mirror.py | 8 +------- lib/spack/spack/package.py | 9 --------- 3 files changed, 22 insertions(+), 18 deletions(-) diff --git a/lib/spack/spack/fetch_strategy.py b/lib/spack/spack/fetch_strategy.py index 4bbf7bb34c..fde2a8805e 100644 --- a/lib/spack/spack/fetch_strategy.py +++ b/lib/spack/spack/fetch_strategy.py @@ -343,7 +343,7 @@ def reset(self): def __repr__(self): url = self.url if self.url else "no url" - return "URLFetchStrategy<%s>" % url + return "%s<%s>" % (self.__class__.__name__, url) def __str__(self): if self.url: @@ -352,6 +352,25 @@ def __str__(self): return "[no url]" +class URLMirrorFetchStrategy(URLFetchStrategy): + """The resource associated with a URL at a mirror may be out of date. + """ + def __init__(self, *args, **kwargs): + super(URLMirrorFetchStrategy, self).__init__(*args, **kwargs) + + @_needs_stage + def fetch(self): + super(URLMirrorFetchStrategy, self).fetch() + if self.digest: + try: + self.check() + except ChecksumError: + # Future fetchers will assume they don't need to download if the + # file remains + os.remove(self.archive_file) + raise + + class VCSFetchStrategy(FetchStrategy): def __init__(self, name, *rev_types, **kwargs): super(VCSFetchStrategy, self).__init__() @@ -816,7 +835,7 @@ def store(self, copyCmd, relativeDst): def fetcher(self, targetPath, digest): url = "file://" + join_path(self.root, targetPath) - return URLFetchStrategy(url, digest) + return URLMirrorFetchStrategy(url, digest) class FetchError(spack.error.SpackError): diff --git a/lib/spack/spack/mirror.py b/lib/spack/spack/mirror.py index 81a2f6afb7..0bbcfba6b4 100644 --- a/lib/spack/spack/mirror.py +++ b/lib/spack/spack/mirror.py @@ -61,13 +61,7 @@ def mirror_archive_filename(spec, fetcher): # Otherwise we'll make a .tar.gz ourselves ext = 'tar.gz' - tokens = [spec.package.name, spec.version] - digests = spec.package.digests - if digests: - # If a package has multiple digests, any one is sufficient to identify it - digestType, digest = sorted(digests.iteritems())[0] - tokens.extend([digestType, digest]) - filename = '-'.join(str(t) for t in tokens) + filename = "%s-%s" % (spec.package.name, spec.version) if ext: filename += ".%s" % ext return filename diff --git a/lib/spack/spack/package.py b/lib/spack/spack/package.py index 70ec243726..cbf50e56f6 100644 --- a/lib/spack/spack/package.py +++ b/lib/spack/spack/package.py @@ -413,15 +413,6 @@ def version(self): raise ValueError("Can only get of package with concrete version.") return self.spec.versions[0] - @property - def digests(self): - """All digests for the concretized package version.""" - versionInfo = self.versions[self.version] - digests = {} - if 'md5' in versionInfo: - digests['md5'] = versionInfo['md5'] - return digests - @memoized def version_urls(self): """Return a list of URLs for different versions of this From a2754894ea67e0e751121411ff92d60ca68ab089 Mon Sep 17 00:00:00 2001 From: Peter Scheibel Date: Tue, 7 Jun 2016 16:26:54 -0700 Subject: [PATCH 24/25] (1) FsCache store now takes a fetcher vs. just a copy command (2) use [1] to conditionally cache resource: only save it if there is a feature which identifies it uniquely (for example do not cache a repository if it pulls the latest state vs. a particular tag/commit) --- lib/spack/spack/fetch_strategy.py | 16 ++++++++++++++-- lib/spack/spack/stage.py | 2 +- 2 files changed, 15 insertions(+), 3 deletions(-) diff --git a/lib/spack/spack/fetch_strategy.py b/lib/spack/spack/fetch_strategy.py index fde2a8805e..3221716b91 100644 --- a/lib/spack/spack/fetch_strategy.py +++ b/lib/spack/spack/fetch_strategy.py @@ -828,10 +828,22 @@ class FsCache(object): def __init__(self, root): self.root = os.path.abspath(root) - def store(self, copyCmd, relativeDst): + def store(self, fetcher, relativeDst): + unique = False + uidGroups = [['tag', 'commit'], ['digest'], ['revision']] + for grp in uidGroups: + try: + unique |= any(getattr(fetcher, x) for x in grp) + except AttributeError: + pass + if unique: + break + if not unique: + return + dst = join_path(self.root, relativeDst) mkdirp(os.path.dirname(dst)) - copyCmd(dst) + fetcher.archive(dst) def fetcher(self, targetPath, digest): url = "file://" + join_path(self.root, targetPath) diff --git a/lib/spack/spack/stage.py b/lib/spack/spack/stage.py index f28934d10a..b08cce43b8 100644 --- a/lib/spack/spack/stage.py +++ b/lib/spack/spack/stage.py @@ -337,7 +337,7 @@ def check(self): def cache_local(self): - spack.cache.store(self.fetcher.archive, self.mirror_path) + spack.cache.store(self.fetcher, self.mirror_path) def expand_archive(self): From 3b71d78f3cddeb1b622b9a49146adadbd1b1d9dc Mon Sep 17 00:00:00 2001 From: Peter Scheibel Date: Wed, 8 Jun 2016 09:57:56 -0700 Subject: [PATCH 25/25] rename URLMirrorFetchStrategy to CacheURLFetchStrategy since it isnt used to manage all mirror URLs - just the cache (the specific behavior that a URL may refer to a stale resource doesn't necessarily apply to mirrors) --- lib/spack/spack/fetch_strategy.py | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/lib/spack/spack/fetch_strategy.py b/lib/spack/spack/fetch_strategy.py index 3221716b91..2607d0a7f4 100644 --- a/lib/spack/spack/fetch_strategy.py +++ b/lib/spack/spack/fetch_strategy.py @@ -352,15 +352,14 @@ def __str__(self): return "[no url]" -class URLMirrorFetchStrategy(URLFetchStrategy): - """The resource associated with a URL at a mirror may be out of date. - """ +class CacheURLFetchStrategy(URLFetchStrategy): + """The resource associated with a cache URL may be out of date.""" def __init__(self, *args, **kwargs): - super(URLMirrorFetchStrategy, self).__init__(*args, **kwargs) + super(CacheURLFetchStrategy, self).__init__(*args, **kwargs) @_needs_stage def fetch(self): - super(URLMirrorFetchStrategy, self).fetch() + super(CacheURLFetchStrategy, self).fetch() if self.digest: try: self.check() @@ -847,7 +846,7 @@ def store(self, fetcher, relativeDst): def fetcher(self, targetPath, digest): url = "file://" + join_path(self.root, targetPath) - return URLMirrorFetchStrategy(url, digest) + return CacheURLFetchStrategy(url, digest) class FetchError(spack.error.SpackError):