From 0217a651c887e7b2bc1636f6bcd92aa1a8456de8 Mon Sep 17 00:00:00 2001 From: Peter Scheibel Date: Sun, 16 Dec 2018 10:15:22 -0800 Subject: [PATCH] mirrors: patches are now properly added to mirrors (#8993) * This fixes a number of bugs: * Patches were not properly downloaded and added to mirrors. * Mirror create didn't respect `list_url` in packages * Update the `spack mirror` command to add all packages in the concretized DAG (where originally it only added the package specified by the user). This is required in order to collect patches that are specified by dependents. Example: * if X->Y and X requires a patch on Y called Pxy, then Pxy will only be discovered if you create a mirror with X. * replace confusing --one-version-per-spec option for `spack mirror create` with --versions-per-spec; support retrieving multiple versions for concrete specs * Implementation details: * `spack mirror create` now uses regular staging logic to download files into a mirror, instead of reimplementing it in `add_single_spec`. * use a separate resource caching object to keep track of new resources and already-existing resources; also accepts storing resources retrieved from a cache (unlike the local cache) * mirror cache object now stores resources that are considered non-cachable, like (e.g. the tip of a branch); * the 'create' function of the mirror module no longer traverses dependencies since this was already handled by the 'mirror' command; * Change handling of `--no-checksum`: * now that 'mirror create' uses stages, the mirror tests disable checksums when creating the mirror * remove `no_checksum` argument from library functions - this is now handled at the Spack-command-level (like for 'spack install') --- lib/spack/spack/caches.py | 22 +++++++++ lib/spack/spack/cmd/mirror.py | 11 +++-- lib/spack/spack/mirror.py | 84 +++++++++++++--------------------- lib/spack/spack/patch.py | 2 +- lib/spack/spack/stage.py | 5 +- lib/spack/spack/test/mirror.py | 42 ++++++++++++++++- 6 files changed, 107 insertions(+), 59 deletions(-) diff --git a/lib/spack/spack/caches.py b/lib/spack/spack/caches.py index 1d74e9dc3b..6afc8b4ffe 100644 --- a/lib/spack/spack/caches.py +++ b/lib/spack/spack/caches.py @@ -7,6 +7,7 @@ import os import llnl.util.lang +from llnl.util.filesystem import mkdirp import spack.paths import spack.config @@ -47,5 +48,26 @@ def _fetch_cache(): return spack.fetch_strategy.FsCache(path) +class MirrorCache(object): + def __init__(self, root): + self.root = os.path.abspath(root) + self.new_resources = set() + self.existing_resources = set() + + def store(self, fetcher, relative_dest): + # Note this will archive package sources even if they would not + # normally be cached (e.g. the current tip of an hg/git branch) + + dst = os.path.join(self.root, relative_dest) + if os.path.exists(dst): + self.existing_resources.add(relative_dest) + else: + self.new_resources.add(relative_dest) + mkdirp(os.path.dirname(dst)) + fetcher.archive(dst) + + #: Spack's local cache for downloaded source archives fetch_cache = llnl.util.lang.Singleton(_fetch_cache) + +mirror_cache = None diff --git a/lib/spack/spack/cmd/mirror.py b/lib/spack/spack/cmd/mirror.py index 4d70df431b..36d7f0cbd7 100644 --- a/lib/spack/spack/cmd/mirror.py +++ b/lib/spack/spack/cmd/mirror.py @@ -44,9 +44,9 @@ def setup_parser(subparser): '-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") + '-n', '--versions-per-spec', type=int, + default=1, + help="the number of versions to fetch for each spec") # used to construct scope arguments below scopes = spack.config.scopes() @@ -192,7 +192,7 @@ def mirror_create(args): # Actually do the work to create the mirror present, mirrored, error = spack.mirror.create( - directory, specs, num_versions=args.one_version_per_spec) + directory, specs, num_versions=args.versions_per_spec) p, m, e = len(present), len(mirrored), len(error) verb = "updated" if existed else "created" @@ -214,4 +214,7 @@ def mirror(parser, args): 'rm': mirror_remove, 'list': mirror_list} + if args.no_checksum: + spack.config.set('config:checksum', False, scope='command_line') + action[args.mirror_command](args) diff --git a/lib/spack/spack/mirror.py b/lib/spack/spack/mirror.py index 3310bc53fb..712a62117a 100644 --- a/lib/spack/spack/mirror.py +++ b/lib/spack/spack/mirror.py @@ -83,7 +83,11 @@ def mirror_archive_path(spec, fetcher, resource_id=None): def get_matching_versions(specs, **kwargs): - """Get a spec for EACH known version matching any spec in the list.""" + """Get a spec for EACH known version matching any spec in the list. + For concrete specs, this retrieves the concrete version and, if more + than one version per spec is requested, retrieves the latest versions + of the package. + """ matching = [] for spec in specs: pkg = spec.package @@ -93,15 +97,22 @@ def get_matching_versions(specs, **kwargs): tty.msg("No safe (checksummed) versions for package %s" % pkg.name) continue - num_versions = kwargs.get('num_versions', 0) + pkg_versions = kwargs.get('num_versions', 1) + + version_order = list(reversed(sorted(pkg.versions))) matching_spec = [] - for i, v in enumerate(reversed(sorted(pkg.versions))): + if spec.concrete: + matching_spec.append(spec) + pkg_versions -= 1 + version_order.remove(spec.version) + + for v in version_order: # Generate no more than num_versions versions for each spec. - if num_versions and i >= num_versions: + if pkg_versions < 1: break # Generate only versions that satisfy the spec. - if v.satisfies(spec.versions): + if spec.concrete or v.satisfies(spec.versions): s = Spec(pkg.name) s.versions = VersionList([v]) s.variants = spec.variants.copy() @@ -109,6 +120,7 @@ def get_matching_versions(specs, **kwargs): # concretization phase s.variants.spec = s matching_spec.append(s) + pkg_versions -= 1 if not matching_spec: tty.warn("No known version matches spec: %s" % spec) @@ -139,9 +151,8 @@ def create(path, specs, **kwargs): to the mirror. Keyword args: - no_checksum: If True, do not checkpoint when fetching (default False) num_versions: Max number of versions to fetch per spec, \ - if spec is ambiguous (default is 0 for all of them) + (default is 1 each spec) Return Value: Returns a tuple of lists: (present, mirrored, error) @@ -163,7 +174,7 @@ def create(path, specs, **kwargs): # Get concrete specs for each matching version of these specs. version_specs = get_matching_versions( - specs, num_versions=kwargs.get('num_versions', 0)) + specs, num_versions=kwargs.get('num_versions', 1)) for s in version_specs: s.concretize() @@ -183,57 +194,26 @@ def create(path, specs, **kwargs): 'error': [] } - # Iterate through packages and download all safe tarballs for each - for spec in version_specs: - add_single_spec(spec, mirror_root, categories, **kwargs) + mirror_cache = spack.caches.MirrorCache(mirror_root) + try: + spack.caches.mirror_cache = mirror_cache + # Iterate through packages and download all safe tarballs for each + for spec in version_specs: + add_single_spec(spec, mirror_root, categories, **kwargs) + finally: + spack.caches.mirror_cache = None + + categories['mirrored'] = list(mirror_cache.new_resources) + categories['present'] = list(mirror_cache.existing_resources) return categories['present'], categories['mirrored'], categories['error'] def add_single_spec(spec, mirror_root, categories, **kwargs): tty.msg("Adding package {pkg} to mirror".format(pkg=spec.format("$_$@"))) - spec_exists_in_mirror = True try: - with spec.package.stage: - # fetcher = stage.fetcher - # fetcher.fetch() - # ... - # fetcher.archive(archive_path) - for ii, stage in enumerate(spec.package.stage): - fetcher = stage.fetcher - if ii == 0: - # create a subdirectory for the current package@version - archive_path = os.path.abspath(os.path.join( - mirror_root, mirror_archive_path(spec, fetcher))) - name = spec.cformat("$_$@") - else: - resource = stage.resource - archive_path = os.path.abspath(os.path.join( - mirror_root, - mirror_archive_path(spec, fetcher, resource.name))) - name = "{resource} ({pkg}).".format( - resource=resource.name, pkg=spec.cformat("$_$@")) - subdir = os.path.dirname(archive_path) - mkdirp(subdir) - - if os.path.exists(archive_path): - tty.msg("{name} : already added".format(name=name)) - else: - spec_exists_in_mirror = False - fetcher.fetch() - if not kwargs.get('no_checksum', False): - fetcher.check() - tty.msg("{name} : checksum passed".format(name=name)) - - # Fetchers have to know how to archive their files. Use - # that to move/copy/create an archive in the mirror. - fetcher.archive(archive_path) - tty.msg("{name} : added".format(name=name)) - - if spec_exists_in_mirror: - categories['present'].append(spec) - else: - categories['mirrored'].append(spec) + spec.package.do_patch() + spec.package.do_clean() except Exception as e: if spack.config.get('config:debug'): diff --git a/lib/spack/spack/patch.py b/lib/spack/spack/patch.py index a865d429a4..cbab403f20 100644 --- a/lib/spack/spack/patch.py +++ b/lib/spack/spack/patch.py @@ -166,7 +166,7 @@ def apply(self, stage): # for a compressed archive, Need to check the patch sha256 again # and the patch is in a directory, not in the same place - if self.archive_sha256: + if self.archive_sha256 and spack.config.get('config:checksum'): checker = Checker(self.sha256) if not checker.check(self.path): raise fs.ChecksumError( diff --git a/lib/spack/spack/stage.py b/lib/spack/spack/stage.py index 32cdaa0090..e97a19ddba 100644 --- a/lib/spack/spack/stage.py +++ b/lib/spack/spack/stage.py @@ -429,12 +429,15 @@ def check(self): "mirror. This means we cannot know a checksum for the " "tarball in advance. Be sure that your connection to " "this mirror is secure!") - else: + elif spack.config.get('config:checksum'): self.fetcher.check() def cache_local(self): spack.caches.fetch_cache.store(self.fetcher, self.mirror_path) + if spack.caches.mirror_cache: + spack.caches.mirror_cache.store(self.fetcher, self.mirror_path) + 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 diff --git a/lib/spack/spack/test/mirror.py b/lib/spack/spack/test/mirror.py index d59f958481..e2ddb85c97 100644 --- a/lib/spack/spack/test/mirror.py +++ b/lib/spack/spack/test/mirror.py @@ -51,7 +51,8 @@ def check_mirror(): # register mirror with spack config mirrors = {'spack-mirror-test': 'file://' + mirror_root} spack.config.set('mirrors', mirrors) - spack.mirror.create(mirror_root, repos, no_checksum=True) + with spack.config.override('config:checksum', False): + spack.mirror.create(mirror_root, repos) # Stage directory exists assert os.path.isdir(mirror_root) @@ -138,3 +139,42 @@ def test_all_mirror( set_up_package('trivial-install-test-package', mock_archive, 'url') check_mirror() repos.clear() + + +def test_mirror_with_url_patches(mock_packages, config, monkeypatch): + spec = Spec('patch-several-dependencies') + spec.concretize() + + files_cached_in_mirror = set() + + def record_store(_class, fetcher, relative_dst): + files_cached_in_mirror.add(os.path.basename(relative_dst)) + + def successful_fetch(_class): + with open(_class.stage.save_filename, 'w'): + pass + + def successful_expand(_class): + expanded_path = os.path.join(_class.stage.path, 'expanded-dir') + os.mkdir(expanded_path) + with open(os.path.join(expanded_path, 'test.patch'), 'w'): + pass + + def successful_apply(_class, stage): + pass + + with Stage('spack-mirror-test') as stage: + mirror_root = os.path.join(stage.path, 'test-mirror') + + monkeypatch.setattr(spack.fetch_strategy.URLFetchStrategy, 'fetch', + successful_fetch) + monkeypatch.setattr(spack.fetch_strategy.URLFetchStrategy, + 'expand', successful_expand) + monkeypatch.setattr(spack.patch.Patch, 'apply', successful_apply) + monkeypatch.setattr(spack.caches.MirrorCache, 'store', record_store) + + with spack.config.override('config:checksum', False): + spack.mirror.create(mirror_root, list(spec.traverse())) + + assert not (set(['urlpatch.patch', 'urlpatch2.patch.gz']) - + files_cached_in_mirror)