[Notes] [Git][BuildStream/buildstream][aevri/are_you_sure] 4 commits:

Angelos Evripiotis pushed to branch aevri/are_you_sure at BuildStream / buildstream

Commits:

a3e2cdd2

by Tom Pollard at 2019-01-28T12:14:40Z

_stream.py: Add use_artifact_config opt arg for load_selection()

use_artifact_config added as an optional default arg, allowing
for loading of given elements artifact remote config.

805baf7d

by Tom Pollard at 2019-01-28T12:14:40Z

Download buildtrees on demand for bst shell --use-buildtree

Provide bst shell --use-buildtree the ability to attempt to
acquire missing buildtrees, given respective option, user
pull-buildtree context and remote availability.

_frontend/cli.py: Refactor logic for determining --use-buildtree
option with given opportunity to attempt pulling a non-local
buildtree. Element loaded with artifact_config to allow remote
querying.

_stream.py: With given user option and element state, construct
PullQueue to fetch remote buildtree. Continue or Error without
buildtree if cannot be attained.

tests/integration/build-tree.py: Update to support new usecases

38356932

by Tom Pollard at 2019-01-28T13:44:37Z

Merge branch 'tpollard/829' into 'master'

Download buildtrees on demand for bst shell --use-buildtree

Closes #829

See merge request BuildStream/buildstream!1050

199d01c2

by Angelos Evripiotis at 2019-01-28T16:06:26Z

BREAK:remove unconditional 'are you sure?' prompts

This is a breaking change, as it affects behaviour that people might be
relying on. An entry has been added to NEWS.

As proposed on the mailing list, this change removes the unconditional
prompts on:

    o: bst workspace reset
    o: bst workspace close --remove-dir

If interactive, these commands would always interrupt you with a prompt
like this:

    This will remove all your changes, are you sure?

This seems like it may just save someone's work some time. It may also
condition folks to hit 'y' quickly without thinking.

This change also makes the non-interactive behaviour consistent with the
interactive behaviour in the default case. There is also the case of the
prompt configured by 'really-workspace-close-project-inaccessible',
which may be tackled in later work.

This change also removes the new config options to suppress those
prompts, and their associated news entry.

The relevant bit of the mailing list conversation is here:
https://mail.gnome.org/archives/buildstream-list/2018-December/msg00106.html

The issue to make interactive and non-interactive behaviour consistent
is here:
https://gitlab.com/BuildStream/buildstream/issues/744

Changes:

NEWS

@@ -50,6 +50,11 @@ buildstream 1.3.1
      an error message and a hint instead, to avoid bothering folks that just
      made a mistake.
 +  o BREAKING CHANGE: The unconditional 'Are you sure?' prompts have been
 +    removed. These would always ask you if you were sure when running
 +    'bst workspace close --remove-dir' or 'bst workspace reset'. They got in
 +    the way too often.
++
    o Failed builds are included in the cache as well.
      `bst checkout` will provide anything in `%{install-root}`.
      A build including cached fails will cause any dependant elements
@@ -87,12 +92,6 @@ buildstream 1.3.1
      instead of just a specially-formatted build-root with a `root` and `scratch`
      subdirectory.
 -  o The buildstream.conf file learned new
 -    'prompt.really-workspace-close-remove-dir' and
 -    'prompt.really-workspace-reset-hard' options. These allow users to suppress
 -    certain confirmation prompts, e.g. double-checking that the user meant to
 -    run the command as typed.
+-
    o Due to the element `build tree` being cached in the respective artifact their
      size in some cases has significantly increased. In *most* cases the build trees
      are not utilised when building targets, as such by default bst 'pull' & 'build'

buildstream/_context.py

@@ -121,18 +121,10 @@ class Context():
          # Whether or not to attempt to pull build trees globally
          self.pull_buildtrees = None
 -        # Boolean, whether we double-check with the user that they meant to
 -        # remove a workspace directory.
 -        self.prompt_workspace_close_remove_dir = None
+-
          # Boolean, whether we double-check with the user that they meant to
          # close the workspace when they're using it to access the project.
          self.prompt_workspace_close_project_inaccessible = None
 -        # Boolean, whether we double-check with the user that they meant to do
 -        # a hard reset of a workspace, potentially losing changes.
 -        self.prompt_workspace_reset_hard = None
+-
          # Whether elements must be rebuilt when their dependencies have changed
          self._strict_build_plan = None
