Fixup JvmCompile to always deliver non-None products that were required by downstream.

Review Request #794 — Created July 30, 2014 and submitted

jsirois
pants
jsirois/jvm_compile/fix_products
415
pants-reviews
benjyw, ity, stuhood
commit 565125b5ee0cc56c7edb13e3c06bc4f19040e224
Author: John Sirois <jsirois@twitter.com>
Date:   Tue Jul 29 21:06:06 2014 -0600

    Fixup JvmCompile to always deliver non-None products that were required by downstream.
    
    This change augments GroupTask with a pre_execute complement to post_execute and
    uses this lifecycle point to ensure JvmCompile delivers on its promise of supply product maps
    regardless of the availability of chunks in the round.  Further adds tests for the new lifecycle
    as well as coverage for pre-existing un-tested always-run semantics of construct, prepare and
    post-execute.

 src/python/pants/backend/core/tasks/group_task.py             | 20 +++++++++++++--
 src/python/pants/backend/jvm/tasks/jvm_compile/jvm_compile.py | 10 +++++---
 tests/python/pants_test/tasks/BUILD                           |  3 ++-
 tests/python/pants_test/tasks/test_group_task.py              | 70 ++++++++++++++++++++++++++++++++++++++++----------
 4 files changed, 83 insertions(+), 20 deletions(-)
Travis CI went green here: https://travis-ci.org/pantsbuild/pants/builds/31199740


Also:
==
$ PANTS_DEV=1 ./pants goal test tests/python/pants_test/tasks/:group_task --test-pytest-options=-v
...
21:12:20 00:01   [test]
21:12:20 00:01     [pytest]
21:12:20 00:01       [run]
                     ============== test session starts ===============
                     platform linux2 -- Python 2.6.9 -- py-1.4.22 -- pytest-2.6.0 -- /usr/bin/python2.6
                     plugins: cov, timeout
                     collected 5 items 
                     
                     tests/python/pants_test/tasks/test_group_task.py@57::GroupIteratorSingleTest::test PASSED
                     tests/python/pants_test/tasks/test_group_task.py@73::GroupIteratorMultipleTest::test PASSED
                     tests/python/pants_test/tasks/test_group_task.py@179::GroupTaskTest::test_groups PASSED
                     tests/python/pants_test/tasks/test_group_task.py@232::EmptyGroupTaskTest::test_groups PASSED
                     tests/python/pants_test/tasks/test_group_task.py@263::TransitiveGroupTaskTest::test_transitive_groups PASSED
                     
                     ============ 5 passed in 0.17 seconds ============


And tested this change live against an internal use-case that was failing before this as such:
==
...
  File "/Users/david/.pex/install/pantsbuild.pants-0.0.21-py2-none-any.whl.0ce746980853d744737145525a44d8f91b0c5d84/pantsbuild.pants-0.0.21-py2-none-any.whl/pants/engine/round_engine.py", line 42, in attempt
    task.execute()
  File "/Users/david/.pex/install/twitter.common.pants-0.4.0-py2-none-any.whl.e1f378c90f5282b48c1ebc94c79373050751b771/twitter.common.pants-0.4.0-py2-none-any.whl/twitter/common/pants/jvm/args/tasks/resource_mapper.py", line 99, in execute
    self._add_args_resources(target, resources_by_target, transitive=False)
  File "/Users/david/.pex/install/twitter.common.pants-0.4.0-py2-none-any.whl.e1f378c90f5282b48c1ebc94c79373050751b771/twitter.common.pants-0.4.0-py2-none-any.whl/twitter/common/pants/jvm/args/tasks/resource_mapper.py", line 118, in _add_args_resources
    collect_resource_dirs(target)
  File "/Users/david/.pex/install/twitter.common.pants-0.4.0-py2-none-any.whl.e1f378c90f5282b48c1ebc94c79373050751b771/twitter.common.pants-0.4.0-py2-none-any.whl/twitter/common/pants/jvm/args/tasks/resource_mapper.py", line 110, in collect_resource_dirs
    mapping = classes_by_target.get(tgt)
AttributeError: 'NoneType' object has no attribute 'get'

Notably, this is the t.c.pants args-apt plugin [1] and it requires 'classes_by_target' [2], but gets None [3] prior to this fix.
[1] https://pypi.python.org/pypi/twitter.common.pants/0.4.0
[2] https://github.com/twitter/commons/blob/master/pants-plugins/src/python/twitter/common/pants/jvm/args/tasks/resource_mapper.py#L89
[3] https://github.com/twitter/commons/blob/master/pants-plugins/src/python/twitter/common/pants/jvm/args/tasks/resource_mapper.py#L105
JS
ST
  1. Makes sense to me.
    
    The first diffs on https://rbcommons.com/s/twitter/r/619/ were technically the beginning of a workaround for this issue... do we want to revert that review and allow empty 'junit_tests' again?
    1. I don't think so - empty sources should always be surprising for a user (moved them, missed killing dead target) - or else they were abusing a library for a dependencies/jar_library/target dep-set.
  2. tests/python/pants_test/tasks/BUILD (Diff revision 1)
     
     
    alpha
  3. 
      
IT
  1. looks good!
  2. >100 chars.
  3. set()? since the order isnt guaranteed.
    1. That's what the sorted(...)'s are for below.  I think you're right for this test, but for GroupTaskTest the actions aren't all hashable so set cannot be used.  I'll stick with the convention for this file. 
  4. 
      
JS
JS
  1. That went green again: https://travis-ci.org/pantsbuild/pants/builds/31343222
    Submitting.
  2. 
      
JS
  1. Thanks folks - submitted @ https://github.com/pantsbuild/pants/commit/11b2db80da918133815569ea685698b0717e38a1
  2. 
      
JS
Review request changed

Status: Closed (submitted)

Loading...