[engine] yield only addresses associated with target specs, so `list` goal will work

Review Request #3873 — Created May 11, 2016 and submitted

wisechengyi
pants
3323, 3415
pants-reviews
jsirois, kwlzn, peiyu, stuhood
  • yield only addresses associated with target specs, instead of the whole transitive closure.
  • hardcode dist dir into --pants-ignore for daemon

https://travis-ci.org/pantsbuild/pants/builds/131043219

Two engine runs:
[tw-mbp-yic pants (3323_better)]$ ./pants --config-override=pants.daemon.ini list examples/tests/java/org/pantsbuild/example/:: | sort
examples/tests/java/org/pantsbuild/example/hello/greet:greet
examples/tests/java/org/pantsbuild/example/useantlr:antlr3_test
examples/tests/java/org/pantsbuild/example/useantlr:antlr4_test
examples/tests/java/org/pantsbuild/example/usejaxb:usejaxb
examples/tests/java/org/pantsbuild/example/useproto:useproto
examples/tests/java/org/pantsbuild/example/useprotoimports:useprotoimports
examples/tests/java/org/pantsbuild/example/usethrift:usethrift
examples/tests/java/org/pantsbuild/example/usewire:usewire

[tw-mbp-yic pants (3323_better)]$ ./pants --config-override=pants.daemon.ini list examples/tests/java/org/pantsbuild/example/:: | sort
examples/tests/java/org/pantsbuild/example/hello/greet:greet
examples/tests/java/org/pantsbuild/example/useantlr:antlr3_test
examples/tests/java/org/pantsbuild/example/useantlr:antlr4_test
examples/tests/java/org/pantsbuild/example/usejaxb:usejaxb
examples/tests/java/org/pantsbuild/example/useproto:useproto
examples/tests/java/org/pantsbuild/example/useprotoimports:useprotoimports
examples/tests/java/org/pantsbuild/example/usethrift:usethrift
examples/tests/java/org/pantsbuild/example/usewire:usewire

Regular run:
[tw-mbp-yic pants (3323_better)]$ ./pants list examples/tests/java/org/pantsbuild/example/:: | sort
examples/tests/java/org/pantsbuild/example/hello/greet:greet
examples/tests/java/org/pantsbuild/example/useantlr:antlr3_test
examples/tests/java/org/pantsbuild/example/useantlr:antlr4_test
examples/tests/java/org/pantsbuild/example/usejaxb:usejaxb
examples/tests/java/org/pantsbuild/example/useproto:useproto
examples/tests/java/org/pantsbuild/example/useprotoimports:useprotoimports
examples/tests/java/org/pantsbuild/example/usethrift:usethrift
examples/tests/java/org/pantsbuild/example/usewire:usewire
  • 0
  • 0
  • 2
  • 0
  • 2
Description From Last Updated
  1. 
      
  2. src/python/pants/engine/legacy/graph.py (Diff revision 3)
     
     
     
     
     

    Not a huge fan of this assumption... maybe add an alternative form of execution_request that takes (Subject,Product) tuples?

    Alternatively, you might be able to filter the roots by root.product is Address.

    1. Thanks! Used filters instead.

  3. src/python/pants/engine/legacy/graph.py (Diff revision 3)
     
     

    This should stably result in a list, so the maybe_list call should not be necessary.

    1. inject_addresses_closure injects a set, e.g. set([examples/src/resources/org/pantsbuild/example/hello:hello]), which would result address_state.value to be just a string.

      Print out of subjects and address_state.value:

      I0512 10:44:44.039100 94577 pailgun_server.py:72] handling pailgun request: `./pants --config-override=pants.daemon.ini list examples/tests/java/org/pantsbuild/example/hello/::`
      
      I0512 10:44:44.110034 94577 graph.py:170] Injecting to <pants.engine.legacy.graph.LegacyBuildGraph object at 0x10842a110>: [DescendantAddresses(directory='examples/tests/java/org/pantsbuild/example/hello')]
      I0512 10:44:44.381186 94577 graph.py:170] Injecting to <pants.engine.legacy.graph.LegacyBuildGraph object at 0x10842a110>: set([examples/src/resources/org/pantsbuild/example/hello:hello])
      
      I0512 10:44:44.430619 94577 graph.py:185] examples/src/resources/org/pantsbuild/example/hello:hello
      I0512 10:44:44.430984 94577 graph.py:185] [examples/tests/java/org/pantsbuild/example/hello/greet:greet]
      
    2. It looks like in the inject_addresses_closure case, you don't need to request the Addresses (the return value is ignored). If possible, would be better to avoid computing this in both cases, rather than doing conditional things based on types. maybe_list is for user input.

    3. added an optional argument expect_return_values=True

  4. 
      
  1. looking good - handful of comments.

  2. pants.ini (Diff revision 4)
     
     

    since dist is universal, maybe this would make more sense as a built-in default - e.g. on the --pants-ignore option?

  3. src/python/pants/engine/legacy/graph.py (Diff revision 4)
     
     
     
     
     
     
     
     

    should this continue to be a generator and yield as it did before?

    seems like you could get rid of the building up of addresses by just yielding items as as they came along unless you need deduping?

    1. yes there would be dups, but thanks for pointing out yield would be better otherwise.

    2. ftr, you can still yield while de-duping via a set.. this would be more in-line with the way the other inject methods work, no?

    3. used yield instead. thanks!

  4. src/python/pants/engine/legacy/graph.py (Diff revision 4)
     
     

    this should use logger.debug('... %s', thing) form for normal runs in the event addresses is huge (think source ::)

    1. changed. the output doesn't seem to vary. is it because format's complexity thus causing performance issue?

  5. should/could this cover ::?

    1. it could, but https://github.com/pantsbuild/pants/issues/3428 creates discrepencies at the moment.

  6. 
      
  1. Ship It!
  2. 
      
  1. lgtm, thanks Yi!

  2. 
      
  1. 
      
  2. src/python/pants/engine/legacy/graph.py (Diff revision 6)
     
     

    do expect_return_values=False for readability

  3. src/python/pants/engine/legacy/graph.py (Diff revision 6)
     
     

    ditto on expect_return_values=False

  4. src/python/pants/engine/legacy/graph.py (Diff revision 6)
     
     

    this needs to change, explain in what cases we don't yield resulting addresses.

  5. 
      
Review request changed

Status: Closed (submitted)

Change Summary:

2ff846d5e4ee650e2bfc9824c9074736b02f4ebe Thanks gents!

Loading...