@@ -260,16 +252,10 @@ class Context():
          prompt = _yaml.node_get(
              defaults, Mapping, 'prompt')
          _yaml.node_validate(prompt, [
 -            'really-workspace-close-remove-dir',
              'really-workspace-close-project-inaccessible',
 -            'really-workspace-reset-hard',
          ])
 -        self.prompt_workspace_close_remove_dir = _node_get_option_str(
 -            prompt, 'really-workspace-close-remove-dir', ['ask', 'yes']) == 'ask'
          self.prompt_workspace_close_project_inaccessible = _node_get_option_str(
              prompt, 'really-workspace-close-project-inaccessible', ['ask', 'yes']) == 'ask'
 -        self.prompt_workspace_reset_hard = _node_get_option_str(
 -            prompt, 'really-workspace-reset-hard', ['ask', 'yes']) == 'ask'
          # Load per-projects overrides
          self._project_overrides = _yaml.node_get(defaults, Mapping, 'projects', default_value={})

buildstream/_frontend/cli.py

@@ -526,7 +526,7 @@ def shell(app, element, sysroot, mount, isolate, build_, cli_buildtree, command)
      else:
          scope = Scope.RUN
 -    use_buildtree = False
 +    use_buildtree = None
      with app.initialized():
          if not element:
@@ -534,7 +534,8 @@ def shell(app, element, sysroot, mount, isolate, build_, cli_buildtree, command)
              if not element:
                  raise AppError('Missing argument "ELEMENT".')
 -        dependencies = app.stream.load_selection((element,), selection=PipelineSelection.NONE)
 +        dependencies = app.stream.load_selection((element,), selection=PipelineSelection.NONE,
 +                                                 use_artifact_config=True)
          element = dependencies[0]
          prompt = app.shell_prompt(element)
          mounts = [
@@ -543,20 +544,31 @@ def shell(app, element, sysroot, mount, isolate, build_, cli_buildtree, command)
+         ]
          cached = element._cached_buildtree()
 -        if cli_buildtree == "always":
 -            if cached:
 -                use_buildtree = True
 -            else:
 -                raise AppError("No buildtree is cached but the use buildtree option was specified")
 -        elif cli_buildtree == "never":
 -            pass
 -        elif cli_buildtree == "try":
 -            use_buildtree = cached
 +        if cli_buildtree in ("always", "try"):
 +            use_buildtree = cli_buildtree
 +            if not cached and use_buildtree == "always":
 +                click.echo("WARNING: buildtree is not cached locally, will attempt to pull from available remotes",
 +                           err=True)
          else:
 -            if app.interactive and cached:
 -                use_buildtree = bool(click.confirm('Do you want to use the cached buildtree?'))
 +            # If the value has defaulted to ask and in non interactive mode, don't consider the buildtree, this
 +            # being the default behaviour of the command
 +            if app.interactive and cli_buildtree == "ask":
 +                if cached and bool(click.confirm('Do you want to use the cached buildtree?')):
 +                    use_buildtree = "always"
 +                elif not cached:
 +                    try:
 +                        choice = click.prompt("Do you want to pull & use a cached buildtree?",
 +                                              type=click.Choice(['try', 'always', 'never']),
 +                                              err=True, show_choices=True)
 +                    except click.Abort:
 +                        click.echo('Aborting', err=True)
 +                        sys.exit(-1)
++
 +                    if choice != "never":
 +                        use_buildtree = choice
++
          if use_buildtree and not element._cached_success():
 -            click.echo("Warning: using a buildtree from a failed build.")
 +            click.echo("WARNING: using a buildtree from a failed build.", err=True)
          try:
              exitcode = app.stream.shell(element, scope, prompt,
@@ -829,11 +841,6 @@ def workspace_close(app, remove_dir, all_, elements):
          if nonexisting:
              raise AppError("Workspace does not exist", detail="\n".join(nonexisting))
 -        if app.interactive and remove_dir and app.context.prompt_workspace_close_remove_dir:
 -            if not click.confirm('This will remove all your changes, are you sure?'):
 -                click.echo('Aborting', err=True)
 -                sys.exit(-1)
+-
          for element_name in elements:
              app.stream.workspace_close(element_name, remove_dir=remove_dir)
@@ -867,11 +874,6 @@ def workspace_reset(app, soft, track_, all_, elements):
          if all_ and not app.stream.workspace_exists():
              raise AppError("No open workspaces to reset")
 -        if app.interactive and not soft and app.context.prompt_workspace_reset_hard:
 -            if not click.confirm('This will remove all your changes, are you sure?'):
 -                click.echo('Aborting', err=True)
 -                sys.exit(-1)
+-
          if all_:
              elements = tuple(element_name for element_name, _ in app.context.get_workspaces().list())

buildstream/_stream.py

@@ -101,19 +101,22 @@ class Stream():
      #    targets (list of str): Targets to pull
      #    selection (PipelineSelection): The selection mode for the specified targets
      #    except_targets (list of str): Specified targets to except from fetching
 +    #    use_artifact_config (bool): If artifact remote config should be loaded
+     #
      # Returns:
      #    (list of Element): The selected elements
      def load_selection(self, targets, *,
                         selection=PipelineSelection.NONE,
 -                       except_targets=()):
 +                       except_targets=(),
 +                       use_artifact_config=False):
          profile_start(Topics.LOAD_SELECTION, "_".join(t.replace(os.sep, '-') for t in targets))
          elements, _ = self._load(targets, (),
                                   selection=selection,
                                   except_targets=except_targets,
 -                                 fetch_subprojects=False)
 +                                 fetch_subprojects=False,
 +                                 use_artifact_config=use_artifact_config)
          profile_end(Topics.LOAD_SELECTION, "_".join(t.replace(os.sep, '-') for t in targets))
