`goal repl` should be transitive

Review Request #781 — Created July 29, 2014 and submitted

stuhood
pants
pants-reviews
benjyw, jsirois, tejal
- Partition by selected language vs all other languages to allow for handling of 'dependencies'/'jar_library' and other inner nodes
- Filter the entire context, rather than just target roots
- Label JarLibrary as a JVM target (and fix related fallout)
new unit test
manual validation of mixed/scala/python contexts:

-----
PANTS_DEV=1 ./pants goal repl src/scala/com/pants/testproject/unicode
...
scala> com.pants.testproject.unicode.shapeless.ShapelessExample.greek()
res1: String = shapeless success

-----
PANTS_DEV=1 ./pants goal repl src/python/example/hello/greet
...
>>> from example.hello.greet.greet import greet
>>> greet("yo")
u'\x1b[32mHello, yo!\x1b[0m'
-----

PANTS_DEV=1 ./pants goal repl src/scala/com/pants/testproject/unicode src/python/example/hello/greet
...
FAILURE: Mutually incompatible targets specified: (at least) PythonLibrary(BuildFileAddress(/Users/stuhood/src/pants/src/python/example/hello/greet/BUILD, greet)) vs ScalaLibrary(BuildFileAddress(/Users/stuhood/src/pants/src/scala/com/pants/testproject/unicode/shapeless/BUILD, shapeless))
  • 1
  • 0
  • 2
  • 0
  • 3
Description From Last Updated
This is a pretty big difference in functionality, and one that needs to be more fully discussed. The old code ... BE benjyw
ST
ST
  1. 
      
  2. oops. from a first cut, will remove.
  3. 
      
ST
JS
  1. 
      
  2. src/python/pants/base/target.py (Diff revision 1)
     
     
    I know this file already leaks with predicates like is_java and is_python, but this further does so and cripples the usefullness of discriminators - ~no 3rdparty plugin can benefit. My 1st thought is you tried to go too DRY here, the codegen targets for one _happen_ to use this predicate set, but - for example - ApacheThriftGen could be doing 10+ predicates.  My second though is there is no nice way for repls to mutual exclude without leaks, so why not just have them run what they can when they can.  If I provide python, scala and java targets, I get 1st a python repl with python targets on the PYTHONPATH and then when it exits up pops the scala repl with java and scala targets on the classpath.  If you don't buy this shift in tactics - it seems to me the right place to house the mutex is in a repl coordinator much like GroupTask.  It would probably be a "TerminalTask" mutex though and only allow for 1 terminal task or else fail fast.  so 1 valid repl, or 1 valid test run, or 1 valid "run" run.
    1. As discussed offline: I think we're going to end up in a place where there is more formalized registration of which target types goals/tasks consume... and this is a crazy primordial approximation of that.
    2. OK - things need to move along here - but I don't think that makes this any better.  Is option #1 below - which also is crazy, but at least totally localized to the consuming task - a no go?
      
    3. Honestly, the proliferation of different "ways to consume a dep graph" seems more insidious to me. I don't know how we'd explain option #1 to people (and particularly the fact that it acts differently from other goals). On the other hand, option #2 is trivial to explain.
      
      Centralizing task filtering logic in the Context and Target classes makes sense to me, and will set us up for the type of registration that would be necessary to make this public.
    4. OK - I don't agree but am not strongly opposed and am happy to see forward progress.  That will be a crazy flag --repl-scala-repl-yes?
    5. Post https://docs.google.com/document/d/1nAWEAJusLZGhrImF-ts-GHsy1Hs6ou9GynUNTBKzrrw/edit#heading=h.rea2jth8ny1m , the flag should just be `goal repl --scala`, since it can be context specific. But I think the flag is an escape hatch that I don't expect people will ever need to use unless they have a very unusual layout.
  3. 
      
TE
  1. 
      
  2. Add this check before calculating the accepted and rejected targest
  3. 
      
