You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@buildstream.apache.org by tv...@apache.org on 2021/10/05 06:51:23 UTC
[buildstream] 04/05: Remove PipelineSelection.PLAN and any related
frontend options
This is an automated email from the ASF dual-hosted git repository.
tvb pushed a commit to branch tristan/remove-plan-selection-mode
in repository https://gitbox.apache.org/repos/asf/buildstream.git
commit 88cf7bc863c75d29cbc34760aa9ecfb60b8a253d
Author: Tristan van Berkom <tr...@codethink.co.uk>
AuthorDate: Tue Oct 5 15:34:05 2021 +0900
Remove PipelineSelection.PLAN and any related frontend options
This removes the `--deps plan` option from all commands, including
the internal selection mode.
Summary of changes:
* _context.py: Allow `none`, `build` or `all` modes in the default
build mode in user configuration.
* _frontend/cli.py: Remove all `plan` possibilities from all commands
- The build command now accepts `none` as the default, which means
to target *only these elements* to be built, implicitly building
dependencies as needed
- The `show` command drops the `plan` mode
- The `source fetch` command now takes `none` as default, which
should be more consistent with all other existing commands than
changing the default to `all` (which would more closely match
the previous behavior of `plan`).
- The `source push` command now takes `none` as default
* _pipeline.py: The `get_selection()` function now takes a new
`depth_sort` keyword argument, which, if specified performs
the depth sorting and calls `Element._set_depth()` as is required
for prioritizing in scheduling.
* _stream.py: Handle the requiring of elements differently, since we
now always recieve the entire dependency graph when loading a
"dynamic_plan" (which is to say elements will be "required" as needed).
* types.py: Remove _PipelineSelection.PLAN
* data/userconfig.yaml: Use `none` as the default behavior for `bst build`
deps (i.e. "only the elements specified")
* tests: Updated tests to stop using `--deps plan`
This is a part of addressing #1349
---
src/buildstream/_context.py | 4 +--
src/buildstream/_frontend/cli.py | 47 ++++++++++++------------------------
src/buildstream/_pipeline.py | 46 ++++++++++++++++++++++++-----------
src/buildstream/_stream.py | 40 ++++++++++++++++++------------
src/buildstream/data/userconfig.yaml | 2 +-
src/buildstream/testing/runcli.py | 2 +-
src/buildstream/types.py | 3 ---
tests/frontend/completions.py | 4 +--
tests/frontend/order.py | 20 +++++++++++----
tests/frontend/show.py | 4 +--
10 files changed, 95 insertions(+), 77 deletions(-)
diff --git a/src/buildstream/_context.py b/src/buildstream/_context.py
index feec102..d7d5e0e 100644
--- a/src/buildstream/_context.py
+++ b/src/buildstream/_context.py
@@ -423,10 +423,10 @@ class Context:
self.build_max_jobs = build.get_int("max-jobs")
dependencies = build.get_str("dependencies")
- if dependencies not in ["plan", "all"]:
+ if dependencies not in ["none", "build", "all"]:
provenance = build.get_scalar("dependencies").get_provenance()
raise LoadError(
- "{}: Invalid value for 'dependencies'. Choose 'plan' or 'all'.".format(provenance),
+ "{}: Invalid value for 'dependencies'. Choose 'none', 'build' or 'all'.".format(provenance),
LoadErrorReason.INVALID_DATA,
)
self.build_dependencies = _PipelineSelection(dependencies)
diff --git a/src/buildstream/_frontend/cli.py b/src/buildstream/_frontend/cli.py
index 0753215..4c3df63 100644
--- a/src/buildstream/_frontend/cli.py
+++ b/src/buildstream/_frontend/cli.py
@@ -427,7 +427,7 @@ def init(app, project_name, min_version, element_path, force, target_directory):
"-d",
default=None,
type=FastEnumType(
- _PipelineSelection, [_PipelineSelection.BUILD, _PipelineSelection.PLAN, _PipelineSelection.ALL],
+ _PipelineSelection, [_PipelineSelection.NONE, _PipelineSelection.BUILD, _PipelineSelection.ALL],
),
help="The dependencies to build",
)
@@ -473,12 +473,15 @@ def build(
When this command is executed from a workspace directory, the default
is to build the workspace element.
- Specify `--deps` to control which dependencies to build:
+ Specify `--deps` to control which dependencies must be built:
\b
- plan: Only dependencies required for the build plan
+ none: No dependencies, just the element itself
build: Build time dependencies, excluding the element itself
all: All dependencies
+
+ Dependencies that are consequently required to build the requested
+ elements will be built on demand.
"""
with app.initialized(session_name="Build"):
ignore_junction_targets = False
@@ -516,13 +519,7 @@ def build(
show_default=True,
type=FastEnumType(
_PipelineSelection,
- [
- _PipelineSelection.NONE,
- _PipelineSelection.PLAN,
- _PipelineSelection.RUN,
- _PipelineSelection.BUILD,
- _PipelineSelection.ALL,
- ],
+ [_PipelineSelection.NONE, _PipelineSelection.RUN, _PipelineSelection.BUILD, _PipelineSelection.ALL,],
),
help="The dependencies to show",
)
@@ -561,7 +558,6 @@ def show(app, elements, deps, except_, order, format_):
\b
none: No dependencies, just the element itself
- plan: Dependencies required for a build plan
run: Runtime dependencies, including the element itself
build: Build time dependencies, excluding the element itself
all: All dependencies
@@ -762,17 +758,11 @@ def source():
@click.option(
"--deps",
"-d",
- default=_PipelineSelection.PLAN,
+ default=_PipelineSelection.NONE,
show_default=True,
type=FastEnumType(
_PipelineSelection,
- [
- _PipelineSelection.PLAN,
- _PipelineSelection.NONE,
- _PipelineSelection.BUILD,
- _PipelineSelection.RUN,
- _PipelineSelection.ALL,
- ],
+ [_PipelineSelection.NONE, _PipelineSelection.BUILD, _PipelineSelection.RUN, _PipelineSelection.ALL,],
),
help="The dependencies to fetch",
)
@@ -798,16 +788,13 @@ def source_fetch(app, elements, deps, except_, source_remotes, ignore_project_so
When this command is executed from a workspace directory, the default
is to fetch the workspace element.
- By default this will only try to fetch sources which are
- required for the build plan of the specified target element,
- omitting sources for any elements which are already built
- and available in the artifact cache.
+ By default this will only try to fetch sources for the specified
+ elements.
Specify `--deps` to control which sources to fetch:
\b
none: No dependencies, just the element itself
- plan: Only dependencies required for the build plan
run: Runtime dependencies, including the element itself
build: Build time dependencies, excluding the element itself
all: All dependencies
@@ -843,13 +830,7 @@ def source_fetch(app, elements, deps, except_, source_remotes, ignore_project_so
show_default=True,
type=FastEnumType(
_PipelineSelection,
- [
- _PipelineSelection.NONE,
- _PipelineSelection.PLAN,
- _PipelineSelection.BUILD,
- _PipelineSelection.RUN,
- _PipelineSelection.ALL,
- ],
+ [_PipelineSelection.NONE, _PipelineSelection.BUILD, _PipelineSelection.RUN, _PipelineSelection.ALL,],
),
help="The dependencies to push",
)
@@ -875,11 +856,13 @@ def source_push(app, elements, deps, except_, source_remotes, ignore_project_sou
When this command is executed from a workspace directory, the default
is to push the sources of the workspace element.
+ By default this will only try to push sources for the specified
+ elements.
+
Specify `--deps` to control which sources to fetch:
\b
none: No dependencies, just the element itself
- plan: Only dependencies required for the build plan
run: Runtime dependencies, including the element itself
build: Build time dependencies, excluding the element itself
all: All dependencies
diff --git a/src/buildstream/_pipeline.py b/src/buildstream/_pipeline.py
index 802e52b..4a18c84 100644
--- a/src/buildstream/_pipeline.py
+++ b/src/buildstream/_pipeline.py
@@ -71,11 +71,14 @@ def dependencies(targets: List[Element], scope: int, *, recurse: bool = True) ->
# targets: The target Elements
# mode: A value from PipelineSelection enumeration
# silent: Whether to silence messages
+# depth_sort: Whether to sort the elements by depth (for an optimal build plan)
#
# Returns:
# A list of Elements appropriate for the specified selection mode
#
-def get_selection(context: Context, targets: List[Element], mode: str, *, silent: bool = True) -> List[Element]:
+def get_selection(
+ context: Context, targets: List[Element], mode: str, *, silent: bool = True, depth_sort: bool = False
+) -> List[Element]:
def redirect_and_log() -> List[Element]:
# Redirect and log if permitted
elements: List[Element] = []
@@ -87,22 +90,37 @@ def get_selection(context: Context, targets: List[Element], mode: str, *, silent
elements.append(new_elm)
return elements
- def plan() -> List[Element]:
+ def plan_all() -> List[Element]:
return _Planner().plan(targets)
- # Work around python not having a switch statement; this is
- # much clearer than the if/elif/else block we used to have.
- #
- # Note that the lambda is necessary so that we don't evaluate
- # all possible values at run time; that would be slow.
- return {
- _PipelineSelection.NONE: lambda: targets,
+ def plan_build() -> List[Element]:
+ build_targets = list(dependencies(targets, _Scope.BUILD, recurse=False))
+ return _Planner().plan(build_targets)
+
+ selection_table = {
_PipelineSelection.REDIRECT: redirect_and_log,
- _PipelineSelection.PLAN: plan,
- _PipelineSelection.ALL: lambda: list(dependencies(targets, _Scope.ALL)),
- _PipelineSelection.BUILD: lambda: list(dependencies(targets, _Scope.BUILD)),
- _PipelineSelection.RUN: lambda: list(dependencies(targets, _Scope.RUN)),
- }[mode]()
+ }
+ if depth_sort:
+ #
+ # Depth sorting is used with `bst build` and assumes a dynamic build planning
+ # mode (Stream() will only mark the toplevel elements as "required", and all
+ # elements will be built on demand).
+ #
+ # In this case, the `none` and `run` selection modes can potentially include
+ # dependencies, and which ones will be dynamically resolved at build time, so
+ # it is essentially equivalent to the `all` selection.
+ #
+ selection_table[_PipelineSelection.NONE] = plan_all
+ selection_table[_PipelineSelection.ALL] = plan_all
+ selection_table[_PipelineSelection.RUN] = plan_all
+ selection_table[_PipelineSelection.BUILD] = plan_build
+ else:
+ selection_table[_PipelineSelection.NONE] = lambda: targets
+ selection_table[_PipelineSelection.ALL] = lambda: list(dependencies(targets, _Scope.ALL))
+ selection_table[_PipelineSelection.RUN] = lambda: list(dependencies(targets, _Scope.RUN))
+ selection_table[_PipelineSelection.BUILD] = lambda: list(dependencies(targets, _Scope.BUILD))
+
+ return selection_table[mode]()
# except_elements():
diff --git a/src/buildstream/_stream.py b/src/buildstream/_stream.py
index fac843e..1b05ae8 100644
--- a/src/buildstream/_stream.py
+++ b/src/buildstream/_stream.py
@@ -366,7 +366,7 @@ class Stream:
self,
targets: Iterable[str],
*,
- selection: str = _PipelineSelection.PLAN,
+ selection: str = _PipelineSelection.NONE,
ignore_junction_targets: bool = False,
artifact_remotes: Iterable[RemoteSpec] = (),
source_remotes: Iterable[RemoteSpec] = (),
@@ -432,7 +432,7 @@ class Stream:
self,
targets: Iterable[str],
*,
- selection: str = _PipelineSelection.PLAN,
+ selection: str = _PipelineSelection.NONE,
except_targets: Iterable[str] = (),
source_remotes: Iterable[RemoteSpec] = (),
ignore_project_source_remotes: bool = False,
@@ -1471,9 +1471,6 @@ class Stream:
# (list of Element): The tracking element selection
#
def _load_tracking(self, targets, *, selection=_PipelineSelection.NONE, except_targets=(), cross_junctions=False):
- # We never want to use a PLAN selection when tracking elements
- assert selection != _PipelineSelection.PLAN
-
elements, except_elements, artifacts = self._load_elements_from_targets(
targets, except_targets, rewritable=True
)
@@ -1663,18 +1660,31 @@ class Stream:
# Now move on to loading primary selection.
#
- selected = _pipeline.get_selection(self._context, self.targets, selection, silent=False)
+ selected = _pipeline.get_selection(
+ self._context, self.targets, selection, silent=False, depth_sort=dynamic_plan
+ )
selected = _pipeline.except_elements(self.targets, selected, except_elements)
- if selection == _PipelineSelection.PLAN and dynamic_plan:
- # We use a dynamic build plan, only request artifacts of top-level targets,
- # others are requested dynamically as needed.
- # This avoids pulling, fetching, or building unneeded build-only dependencies.
- for element in elements:
- element._set_required()
- else:
- for element in selected:
- element._set_required()
+ # Mark the appropriate required elements
+ #
+ required_elements = []
+ if dynamic_plan:
+ #
+ # In a dynamic build plan, we only require the top-level targets and
+ # rely on state changes during processing to determine which elements
+ # must be processed.
+ #
+ if selection == _PipelineSelection.NONE:
+ required_elements = elements
+ elif selection == _PipelineSelection.BUILD:
+ required_elements = list(_pipeline.dependencies(elements, _Scope.BUILD, recurse=False))
+
+ # Without a dynamic build plan, or if `all` selection was made, then everything is required
+ if not required_elements:
+ required_elements = selected
+
+ for element in required_elements:
+ element._set_required()
return selected
diff --git a/src/buildstream/data/userconfig.yaml b/src/buildstream/data/userconfig.yaml
index 32472ef..3232de3 100644
--- a/src/buildstream/data/userconfig.yaml
+++ b/src/buildstream/data/userconfig.yaml
@@ -68,7 +68,7 @@ build:
#
# Control which dependencies to build
#
- dependencies: plan
+ dependencies: none
#
diff --git a/src/buildstream/testing/runcli.py b/src/buildstream/testing/runcli.py
index 7c29174..088c2a8 100644
--- a/src/buildstream/testing/runcli.py
+++ b/src/buildstream/testing/runcli.py
@@ -463,7 +463,7 @@ class Cli:
# Fetch the elements that would be in the pipeline with the given
# arguments.
#
- def get_pipeline(self, project, elements, except_=None, scope="plan"):
+ def get_pipeline(self, project, elements, except_=None, scope="all"):
if except_ is None:
except_ = []
diff --git a/src/buildstream/types.py b/src/buildstream/types.py
index b01ecab..ac3b180 100644
--- a/src/buildstream/types.py
+++ b/src/buildstream/types.py
@@ -281,9 +281,6 @@ class _PipelineSelection(FastEnum):
# As NONE, but redirect elements that are capable of it
REDIRECT = "redirect"
- # Select elements which must be built for the associated targets to be built
- PLAN = "plan"
-
# All dependencies of all targets, including the targets
ALL = "all"
diff --git a/tests/frontend/completions.py b/tests/frontend/completions.py
index 0632af0..f64a087 100644
--- a/tests/frontend/completions.py
+++ b/tests/frontend/completions.py
@@ -156,8 +156,8 @@ def test_options(cli, cmd, word_idx, expected):
[
("bst --on-error ", 2, ["continue ", "quit ", "terminate "]),
("bst --cache-buildtrees ", 2, ["always ", "auto ", "never "]),
- ("bst show --deps ", 3, ["all ", "build ", "none ", "plan ", "run "]),
- ("bst show --deps=", 2, ["all ", "build ", "none ", "plan ", "run "]),
+ ("bst show --deps ", 3, ["all ", "build ", "none ", "run "]),
+ ("bst show --deps=", 2, ["all ", "build ", "none ", "run "]),
("bst show --deps b", 3, ["build "]),
("bst show --deps=b", 2, ["build "]),
("bst show --deps r", 3, ["run "]),
diff --git a/tests/frontend/order.py b/tests/frontend/order.py
index fbeb7c3..b57f373 100644
--- a/tests/frontend/order.py
+++ b/tests/frontend/order.py
@@ -48,13 +48,14 @@ def create_element(project, name, dependencies):
#
@pytest.mark.datafiles(os.path.join(DATA_DIR))
@pytest.mark.parametrize(
- "target,template,expected",
+ "target,template,expected_stage_order,expected_build_order",
[
# First simple test
(
"3.bst",
{"0.bst": ["1.bst"], "1.bst": [], "2.bst": ["0.bst"], "3.bst": ["0.bst", "1.bst", "2.bst"]},
["1.bst", "0.bst", "2.bst", "3.bst"],
+ ["1.bst", "0.bst", "2.bst", "3.bst"],
),
# A more complicated test with build of build dependencies
(
@@ -67,12 +68,14 @@ def create_element(project, name, dependencies):
"app.bst": [{"filename": "middleware.bst", "type": "build"}],
"target.bst": ["a.bst", "base.bst", "middleware.bst", "app.bst", "timezones.bst"],
},
+ ["a.bst", "base.bst", "middleware.bst", "app.bst", "timezones.bst", "target.bst"],
["base.bst", "middleware.bst", "a.bst", "app.bst", "timezones.bst", "target.bst"],
),
],
+ ids=["simple", "complex"],
)
@pytest.mark.parametrize("operation", [("show"), ("fetch"), ("build")])
-def test_order(cli, datafiles, operation, target, template, expected):
+def test_order(cli, datafiles, operation, target, template, expected_stage_order, expected_build_order):
project = str(datafiles)
# Configure to only allow one fetcher at a time, make it easy to
@@ -87,17 +90,24 @@ def test_order(cli, datafiles, operation, target, template, expected):
# Run test and collect results
if operation == "show":
- result = cli.run(args=["show", "--deps", "plan", "--format", "%{name}", target], project=project, silent=True)
+ result = cli.run(args=["show", "--deps", "all", "--format", "%{name}", target], project=project, silent=True)
result.assert_success()
results = result.output.splitlines()
else:
if operation == "fetch":
- result = cli.run(args=["source", "fetch", target], project=project, silent=True)
+ result = cli.run(args=["source", "fetch", "--deps", "all", target], project=project, silent=True)
else:
- result = cli.run(args=[operation, target], project=project, silent=True)
+ result = cli.run(args=["build", target], project=project, silent=True)
result.assert_success()
results = result.get_start_order(operation)
+ # When running `bst build`, the elements are depth sorted for optimal processing
+ if operation == "build":
+ expected = expected_build_order
+ else:
+ # Otherwise, we get the usual deterministic staging order
+ expected = expected_stage_order
+
# Assert the order
print("Expected order: {}".format(expected))
print("Observed result order: {}".format(results))
diff --git a/tests/frontend/show.py b/tests/frontend/show.py
index 3d74bf6..456123b 100644
--- a/tests/frontend/show.py
+++ b/tests/frontend/show.py
@@ -217,12 +217,12 @@ def test_target_is_dependency(cli, datafiles):
project = str(datafiles)
elements = ["multiple_targets/dependency/zebry.bst", "multiple_targets/dependency/horsey.bst"]
- args = ["show", "-d", "plan", "-f", "%{name}", *elements]
+ args = ["show", "-d", "all", "-f", "%{name}", *elements]
result = cli.run(project=project, args=args)
result.assert_success()
- # Get the planned order
+ # Get the order
names = result.output.splitlines()
names = [name[len("multiple_targets/dependency/") :] for name in names]