Allow targets to have sensible defaults for sources=.

Review Request #4300 — Created Oct. 12, 2016 and submitted

benjyw
pants
pants-reviews
jsirois, nhoward_tw
95% of the time, a java_library() target will have
sources=globs('*.java'), and it seems unnecessary to require this
boilerplate to be repeated everywhere.

This change provides a way for target to say "if no explicit sources
are specified, use this glob".

The change is gated behind an option, because it can have unintended
consequences.  For example, in the Pants codebase itself we had a few
places where we were using a python_library() with no sources as a
dependency aggregator, when we should have been using a target().

This change fixes such quirks in our own codebase.

It also modifies a couple of our example targets to take advantage
of this functionality, as proof-of-concept.

This change required modifying quite a few tests that were creating
library targets with no sources: These tests didn't init the relevant
subsystem, and so couldn't consult the new option to see what to do
when no sources= were provided.  These tests were fixed in one of
two ways: either by initializing the subsystem, or by providing
explicit sources=[], whichever made more sense.

Note that instead of creating a new subsystem, I added this option
to the Target.UnknownArguments subsystem.  This implied an expanded
role for that subsystem, so its name was no longer applicable.
I renamed it to Target.Arguments, and deprecated its scope.

Finally, note that the defaults for library and test targets make
it easy to have tests live side-by-side with the code they test,
instead of in a separate source tree, which is a really useful
idiom IMO.

CI passes: https://travis-ci.org/pantsbuild/pants/builds/168486240

  • 0
  • 0
  • 2
  • 0
  • 2
Description From Last Updated
BE
JS
  1. 
      
  2. Kill the :no_sources target and let the target fix to :all stand?

  3. ...and if so on above - revert this.
    1. Alas no, if we do that then we won't get the "No jars to be analyzed" message (presumably because the target() targets get elided over).

  4. This probably deserves a TODO calling the shot that our modelling is broken and this global knowledge of extensions does not scale (the old hypothetical problem of '*.clj', '*.groovy', etc).
    1. Not sure I concur that this doesn't scale. Editing this file once when we add support for a new JVM language doesn't seem like such a burden.

      Not to mention the fact that things like *Spec.scala are very framework-specific and not something we could reasonably derive even with better modeling of languages.

  5. src/python/pants/build_graph/target.py (Diff revision 1)
     
     

    Is this a legacy comment re the resources bit at the end? The code appears to work more generally now, only examining supports_default_sources.

    1. True, but create_sources_field() is called to create the resources field in python_target.py, as well as the sources field. Hence the check.

      I clarified the comment.

  6. 
      
ST
  1. 
      
  2. Better yet, time to re-add per-language aliases? I think that would address John's scalability concern via inheritance from the base class for *.clj, etc.

    1. We can discuss out of band, but clearly that is work for a separate change.

  3. Would we actually want a test helper library and tests to be defined in the same directory? That would be a package collison, and a 1:1:1 violation.

    It's also probably not scalable to (for example) including resources in the same directory, etc.

    Or maybe it is. But it feels haphazard in a universe where we don't validate that globs are disjoint.

    1. I actually think the code under test itself, should be kept together with its tests. Separate roots for sources vs. tests are the industry norm now, at least for JVM languages, and maybe Python. But I personally don't like it. It's way easier to find the tests of your code, see what isn't tested, etc., when they live in the same dir. This is how Go does it, as do some C++ shops (notably Google).

      I greatly prefer this:

      Bar.java
      BarTest.java
      Foo.java
      FooTest.java
      IAmNotTested.java
      

      and this:

      bar.py
      bar_test.py
      foo.py
      foo_test.py
      i_am_not_tested.py
      

      Than having the tests live in some parallel structure.

      But in any case, this just enables that, it doesn't require it.

  4. 
      
BE
ST
  1. Thanks a lot Benjy. Any thoughts on how to go about making this the default? Maybe adding a warning in cases where sources are None, but were expected, and then removing it when we flip the default here?

    1. I don't know. I didn't think of putting this behind an option at first, but when I saw that this broke pantsbuild, and we're relatively careful about not using the wrong target types for dep aggregation, I realized that the potential effects were non-trivial. I'm not sure how we tell the difference between "java_library() with no sources that should default" and "java_library() with no sources that really should be a target()". But I'm very open to ideas!

    2. I think it might be as simple as warning whenever sources are requested, but the option is disabled (the create_sources_field call is explicit, so it would only trigger in cases where sources were expected)? This would give folks with ambiguous targets fair warning to do what you did here and explicitly add sources=[] or switch to target/alias aliases.

      Then, when the deprecation fires, we'd assume enough warning and switch the option to the default.

    3. That sounds reasonable. How about I add that in a followup change, so we can futz with it independently of the actual functionality change?

    4. Yep, fine with me. You've got my shipit.

  2. pants.ini (Diff revision 2)
     
     

    Can you add a blurb to the docs about this feature?

    I think it is mostly intuitive, but the one potential rough edge is that any *_library code suffixed with *Test.* will not be picked up. Again, that's probably more than intuitive once folks are used to it, but not on day one.

    1. Good idea, done.

  3. src/python/pants/backend/jvm/targets/java_library.py (Diff revision 2)
     
     
     
     
     

    Worth referring to the same set of static values here? Would make the comment unnecessary.

    1. Not sure I understood the note correctly. Take a look at the fix commit and let me know if that's what you meant.

  4. 
      
NH
  1. 
      
  2. How about calling this default_sources_exclude_globs, since the kwarg is exclude?

    1. Good idea, done.

  3. Perhaps we should thread these through to the build dictionary somehow? The simplest solution would be to update docstrings with the globs, but I think that could end up being a little fragile.

    1. That sounds mildly tricky, but I can look at it.

      Whatever we do automatically will be incorrect if a target overrides default_sources() though.

  4. Should these be tuples as in the java case?

    1. Definitely. Fixed.

  5. Should this be a tuple?

    1. Fixed (python_tests and python_library share the same literal now).

  6. src/python/pants/build_graph/target.py (Diff revision 2)
     
     

    Could you include this as a new clause in the if / elif block below? Otherwise the control flow complexity here gets a little higher.

    1. Good idea. I tidied up that whole if/elif chain while I was at it.

  7. Maybe just pop('sources', [])? Or is the or here meant to catch explicitly passed Nones et al?

    1. Yeah, that should work. Changed.

  8. Could you add tests for the other key_arg cases?

  9. 
      
BE
BE
ST
  1. Ship It!
  2. 
      
NH
  1. 
      
  2. src/docs/first_tutorial.md (Diff revision 3)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     

    nit: ws on end of lines.

  3. 
      
BE
BE
BE
BE
BE
BE
Review request changed

Status: Closed (submitted)

Change Summary:

127867b852cff0ceda14d0bd427b2ffe9cdbe5a0

BE
  1. Submitted as 127867b852cff0ceda14d0bd427b2ffe9cdbe5a0. Thanks!

  2. 
      
Loading...