From 47dc96224d575633653391192522a3807739c477 Mon Sep 17 00:00:00 2001 From: Todd Gamblin Date: Fri, 27 Apr 2018 14:41:12 -0700 Subject: init: remove package_testing global - refactor the way test dependencies are passed to the concretizer - remove global state - update tests --- lib/spack/spack/__init__.py | 5 ----- lib/spack/spack/cmd/__init__.py | 5 +++-- lib/spack/spack/cmd/bootstrap.py | 6 ------ lib/spack/spack/cmd/install.py | 10 +++++---- lib/spack/spack/package.py | 32 +++++++++++++++++++++++++--- lib/spack/spack/package_prefs.py | 19 ----------------- lib/spack/spack/spec.py | 34 ++++++++++++++++++++---------- lib/spack/spack/test/cmd/install.py | 42 ++++++++++++++++++------------------- lib/spack/spack/test/spec_dag.py | 4 +--- 9 files changed, 82 insertions(+), 75 deletions(-) (limited to 'lib') diff --git a/lib/spack/spack/__init__.py b/lib/spack/spack/__init__.py index 002e9185de..05cfa4af58 100644 --- a/lib/spack/spack/__init__.py +++ b/lib/spack/spack/__init__.py @@ -43,11 +43,6 @@ except spack.error.SpackError as e: tty.die('while initializing Spack RepoPath:', e.message) -#: Needed for test dependencies -from spack.package_prefs import PackageTesting -package_testing = PackageTesting() - - #----------------------------------------------------------------------------- # When packages call 'from spack import *', we import a set of things that # should be useful for builds. diff --git a/lib/spack/spack/cmd/__init__.py b/lib/spack/spack/cmd/__init__.py index 0f645af5f0..f57e86ee27 100644 --- a/lib/spack/spack/cmd/__init__.py +++ b/lib/spack/spack/cmd/__init__.py @@ -139,14 +139,15 @@ def parse_specs(args, **kwargs): """ concretize = kwargs.get('concretize', False) normalize = kwargs.get('normalize', False) + tests = kwargs.get('tests', False) try: specs = spack.spec.parse(args) for spec in specs: if concretize: - spec.concretize() # implies normalize + spec.concretize(tests=tests) # implies normalize elif normalize: - spec.normalize() + spec.normalize(tests=tests) return specs diff --git a/lib/spack/spack/cmd/bootstrap.py b/lib/spack/spack/cmd/bootstrap.py index 6361d02132..41653ca1b6 100644 --- a/lib/spack/spack/cmd/bootstrap.py +++ b/lib/spack/spack/cmd/bootstrap.py @@ -50,11 +50,6 @@ def setup_parser(subparser): cd_group = subparser.add_mutually_exclusive_group() arguments.add_common_arguments(cd_group, ['clean', 'dirty']) - subparser.add_argument( - '--run-tests', action='store_true', dest='run_tests', - help="run package level tests during installation" - ) - def bootstrap(parser, args, **kwargs): kwargs.update({ @@ -62,7 +57,6 @@ def bootstrap(parser, args, **kwargs): 'keep_stage': args.keep_stage, 'install_deps': 'dependencies', 'make_jobs': args.jobs, - 'run_tests': args.run_tests, 'verbose': args.verbose, 'dirty': args.dirty }) diff --git a/lib/spack/spack/cmd/install.py b/lib/spack/spack/cmd/install.py index aed5fddbda..93aeb1009a 100644 --- a/lib/spack/spack/cmd/install.py +++ b/lib/spack/spack/cmd/install.py @@ -202,14 +202,16 @@ def install(parser, args, **kwargs): reporter.filename = args.log_file specs = spack.cmd.parse_specs(args.package) + tests = False if args.test == 'all' or args.run_tests: - spack.package_testing.test_all() + tests = True elif args.test == 'root': - for spec in specs: - spack.package_testing.test(spec.name) + tests = [spec.name for spec in specs] + kwargs['tests'] = tests try: - specs = spack.cmd.parse_specs(args.package, concretize=True) + specs = spack.cmd.parse_specs( + args.package, concretize=True, tests=tests) except SpackError as e: reporter.concretization_report(e.message) raise diff --git a/lib/spack/spack/package.py b/lib/spack/spack/package.py index a2e588079e..8df46c9963 100644 --- a/lib/spack/spack/package.py +++ b/lib/spack/spack/package.py @@ -1380,6 +1380,7 @@ class PackageBase(with_metaclass(PackageMeta, object)): make_jobs=None, fake=False, explicit=False, + tests=False, dirty=None, **kwargs): """Called by commands to install a package and its dependencies. @@ -1405,6 +1406,8 @@ class PackageBase(with_metaclass(PackageMeta, object)): fake (bool): Don't really build; install fake stub files instead. explicit (bool): True if package was explicitly installed, False if package was implicitly installed (as a dependency). + tests (bool or list or set): False to run no tests, True to test + all packages, or a list of package names to run tests for some dirty (bool): Don't clean the build environment before installing. force (bool): Install again, even if already installed. """ @@ -1435,7 +1438,6 @@ class PackageBase(with_metaclass(PackageMeta, object)): # is installed if keep_stage is False: self.stage.destroy() - return self._update_explicit_entry_in_db(rec, explicit) self._do_install_pop_kwargs(kwargs) @@ -1454,6 +1456,7 @@ class PackageBase(with_metaclass(PackageMeta, object)): skip_patch=skip_patch, verbose=verbose, make_jobs=make_jobs, + tests=tests, dirty=dirty, **kwargs) @@ -1470,8 +1473,9 @@ class PackageBase(with_metaclass(PackageMeta, object)): tty.msg('No binary for %s found: installing from source' % self.name) - # Set run_tests flag before starting build. - self.run_tests = spack.package_testing.check(self.name) + # Set run_tests flag before starting build + self.run_tests = (tests is True or + tests and self.name in tests) # Set parallelism before starting build. self.make_jobs = make_jobs @@ -1555,6 +1559,11 @@ class PackageBase(with_metaclass(PackageMeta, object)): # preserve verbosity across runs return echo + # hook that allow tests to inspect this Package before installation + # see unit_test_check() docs. + if not self.unit_test_check(): + return + try: # Create the install prefix and fork the build process. if not os.path.exists(self.prefix): @@ -1594,6 +1603,23 @@ class PackageBase(with_metaclass(PackageMeta, object)): # check the filesystem for it. self.stage.created = False + def unit_test_check(self): + """Hook for unit tests to assert things about package internals. + + Unit tests can override this function to perform checks after + ``Package.install`` and all post-install hooks run, but before + the database is updated. + + The overridden function may indicate that the install procedure + should terminate early (before updating the database) by + returning ``False`` (or any value such that ``bool(result)`` is + ``False``). + + Return: + (bool): ``True`` to continue, ``False`` to skip ``install()`` + """ + return True + def check_for_unfinished_installation( self, keep_prefix=False, restage=False): """Check for leftover files from partially-completed prior install to diff --git a/lib/spack/spack/package_prefs.py b/lib/spack/spack/package_prefs.py index 60b501acee..ca27210678 100644 --- a/lib/spack/spack/package_prefs.py +++ b/lib/spack/spack/package_prefs.py @@ -204,25 +204,6 @@ class PackagePrefs(object): if name in pkg.variants) -class PackageTesting(object): - def __init__(self): - self.packages_to_test = set() - self._test_all = False - - def test(self, package_name): - self.packages_to_test.add(package_name) - - def test_all(self): - self._test_all = True - - def clear(self): - self._test_all = False - self.packages_to_test.clear() - - def check(self, package_name): - return self._test_all or (package_name in self.packages_to_test) - - def spec_externals(spec): """Return a list of external specs (w/external directory path filled in), one for each known external installation.""" diff --git a/lib/spack/spack/spec.py b/lib/spack/spack/spec.py index cb21e551f0..87800504f7 100644 --- a/lib/spack/spack/spec.py +++ b/lib/spack/spack/spec.py @@ -1808,10 +1808,14 @@ class Spec(object): return changed - def concretize(self): + def concretize(self, tests=False): """A spec is concrete if it describes one build of a package uniquely. This will ensure that this spec is concrete. + Args: + tests (list or bool): list of packages that will need test + dependencies, or True/False for test all/none + If this spec could describe more than one version, variant, or build of a package, this will add constraints to make it concrete. @@ -1830,7 +1834,7 @@ class Spec(object): force = False while changed: - changes = (self.normalize(force), + changes = (self.normalize(force, tests), self._expand_virtual_packages(), self._concretize_helper()) changed = any(changes) @@ -2050,7 +2054,7 @@ class Spec(object): raise UnsatisfiableProviderSpecError(required[0], vdep) def _merge_dependency( - self, dependency, visited, spec_deps, provider_index): + self, dependency, visited, spec_deps, provider_index, tests): """Merge dependency information from a Package into this Spec. Args: @@ -2146,10 +2150,10 @@ class Spec(object): self._add_dependency(spec_dependency, dependency.type) changed |= spec_dependency._normalize_helper( - visited, spec_deps, provider_index) + visited, spec_deps, provider_index, tests) return changed - def _normalize_helper(self, visited, spec_deps, provider_index): + def _normalize_helper(self, visited, spec_deps, provider_index, tests): """Recursive helper function for _normalize.""" if self.name in visited: return False @@ -2170,16 +2174,23 @@ class Spec(object): for dep_name in self.package_class.dependencies: # Do we depend on dep_name? If so pkg_dep is not None. dep = self._evaluate_dependency_conditions(dep_name) + # If dep is a needed dependency, merge it. - if dep and (spack.package_testing.check(self.name) or - set(dep.type) - set(['test'])): - changed |= self._merge_dependency( - dep, visited, spec_deps, provider_index) + if dep: + merge = ( + # caller requested test dependencies + tests is True or (tests and self.name in tests) or + # this is not a test-only dependency + dep.type - set(['test'])) + + if merge: + changed |= self._merge_dependency( + dep, visited, spec_deps, provider_index, tests) any_change |= changed return any_change - def normalize(self, force=False): + def normalize(self, force=False, tests=False): """When specs are parsed, any dependencies specified are hanging off the root, and ONLY the ones that were explicitly provided are there. Normalization turns a partial flat spec into a DAG, where: @@ -2220,7 +2231,8 @@ class Spec(object): # to package files & their 'when' specs visited = set() - any_change = self._normalize_helper(visited, spec_deps, provider_index) + any_change = self._normalize_helper( + visited, spec_deps, provider_index, tests) # If there are deps specified but not visited, they're not # actually deps of this package. Raise an error. diff --git a/lib/spack/spack/test/cmd/install.py b/lib/spack/spack/test/cmd/install.py index f9d2ad74cc..f5252fe4b1 100644 --- a/lib/spack/spack/test/cmd/install.py +++ b/lib/spack/spack/test/cmd/install.py @@ -53,10 +53,8 @@ def parser(): @pytest.fixture() def noop_install(monkeypatch): - def noop(*args, **kwargs): - return - + pass monkeypatch.setattr(spack.package.PackageBase, 'do_install', noop) @@ -77,31 +75,31 @@ def test_install_package_and_dependency( assert 'errors="0"' in content -@pytest.mark.usefixtures('noop_install', 'builtin_mock', 'config') -def test_install_runtests(): - assert not spack.package_testing._test_all - assert not spack.package_testing.packages_to_test - - install('--test=root', 'dttop') - assert not spack.package_testing._test_all - assert spack.package_testing.packages_to_test == set(['dttop']) +@pytest.mark.disable_clean_stage_check +def test_install_runtests_notests(monkeypatch, builtin_mock, install_mockery): + def check(pkg): + assert not pkg.run_tests + monkeypatch.setattr(spack.package.PackageBase, 'unit_test_check', check) + install('-v', 'dttop') - spack.package_testing.clear() - install('--test=all', 'a') - assert spack.package_testing._test_all - assert not spack.package_testing.packages_to_test +@pytest.mark.disable_clean_stage_check +def test_install_runtests_root(monkeypatch, builtin_mock, install_mockery): + def check(pkg): + assert pkg.run_tests == (pkg.name == 'dttop') - spack.package_testing.clear() + monkeypatch.setattr(spack.package.PackageBase, 'unit_test_check', check) + install('--test=root', 'dttop') - install('--run-tests', 'a') - assert spack.package_testing._test_all - assert not spack.package_testing.packages_to_test - spack.package_testing.clear() +@pytest.mark.disable_clean_stage_check +def test_install_runtests_all(monkeypatch, builtin_mock, install_mockery): + def check(pkg): + assert pkg.run_tests - assert not spack.package_testing._test_all - assert not spack.package_testing.packages_to_test + monkeypatch.setattr(spack.package.PackageBase, 'unit_test_check', check) + install('--test=all', 'a') + install('--run-tests', 'a') def test_install_package_already_installed( diff --git a/lib/spack/spack/test/spec_dag.py b/lib/spack/spack/test/spec_dag.py index f8dd7a7f74..671eb06979 100644 --- a/lib/spack/spack/test/spec_dag.py +++ b/lib/spack/spack/test/spec_dag.py @@ -98,16 +98,14 @@ w->y deptypes are (link, build), w->x and y->z deptypes are (test) mock_repo = MockPackageMultiRepo([w, x, y, z]) try: - spack.package_testing.test(w.name) spack.repo = mock_repo spec = Spec('w') - spec.concretize() + spec.concretize(tests=(w.name,)) assert ('x' in spec) assert ('z' not in spec) finally: spack.repo = saved_repo - spack.package_testing.clear() @pytest.mark.usefixtures('refresh_builtin_mock') -- cgit v1.2.3-70-g09d2