[Notes] [Git][BuildStream/buildstream][Qinusty/600-recursive-variables]

Qinusty pushed to branch Qinusty/600-recursive-variables at BuildStream / buildstream

Commits:

ddbf7bc2

by Jürg Billeter at 2018-08-28T08:56:06Z

element.py: Schedule assemble for key of workspaced elements

For uncached workspaced elements, assemble is required even just to
calculate the cache key. As dynamic scheduling relies on cache keys,
schedule assemble for uncached workspace elements even if they have not
been marked as required yet.

Fixes #461.

ffc556ff

by Valentin David at 2018-08-28T09:01:05Z

tests/frontend/workspace.py: Add test for workspaced dependencies

This adds a regression test for #461.

dd90d53d

by Jürg Billeter at 2018-08-28T09:01:05Z

element.py: Do not attempt to pull workspaced elements

6a5a8e7a

by Tristan Van Berkom at 2018-08-28T11:29:22Z

Merge branch 'juerg/workspaced-dependencies' into 'master'

Fix key invalidation for workspaced dependencies

Closes #461

See merge request BuildStream/buildstream!740

6047a575

by Tristan Van Berkom at 2018-08-28T13:41:36Z

tests/frontend/workspaces.py: Removing some redundant tests

Removed redundant tests from recently merged !740, this new
test does not need to run for every different source kind.

7535fda8

by Jürg Billeter at 2018-08-28T14:25:35Z

Merge branch 'tristan/remove-redundant-test' into 'master'

tests/frontend/workspaces.py: Removing some redundant tests

See merge request BuildStream/buildstream!741

c16c0801

by Josh Smith at 2018-08-29T09:47:05Z

Add cyclic check within variable resolution

This aims to address #600, this will raise an exception when a resolved
variable contains a reference to the variable.

fc5498a7
by Josh Smith at 2018-08-29T10:54:47Z
```
Add tests for cyclic variables check
```

9 changed files:

buildstream/_exceptions.py
buildstream/_variables.py
buildstream/element.py
dev-requirements.txt
tests/format/variables.py
+ tests/format/variables/cyclic_variables/cyclic.bst
+ tests/format/variables/cyclic_variables/project.conf
tests/frontend/workspace.py
tests/testutils/runcli.py

Changes:

buildstream/_exceptions.py

@@ -217,6 +217,9 @@ class LoadErrorReason(Enum):
      # A recursive include has been encountered.
      RECURSIVE_INCLUDE = 21
 +    # A recursive variable has been encountered
 +    RECURSIVE_VARIABLE = 22
++
  # LoadError
+ #

buildstream/_variables.py

@@ -132,6 +132,12 @@ class Variables():
                  value = _yaml.node_get(variables, str, key)
                  resolved_var, item_unmatched = self._subst(value, variables)
 +                if _wrap_variable(key) in resolved_var:
 +                    raise LoadError(LoadErrorReason.RECURSIVE_VARIABLE,
 +                                    "{}: ".format(_yaml.node_get_provenance(variables, key)) +
 +                                    ("Variable '{}' references itself recursively through" +
 +                                     " another variable declariation.").format(key))
++
                  resolved[key] = resolved_var
                  unmatched += item_unmatched
@@ -168,8 +174,12 @@ class Variables():
      # Helper function to fetch information about the node referring to a variable
+     #
      def _find_references(self, varname):
 -        fullname = '%{' + varname + '}'
 +        fullname = _wrap_variable(varname)
          for key, value in _yaml.node_items(self.original):
              if fullname in value:
                  provenance = _yaml.node_get_provenance(self.original, key)
                  yield (key, provenance)
++
++
 +def _wrap_variable(var):
 +    return "%{" + var + "}"

buildstream/element.py

