Include scala_library:java_sources in targets

Review Request #733 — Created July 24, 2014 and submitted

ity
pants
390
pants-reviews
johanoskarsson, jsirois, patricklaw, stuhood
Include java_sources in targets - the final target set returned by context.targets() was not considering targets that weren't specified in the "dependencies" of the scala_library. In this case, a target was specified using "scala_library:java_sources" which was being dropped. This in turn was causing idea not to load the project with the java sources it should have had.

I have added the logic in BuildGraph to get it working but it doesn't seem particularly elegant. 

When adding java_sources we could attach a specific type to the target like "scala_java_sources_type" and then check for that during the target inclusion test instead of the extensive checking that I do now. Or folks who know BuildGraph might have better ideas - open to suggestions.
CI passed locally. Idea project generated successfully with java_sources and compiles:

$./pants goal idea src/scala/com/pants/testproject/javasources::

And travis underway:
https://travis-ci.org/pantsbuild/pants/builds/30786335
  • 0
  • 0
  • 4
  • 0
  • 4
Description From Last Updated
JS
  1. 
      
  2. src/python/pants/base/build_graph.py (Diff revision 1)
     
     
    It would be really nice to keep this hack in the jvm backend.  IIUC you need Context.targets(...) to work for scala_library.java_sources cases.  How about this instead:
    1.) revert this change
    2.) fix Context.targets to gather the unfiltered set by unioning the target_root.closure()s
    3.) subclass closure() for ScalaLibrary to handle its own .java_sources case.
  3. 
      
TE
  1. I am a little bit confused. If java_sources are only used in scala_library, can we make this change there?. i see the scala_lib property traversable_specs returns java_sources spec if any. 
    
    Can we add the same logic to traversable_dependency_specs?
    
     
    1. What i meant was:
      *Can we add the same logic `traversable_dependency_specs` or some special case handling for scala_library target get java_sources 
      too.
      
      
      But never mind, i updated the page and saw @John's comment.
      
    2. but thats already happening - java_sources are present in the specs and also in the targets_by_address for BuildGraph. The issue isn't that they are not parsed - they are. The issue is they are dropped in the Context.targets()
    3. umm ok. i thought walk_transitive_dependency_graph looks at _traversable_dependency_specs which is not returning the java_sources.
      
    4. Hopefully I can clear this up.
      
      The java_sources are injected into the graph via this override: https://github.com/pantsbuild/pants/blob/master/src/python/pants/backend/jvm/targets/scala_library.py#L64
      But very importantly _not_ as dependencies, just as disconnected target closures.  This is deliberate since in the java_sources case - the pointed-at JavaLibrary targets must have a dependency back on the ScalaLibrary to accomplish the dual-compile needed for these cycles.  In short pants needs to model the cycle, but it cannot do so directly or else it breaks its the DAG and we get a graph with cycles and pants blows up early since it validates no dependency cycles at its core.
      
      So java_sources targets are in the graph and resolved, they're just not connected; thus invisible to closure-scoped operations like context.targets() and target.closure() prior to Ity's fix.
  2. 
      
IT
IT
JS
  1. 
      
  2. This works for our limited test case where java_sources itself is a singleton set of targets with 0 dependencies of its own.  TO be correct you need to union the closure of each java_source in java_sources.
  3. src/python/pants/goal/context.py (Diff revision 3)
     
     
    I think this whole method can just be:
    target_set = set()
    for target in self._target_roots:
      target_set.update(target.closure())
    return filter(predicate, target_set)
    
    The target_root_addresses and transitive_subgraph_of_addresses call is redundant.
    
  4. 
      
IT
JS
  1. LGTM mod bug is still there to fix.
  2. This still has the same bug, you just re-spelled union(...).  You want:
    target_set.add(java_source.closure())
    
    That gets the JavaLibrary dependencies if any (we have 0 in our Twitter case).
  3. 
      
IT
PA
  1. I think this is a good solution to the problem in terms of localizing it to ScalaTarget.  Ideally some day we can back out the need for `Target.closure`, as I think this is logic that belongs in BuildGraph, and should be as simple as possible.
  2. 
      
IT
JS
  1. 
      
  2. unused - revert
  3. unused - revert
  4. src/python/pants/goal/goal.py (Diff revision 6)
     
     
    This should not be needed - with the current state of Phase as singleton / singleton registry, tests must Phase.clear().  Pretty sure there is just a missing instance or two of this
  5. 
      
ZU
  1. 
      
  2. Will we need to do this in java_library as well?
  3. 
      
PA
  1. 
      
  2. src/python/pants/goal/context.py (Diff revision 6)
     
     

    When trying to update our internal release to master, I discovered that this causes a serious performance regression. I believe we're going to have to back this particular change out and find another way for IDEA to get its complete target set.

    When running a goal on a large chunk of our build graph, this caused the time for a call to Context.targets() to go from under a second to over 30 seconds.

  3. 
      
IT
Review request changed

Status: Closed (submitted)

Loading...