Refactor classpath consolidation into a separate task.

Review Request #4152 — Created Aug. 12, 2016 and submitted

molsen
pants
3778
pants-reviews
jsirois, mateor, nhoward_tw, patricklaw, stuhood, zundel

Previously there were two invalidation blocks inside of bundle_create.
In some cases where the classpath was changed by another task this caused
a vt to be validated before bundle happened and bundle was then skipped.

  • Move consolidate_classpath to a separate task.
  • Update bundle_create to rely on the consolidated classpath.

CI green: https://travis-ci.org/pantsbuild/pants/builds/154516443
personal fork is green

Tested in twitter monorepo and the case that previously crashes now works correctly.

  • 0
  • 0
  • 6
  • 0
  • 6
Description From Last Updated
  1. 
      
  2. nit: Docstring please.

  3. nit: capitalize + period

  4. nit: capitalize + period

  5. nit: period for comment sentence

  6. I don't think this helper buys enough. Inline please.

  7. Might make sense to add tests for

    • asserting that jars are left as is
    • intransitive scoped directory dependencies are not consolidated
    1. I will add back in the extra jars from the bundle_create test that I removed and validate that these don't get altered. Chatted with Nick offline and this will satisfy what he is lookingfor.

  8. Could you inline this one as well?

  9. 
      
  1. 
      
  2. Should this be added to bundle_create library?

  3. 
      
  1. Thanks!

  2. This name should be verby probably.

  3. I don't really think it makes sense to include names in TODOs, let alone in NBs. Should drop.

    (and thanks for implementing the original Peiyu... we won't forget!)

  4. 
      
  1. 
      
  2. this is doing the exactly same thing as ensure_classpath_products, is this method necessary?

    1. No, this is dead code... removing.

  3. 
      
  1. Ship It!
  2. 
      
  1. Ship It!
  2. 
      
  1. Thanks for waiting on me, Matt.

    We have a use case for using this task in-abscence of the bundle goal. Probably java shops would as well, you could imagine wanting this to be scheduled before the compile phase to take advantage of the jar-classpath speed hack. But that means we would at one point want finer invalidation to avoid doing classpath scans for unchanged targets.

    I originally tried to convince you to dump the consolidated classpath to a synthetic jar. But I decided to leave that conversation for another day and just ask for a TODO instead :)

  2. I know this was not your code, but the runtime_classpath product name is overloaded, imo. jar_task requires it and mutates it, as bundle_create did, among others. If we used a unique product name then the task could be opened up to other goals, the ordering determined by the scheduler.

    Just a general gripe, I guess - I don't expect you to solve it here.

  3. Could you add a TODO to find a way to not process classpath entries for valid VTs?

  4. You could also consider just constructing a new classpath instead of mutating the copy.

    classpath = OrderedSet()
    If a dir:
    add jar
    else
    add entry

    Leaving it as-is is fine with me and may even be more performant. I just would prefer not mutate a product, in general.

  5. So the only thing that find_consolidate_classpath_candidates appears to buy is that targets with loose directories can have the scan short-circuited. But every entry is eventually checked once those candidates hit the consolidate_classpath method.

    I think that it would save some system calls to delete that method and just do the full scan once. The savings would probably not be too extreme - my read is that it currently processes some N entries twice if the classpath has a directory, when it could be just once.

    But it would also be a cleaner interface, since you could then just delete the find_consolidate_classpath_candidates method completely.

         def execute(self):
           runtime_classpath = self.context.products.get_data('runtime_classpath')
           consolidated_classpath = self.context.products.get_data('consolidated_classpath', runtime_classpath.copy)
           targets = self.context.targets(**self._target_closure_kwargs)
           self._consolidate_classpath(targets, consolidated_classpath)
    
  6. This reads more as a comment (as opposed to docstring) to me as well. Seems off to docstring a private test method.

  7. I don't know that the test needs a docstring, since there won't be any callers. Make a comment?

  8. Can you add a unittest check that verifies that there are no longer any loose files on the classpath?

    Maybe a check that they exist before the task.execute and then another that ensures they are no longer present afterwards.

    You already have determined that below - but I think it would be useful since it is the ground truth of what the task is designed to do.

  9. 
      
  1. Thanks Matt!

  2. 
      
Review request changed

Status: Closed (submitted)

Change Summary:

commit f27e77b315e8741d763583f71cdc74504cc028ed

Loading...