Fixup RoundEngine scheduling.

Review Request #627 — Created July 2, 2014 and submitted

jsirois
pants
jsirois/rounds/walk_then_tsort
314, 316
pants-reviews
ity, zundel
commit 04911d5579afc1ae968b5f269b5a3ed0d4de8185
Author: John Sirois <jsirois@twitter.com>
Date:   Wed Jul 2 13:52:39 2014 -0600

    Fixup RoundEngine scheduling.
    
    Do a recursive walk to construct and prepare Tasks as before
    but then do a tsort after the full walk when all the dependency
    edges are known.
    
    This also takes a pass over all jvm Tasks to ensure proper deps
    are requested in prepare and in the cases where the deps are
    improper TODOs are added calling out whats needed.

 src/python/pants/backend/core/tasks/group_task.py             |  1 +
 src/python/pants/backend/core/tasks/prepare_resources.py      | 10 +++++--
 src/python/pants/backend/jvm/tasks/benchmark_run.py           |  7 +++++
 src/python/pants/backend/jvm/tasks/checkstyle.py              |  4 +++
 src/python/pants/backend/jvm/tasks/junit_run.py               |  3 +-
 src/python/pants/backend/jvm/tasks/jvm_compile/jvm_compile.py | 14 ++++++---
 src/python/pants/backend/jvm/tasks/jvm_run.py                 | 21 +++++++-------
 src/python/pants/backend/jvm/tasks/jvm_task.py                |  7 +++++
 src/python/pants/backend/jvm/tasks/jvmdoc_gen.py              | 10 ++++---
 src/python/pants/backend/jvm/tasks/scala_repl.py              |  7 +++--
 src/python/pants/backend/jvm/tasks/specs_run.py               |  4 ++-
 src/python/pants/engine/round_engine.py                       | 77 +++++++++++++++++++++++++++++++++++++-------------
 src/python/pants/engine/round_manager.py                      | 39 +++++++++++--------------
 tests/python/pants_test/engine/test_round_engine.py           |  4 +--
 14 files changed, 138 insertions(+), 70 deletions(-)
CI passes locally and travis is away here: https://travis-ci.org/pantsbuild/pants/builds/28997865

I did these sanity checks for jvm goals:
$ function sanity() { for goal in "$@"; do echo "$goal:" && PANTS_DEV=1 ./pants goal --explain $goal 2>/dev/null | head -3 | tail -1 && echo; done; }
$ sanity gen resolve compile test run repl doc jar binary bundle
gen:
bootstrap -> gen

resolve:
bootstrap -> gen -> check-exclusives -> resolve

compile:
bootstrap -> gen -> check-exclusives -> resolve -> compile

test:
bootstrap -> gen -> check-exclusives -> resolve -> compile -> resources -> test

run:
bootstrap -> gen -> check-exclusives -> resolve -> compile -> resources -> run

repl:
bootstrap -> gen -> check-exclusives -> resolve -> compile -> resources -> repl

doc:
bootstrap -> gen -> check-exclusives -> resolve -> compile -> doc

jar:
bootstrap -> gen -> check-exclusives -> resolve -> compile -> resources -> jar

binary:
bootstrap -> gen -> check-exclusives -> resolve -> compile -> resources -> binary

bundle:
bootstrap -> gen -> check-exclusives -> resolve -> compile -> resources -> bundle



Also checking the un-constrained cli handling - note clean-all killserver server order is maintained and compile is de-duped:
$ PANTS_DEV=1 ./pants goal --explain clean-all killserver server test depmap compile
*** Running pants in dev mode from /home/jsirois/dev-jsirois-pants/src/python/pants/bin/pants_exe.py ***
(using pantsrc expansion: pants goal --explain clean-all killserver server test depmap compile --server-open)
Phase Execution Order:

clean-all -> killserver -> server -> bootstrap -> gen -> check-exclusives -> resources -> resolve -> compile -> test -> depmap
  • 0
  • 0
  • 1
  • 0
  • 1
Description From Last Updated
JS
  1. I want to beef up unit tests but many meetings this afternoon.
    If you're willing to have this fix in then a follow up RB let me know.
    1. absolutely! we should get this fix in - and then follow up with tests.
  2. 
      
ZU
  1. I think it would be nice to have a test that replicates what you did in your manual tests, but that can be a followon.
  2. 
      
IT
  1. Thanks for finding the bug and the speedy fix! It would be great if we could add tests for these in order to catch these in ci in the future. I will add issues for whichever ones I can see.
    
    Also, added this issue yesterday - https://github.com/pantsbuild/pants/issues/310, might be an idea to mention this instead of the dup TODOs across the board?
    
  2. s/.  Introduce/. Introduce/
    1. Fixed throughout TODOs
  3. super call missing
    1. Good catch - fixed
  4. You could remove "classes_by_target" from here now - since you have it in the superclass (jvm_run).
    
    In fact, you could potentially move both of these.
    1. Super is [ivy_jar_products, exclusives_groups] so I'll leave this.  SpecsRun logically needs the runtime classpath and super just needs the compile time classpath
  5. 
      
JS
JS
  1. Thanks folks - submitted @ https://github.com/pantsbuild/pants/commit/38d994a74f40e481f10a7dd90fbddeb0196f0b4a
    I will follow up after next meeting with unit tests for this stuff.
    1. This broke a unit test, but in a good way, it reveals a dep cycle:
      ...
                           E       ValueError: Cycle detected in phase dependencies:
                           E       	publish <- [publish]
                           E       	compile <- [publish, jar]
                           E       	resources <- [publish, jar]
                           E       	jar <- [publish]
                           E       	resolve <- [compile]
                           E       	check-exclusives <- [resolve, compile, resources]
                           E       	gen <- [compile, check-exclusives]
                           E       	bootstrap <- [gen]
      
      I'm on it...
    2. Did either 6889671ae337e7d9f811a93fe655a95fb0dd1011 or 427bd0f33076c2595dceb0dfd5bca00a06f95498 fix this?
    3. Not sure - being lazy - the issue is fixed on pantsbuild/pants master HEAD for sure though.
  2. 
      
JS
Review request changed

Status: Closed (submitted)

Loading...