commit 9b3ed1d5e912b8e9195060c3401da9b5370f2fce
author Robbie Iannucci <iannucci@chromium.org> Mon May 13 20:40:38 2019
committer Commit Bot <commit-bot@chromium.org> Mon May 13 20:40:38 2019
tree 6aa9a88acfcefaa397b3e6126b8b5cdb919224dd
parent 440a45cae6b329192f60313e41714bd78e886c8d

Revert "Refactor recipe engine step execution."

This reverts commit 440a45cae6b329192f60313e41714bd78e886c8d.

Reason for revert: Resolving cmd0 fails for commands provided with relative paths (e.g. './foo'): https://skia-review.googlesource.com/c/buildbot/+/213520

Original change's description:
> Refactor recipe engine step execution.
> 
> This CL is unfortunately massive, but because the logic was previously
> tangled across the whole codebase it was difficult to make this refactor
> smaller.
> 
> 1. The StepRunner API is vastly simplified.
> 
> StepRunner API has been improved to have a small collection of well-defined
> methods, replacing the previous API which required implementations to return
> active 'OpenStep' implementations and implement context handlers.
> 
> None of these new methods are allowed to raise an exception, meaning that
> the engine can operate purely in terms of their stated API. If an
> implementation is naughty and raises an exception anyway, it will be logged
> to the UI (as a RECIPE CRASH step) and then the engine will abort.
> 
> 
> 2. The StreamEngine is now solely owned by the Engine.
> 
> The StreamEngine is now owned entirely by the RecipeEngine. The
> StepRunner is purely concerned with running steps, not with
> manipulating or reporting anything to the UI.
> 
> 
> 3. All recipe running logic is now implemented in the Engine.
> 
> The engine now handles all calculation of step exceptions. Previously
> the raising of exceptions was handled by various parties in various
> ways around the engine and StepRunner, leading to bugs like crbug.com/931473.
> 
> 
> 4. Unexpected errors are now reported and handled much better.
> 
> All steps now have a '$debug' log which records the engine's internal
> thoughts about how it's processing the step. Now if anything goes wrong
> while attempting to run the step, it will be recorded here. This includes
> placeholders raising exceptions, bugs in the engine, failure to locate
> the executable to run, failure to CD into the target directory of the
> step, etc.
> 
> 
> 5. Recipe engine logs are no longer mixed into stdout of steps.
> 
> Previously information about the step would be dumped into the step's
> stdout log (e.g. the command we're running, the working directory,
> environment tweaks, the final return code of the step, etc.). Now all
> of this data is reported to an 'execution details' log on every step.
> 
> 
> 6. Timeouts are now handled orthogonally to step status.
> 
> Previously timeouts would raise a StepTimeout and (I think? It's
> hard to tell) would turn the step red regardless of if it was an
> infra step or not. Now timeouts are recorded alongside the failure,
> and `infra_step` correctly controls the default color and exception.
> 
> 
> 7. Parent steps are now explicitly handled vs. non-parent steps
> 
> Previously 'parent steps' were handled by the engine as-if they were
> normal steps without a command. However their semantics are very
> different since they 'remain open' even while other steps run (i.e.
> the steps inside of them). The engine now explicitly handles parent
> steps separately from leaf steps.
> 
> This also means that parent nesting steps now have much clearer
> defined behavior in terms of what color they'll turn under different
> circumstances.
> 
> 
> 8. Test runner is much improved.
> 
> Simulation tests now exercise a much greater portion of the recipe
> engine logic. Test expectations now include (normalized) stack traces
> when the recipe has exception(s), and (non-dropped) test expectations
> are always written to disk, both of which make debugging failing
> recipe simulations much easier.
> 
> 
> 9. Placeholder data on StepData will always be populated with None
> as a default, even if one of the placeholders crashes.
> 
> This can help avoid secondary errors where one placeholder crashes
> and the second placeholder's collection step isn't able to run.
> Previously placeholder data was populated in-order for placeholders
> and if a placeholder didn't run there was nothing added to StepData
> for that value (leading to AttributeError's in unrelated areas of
> the recipe).
> 
> 
> Bug: 931473, 910369
> Recipe-Manual-Change: build
> Recipe-Manual-Change: release_scripts
> Recipe-Nontrivial-Roll: build_limited_scripts_slave
> Recipe-Nontrivial-Roll: chromiumos
> Recipe-Nontrivial-Roll: depot_tools
> Recipe-Nontrivial-Roll: infra
> Recipe-Nontrivial-Roll: skia
> Change-Id: Ie85704e30ff20eb7cc43c6a814a6b4f6ad2eae65
> Reviewed-on: https://chromium-review.googlesource.com/c/infra/luci/recipes-py/+/1534344
> Reviewed-by: Andrii Shyshkalov <tandrii@chromium.org>
> Commit-Queue: Robbie Iannucci <iannucci@chromium.org>

TBR=iannucci@chromium.org,nodir@chromium.org,tandrii@chromium.org,martiniss@chromium.org

Change-Id: Ic6b7f2c2ecb2d9277f25b7ec14c56c1a9d805b68
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 931473, 910369
Reviewed-on: https://chromium-review.googlesource.com/c/infra/luci/recipes-py/+/1610702
Reviewed-by: Robbie Iannucci <iannucci@chromium.org>
Commit-Queue: Robbie Iannucci <iannucci@chromium.org>