Re-enable filedeps - it was using a now-gone .expand_files() API.

Review Request #939 — Created Aug. 25, 2014 and submitted

jsirois
pants
jsirois/issues/437
437, 522
d52b9d8...
pants-reviews
jinfeng, patricklaw

commit 1a54a4180ba9c264046235f96c1170249d3ce5bf
Author: John Sirois <jsirois@twitter.com>
Date: Mon Aug 25 16:56:56 2014 -0700

Re-enable filedeps - it was using a now-gone .expand_files() API.

This maintains a hack with special knowledge of the JvmApp target and
adds one more to deal with ScalaLibrary.java_sources.

Both of these hacks have unit-test coverage to help back up future
refactors away from these hacks.

src/python/pants/backend/jvm/tasks/BUILD | 3 +
src/python/pants/backend/jvm/tasks/filedeps.py | 34 ++++++++-
tests/python/pants_test/tasks/BUILD | 14 ++++
tests/python/pants_test/tasks/test_base.py | 2 +-
tests/python/pants_test/tasks/test_filedeps.py | 203 ++++++++++++++++++++++++++++++++++++++++++++++++++
5 files changed, 252 insertions(+), 4 deletions(-)

$ PANTS_DEV=1 ./pants goal test tests/python/pants_test/:all

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

  • 0
  • 0
  • 2
  • 0
  • 2
Description From Last Updated
JS
IT
  1. 
      
  2. sort.

  3. it might be a good idea to extract out this to a generic setup that could be utilized for other tests. the scala_library, jar_library, java_library, resources, jvm_app, and so on targets that are created here look generic enough to be used in wider testing and could provide the foundation for it.

    we could in fact, make it easier to construct them and be included in tests? thoughts? this is more me thinking and trying to figure out the next steps - not a blocker at all.

    1. I agree and its on my list of tasks from the summit - rationalize and doc the various test infra we have today. Its all there, but there is dup code, dup fixturing and some dup functionality. In short you probably have to read to much code to write a test today. I'll defer in this review but circle back in a follow-up review or 2 and do this. I created this fellow to track: https://github.com/pantsbuild/pants/issues/525

  4. 
      
JI
  1. Ship It!

  2. 
      
PA
  1. lgtm minus a very tiny nit. Thanks for the tests!

  2. This is a little awkward. It might be that if you extract the concrete target and it doesn't have a BuildFileAddress, you want to raise or warn.

    1. How do I know its a concrete target though? Say the command line is ./pants goal test filedeps :: - It may be that test, which runs code gen, causes synthetic targets to be injected into the BuildGraph.

    2. You don't, I'm saying that the tests (for has BuildFileAddress vs is equal to its concrete ancestor) should be equivalent though. And if they aren't, that's an interesting place for an assertion. It's not a big deal, I'm just wary of testing for BuildFileAddress ever, and I kind of regret making it a thing (I should have just had an optional attribute on Address).

      But here is one of the few places where it probably doesn't matter.

    3. Aha. You clearly mean using this: https://github.com/pantsbuild/pants/blob/master/src/python/pants/base/target.py#L264
      And that makes total sense. I'm slow to catch up.
      Diff coming.

  3. 
      
JS
JS
PA
  1. 
      
  2. This is going to add codegened sources. Is this the intent? Either way, a test should codify this fact.

    1. Thats a good point - old code did too, but I'd call that a bug.

    2. Fixed

  3. 
      
JS
JS
JS
JS
IT
  1. Ship It!

  2. 
      
PA
  1. 
      
  2. Note that targets in java_sources bypass the check to make sure only concrete targets are added--but I think in practice it's impossible (unless you override ScalaLibrary) to get a synthetic target in java_sources.

    1. Noted but agreed. The concrete_targets are read from disk so I used that as a logical stopping point. A task could inject _java_sources_specs such that java_sources returned targets not noted in a BUILD on disk but I'm satisfied considering that unlikely or a misuse.

  3. 
      
JS
  1. Thanks folks - submitted @ https://github.com/pantsbuild/pants/commit/f72a22e624268766b6d371dc82f07699ea164f9b

  2. 
      
JS
Review request changed

Status: Closed (submitted)

Loading...