Get rid of the 'codegen' target label and associated checks.

Review Request #4316 — Created Oct. 16, 2016 and discarded

benjyw
pants
pants-reviews
jsirois, kwlzn, zundel

Instead, use type checks on a new CodegenLibraryMixin, created
to tag codegen libraries.

Note that this mixin lives in src/python/pants/build_graph,
as this is a good central point for it. Putting it in, say,
the codegen backend, would require other backends to depend on
codegen, just to import the mixin. Dependencies between backends
is something we'd like to avoid as much as possible.
So the pants core now recognizes "codegen" as a concept, even
though it doesn't provide any codegen implementations.
That seems fine to me - in any case, the "codegen" backend should
probably be split up anyway, so there's no other natural home
for this.

This is part of an effort to get rid of the long-deprecated
target labelling system, and its associated is_* methods.

Closed in favor of the simpler https://rbcommons.com/s/twitter/r/4318/.

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

JS
  1. 
      
  2. I'm not hugely opposed, but I truly do think this sort of thing represents a plugin model scalability problem.  Each generic target type that comes up is going to need a mixin added to the core in this model.  Isn't `has_sources('*.thrift')` enough to replace the existing predicate at use-sites? Presumably any task that needs to act on thrift is justified in asserting / repeating - "the targets I care about must own thrift files".
    1. Obviously not well thought out comment, I need to read more!
    2. :) Generally, absolutely. Trouble is, we use the concept of "codegen" in core parts of Pants for things like "should we lint these source files".

      But we don't care about the type of codegen there. So I'm comfortable with "codegen" being a core concept, just like "sources" is, as long as "thrift", "protobuf" are not, just as "python" and "java" are not.

    3. OK - more refined: I'm disturbed by is_synthetic and is_codegen.  The code in the LHS of the diff should / could already be using is_synthetic afaict and so is_codegen could be totally dropped and the mixin not introduced... maybe.  All the `is_codegen` queries I see on the LHS could either be completely dropped or else replaced with `has_sources('*.java', '*.scala') and not is_synthetic` ... which of course re-injects my scalability concern in a different way, but at least maybe a centrally solvable way - ie a subsystem for jvm targets that allows plugins and core to register extensions that convert to classfiles via either the builtin compile tasks, or ones providd by plugins.
      
      So, clearly scope balooning way beyond this RB even if you agree!  I'll review on the narrow merits instead, but I do believe all the above.
    4. Yeah, good point. Synthetic targets are an implementation detail, but it's precisely that implementation detail that causes things to act on codegenned code when they shouldn't.

  3. 
      
JS
  1. 
      
  2. This needs to be `:API: public`, unless the strategy is lazy, but the `:API:` business never seems to have fully risen to the level of premeditated strategy, so this may be invention of a policy time.
  3. 
      
BE
ZU
  1. It looks like https://rbcommons.com/s/twitter/r/4318 is less disruptive and I'm not sure that the new mixins specified here add any value at this point.

    In the future there might be some advantage to labeling these separately, so it might be worth going the more complex route, but I don't know what that future need might be.

    1. Yeah, I would definitely prefer that one, if you concur that it's sufficient. I'm pretty certain it is. As far as I can tell, in practice we never actually need to know if a handwritten target is_codegen, we just need to know if the synthetic target we generate from it (which inherits its labels) is. So in a way this is more precise.

    2. I think we were using it at one point with the deferred sources stuff but that's been re-done.

  2. 
      
BE
BE
Review request changed

Status: Discarded

Loading...