@@ -1101,9 +1101,12 @@ class Element(Plugin):
              # until the full cache query below.
              if (not self.__assemble_scheduled and not self.__assemble_done and
                      not self.__cached_success(keystrength=_KeyStrength.WEAK) and
 -                    not self._pull_pending() and self._is_required()):
 -                self._schedule_assemble()
 -                return
 +                    not self._pull_pending()):
 +                # For uncached workspaced elements, assemble is required
 +                # even if we only need the cache key
 +                if self._is_required() or self._get_workspace():
 +                    self._schedule_assemble()
 +                    return
          if self.__strict_cache_key is None:
              dependencies = [
@@ -1126,13 +1129,17 @@ class Element(Plugin):
                  self.__weak_cached = self.__artifacts.contains(self, self.__weak_cache_key)
          if (not self.__assemble_scheduled and not self.__assemble_done and
 -                not self._cached_success() and not self._pull_pending() and self._is_required()):
 +                not self._cached_success() and not self._pull_pending()):
              # Workspaced sources are considered unstable if a build is pending
              # as the build will modify the contents of the workspace.
              # Determine as early as possible if a build is pending to discard
              # unstable cache keys.
 -            self._schedule_assemble()
 -            return
++
 +            # For uncached workspaced elements, assemble is required
 +            # even if we only need the cache key
 +            if self._is_required() or self._get_workspace():
 +                self._schedule_assemble()
 +                return
          if self.__cache_key is None:
              # Calculate strong cache key
@@ -1430,7 +1437,6 @@ class Element(Plugin):
      # in a subprocess.
+     #
      def _schedule_assemble(self):
 -        assert self._is_required()
          assert not self.__assemble_scheduled
          self.__assemble_scheduled = True
@@ -1438,6 +1444,8 @@ class Element(Plugin):
          for dep in self.dependencies(Scope.BUILD, recurse=False):
              dep._set_required()
 +        self._set_required()
++
          # Invalidate workspace key as the build modifies the workspace directory
          workspace = self._get_workspace()
          if workspace:
@@ -1661,6 +1669,10 @@ class Element(Plugin):
      #   (bool): Whether a pull operation is pending
+     #
      def _pull_pending(self):
 +        if self._get_workspace():
 +            # Workspace builds are never pushed to artifact servers
 +            return False
++
          if self.__strong_cached:
              # Artifact already in local cache
              return False

dev-requirements.txt

@@ -8,3 +8,4 @@ pytest-env
  pytest-pep8
  pytest-pylint
  pytest-xdist
 +pytest-timeout

tests/format/variables.py

  import os
  import pytest
 +import sys
  from buildstream import _yaml
  from buildstream._exceptions import ErrorDomain, LoadErrorReason
  from tests.testutils.runcli import cli
@@ -72,3 +73,20 @@ def test_missing_variable(cli, datafiles, tmpdir):
      ])
      result.assert_main_error(ErrorDomain.LOAD,
                               LoadErrorReason.UNRESOLVED_VARIABLE)
++
 +@pytest.mark.timeout(3, method="signal")
 +@pytest.mark.datafiles(os.path.join(DATA_DIR, 'cyclic_variables'))
 +def test_cyclic_variables(cli, datafiles):
 +    print_warning("Performing cyclic test, if this test times out it will " +
 +                  "exit the test sequence")
 +    project = os.path.join(datafiles.dirname, datafiles.basename)
 +    result = cli.run(project=project, silent=True, args=[
 +        "build", "cyclic.bst"
 +    ])
 +    result.assert_main_error(ErrorDomain.LOAD, LoadErrorReason.RECURSIVE_VARIABLE)
++
++
 +def print_warning(msg):
 +    RED, END = "\033[91m", "\033[0m"
 +    print(("\n{}{}{}").format(RED, msg, END), file=sys.stderr)
++

tests/format/variables/cyclic_variables/cyclic.bst

 +kind: manual
++
 +variables:
 +  a: "%{prefix}/a"
 +  prefix: "%{a}/some_prefix/"
 \ No newline at end of file

tests/format/variables/cyclic_variables/project.conf

+name: test

tests/frontend/workspace.py

@@ -780,3 +780,73 @@ def test_inconsitent_pipeline_message(cli, tmpdir, datafiles, kind):
          'build', element_name
      ])
      result.assert_main_error(ErrorDomain.PIPELINE, "inconsistent-pipeline-workspaced")