@@ -131,7 +134,7 @@ class Stream():
      #    mounts (list of HostMount): Additional directories to mount into the sandbox
      #    isolate (bool): Whether to isolate the environment like we do in builds
      #    command (list): An argv to launch in the sandbox, or None
 -    #    usebuildtree (bool): Wheather to use a buildtree as the source.
 +    #    usebuildtree (str): Whether to use a buildtree as the source, given cli option
+     #
      # Returns:
      #    (int): The exit code of the launched shell
@@ -141,7 +144,7 @@ class Stream():
                mounts=None,
                isolate=False,
                command=None,
 -              usebuildtree=False):
 +              usebuildtree=None):
          # Assert we have everything we need built, unless the directory is specified
          # in which case we just blindly trust the directory, using the element
@@ -156,8 +159,31 @@ class Stream():
                  raise StreamError("Elements need to be built or downloaded before staging a shell environment",
                                    detail="\n".join(missing_deps))
 +        buildtree = False
 +        # Check if we require a pull queue attempt, with given artifact state and context
 +        if usebuildtree:
 +            if not element._cached_buildtree():
 +                require_buildtree = self._buildtree_pull_required([element])
 +                # Attempt a pull queue for the given element if remote and context allow it
 +                if require_buildtree:
 +                    self._message(MessageType.INFO, "Attempting to fetch missing artifact buildtree")
 +                    self._add_queue(PullQueue(self._scheduler))
 +                    self._enqueue_plan(require_buildtree)
 +                    self._run()
 +                    # Now check if the buildtree was successfully fetched
 +                    if element._cached_buildtree():
 +                        buildtree = True
 +                if not buildtree:
 +                    if usebuildtree == "always":
 +                        raise StreamError("Buildtree is not cached locally or in available remotes")
 +                    else:
 +                        self._message(MessageType.INFO, """Buildtree is not cached locally or in available remotes,
 +                                                        shell will be loaded without it""")
 +            else:
 +                buildtree = True
++
          return element._shell(scope, directory, mounts=mounts, isolate=isolate, prompt=prompt, command=command,
 -                              usebuildtree=usebuildtree)
 +                              usebuildtree=buildtree)
      # build()
+     #

buildstream/data/userconfig.yaml

@@ -112,14 +112,6 @@ logging:
+ #
  prompt:
 -  # Whether to really proceed with 'bst workspace close --remove-dir' removing
 -  # a workspace directory, potentially losing changes.
 -  #
 -  #  ask - Ask the user if they are sure.
 -  #  yes - Always remove, without asking.
 -  #
 -  really-workspace-close-remove-dir: ask
+-
    # Whether to really proceed with 'bst workspace close' when doing so would
    # stop them from running bst commands in this workspace.
+   #
@@ -127,11 +119,3 @@ prompt:
    #  yes - Always close, without asking.
+   #
    really-workspace-close-project-inaccessible: ask
+-
 -  # Whether to really proceed with 'bst workspace reset' doing a hard reset of
 -  # a workspace, potentially losing changes.
 -  #
 -  #  ask - Ask the user if they are sure.
 -  #  yes - Always hard reset, without asking.
 -  #
 -  really-workspace-reset-hard: ask

tests/integration/build-tree.py

@@ -101,7 +101,7 @@ def test_buildtree_from_failure(cli_integration, tmpdir, datafiles):
          'shell', '--build', element_name, '--use-buildtree', 'always', '--', 'cat', 'test'
      ])
      res.assert_success()
 -    assert "Warning: using a buildtree from a failed build" in res.output
 +    assert "WARNING: using a buildtree from a failed build" in res.stderr
      assert 'Hi' in res.output
