Support Scrooge generation of .lua files.

Review Request #3823 — Created May 4, 2016 and submitted

wolframarnold
pants
scrooge_lua_gen_support
3333
3876
pants-reviews
stuhood

In order to support other Scrooge backends without requiring future code changes,
the hardcoded list of target types for supported languages has been removed. The list
of supported languages and their target types can now be set via command line option.

The motivation for this is supporting a Lua backend for Scrooge. A tiny change was made
to java_thrift_library.py to add lua to the list of languages. An imminent refactor
by Stu will (see added link) will remove this.

In combination with the scrooge changes, I was able to generate lua build targets.
CI test: https://travis-ci.org/pantsbuild/pants/builds/130716768

  • 0
  • 0
  • 1
  • 1
  • 2
Description From Last Updated
  1. Thanks!

    I think it's possible to do this mostly in internal plugins with a few simple changes here. Then Twitter can define additional libraries internally without upstream patches.

  2. I think that this can be converted to an options dict, which would allow users to add new languages/target types entirely from their options.

    To do this, move this dict into the def register_options block as a str->str dict, with a default of the existing value, stringified:

    register('--target-types',
             default={'scala': 'scala_library', 'java': 'java_library', 'android': 'java_library'},
             advanced=True,
             type=dict,
             help='Registered target types.')
    

    Then, to convert the dict of target types back to it's current form (str->Target), you'd:

    target_types = {k: self.context.build_file_parser.registered_aliases().target_types_by_alias[v]
                    for k, v in self.get_options().target_types}
    
  3. I think that it would clean this up a lot to move both _LANGUAGES and _RPC_STYLES checking into the relevant compilers... then the only "hack" in here (see the TODO) would be that targets know which compilers are valid.

    Please split/move this validation into contrib/scrooge/src/python/pants/contrib/scrooge/tasks/scrooge_gen.py and src/python/pants/backend/codegen/tasks/apache_thrift_gen.py and delete it here.

    1. As discussed offline, I'll do this in a separate review.

  4. 
      
  1. Please drop LuaLibrary itself from this review: we can define that internally.

    1. Do you want me to also remove LuaToResources?

    2. Yes, all of it. That can all be defined internally... only the edits to scrooge need to be in this review.

  2. Can mark this @memoized_method to memoize this lookup.

  3. pop is a bit scary... should consider defensively copying target_types before mutating it, or use a non-mutable method to get the entry.

  4. src/python/pants/CHANGELOG.rst (Diff revision 4)
     
     
     
     
     
     

    Can drop... we don't currently edit this via individual commits.

  5. Please point this TODO to https://github.com/pantsbuild/pants/issues/3411

    1. https://rbcommons.com/s/twitter/r/3876/

  6. 
      
  1. I should not comment but will. This is insane by any reasonable definition [1].  Apache thrift already supports Lua [2], amongst other things.  If Twitter would contribute their Scala support to Apache thrift, it could abandon Scrooge and profit at the same time.
    
    [1] I realize _you_ are not insane, but this endeavor is
    [2] https://github.com/apache/thrift/blob/master/compiler/cpp/src/generate/t_lua_generator.cc
    1. It's a good point. The apache thrift project has not always been as healthy as it seems to be now, so there perhaps used to be a good reason to be forked like that. Will bump the issue internally, as I don't know how much of the recent strategy has just been maintaining the status quo.

    2. ...but looking at that generator, it still seems that thrift is doing codegen with C++ rather than a templating language, which is pretty nutty on its own.

    3. Scrooge is open sourced. We'll open source the Lua support as well. I don't see any internal momentum toward adopting Apache Thrift and I don't know the history of why Scrooge came to be. Codegen in C++ might well have been one of the reasons.

    4. Another reason we don't want the Apache Lua implementation is that it's woefully non-performant. The backend of our Thrift-Lua implementation relies on a C-library that does the serialization/de-serialization. That library is also open sourced.

  2. 
      
  1. Ship It!
  2. 
      
Review request changed

Status: Closed (submitted)

Change Summary:

Merged as c669036d3a830853c17d5386543a1d3142400beb with an updated title.

Loading...