From 9f0e12f15f29ea48f1d8b242d233889806942cf8 Mon Sep 17 00:00:00 2001 From: Benjamin Schubert Date: Wed, 7 Nov 2018 15:52:29 +0000 Subject: Move bwrap checks in platform/linux.py Remove the bwraps checks from _site.py and put them in platform.linux which is the only place where they are run. This allows the removal of a double level of caching, making reasoning about tests easier --- buildstream/_platform/linux.py | 35 +++++++++++++++++++++++++++++----- buildstream/_site.py | 43 ------------------------------------------ 2 files changed, 30 insertions(+), 48 deletions(-) diff --git a/buildstream/_platform/linux.py b/buildstream/_platform/linux.py index 6e488472f..afdf81c79 100644 --- a/buildstream/_platform/linux.py +++ b/buildstream/_platform/linux.py @@ -18,9 +18,9 @@ # Tristan Maat import os +import shutil import subprocess -from .. import _site from .. import utils from ..sandbox import SandboxDummy @@ -37,12 +37,19 @@ class Linux(Platform): self._gid = os.getegid() self._have_fuse = os.path.exists("/dev/fuse") - self._bwrap_exists = _site.check_bwrap_version(0, 0, 0) - self._have_good_bwrap = _site.check_bwrap_version(0, 1, 2) - self._local_sandbox_available = self._have_fuse and self._have_good_bwrap + bwrap_version = self._get_bwrap_version() - self._die_with_parent_available = _site.check_bwrap_version(0, 1, 8) + if bwrap_version is None: + self._bwrap_exists = False + self._have_good_bwrap = False + self._die_with_parent_available = False + else: + self._bwrap_exists = True + self._have_good_bwrap = (0, 1, 2) <= bwrap_version + self._die_with_parent_available = (0, 1, 8) <= bwrap_version + + self._local_sandbox_available = self._have_fuse and self._have_good_bwrap if self._local_sandbox_available: self._user_ns_available = self._check_user_ns_available() @@ -112,3 +119,21 @@ class Linux(Platform): output = '' return output == 'root' + + def _get_bwrap_version(self): + # Get the current bwrap version + # + # returns None if no bwrap was found + # otherwise returns a tuple of 3 int: major, minor, patch + bwrap_path = shutil.which('bwrap') + + if not bwrap_path: + return None + + cmd = [bwrap_path, "--version"] + try: + version = str(subprocess.check_output(cmd).split()[1], "utf-8") + except subprocess.CalledProcessError: + return None + + return tuple(int(x) for x in version.split(".")) diff --git a/buildstream/_site.py b/buildstream/_site.py index 30e1000d4..d64390b5d 100644 --- a/buildstream/_site.py +++ b/buildstream/_site.py @@ -18,8 +18,6 @@ # Tristan Van Berkom import os -import shutil -import subprocess # # Private module declaring some info about where the buildstream @@ -46,44 +44,3 @@ build_all_template = os.path.join(root, 'data', 'build-all.sh.in') # Module building script template build_module_template = os.path.join(root, 'data', 'build-module.sh.in') - -# Cached bwrap version -_bwrap_major = None -_bwrap_minor = None -_bwrap_patch = None - - -# check_bwrap_version() -# -# Checks the version of installed bwrap against the requested version -# -# Args: -# major (int): The required major version -# minor (int): The required minor version -# patch (int): The required patch level -# -# Returns: -# (bool): Whether installed bwrap meets the requirements -# -def check_bwrap_version(major, minor, patch): - # pylint: disable=global-statement - - global _bwrap_major - global _bwrap_minor - global _bwrap_patch - - # Parse bwrap version and save into cache, if not already cached - if _bwrap_major is None: - bwrap_path = shutil.which('bwrap') - if not bwrap_path: - return False - cmd = [bwrap_path, "--version"] - try: - version = str(subprocess.check_output(cmd).split()[1], "utf-8") - except subprocess.CalledProcessError: - # Failure trying to run bubblewrap - return False - _bwrap_major, _bwrap_minor, _bwrap_patch = map(int, version.split(".")) - - # Check whether the installed version meets the requirements - return (_bwrap_major, _bwrap_minor, _bwrap_patch) >= (major, minor, patch) -- cgit v1.2.1 From cf2e0059880058f0672aebb38f57fc43d347ae9a Mon Sep 17 00:00:00 2001 From: Benjamin Schubert Date: Thu, 1 Nov 2018 11:25:36 -0400 Subject: conftest.py: Ensure platform is not maintained between tests This removes the `_instance` on the platform object that we use for caching and not recreating the object everytime at the start of every test. This is to ensure our tests share the least amount of state. The performance penalty is from 5 to 10% accross the whole test suite. The readings were done 5 times for each before and after the change and on the same computer. --- conftest.py | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/conftest.py b/conftest.py index 99a602db4..1f0259f0f 100755 --- a/conftest.py +++ b/conftest.py @@ -23,6 +23,8 @@ import shutil import pytest +from buildstream._platform.platform import Platform + def pytest_addoption(parser): parser.addoption('--integration', action='store_true', default=False, @@ -52,3 +54,8 @@ def integration_cache(request): shutil.rmtree(os.path.join(cache_dir, 'artifacts')) except FileNotFoundError: pass + + +@pytest.fixture(autouse=True) +def clean_platform_cache(): + Platform._instance = None -- cgit v1.2.1 From c51ba01b7dce07c713ba7d9fd5505a5b9d575c5b Mon Sep 17 00:00:00 2001 From: Benjamin Schubert Date: Thu, 1 Nov 2018 13:24:12 -0400 Subject: Test that helpful messages are raised when missing dependencies This adds a `reason` to the SandboxEror thrown in sandboxdummy to be able to understand where the error comes from --- buildstream/sandbox/_sandboxdummy.py | 3 +- .../missing-dependencies/elements/base.bst | 4 + .../missing-dependencies/files/base/bin/sh | 1 + tests/sandboxes/missing-dependencies/project.conf | 4 + tests/sandboxes/missing_dependencies.py | 86 ++++++++++++++++++++++ 5 files changed, 97 insertions(+), 1 deletion(-) create mode 100755 tests/sandboxes/missing-dependencies/elements/base.bst create mode 100755 tests/sandboxes/missing-dependencies/files/base/bin/sh create mode 100755 tests/sandboxes/missing-dependencies/project.conf create mode 100644 tests/sandboxes/missing_dependencies.py diff --git a/buildstream/sandbox/_sandboxdummy.py b/buildstream/sandbox/_sandboxdummy.py index c0a86a0bb..0e3754c1b 100644 --- a/buildstream/sandbox/_sandboxdummy.py +++ b/buildstream/sandbox/_sandboxdummy.py @@ -42,4 +42,5 @@ class SandboxDummy(Sandbox): "'{}'".format(command[0]), reason='missing-command') - raise SandboxError("This platform does not support local builds: {}".format(self._reason)) + raise SandboxError("This platform does not support local builds: {}".format(self._reason), + reason="unavailable-local-sandbox") diff --git a/tests/sandboxes/missing-dependencies/elements/base.bst b/tests/sandboxes/missing-dependencies/elements/base.bst new file mode 100755 index 000000000..dcd6a9d60 --- /dev/null +++ b/tests/sandboxes/missing-dependencies/elements/base.bst @@ -0,0 +1,4 @@ +kind: import +sources: +- kind: local + path: files/base/ diff --git a/tests/sandboxes/missing-dependencies/files/base/bin/sh b/tests/sandboxes/missing-dependencies/files/base/bin/sh new file mode 100755 index 000000000..c102e04c1 --- /dev/null +++ b/tests/sandboxes/missing-dependencies/files/base/bin/sh @@ -0,0 +1 @@ +# This is the original bash diff --git a/tests/sandboxes/missing-dependencies/project.conf b/tests/sandboxes/missing-dependencies/project.conf new file mode 100755 index 000000000..080ab758f --- /dev/null +++ b/tests/sandboxes/missing-dependencies/project.conf @@ -0,0 +1,4 @@ +# Project config for missing dependencies test +name: test + +element-path: elements diff --git a/tests/sandboxes/missing_dependencies.py b/tests/sandboxes/missing_dependencies.py new file mode 100644 index 000000000..d77674c64 --- /dev/null +++ b/tests/sandboxes/missing_dependencies.py @@ -0,0 +1,86 @@ +import os +import pytest +from tests.testutils import cli +from tests.testutils.site import IS_LINUX + +from buildstream import _yaml +from buildstream._exceptions import ErrorDomain + + +# Project directory +DATA_DIR = os.path.join( + os.path.dirname(os.path.realpath(__file__)), + "missing-dependencies", +) + + +@pytest.mark.skipif(not IS_LINUX, reason='Only available on Linux') +@pytest.mark.datafiles(DATA_DIR) +def test_missing_brwap_has_nice_error_message(cli, datafiles): + project = os.path.join(datafiles.dirname, datafiles.basename) + element_path = os.path.join(project, 'elements', 'element.bst') + + # Write out our test target + element = { + 'kind': 'script', + 'depends': [ + { + 'filename': 'base.bst', + 'type': 'build', + }, + ], + 'config': { + 'commands': [ + 'false', + ], + }, + } + _yaml.dump(element, element_path) + + # Build without access to host tools, this should fail with a nice error + result = cli.run( + project=project, args=['build', 'element.bst'], env={'PATH': ''}) + result.assert_task_error(ErrorDomain.SANDBOX, 'unavailable-local-sandbox') + assert "not found" in result.stderr + + +@pytest.mark.skipif(not IS_LINUX, reason='Only available on Linux') +@pytest.mark.datafiles(DATA_DIR) +def test_old_brwap_has_nice_error_message(cli, datafiles, tmp_path): + bwrap = tmp_path.joinpath('bin/bwrap') + bwrap.parent.mkdir() + with bwrap.open('w') as fp: + fp.write(''' + #!/bin/sh + echo bubblewrap 0.0.1 + '''.strip()) + + bwrap.chmod(0o755) + + project = os.path.join(datafiles.dirname, datafiles.basename) + element_path = os.path.join(project, 'elements', 'element3.bst') + + # Write out our test target + element = { + 'kind': 'script', + 'depends': [ + { + 'filename': 'base.bst', + 'type': 'build', + }, + ], + 'config': { + 'commands': [ + 'false', + ], + }, + } + _yaml.dump(element, element_path) + + # Build without access to host tools, this should fail with a nice error + result = cli.run( + project=project, + args=['--debug', '--verbose', 'build', 'element3.bst'], + env={'PATH': str(tmp_path.joinpath('bin'))}) + result.assert_task_error(ErrorDomain.SANDBOX, 'unavailable-local-sandbox') + assert "too old" in result.stderr -- cgit v1.2.1