@@ -141,7 +141,7 @@ def test_buildtree_pulled(cli, tmpdir, datafiles):
          res.assert_success()
 -# This test checks for correct behaviour if a buildtree is not present.
 +# This test checks for correct behaviour if a buildtree is not present in the local cache.
  @pytest.mark.datafiles(DATA_DIR)
  @pytest.mark.skipif(IS_LINUX and not HAVE_BWRAP, reason='Only available with bubblewrap on Linux')
  def test_buildtree_options(cli, tmpdir, datafiles):
@@ -156,6 +156,7 @@ def test_buildtree_options(cli, tmpdir, datafiles):
          result = cli.run(project=project, args=['build', element_name])
          result.assert_success()
          assert cli.get_element_state(project, element_name) == 'cached'
 +        assert share.has_artifact('test', element_name, cli.get_element_key(project, element_name))
          # Discard the cache
          cli.configure({
@@ -168,8 +169,6 @@ def test_buildtree_options(cli, tmpdir, datafiles):
          result = cli.run(project=project, args=['artifact', 'pull', '--deps', 'all', element_name])
          result.assert_success()
 -        # The above is the simplest way I know to create a local cache without any buildtrees.
+-
          # Check it's not using the cached build tree
          res = cli.run(project=project, args=[
              'shell', '--build', element_name, '--use-buildtree', 'never', '--', 'cat', 'test'
@@ -177,24 +176,51 @@ def test_buildtree_options(cli, tmpdir, datafiles):
          res.assert_shell_error()
          assert 'Hi' not in res.output
 -        # Check it's not correctly handling the lack of buildtree
 +        # Check it's not using the cached build tree, default is to ask, and fall back to not
 +        # for non interactive behavior
          res = cli.run(project=project, args=[
 -            'shell', '--build', element_name, '--use-buildtree', 'try', '--', 'cat', 'test'
 +            'shell', '--build', element_name, '--', 'cat', 'test'
          ])
          res.assert_shell_error()
          assert 'Hi' not in res.output
 -        # Check it's not using the cached build tree, default is to ask, and fall back to not
 -        # for non interactive behavior
 +        # Check correctly handling the lack of buildtree, with 'try' not attempting to
 +        # pull the buildtree as the user context is by default set to not pull them
          res = cli.run(project=project, args=[
 -            'shell', '--build', element_name, '--', 'cat', 'test'
 +            'shell', '--build', element_name, '--use-buildtree', 'try', '--', 'cat', 'test'
          ])
 -        res.assert_shell_error()
          assert 'Hi' not in res.output
 +        assert 'Attempting to fetch missing artifact buildtrees' not in res.stderr
 +        assert """Buildtree is not cached locally or in available remotes,
 +                shell will be loaded without it"""
 -        # Check it's using the cached build tree
 +        # Check correctly handling the lack of buildtree, with 'try' attempting and succeeding
 +        # to pull the buildtree as the user context allow the pulling of buildtrees and it is
 +        # available in the remote
 +        res = cli.run(project=project, args=[
 +            '--pull-buildtrees', 'shell', '--build', element_name, '--use-buildtree', 'try', '--', 'cat', 'test'
 +        ])
 +        assert 'Attempting to fetch missing artifact buildtree' in res.stderr
 +        assert 'Hi' in res.output
 +        shutil.rmtree(os.path.join(os.path.join(cli.directory, 'artifacts2')))
 +        assert cli.get_element_state(project, element_name) != 'cached'
++
 +        # Check it's not loading the shell at all with always set for the buildtree, when the
 +        # user context does not allow for buildtree pulling
 +        result = cli.run(project=project, args=['artifact', 'pull', '--deps', 'all', element_name])
 +        result.assert_success()
          res = cli.run(project=project, args=[
              'shell', '--build', element_name, '--use-buildtree', 'always', '--', 'cat', 'test'
          ])
          res.assert_main_error(ErrorDomain.PROG_NOT_FOUND, None)
 +        assert 'Buildtree is not cached locally or in available remotes' in res.stderr
          assert 'Hi' not in res.output
 +        assert 'Attempting to fetch missing artifact buildtree' not in res.stderr
++
 +        # Check that when user context is set to pull buildtrees and a remote has the buildtree,
 +        # 'always' will attempt and succeed at pulling the missing buildtree.
 +        res = cli.run(project=project, args=[
 +            '--pull-buildtrees', 'shell', '--build', element_name, '--use-buildtree', 'always', '--', 'cat', 'test'
 +        ])
 +        assert 'Hi' in res.output
 +        assert 'Attempting to fetch missing artifact buildtree' in res.stderr

[Notes] [Git][BuildStream/buildstream][aevri/are_you_sure] 4 commits: _stream.py: Add use_artifact_config opt arg for load_selection()

Angelos Evripiotis pushed to branch aevri/are_you_sure at BuildStream / buildstream

Commits:

6 changed files:

Changes: