Open source the spindle plugin for pants into contrib

Review Request #2033 — Created April 5, 2015 and submitted

patricklaw
pants
https://github.com/pantsbuild/pants/pull/1369
3a2f626...
pants-reviews
benjyw, jsirois, stuhood, zundel

Open source the spindle plugin for pants into contrib.

This is in anticipation of upstreaming buildgen, which has support for inferring BUILD dependencies from codegenned scala and mapping it back to the concrete target.

For more information about spindle, see github.com/foursquare/spindle

Include a scala unit test that depends on and exercises the full spindle codegen stack.

CI is green: https://travis-ci.org/pantsbuild/pants/builds/57372086

  • 0
  • 0
  • 1
  • 0
  • 1
Description From Last Updated
PA
PA
PA
PA
PA
ST
  1. 
      
  2. Makes sense to land this without a ton of reformatting, but can we add a TODO about removing the custom formatting post commit?

    1. I copied this verbatim (minus replacement) from the equivalent scrooge file. What exactly should be changed?

    2. This is correct.  For each contrib there is a different 1st party so each contrib needs its own isort.  I know of know way to get the .isort.cfgs to inherit short of modifying isort code or writing a custom isort runner.  As such, Patrick has to copy the remaining config.
  3. should be specified via an option

  4. Can we link this comment to a ticket/issue somewhere? The two approaches to target injection in this method complex-ify things a bit.

    1. I'll open an issue on GH.

  5. two lines between toplevel

  6. 
      
PA
ZU
  1. 
      
  2. contrib/spindle/3rdparty/BUILD (Diff revision 4)
     
     

    Copyright header?

  3. I thought mixin decls were supposed to go on the left?

  4. We aren't consistent when setting they type and action for --jvm-options. Sometimes its action="append", sometimes we set the type=Options.list

    1. Unless we have a strong desire either way, I'm going to leave this.

  5. Why do you do this?

    1. This was our very old (but effective) hack to make ./pants compile some/codegen.thrift work. I would rely on the new way of doing this (let context.create_new_target take care of it), except that we bypass that convenience method because it breaks java_sources, and also is sufficiently unperformant that we do invalidation by hand below.

  6. This is what I am planning to do for wire, but I don't feel all that great about invoking external tools to calculate the build graph, which is where I think this gets called. It would be getting called once per target, which could be a performance issue.

  7. 
      
JS
  1. 
      
  2. This is fine but un-needed unless you plan on adding more pants.contrib.spindle packages in separate trees, which seems unlikely.
  3. This can be killed.  If no source root applies, `SourceRoot.find` returns the spec_path: https://github.com/pantsbuild/pants/blob/master/src/python/pants/base/source_root.py#L219
    
    IOW - apparently spindle_thrift_library targets don't actually care about source roots; ie: if the target were jarred it would have a flat set of thrift files and thats either OK or never happens so its also OK.
    1. This is a good point--spindle actually specifically cares about source roots! It passes the universe of source roots into the tool so the tool can correctly handle includes. And it makes me realize that putting the source root in this directory is a very bad example and would break any non-trivial use of spindle.

      So I'm back to what I can only describe as a flaw in our current contrib layout: we declare source roots in contrib/BUILD (already a bad precedent, we want to get BUILD files out of the business of affecting global state), but we rely on being able to parse contrib/BUILD at times when the contrib backends haven't been loaded, and therefore the aliases aren't available.

      I would really like a "quick" fix for this--as in, leaving it here for now with a big warning.

      Perhaps the real problem here is that this unit test belongs in the top level examples directory, or something like that? Do we consider failures to build example code to be failures in the CI?

      But I'd also like to propose that we figure out a better way to describe source roots for contrib projects: it can't be in the contrib's register.py, since that is also run by end users, and their source root layout might be different. And it also ideally shouldn't be in a BUILD file. Ideas?

    2. I'm fine with the quick fix - thinking on the publishing issue - which - IIUC is the only arrangement bit by this in CI.
    3. I'm working this issue here: https://github.com/pantsbuild/pants/issues/1370
  4. Does spindle honor this?  If not it looks like python file header copypasta.
  5. 
      
PA
JS
  1. Ship It!
  2. 
      
ZU
  1. Ship It!
  2. 
      
PA
PA
PA
PA
Review request changed

Status: Closed (submitted)

Change Summary:

Thanks for the reviews, submitted @ 19da068af22278e70cbd9d3b3acb7deb48c90e51

Loading...