TE
  1. 
      
  2. src/python/pants/base/target.py (Diff revision 2)
     
     
    Its a little bit complicated to read. 
    Its clear what you are trying to do here, but would this much cleaner if we assume there is a build graph validation in place which will spit out ScalaLib depending on Python Targets and vice-versa?
    
    If, that is the case, we just have to validate the inputs given to the command are of same type. 
    
    
    1. The tricky part is this, in-practice we have users supply target_roots that are Dependencies targets and the graphs rooted at these may contain a mix of Dependencies, JarLibrary and JvmLibrary target types.  In order to "resolve" past the Dependencies root you need to walk, and that walk will hit non JvmLibrary Target types that you probably don't want to blow repl up on.  Stu's approach handles that since it lists "important" predicates, python and jvm, and ignores the others, like Dependencies and JarLibrary.  So it gets the mechanics correct.  I'm just worried more about a future when an external plugin adds a new repl that handles GoLibrary targets.  In this code here we have no clue about is_go.
  3. 
      
BE
  1. 
      
  2. This is a pretty big difference in functionality, and one that needs to be more fully discussed.
    
    The old code was basically a way to coherently infer which language's repl you intended to run.
    
    Your new code basically forbids jvm targets from having deps on python targets and vice versa (at least if you want to repl them). 
    
    That's not something we've enforced before, or even declared as desirable before. This is probably not the right change in which to introduce such a strict constraint. That is best done after a lot more discussion, and is in any case a rather heavyweight thing to introduce to solve a relatively minor problem. 
    
    Alternative solution #1:
    
    - Make jar_library be an "is_jvm" target perhaps (or some other label, we do need to sort out all these labels soon anyway...) 
    
    - Expand dependencies targets one dependency hop out and then apply the existing logic.
    
    Alternative solution #2:
    
    - We set the language with a flag, with the fallback being to attempt to infer using the existing logic:
    
    ./pants goal repl --scala
    
    This will be possible after my new flags work is done.
    
    Alternative solution #3:
    
    - My least favorite: we can not have a single "repl" phase for all languages, and instead make the user say 
    
    ./pants goal scala-repl
    
    
    I'm open to other solutions, but let's not go hastily down the path of forbidding cross-language deps.
    
    1. I had basically proposed #2 offline.  In the pre-target refactor this would be:
      concrete_roots = set()
      for target_root in self.context_target_roots:
        concrete_roots.update(tgt for tgt in target_root.resolve() if tgt.is_concrete)
      
      # Now apply logic efore this change against concrete_roots
    2. Um, #1
    3. > Make jar_library be an "is_jvm" target perhaps
      Yea, we should probably do this anyway. Will add in this review.
      
      > Expand dependencies targets one dependency hop out
      The idea of special casing some targets to act more or less like a dep graph/closure makes me uncomfortable, but I like solution #2.
      
      > We set the language with a flag
      This option sounds good, and isn't really mutually exclusive... once the flag exists, we could change the "Mutually incompatible" message to include a "If you meant to open a scala repl, please explicitly specify --scala". Would you be alright with adding this new logic, and then allowing for the explicit override when it becomes easier to add the flag? I'll leave a TODO next to the message/exception referring to the flags/options code.
  3. src/python/pants/base/target.py (Diff revision 1)
     
     
    Use a dict literal:
    
    LANG_DISCRIMINATORS = {
      'java': lambda t: t.is_jvm,
      'python': lambda t: t.is_python
    }
    
    PS Knowing specific discriminators here is an abstraction leak, although no worse than our existing ones in this file...
    
    
  4. src/python/pants/base/target.py (Diff revision 1)
     
     
    The first sentence of a multiline docstring should fit on a single line, followed by a blank line, followed by further sentences. See http://legacy.python.org/dev/peps/pep-0257/#multi-line-docstrings.
  5. 
      
ST
ST
ST
JS
  1. Ship It!
  2. 
      
ST
ST
Review request changed

Status: Closed (submitted)

Loading...