++
++
 +@pytest.mark.datafiles(DATA_DIR)
 +@pytest.mark.parametrize("strict", [("strict"), ("non-strict")])
 +def test_cache_key_workspace_in_dependencies(cli, tmpdir, datafiles, strict):
 +    checkout = os.path.join(str(tmpdir), 'checkout')
 +    element_name, project, workspace = open_workspace(cli, os.path.join(str(tmpdir), 'repo-a'),
 +                                                      datafiles, 'git', False)
++
 +    element_path = os.path.join(project, 'elements')
 +    back_dep_element_name = 'workspace-test-back-dep.bst'
++
 +    # Write out our test target
 +    element = {
 +        'kind': 'compose',
 +        'depends': [
 +            {
 +                'filename': element_name,
 +                'type': 'build'
 +            }
 +        ]
 +    }
 +    _yaml.dump(element,
 +               os.path.join(element_path,
 +                            back_dep_element_name))
++
 +    # Modify workspace
 +    shutil.rmtree(os.path.join(workspace, 'usr', 'bin'))
 +    os.makedirs(os.path.join(workspace, 'etc'))
 +    with open(os.path.join(workspace, 'etc', 'pony.conf'), 'w') as f:
 +        f.write("PONY='pink'")
++
 +    # Configure strict mode
 +    strict_mode = True
 +    if strict != 'strict':
 +        strict_mode = False
 +    cli.configure({
 +        'projects': {
 +            'test': {
 +                'strict': strict_mode
 +            }
 +        }
 +    })
++
 +    # Build artifact with dependency's modified workspace
 +    assert cli.get_element_state(project, element_name) == 'buildable'
 +    assert cli.get_element_key(project, element_name) == "{:?<64}".format('')
 +    assert cli.get_element_state(project, back_dep_element_name) == 'waiting'
 +    assert cli.get_element_key(project, back_dep_element_name) == "{:?<64}".format('')
 +    result = cli.run(project=project, args=['build', back_dep_element_name])
 +    result.assert_success()
 +    assert cli.get_element_state(project, element_name) == 'cached'
 +    assert cli.get_element_key(project, element_name) != "{:?<64}".format('')
 +    assert cli.get_element_state(project, back_dep_element_name) == 'cached'
 +    assert cli.get_element_key(project, back_dep_element_name) != "{:?<64}".format('')
 +    result = cli.run(project=project, args=['build', back_dep_element_name])
 +    result.assert_success()
++
 +    # Checkout the result
 +    result = cli.run(project=project, args=[
 +        'checkout', back_dep_element_name, checkout
 +    ])
 +    result.assert_success()
++
 +    # Check that the pony.conf from the modified workspace exists
 +    filename = os.path.join(checkout, 'etc', 'pony.conf')
 +    assert os.path.exists(filename)
++
 +    # Check that the original /usr/bin/hello is not in the checkout
 +    assert not os.path.exists(os.path.join(checkout, 'usr', 'bin', 'hello'))

tests/testutils/runcli.py

@@ -94,14 +94,28 @@ class Result():
      #    error_reason (any): The reason field of the error which occurred
      #    fail_message (str): An optional message to override the automatic
      #                        assertion error messages
 +    #    debug (bool): If true, prints information regarding the exit state of the result()
      # Raises:
      #    (AssertionError): If any of the assertions fail
+     #
      def assert_main_error(self,
                            error_domain,
                            error_reason,
 -                          fail_message=''):
+-
 +                          fail_message='',
 +                          *, debug=False):
 +        if debug:
 +            print(
 +                """
 +                Exit code: {}
 +                Exception: {}
 +                Domain:    {}
 +                Reason:    {}
 +                """.format(
 +                    self.exit_code,
 +                    self.exception,
 +                    self.exception.domain,
 +                    self.exception.reason
 +                ))
          assert self.exit_code == -1, fail_message
          assert self.exc is not None, fail_message
          assert self.exception is not None, fail_message

[Notes] [Git][BuildStream/buildstream][Qinusty/600-recursive-variables] 8 commits: element.py: Schedule assemble for key of workspaced elements

Qinusty pushed to branch Qinusty/600-recursive-variables at BuildStream / buildstream

Commits:

9 changed files:

Changes: