Fix issue with isolated strategy and sources owned by multiple targets

Review Request #2061 — Created April 11, 2015 and submitted

stuhood
pants
e23761a...
pants-reviews
fkorotkov, jsirois, nhoward_tw, patricklaw
  • Test all strategies in more cases.
  • Execute multiple iterations of compilation to catch bugs related to registration of targets that are already 'valid'
  • Add testproject and reproduction for an error in the "sources-with-multiple-owners" case for isolation.
  • Split sources by context in compute_classes_by_source case.

This fixes an error we noticed while testing the isolated strategy where sources owned by multiple targets (yea, it's gross, I know) would end up with distinct classfiles for each target. This would cause classes_by_source to be ambiguous.

I know we've had performance issues with this code before, but in this case I feel confident that we haven't introduced any additional analysis parsing.

https://github.com/pantsbuild/pants/pull/1400

ST
BE
  1. Just a couple of minor comments.

  2. Remove trailing whitespace.

  3. It's funny, I often start out by using a namedtuple for convenience and invariably end up replacing it with a 'real' class...

    But in this case wouldn't namedtuple's equality semantics have been OK?

    1. I initially did this because I got an error indicating that 'list' was unhashable, without really realizing that it was trying to hash the sources field rather than the tuple itself. After making the change though, it was obvious that sources didn't need to be included in the id.

  4. 
      
ST
ST
Review request changed

Status: Closed (submitted)

Loading...