Merge branch 'tiagogomes/issue-287-backport' into 'bst-1.2'

Backport of !678 (Add validation of configuration variables) to 1.2 branch.

See merge request BuildStream/buildstream!789

8 files changed, 140 insertions, 27 deletions

diff --git a/buildstream/_exceptions.py b/buildstream/_exceptions.py
index a4e3ce69b..fa595852b 100644
--- a/buildstream/_exceptions.py
+++ b/buildstream/_exceptions.py
@@ -219,6 +219,9 @@ class LoadErrorReason(Enum):
     # A recursive variable has been encountered
     RECURSIVE_VARIABLE = 22
 
+    # An attempt so set the value of a protected variable
+    PROTECTED_VARIABLE_REDEFINED = 23
+
 
 # LoadError
 #
diff --git a/buildstream/data/projectconfig.yaml b/buildstream/data/projectconfig.yaml
index 039bcca2d..247a4536a 100644
--- a/buildstream/data/projectconfig.yaml
+++ b/buildstream/data/projectconfig.yaml
@@ -20,21 +20,7 @@ fail-on-overlap: False
 # Variable Configuration
 #
 variables:
-
-  # Maximum number of parallel build processes within a given
-  # build, support for this is conditional on the element type
-  # and the build system used (any element using 'make' can
-  # implement this).
-  #
-  # Note: this value defaults to the number of cores available
-  max-jobs: 4
-
-  # Note: These variables are defined later on in element.py and _project.py
-  element-name: ""
-  project-name: ""
-
   # Path configuration, to be used in build instructions.
-  #
   prefix: "/usr"
   exec_prefix: "%{prefix}"
   bindir: "%{exec_prefix}/bin"
@@ -93,7 +79,6 @@ variables:
     find "%{install-root}" -name '*.pyc' -exec \
       dd if=/dev/zero of={} bs=1 count=4 seek=4 conv=notrunc ';'
 
-
 # Base sandbox environment, can be overridden by plugins
 environment:
   PATH: /usr/bin:/bin:/usr/sbin:/sbin
diff --git a/buildstream/element.py b/buildstream/element.py
index 4f7fc0564..6b0a728e6 100644
--- a/buildstream/element.py
+++ b/buildstream/element.py
@@ -2166,7 +2166,8 @@ class Element(Plugin):
     # substituting command strings to be run in the sandbox
     #
     def __extract_variables(self, meta):
-        default_vars = _yaml.node_get(self.__defaults, Mapping, 'variables', default_value={})
+        default_vars = _yaml.node_get(self.__defaults, Mapping, 'variables',
+                                      default_value={})
 
         project = self._get_project()
         if self.__is_junction:
@@ -2179,6 +2180,13 @@ class Element(Plugin):
         _yaml.composite(variables, meta.variables)
         _yaml.node_final_assertions(variables)
 
+        for var in ('project-name', 'element-name', 'max-jobs'):
+            provenance = _yaml.node_get_provenance(variables, var)
+            if provenance and provenance.filename != '':
+                raise LoadError(LoadErrorReason.PROTECTED_VARIABLE_REDEFINED,
+                                "{}: invalid redefinition of protected variable '{}'"
+                                .format(provenance, var))
+
         return variables
 
     # This will resolve the final configuration to be handed
diff --git a/doc/source/format_declaring.rst b/doc/source/format_declaring.rst
index 4631ee3e8..a809676fc 100644
--- a/doc/source/format_declaring.rst
+++ b/doc/source/format_declaring.rst
@@ -420,3 +420,25 @@ dependency and that all referenced variables are declared, the following is fine
      install-commands:
      - |
        %{make-install} RELEASE_TEXT="%{release-text}"
+
+
+Variables declared by BuildStream
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+BuildStream declares a set of :ref:`builtin <project_builtin_defaults>`
+variables that may be overridden. In addition, the following
+read-only variables are also dynamically declared by BuildStream:
+
+* ``element-name``
+
+  The name of the element being processed (e.g base/alpine.bst).
+
+* ``project-name``
+
+  The name of project where BuildStream is being used.
+
+* ``max-jobs``
+
+  Maximum number of parallel build processes within a given
+  build, support for this is conditional on the element type
+  and the build system used (any element using 'make' can
+  implement this).
diff --git a/tests/integration/manual.py b/tests/integration/manual.py
index e71ccdd79..241ea37a8 100644
--- a/tests/integration/manual.py
+++ b/tests/integration/manual.py
@@ -64,7 +64,7 @@ strip
 
 
 @pytest.mark.datafiles(DATA_DIR)
-def test_manual_element_noparallel(cli, tmpdir, datafiles):
+def test_manual_element_environment(cli, tmpdir, datafiles):
     project = os.path.join(datafiles.dirname, datafiles.basename)
     checkout = os.path.join(cli.directory, 'checkout')
     element_path = os.path.join(project, 'elements')
@@ -72,15 +72,11 @@ def test_manual_element_noparallel(cli, tmpdir, datafiles):
 
     create_manual_element(element_name, element_path, {
         'install-commands': [
-            "echo $MAKEFLAGS >> test",
             "echo $V >> test",
             "cp test %{install-root}"
         ]
     }, {
-        'max-jobs': 2,
-        'notparallel': True
     }, {
-        'MAKEFLAGS': '-j%{max-jobs} -Wall',
         'V': 2
     })
 
