Compile/Test "changed" targets

Review Request #1572 — Created Jan. 7, 2015 and submitted

davidt
pants
910
bea3321...
pants-reviews
benjyw, dturner-tw, jsirois, patricklaw, zundel

I think pretty much everyone has implmented some variation on this in wrappers, but with some refactoring recently in WhatChanged and elsehwere, actual implementation of this as a proper pants task is pretty trivial now -- the comments are longer then the code.

  • SCM changed target calculation is provided by a mixin (the same as is used by WhatChanged).
  • The actual "compile" or "test" is just done by the existing goal -- this just asks round manager to schedule that.

Thus "compile-changed" doesn't subclass a compile task, or even have any "compile" related functionality, but rather just asks the round manager to run "compile" for it after it replaces the target roots by consulting WhatChanged at the begining of its prepare method.

I'm not super crazy about doing the root replacement in prepare -- ideally I'd prefer a dedicated method on Task that can optionally return alternative target roots. Tasks that want to find their own targets, like those in this change, would override that, then the round engine could ensure at-most-one target returned non-None and do the swap. However that currently isn't possible (discussed further here: https://docs.google.com/a/foursquare.com/document/d/1HM_vR8h5GR3JD8_yXXxoGWIieAiR5Bipdf6gwf51W3Y/edit#)

Thanks to https://rbcommons.com/s/twitter/r/1543/, the approach used here should be safe though -- that change should at least ensure that if one of these tasks were mis-scheduled, it would crash and fail tests quickly, rather than potentially harming correctness more sublety later.

PANTS_DEV=1 ./pants test tests/python/pants_test/tasks:changed_target_integration

https://travis-ci.org/pantsbuild/pants/builds/46216870

Note the change to Greeting.java actually needs to be merged in a separate commit first, and its SHA placed in ref_for_greet_change in the integration test. An integration test seemed like the best way to test this, but unfortunately that makes mocking SCM difficult, so this needs an actual change ref in the pants commit log that changes a known target for it to compile/test.

  • 0
  • 0
  • 7
  • 0
  • 7
Description From Last Updated
DA
DT
  1. 
      
  2. nit: spelling of scheduling

  3. 
      
DT
  1. What we do at Twitter is generate a list of changed targets and then farm it out to many machines. So I don't think this helps us much; we would rarely want to run all changed targets on a single machine.

    1. We do both here -- engineers often just ask pants to compile changed targets, when their CL is limited to a few targets. For bigger changes and refactors that hit more than a handful of files, we have a script that does a similar farm out to jenkins executors -- and I get the impression others have developed workflows that also involve finding and running all changed targets in a single run.

  2. 
      
DT
  1. 
      
  2. nit: I prefer not parenthesizing the importees unless they're going to cross multiple lines.

  3. nit: extra whitespace

  4. 
      
DA
BE
  1. My comments are 90% comment typos and grammatical nits. There are one or two slightly more substantial ones though...

  2. Since you're here anyway, indent this correctly (2 spaces, not 4)?

  3. No parens, here or below.

  4. Can you add more details in the comments about why these stubs are needed?

  5. Shouldn't the mixin be to the right of the main base class? It affects the mro, which may not matter in this case, but it's still probably good hygiene.

    For example, if someone inadvertently adds a register_options() classmethod to the mixin, our register_options() will call that, and the base class's will not be called at all (unless the mixin calls it, which is unlikely).

    1. Patrick insisted I make ChangedFileTaskMixin purely addititive so MRO doesn't matter, but if it weren't, I believe in general, mixins must always go on the left of base classes.

      Mixins should always call super when overriding methods (though ideally they don't override and then MRO is a non-issue), but that means the base class, which provides the method needs to be to the right (since obviously the base class can't call super since it's parent, object doesn't have that method).

    2. I moved it anyway since we appear to be putting them to the right elsehwere, but I think we should fix that and move all mixins to the left of base classes, as otherwise it is just a matter of time until an MRO gets messed up.

  6. s/which/that/, since the intent of that clause is restrictive.

  7. s/an existing tasks/an existing task/

  8. s/are changed target/are changed targets/

  9. End comment with a period (ditto below).

  10. s/butsome/but some/

  11. 
      
PA
  1. Ship It!
  2. 
      
DA
DA
Review request changed

Status: Closed (submitted)

Change Summary:

d09a325e8daf80842e843c096f1a12f64ba18f2c

Loading...