buildelement: Ensure command-subdir is part of the cache key

`command-subdir` was previously missing from the cache key. This patch
makes it a part of the cache key _if_ it was specified. It means that
the cache key will not change for elements that had not defined a
`command-subdir`. However, it will change for every element that did
define it.

Fixes #1295.

4 files changed, 61 insertions, 4 deletions

diff --git a/src/buildstream/buildelement.py b/src/buildstream/buildelement.py
index ce6bb3393..aa427064b 100644
--- a/src/buildstream/buildelement.py
+++ b/src/buildstream/buildelement.py
@@ -171,6 +171,8 @@ class BuildElement(Element):
         #        extend the configuration
         node.validate_keys(_command_steps)
 
+        self._command_subdir = self.get_variable("command-subdir")  # pylint: disable=attribute-defined-outside-init
+
         for command_name in _legacy_command_steps:
             self.__commands[command_name] = node.get_str_list(command_name, [])
 
@@ -183,6 +185,9 @@ class BuildElement(Element):
         for command_name, command_list in self.__commands.items():
             dictionary[command_name] = command_list
 
+        if self._command_subdir:
+            dictionary["command-subdir"] = self._command_subdir
+
         # Specifying notparallel for a given element effects the
         # cache key, while having the side effect of setting max-jobs to 1,
         # which is normally automatically resolved and does not affect
@@ -201,9 +206,8 @@ class BuildElement(Element):
         sandbox.mark_directory(install_root)
 
         # Allow running all commands in a specified subdirectory
-        command_subdir = self.get_variable("command-subdir")
-        if command_subdir:
-            command_dir = os.path.join(build_root, command_subdir)
+        if self._command_subdir:
+            command_dir = os.path.join(build_root, self._command_subdir)
         else:
             command_dir = build_root
         sandbox.set_work_directory(command_dir)
diff --git a/tests/integration/manual.py b/tests/integration/manual.py
index 23fa68e85..defc2503c 100644
--- a/tests/integration/manual.py
+++ b/tests/integration/manual.py
@@ -2,6 +2,7 @@
 # pylint: disable=redefined-outer-name
 
 import os
+import shutil
 import pytest
 
 from buildstream import _yaml
@@ -16,7 +17,7 @@ pytestmark = pytest.mark.integration
 DATA_DIR = os.path.join(os.path.dirname(os.path.realpath(__file__)), "project")
 
 
-def create_manual_element(name, path, config, variables, environment):
+def create_manual_element(name, path, config, variables, environment, sources=None):
     element = {
         "kind": "manual",
         "depends": [{"filename": "base.bst", "type": "build"}],
@@ -24,6 +25,8 @@ def create_manual_element(name, path, config, variables, environment):
         "variables": variables,
         "environment": environment,
     }
+    if sources:
+        element["sources"] = sources
     os.makedirs(os.path.dirname(os.path.join(path, name)), exist_ok=True)
     _yaml.roundtrip_dump(element, os.path.join(path, name))
 
@@ -153,3 +156,51 @@ def test_manual_element_logging(cli, datafiles):
     assert "echo build" in res.stderr
     assert "echo install" in res.stderr
     assert "echo strip" in res.stderr
+
+
+# Regression test for https://gitlab.com/BuildStream/buildstream/-/issues/1295.
+#
+# Test that the command-subdir variable works as expected.
+@pytest.mark.datafiles(DATA_DIR)
+@pytest.mark.skipif(not HAVE_SANDBOX, reason="Only available with a functioning sandbox")
+def test_manual_command_subdir(cli, datafiles):
+    project = str(datafiles)
+    checkout = os.path.join(cli.directory, "checkout")
+    element_path = os.path.join(project, "elements")
+    element_name = "manual/command-subdir.bst"
+    sources = [{"kind": "local", "path": "files/manual-element/root"}]
+
+    create_manual_element(
+        element_name, element_path, {"install-commands": ["cp hello %{install-root}"]}, {}, {}, sources=sources,
+    )
+
+    # First, verify that element builds, and has the correct expected output.
+    result = cli.run(project=project, args=["build", element_name])
+    result.assert_success()
+    result = cli.run(project=project, args=["artifact", "checkout", element_name, "--directory", checkout])
+    result.assert_success()
+    with open(os.path.join(checkout, "hello")) as f:
+        assert f.read() == "hello from root\n"
+
+    # Now, change element configuration to have a different command-subdir.
+    # This should result in a different cache key.
+    create_manual_element(
+        element_name,
+        element_path,
+        {"install-commands": ["cp hello %{install-root}"]},
+        {"command-subdir": "subdir"},
+        {},
+        sources=sources,
+    )
+
+    # Verify that the element needs to be rebuilt.
+    assert cli.get_element_state(project, element_name) == "buildable"
+
+    # Finally, ensure that the variable actually takes effect.
+    result = cli.run(project=project, args=["build", element_name])
+    result.assert_success()
+    shutil.rmtree(checkout)
+    result = cli.run(project=project, args=["artifact", "checkout", element_name, "--directory", checkout])
+    result.assert_success()
+    with open(os.path.join(checkout, "hello")) as f:
+        assert f.read() == "hello from subdir\n"
diff --git a/tests/integration/project/files/manual-element/root/hello b/tests/integration/project/files/manual-element/root/hello
new file mode 100644
index 000000000..14dee1765
--- /dev/null
+++ b/tests/integration/project/files/manual-element/root/hello
@@ -0,0 +1 @@
+hello from root
diff --git a/tests/integration/project/files/manual-element/root/subdir/hello b/tests/integration/project/files/manual-element/root/subdir/hello
new file mode 100644
index 000000000..ae355a89e
--- /dev/null
+++ b/tests/integration/project/files/manual-element/root/subdir/hello
@@ -0,0 +1 @@
+hello from subdir