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 <[email protected]> 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]
