walk synthetic targets dependencies when constructing context.target()

Review Request #1914 — Created March 13, 2015 and submitted

nhoward_tw
pants
1255
197c464...
pants-reviews
dturner-tw, ity, jsirois, tejal

A recent change made it so that synthetic targets dependencies aren't traversed which caused their 3rdparty deps to be not added to the classpath in certain cases. This caused compilation errors. This patch retains the derivation checks from the previous one, but expands the synthetic targets after determining which ones are needed.

RB of the patch that changed the ordering which caused the issue: https://rbcommons.com/s/twitter/r/1863/

wrote unit test, verified existing behavior. ran context tests. CI baking at https://travis-ci.org/pantsbuild/pants/builds/54317247

NH
TE
  1. Looks good to me

  2. 
      
IT
  1. Ship It!
  2. 
      
PA
  1. I would hold off on this. I'm in the process of putting together a repro of a (somewhat critical) failure due to the original synthetic target change to Context.targets. I'll suggest reverting that there, which would also necessitate reverting this.

    1. We need this change due to another failure we see and this fixes this issue.
      Do you see a compile failure due to missing deps? If yes this fixes the issue.

    2. No, the failure is more fundamental--synthetic java targets that we create are being pulled into the graph when they shouldn't be. In particular, they are java_sources parameters to synthetic scala targets, and jmake is not supposed to operate over them (zinc is). 327df46ca3e1f359cd9de0260d2d49839fb22a06 causes them to be seen by jmake, which is incorrect in the pure sense, and also is a real break for us.

    3. It seems like there are really two types of java targets: those which should be compiled by jmake, and those that should be compiled by zinc. From my perspective, the problem is that we do not distinguish between these.

      In other words, 327df46ca3 might have been the trigger for your issue, but reverting it isn't necessarily the best fix.

    4. Patrick, while I think the issue you are raising is one that is important to resolve, we need this patch for something that's blocking us. It's fixing an important issue with the existing synthetic target implementation. I'm going to merge it in.

      That said, I think we can come up with a better solution that solves both issues. If that means reverting the existing synthetic handling later, I'm fine with that.

    5. The issue you're running into, I think, is that there are synthetic targets that are derived from the targets traversible from the Pants invocation's target roots.

      If the tasks injecting the java_sources referenced targets could set the derived_from argument to None on injecting them into the context, then with the current implementation, they would be ignored.

      Could you try that?

    6. It's also possible for me to use the underlying BuildGraph methods to accomplish the same goal--Context is just a thin helper in this situation. Since things are in such flux around how codegen works anyway--and java_sources are such a confusing and rarely used hack--I'm okay with just working around this in our codegen.

  2. 
      
DT
  1. Ship It!
  2. 
      
PA
  1. Since this has been pushed upstream, could you please mark it as submitted and note the SHA in the message?

  2. 
      
NH
Review request changed

Status: Closed (submitted)

Change Summary:

submitted as 8c4190ca956a83f3286bc0d104041e94d53eaa59

Loading...