Be more conservative about caching incremental compiles

Review Request #2940 — Created Oct. 7, 2015 and submitted

stuhood
pants
2335
fdf8888...
pants-reviews
areitz, benjyw, ity, nhoward_tw, zundel

We like allowing folks to write to a local buildcache to handle cases where they miss the remote cache for some reason, but unfortunately this can allow for caching of bugs in incremental compilation, which can get pretty sticky. To bias toward correctness, this patch adds an option to control whether incremental compiles (defined as having an analysis file present) are written to the cache.

Internally, we will disable incremental compile caching on laptops, but enable it in CI where we guarantee a clean-all after every source change.

  • Add a flag to be more conservative about caching of incremental compiles
  • Skip the cache write when we've hit the cache during a double check

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

  • 0
  • 0
  • 2
  • 1
  • 3
Description From Last Updated
ZU
  1. Somewhat related question. Does Twitter configure a local buildcache in CI as well as user laptops?

    1. Yes we do. And because of the "clean-all after source changes" we write it to the CI user's home directory, whereas locally we write into the buildroot.

  2. 
      
BE
  1. Ship It!
  2. 
      
NH
  1. Look's good to me aside from what might be a test setup change.

  2. nit: Could you flip the and's order to reflect the comment? I think it'd be a little easier to follow.

    ie

    should_cache = not hit_cache and ...

  3. Since the option defaults to false, do we need to force it true here to maintain the setup for these tests?

    1. Hm, oops. Yes, we do. Imagine I hit "Fixed" on that issue.

    2. Oh, actually: no. The 3rd compile in this test still hits the cache, and still tests the overwrite. It's just that the comments are stale.

    3. Hmm. Could you add a test that checks that artifacts are written if it's enabled then? It looks like that case might not be covered yet.

  4. nit: If you're only testing zinc, you could remove this line.

  5. 
      
AR
  1. Thanks for this, overall LGTM. One question below.

  2. When do you test with this value set to True. I realize there are upstream bugs, but it feels possible to have a simple incremental case in a test that works irrespective of the bugs?

  3. 
      
ST
AR
  1. Thanks for the extra test & debug output.

  2. 
      
NH
  1. Ship It!
  2. 
      
ST
IT
  1. awesome, thanks for doing this!

  2. 
      
ST
ST
Review request changed

Status: Closed (submitted)

Change Summary:

Merged as 6f5688081d0b539b709000121ea73188d18f1658

Loading...