Improve RoundEngine lifecycle.

Review Request #1665 - Created Jan. 25, 2015 and submitted

Information
John Sirois
pants
jsirois/engine/lifecycle_improvements
992
6a0636e...
Reviewers
pants-reviews
benjyw, davidt, fkorotkov, ity, patricklaw

This change lifts all engine planning methods needed to prepare the task
graph for a run to be task classmethods. In addtion it introduces a new
alternate_target_roots lifecycle method to support more disciplined
replacement of the active target set. As part of this move,
Context.replace_targets is made ~private to discourage its direct use.

The task graph preparation lifecycle is now the following sequence of
task classmethods:
1. register_options - declare options configurable via cmd-line flag
or config file.
2. product_types - declare the product types your task is capable of
producing.
3. alternate_target_roots - propose a different set of target roots to
use than those specified via the CLI for the active pants run.
4. prepare - request any products needed from other tasks.

The task execution lifecycle is now just construct and then execute,
back to back.

Note that this fix is just a temporary aid in the transition to the tuple
engine.

Fixes along the way:
+ Fixup all Tasks to use the new lifecycle.
+ Fixup tests using Context.replace_targets when they need not have been.
+ Re-work ChangedFileTaskMixin for use with alternate_target_roots in the changed goals.
+ Kill broken and unused BuildLint task.

Several more fixes pointed at by broken tests:
+ Have Depmap use prepare to create a conditional dependency on resolve
and cleanup DepmapIntegrationTest.
+ Fix ProtobufIntegrationTest.test_source_ordering to ensure its
assumption of a fresh gen, also fixup tests for "output contains".
+ Fix WireIntegrationTest.test_good to ensure its assumption of a fresh
gen, also fixup tests for "output contains".

src/python/pants/backend/android/tasks/aapt_builder.py | 11 ++--
src/python/pants/backend/android/tasks/aapt_gen.py | 20 ++++----
src/python/pants/backend/android/tasks/dx_compile.py | 21 ++++----
src/python/pants/backend/android/tasks/jarsigner_task.py | 11 ++--
src/python/pants/backend/codegen/tasks/code_gen.py | 7 +--
src/python/pants/backend/codegen/tasks/protobuf_gen.py | 21 +++++---
src/python/pants/backend/codegen/tasks/thrift_linter.py | 15 +++---
src/python/pants/backend/core/register.py | 3 --
src/python/pants/backend/core/tasks/BUILD | 12 -----
src/python/pants/backend/core/tasks/build_lint.py | 116 ------------------------------------------
src/python/pants/backend/core/tasks/changed_target_goals.py | 28 +++++++----
src/python/pants/backend/core/tasks/confluence_publish.py | 7 +--
src/python/pants/backend/core/tasks/deferred_sources_mapper.py | 11 ++--
src/python/pants/backend/core/tasks/group_task.py | 26 ++++++----
src/python/pants/backend/core/tasks/prepare_resources.py | 14 +++---
src/python/pants/backend/core/tasks/task.py | 76 ++++++++++++++++++++--------
src/python/pants/backend/core/tasks/what_changed.py | 137 +++++++++++++++++++++++++++++++++-----------------
src/python/pants/backend/jvm/tasks/benchmark_run.py | 18 +++----
src/python/pants/backend/jvm/tasks/checkstyle.py | 11 ++--
src/python/pants/backend/jvm/tasks/depmap.py | 9 ++--
src/python/pants/backend/jvm/tasks/detect_duplicates.py | 9 ++--
src/python/pants/backend/jvm/tasks/ide_gen.py | 43 ++++++++--------
src/python/pants/backend/jvm/tasks/ivy_imports.py | 9 ++--
src/python/pants/backend/jvm/tasks/ivy_resolve.py | 28 +++++------
src/python/pants/backend/jvm/tasks/jar_publish.py | 11 ++--
src/python/pants/backend/jvm/tasks/jar_task.py | 9 ++--
src/python/pants/backend/jvm/tasks/junit_run.py | 17 ++++---
src/python/pants/backend/jvm/tasks/jvm_binary_task.py | 9 ++--
src/python/pants/backend/jvm/tasks/jvm_compile/jvm_compile.py | 38 +++++++-------
src/python/pants/backend/jvm/tasks/jvm_run.py | 15 +++---
src/python/pants/backend/jvm/tasks/jvm_task.py | 7 +--
src/python/pants/backend/jvm/tasks/jvmdoc_gen.py | 15 +++---
src/python/pants/backend/jvm/tasks/provides.py | 9 ++--
src/python/pants/backend/jvm/tasks/scala_repl.py | 3 +-
src/python/pants/backend/jvm/tasks/specs_run.py | 17 ++++---
src/python/pants/backend/jvm/tasks/unpack_jars.py | 13 +++--
src/python/pants/base/lazy_source_mapper.py | 17 ++++---
src/python/pants/engine/round_engine.py | 70 ++++++++++++++++++--------
src/python/pants/goal/BUILD | 1 -
src/python/pants/goal/context.py | 28 ++++-------
src/python/pants/goal/error.py | 4 --
src/python/pants/goal/workspace.py | 7 ++-
src/python/pants/option/options.py | 5 ++
tests/python/pants_test/backend/jvm/tasks/test_checkstyle.py | 63 ++++++++++++-----------
tests/python/pants_test/backend/jvm/tasks/test_scalastyle.py | 72 ++++++++++++++------------
tests/python/pants_test/backend/jvm/tasks/test_unpack_jars.py | 2 +-
tests/python/pants_test/base/context_utils.py | 2 +
tests/python/pants_test/base/test_lazy_source_mapper.py | 9 +---
tests/python/pants_test/base_test.py | 4 +-
tests/python/pants_test/engine/test_round_engine.py | 51 ++++++++++++++-----
tests/python/pants_test/tasks/BUILD | 1 -
tests/python/pants_test/tasks/test_context.py | 22 ++------
tests/python/pants_test/tasks/test_depmap_integration.py | 31 +++---------
tests/python/pants_test/tasks/test_group_task.py | 40 ++++++++-------
tests/python/pants_test/tasks/test_jvm_run.py | 2 +-
tests/python/pants_test/tasks/test_protobuf_integration.py | 9 ++--
tests/python/pants_test/tasks/test_what_changed.py | 8 +--
tests/python/pants_test/tasks/test_wire_integration.py | 14 ++++--
58 files changed, 652 insertions(+), 636 deletions(-)

CI went green here: https://travis-ci.org/pantsbuild/pants/builds/48291884

Issues

  • 0
  • 2
  • 0
  • 2
Description From Last Updated
John Sirois
John Sirois
Benjy Weinberger
John Sirois
John Sirois
John Sirois
Benjy Weinberger
John Sirois
David Taylor
John Sirois
John Sirois
Review request changed

Status: Closed (submitted)

Ity Kaul

this is awesome - thank you for cleaning this up!

Patrick Lawson

Sorry for being super late to the game on this one. I'm very excited for this change, and it was well executed. I have a couple of comments and questions, but nothing major enough to warrant raising a post-submit issue.

Why did we leave this as a (class)method that actually plumbs the round_manager? I was under the impression that it would be a classmethod that takes the options and yields up a list of products.

  1. As you figured out below - there are two types of products today and the odler non-data product is tricky with the predicate. This was the minimal use classmethods / add alternate_target_roots lifecycle method change.

It's going to be very important that we plumb through spec_excludes and whatever whitelist/blacklist system we settle on here--but I see that's unrelated to your change.

Note that even in our pathological case we could still just optionally yield

Ah, maybe this predicate is a blocker for just yielding products?

  1. Yes. This wall fall out of the full new engine change it was out of scope for this more limited bandaid on the current engine.

I know it's just rearranging, but this should definitely call super

  1. I already pushed back with Benjy's suggestion of said-same and only updated prepares failing to call super for the cases where Task was not the base class.

  2. Okay sounds good

Ditto super

Mateo Rodriguez

This removed the inits from deferred_sources_mapper and aapt_builder?

  1. Looks like they were no-ops.

  2. Thanks. I have a change coming where I can just follow this lead then.

Loading...