@@ -93,13 +89,11 @@ def test_manual_element_noparallel(cli, tmpdir, datafiles):
     with open(os.path.join(checkout, 'test')) as f:
         text = f.read()
 
-    assert text == """-j1 -Wall
-2
-"""
+    assert text == "2\n"
 
 
 @pytest.mark.datafiles(DATA_DIR)
-def test_manual_element_environment(cli, tmpdir, datafiles):
+def test_manual_element_noparallel(cli, tmpdir, datafiles):
     project = os.path.join(datafiles.dirname, datafiles.basename)
     checkout = os.path.join(cli.directory, 'checkout')
     element_path = os.path.join(project, 'elements')
@@ -112,7 +106,7 @@ def test_manual_element_environment(cli, tmpdir, datafiles):
             "cp test %{install-root}"
         ]
     }, {
-        'max-jobs': 2
+        'notparallel': True
     }, {
         'MAKEFLAGS': '-j%{max-jobs} -Wall',
         'V': 2
@@ -127,6 +121,6 @@ def test_manual_element_environment(cli, tmpdir, datafiles):
     with open(os.path.join(checkout, 'test')) as f:
         text = f.read()
 
-    assert text == """-j2 -Wall
+    assert text == """-j1 -Wall
 2
 """
diff --git a/tests/loader/variables.py b/tests/loader/variables.py
new file mode 100644
index 000000000..9871d63c6
--- /dev/null
+++ b/tests/loader/variables.py
@@ -0,0 +1,99 @@
+import os
+import pytest
+
+from buildstream import _yaml
+from buildstream._exceptions import ErrorDomain, LoadErrorReason
+from tests.testutils import cli
+
+DATA_DIR = os.path.join(
+    os.path.dirname(os.path.realpath(__file__)),
+    'variables',
+)
+
+PROTECTED_VARIABLES = [('project-name'), ('element-name'), ('max-jobs')]
+
+
+@pytest.mark.parametrize('protected_var', PROTECTED_VARIABLES)
+@pytest.mark.datafiles(DATA_DIR)
+def test_use_of_protected_var_project_conf(cli, tmpdir, datafiles, protected_var):
+    project = os.path.join(str(datafiles), 'simple')
+
+    conf = {
+        'name': 'test',
+        'variables': {
+            protected_var: 'some-value'
+        }
+    }
+    _yaml.dump(conf, os.path.join(project, 'project.conf'))
+
+    element = {
+        'kind': 'import',
+        'sources': [
+            {
+                'kind': 'local',
+                'path': 'foo.txt'
+            }
+        ],
+    }
+    _yaml.dump(element, os.path.join(project, 'target.bst'))
+
+    result = cli.run(project=project, args=['build', 'target.bst'])
+    result.assert_main_error(ErrorDomain.LOAD,
+                             LoadErrorReason.PROTECTED_VARIABLE_REDEFINED)
+
+
+@pytest.mark.parametrize('protected_var', PROTECTED_VARIABLES)
+@pytest.mark.datafiles(DATA_DIR)
+def test_use_of_protected_var_element_overrides(cli, tmpdir, datafiles, protected_var):
+    project = os.path.join(str(datafiles), 'simple')
+
+    conf = {
+        'name': 'test',
+        'elements': {
+            'manual': {
+                'variables': {
+                    protected_var: 'some-value'
+                }
+            }
+        }
+    }
+    _yaml.dump(conf, os.path.join(project, 'project.conf'))
+
+    element = {
+        'kind': 'manual',
+        'sources': [
+            {
+                'kind': 'local',
+                'path': 'foo.txt'
+            }
+        ],
+    }
+    _yaml.dump(element, os.path.join(project, 'target.bst'))
+
+    result = cli.run(project=project, args=['build', 'target.bst'])
+    result.assert_main_error(ErrorDomain.LOAD,
+                             LoadErrorReason.PROTECTED_VARIABLE_REDEFINED)
+
+
+@pytest.mark.parametrize('protected_var', PROTECTED_VARIABLES)
+@pytest.mark.datafiles(DATA_DIR)
+def test_use_of_protected_var_in_element(cli, tmpdir, datafiles, protected_var):
+    project = os.path.join(str(datafiles), 'simple')
+
+    element = {
+        'kind': 'import',
+        'sources': [
+            {
+                'kind': 'local',
+                'path': 'foo.txt'
+            }
+        ],
+        'variables': {
+            protected_var: 'some-value'
+        }
+    }
+    _yaml.dump(element, os.path.join(project, 'target.bst'))
+
+    result = cli.run(project=project, args=['build', 'target.bst'])
+    result.assert_main_error(ErrorDomain.LOAD,
+                             LoadErrorReason.PROTECTED_VARIABLE_REDEFINED)
diff --git a/tests/loader/variables/simple/foo.txt b/tests/loader/variables/simple/foo.txt
new file mode 100644
index 000000000..257cc5642
--- /dev/null
+++ b/tests/loader/variables/simple/foo.txt
@@ -0,0 +1 @@
+foo
diff --git a/tests/loader/variables/simple/project.conf b/tests/loader/variables/simple/project.conf
new file mode 100644
index 000000000..5a240e3ed
--- /dev/null
+++ b/tests/loader/variables/simple/project.conf
@@ -0,0 +1 @@
+name: foo