Merge branch 'coldtom/filter-element-improvements' into 'master'

plugins/elements/filter.py: Allow passing integration commands through, fail if dep is a stack element

Closes #1107 and #1104

See merge request BuildStream/buildstream!1593

13 files changed, 158 insertions, 1 deletions

diff --git a/src/buildstream/element.py b/src/buildstream/element.py
index 2efdd3abb..044b97458 100644
--- a/src/buildstream/element.py
+++ b/src/buildstream/element.py
@@ -211,6 +211,12 @@ class Element(Plugin):
     *Since: 1.4*
     """
 
+    BST_ELEMENT_HAS_ARTIFACT = True
+    """Whether the element produces an artifact when built.
+
+    *Since: 1.90*
+    """
+
     def __init__(self, context: 'Context', project: 'Project', meta: 'MetaElement', plugin_conf: Dict[str, Any]):
 
         self.__cache_key_dict = None            # Dict for cache key calculation
diff --git a/src/buildstream/plugins/elements/filter.py b/src/buildstream/plugins/elements/filter.py
index c2c2e0125..d808c9e5a 100644
--- a/src/buildstream/plugins/elements/filter.py
+++ b/src/buildstream/plugins/elements/filter.py
@@ -168,7 +168,7 @@ class FilterElement(Element):
 
     def configure(self, node):
         node.validate_keys([
-            'include', 'exclude', 'include-orphans'
+            'include', 'exclude', 'include-orphans', 'pass-integration'
         ])
 
         self.include_node = node.get_sequence('include')
@@ -177,6 +177,7 @@ class FilterElement(Element):
         self.include = self.include_node.as_str_list()
         self.exclude = self.exclude_node.as_str_list()
         self.include_orphans = node.get_bool('include-orphans')
+        self.pass_integration = node.get_bool('pass-integration', False)
 
     def preflight(self):
         # Exactly one build-depend is permitted
@@ -199,6 +200,14 @@ class FilterElement(Element):
                                .format(self, type(self).__name__),
                                detail=detail, reason="filter-bdepend-also-rdepend")
 
+        # If a parent does not produce an artifact, fail and inform user that the dependency
+        # must produce artifacts
+        if not build_deps[0].BST_ELEMENT_HAS_ARTIFACT:
+            detail = "{} does not produce an artifact, so there is nothing to filter".format(build_deps[0].name)
+            raise ElementError("{}: {} element's build dependency must produce an artifact"
+                               .format(self, type(self).__name__),
+                               detail=detail, reason="filter-bdepend-no-artifact")
+
     def get_unique_key(self):
         key = {
             'include': sorted(self.include),
@@ -252,6 +261,12 @@ class FilterElement(Element):
         output_elm = build_deps[0]._get_source_element()
         return output_elm
 
+    def integrate(self, sandbox):
+        if self.pass_integration:
+            for dep in self.dependencies(Scope.BUILD, recurse=False):
+                dep.integrate(sandbox)
+        super().integrate(sandbox)
+
 
 def setup():
     return FilterElement
diff --git a/src/buildstream/plugins/elements/filter.yaml b/src/buildstream/plugins/elements/filter.yaml
index 9c2bf69f4..12a82a9cb 100644
--- a/src/buildstream/plugins/elements/filter.yaml
+++ b/src/buildstream/plugins/elements/filter.yaml
@@ -27,3 +27,8 @@ config:
   # the parent element.
   #
   include-orphans: False
+
+  # Whether to pass the 'integration-commands' of the
+  # parent element through the filter.
+  #
+  pass-integration: False
diff --git a/src/buildstream/plugins/elements/stack.py b/src/buildstream/plugins/elements/stack.py
index 97517ca48..dbb59a43d 100644
--- a/src/buildstream/plugins/elements/stack.py
+++ b/src/buildstream/plugins/elements/stack.py
@@ -33,6 +33,9 @@ class StackElement(Element):
     # This plugin has been modified to avoid the use of Sandbox.get_directory
     BST_VIRTUAL_DIRECTORY = True
 
+    # This plugin does not produce any artifacts when built
+    BST_ELEMENT_HAS_ARTIFACT = False
+
     def configure(self, node):
         pass
 
diff --git a/tests/elements/filter.py b/tests/elements/filter.py
index d8370c6bb..af3f348b2 100644
--- a/tests/elements/filter.py
+++ b/tests/elements/filter.py
@@ -522,3 +522,37 @@ def test_filter_fails_for_nonexisting_domain(datafiles, cli):
     error = "Unknown domains were used in output-include-nonexistent-domain.bst [line 7 column 2]"
     assert error in result.stderr
     assert '- unknown_file' in result.stderr
+
+
+@pytest.mark.datafiles(os.path.join(DATA_DIR, 'basic'))
+def test_filter_pass_integration(datafiles, cli):
+    project = str(datafiles)
+
+    # Explicitly not passing integration commands should be fine
+    result = cli.run(project=project, args=['build', 'no-pass-integration.bst'])
+    result.assert_success()
+
+    # Passing integration commands should build nicely
+    result = cli.run(project=project, args=['build', 'pass-integration.bst'])
+    result.assert_success()
+
+    # Checking out elements which don't pass integration commands should still work
+    checkout_dir = os.path.join(project, 'no-pass')
+    result = cli.run(project=project, args=['artifact', 'checkout', '--integrate',
+                                            '--directory', checkout_dir, 'no-pass-integration.bst'])
+    result.assert_success()
+
+    # Checking out the artifact should fail if we run integration commands, as
+    # the staged artifacts don't have a shell
+    checkout_dir = os.path.join(project, 'pass')
+    result = cli.run(project=project, args=['artifact', 'checkout', '--integrate',
+                                            '--directory', checkout_dir, 'pass-integration.bst'])
+    result.assert_main_error(ErrorDomain.STREAM, "missing-command")
+
+
+@pytest.mark.datafiles(os.path.join(DATA_DIR, 'basic'))
+def test_filter_stack_depend_failure(datafiles, cli):
+    project = str(datafiles)
+
+    result = cli.run(project=project, args=['build', 'forbidden-stack-dep.bst'])
+    result.assert_main_error(ErrorDomain.ELEMENT, "filter-bdepend-no-artifact")
diff --git a/tests/elements/filter/basic/elements/forbidden-stack-dep.bst b/tests/elements/filter/basic/elements/forbidden-stack-dep.bst
new file mode 100644
index 000000000..6624c5d9a
--- /dev/null
+++ b/tests/elements/filter/basic/elements/forbidden-stack-dep.bst
@@ -0,0 +1,4 @@
+kind: filter
+depends:
+- filename: stack.bst
+  type: build
diff --git a/tests/elements/filter/basic/elements/input.bst b/tests/elements/filter/basic/elements/input.bst
index fb3f5d194..94d8c17c4 100644
--- a/tests/elements/filter/basic/elements/input.bst
+++ b/tests/elements/filter/basic/elements/input.bst
@@ -9,3 +9,6 @@ public:
       - /foo
       bar:
       - /bar
+    integration-commands:
+    - foo
+    - bar
diff --git a/tests/elements/filter/basic/elements/no-pass-integration.bst b/tests/elements/filter/basic/elements/no-pass-integration.bst
new file mode 100644
index 000000000..e512ade01
--- /dev/null
+++ b/tests/elements/filter/basic/elements/no-pass-integration.bst
@@ -0,0 +1,6 @@
+kind: filter
+depends:
+- filename: input.bst
+  type: build
+config:
+  pass-integration: False
diff --git a/tests/elements/filter/basic/elements/pass-integration.bst b/tests/elements/filter/basic/elements/pass-integration.bst
new file mode 100644
index 000000000..77b462cd0
--- /dev/null
+++ b/tests/elements/filter/basic/elements/pass-integration.bst
@@ -0,0 +1,6 @@
+kind: filter
+depends:
+- filename: input.bst
+  type: build
+config:
+  pass-integration: True
diff --git a/tests/elements/filter/basic/elements/stack.bst b/tests/elements/filter/basic/elements/stack.bst
new file mode 100644
index 000000000..daea67ccf
--- /dev/null
+++ b/tests/elements/filter/basic/elements/stack.bst
@@ -0,0 +1,3 @@
+kind: stack
+depends:
+- filename: input.bst
diff --git a/tests/integration/filter.py b/tests/integration/filter.py
new file mode 100644
index 000000000..1d9c11b01
--- /dev/null
+++ b/tests/integration/filter.py
@@ -0,0 +1,36 @@
+# Pylint doesn't play well with fixtures and dependency injection from pytest
+# pylint: disable=redefined-outer-name
+
+import os
+import shutil
+import pytest
+
+from buildstream.testing import cli  # pylint: disable=unused-import
+from buildstream.testing.integration import assert_contains
+from buildstream.testing._utils.site import HAVE_SANDBOX
+
+
+DATA_DIR = os.path.join(
+    os.path.dirname(os.path.realpath(__file__)),
+    'project'
+)
+
+
+@pytest.mark.datafiles(os.path.join(DATA_DIR))
+@pytest.mark.skipif(not HAVE_SANDBOX, reason='Only available with a functioning sandbox')
+def test_filter_pass_integration(datafiles, cli):
+    project = str(datafiles)
+
+    # Passing integration commands should build nicely
+    result = cli.run(project=project, args=['build', 'filter/filter.bst'])
+    result.assert_success()
+
+    # Checking out the element should work
+    checkout_dir = os.path.join(project, 'filter')
+    result = cli.run(project=project, args=['artifact', 'checkout', '--integrate', '--hardlinks',
+                                            '--directory', checkout_dir, 'filter/filter.bst'])
+    result.assert_success()
+
+    # Check that the integration command was run
+    assert_contains(checkout_dir, ['/foo'])
+    shutil.rmtree(checkout_dir)
diff --git a/tests/integration/project/elements/filter/filter.bst b/tests/integration/project/elements/filter/filter.bst
new file mode 100644
index 000000000..78b5ff12d
--- /dev/null
+++ b/tests/integration/project/elements/filter/filter.bst
@@ -0,0 +1,16 @@
+kind: filter
+depends:
+- filename: filter/parent.bst
+  type: build
+config:
+  pass-integration: True
+
+  # `sh` is `/bin/sh`, and `/bin` is not covered by any split rule
+  # As a result, to include `sh` in the image, we need to include orphans
+  #
+  # However we can use this to minimise the size of the artifacts, which
+  # will hopefully reduce test time
+  #
+  include-orphans: True
+  exclude:
+  - runtime
diff --git a/tests/integration/project/elements/filter/parent.bst b/tests/integration/project/elements/filter/parent.bst
new file mode 100644
index 000000000..35ac74d35
--- /dev/null
+++ b/tests/integration/project/elements/filter/parent.bst
@@ -0,0 +1,20 @@
+kind: compose
+depends:
+- filename: base.bst
+  type: build
+public:
+  bst:
+    integration-commands:
+    - touch /foo
+config:
+  # `sh` is `/bin/sh`, and `/bin` is not covered by any split rule
+  # As a result, to include `sh` in the image, we need to include orphans
+  #
+  # However we can use this to minimise the size of the artifacts, which
+  # will hopefully reduce test time
+  #
+  include-orphans: True
+  include:
+  - runtime
+  exclude:
+  - runtime