Be more conservative about caching incremental compiles

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

Testing Done:
pantsbuild#2335

Bugs closed: 2335

Reviewed at https://rbcommons.com/s/twitter/r/2940/

src/python/pants/backend/jvm/tasks/jvm_compile/jvm_compile.py

@@ -144,6 +144,14 @@ def register_options(cls, register):
              fingerprint=True,
              help='Capture compilation output to per-target logs.')
 
+    # TODO: Defaulting to false due to a few upstream issues for which we haven't pulled down fixes:
+    #  https://github.com/sbt/sbt/pull/2085
+    #  https://github.com/sbt/sbt/pull/2160
+    register('--incremental-caching', advanced=True, action='store_true', default=False,
+             help='When set, the results of incremental compiles will be written to the cache. '
+                  'This is unset by default, because it is generally a good precaution to cache '
+                  'only clean/cold builds.')
+
   @classmethod
   def product_types(cls):
     raise TaskError('Expected to be installed in GroupTask, which has its own '
@@ -686,14 +694,19 @@ def work_for_vts(vts, compile_context, target_closure):
       log_file = self._capture_log_file(compile_context.target)
 
       # Double check the cache before beginning compilation
-      if not check_cache(vts):
+      hit_cache = check_cache(vts)
+      incremental = False
+
+      if not hit_cache:
         # Mutate analysis within a temporary directory, and move it to the final location
         # on success.
         tmpdir = os.path.join(self.analysis_tmpdir, compile_context.target.id)
         safe_mkdir(tmpdir)
         tmp_analysis_file = self._analysis_for_target(
             tmpdir, compile_context.target)
+        # If the analysis exists for this context, it is an incremental compile.
         if os.path.exists(compile_context.analysis_file):
+          incremental = True
           shutil.copy(compile_context.analysis_file, tmp_analysis_file)
         target, = vts.targets
         compile_vts(vts,
@@ -713,8 +726,20 @@ def work_for_vts(vts, compile_context, target_closure):
       # Update the products with the latest classes.
       register_vts([compile_context])
 
-      # Kick off the background artifact cache write.
-      if update_artifact_cache_vts_work:
+      # We write to the cache only if we didn't hit during the double check, and optionally
+      # only for clean builds.
+      is_cacheable = not hit_cache and (self.get_options().incremental_caching or not incremental)
+      self.context.log.debug(
+          'Completed compile for {}. '
+          'Hit cache: {}, was incremental: {}, is cacheable: {}, cache writes enabled: {}.'.format(
+            compile_context.target.address.spec,
+            hit_cache,
+            incremental,
+            is_cacheable,
+            update_artifact_cache_vts_work is not None
+            ))
+      if is_cacheable and update_artifact_cache_vts_work:
+        # Kick off the background artifact cache write.
         self._write_to_artifact_cache(vts, compile_context, update_artifact_cache_vts_work)
 
     jobs = []

tests/python/pants_test/backend/jvm/tasks/jvm_compile/java/BUILD

@@ -31,6 +31,7 @@ python_tests(
   name='cache_compile_integration',
   sources=['test_cache_compile_integration.py'],
   dependencies=[
+    'src/python/pants/backend/jvm/tasks/jvm_compile:zinc',
     'src/python/pants/base:build_environment',
     'src/python/pants/fs',
     'src/python/pants/util:contextutil',

tests/python/pants_test/backend/jvm/tasks/jvm_compile/java/test_cache_compile_integration.py

@@ -8,6 +8,7 @@
 import os
 from textwrap import dedent
 
+from pants.backend.jvm.tasks.jvm_compile.zinc.zinc_compile import ZincCompile
 from pants.base.build_environment import get_buildroot
 from pants.util.contextutil import temporary_dir
 from pants.util.dirutil import safe_open
@@ -20,9 +21,7 @@ def run_compile(self, target_spec, config, workdir, tool_name):
     if tool_name == 'zinc':
       args.append('--no-compile-java-use-jmake')
 
-    pants_run = self.run_pants_with_workdir(
-      args,
-      workdir, config)
+    pants_run = self.run_pants_with_workdir(args, workdir, config)
     self.assert_success(pants_run)
 
   def create_file(self, path, value):
@@ -43,13 +42,17 @@ def _do_test_stale_artifacts_rmd_when_cache_used(self, tool_name):
       config = {
         'cache.compile.{}'.format(tool_name): {'write_to': [cache_dir], 'read_from': [cache_dir]},
         'compile.java': {'use_jmake': tool_name == 'java' },
+        'compile.zinc': {'incremental_caching': True },
       }
 
-      self.create_file(os.path.join(src_dir, 'org', 'pantsbuild', 'cachetest', 'A.java'),
+      srcfile = os.path.join(src_dir, 'org', 'pantsbuild', 'cachetest', 'A.java')
+      buildfile = os.path.join(src_dir, 'org', 'pantsbuild', 'cachetest', 'BUILD')
+
+      self.create_file(srcfile,
                        dedent("""package org.pantsbuild.cachetest;
                           class A {}
                           class Main {}"""))
-      self.create_file(os.path.join(src_dir, 'org', 'pantsbuild', 'cachetest', 'BUILD'),
+      self.create_file(buildfile,
                        dedent("""java_library(name='cachetest',
                                        sources=['A.java']
                           )"""))
@@ -60,14 +63,14 @@ class Main {}"""))
       # Caches values A.class, Main.class
       self.run_compile(cachetest_spec, config, workdir, tool_name)
 
-      self.create_file(os.path.join(src_dir, 'org', 'pantsbuild', 'cachetest', 'A.java'),
+      self.create_file(srcfile,
                        dedent("""package org.pantsbuild.cachetest;
                             class A {}
                             class NotMain {}"""))
       # Caches values A.class, NotMain.class and leaves them on the filesystem
       self.run_compile(cachetest_spec, config, workdir, tool_name)
 
-      self.create_file(os.path.join(src_dir, 'org', 'pantsbuild', 'cachetest', 'A.java'),
+      self.create_file(srcfile,
                        dedent("""package org.pantsbuild.cachetest;
                           class A {}
                           class Main {}"""))
@@ -88,3 +91,42 @@ class Main {}"""))
                                       'cachetest',
                                       )
       self.assertEqual(sorted(os.listdir(class_file_dir)), sorted(['A.class', 'Main.class']))
+
+  def test_incremental_caching(self):
+    """Tests that with --no-incremental-caching, we don't write incremental artifacts."""
+    with temporary_dir() as cache_dir, \
+        temporary_dir(root_dir=self.workdir_root()) as workdir, \
+        temporary_dir(root_dir=get_buildroot()) as src_dir:
+
+      tool_name = 'zinc'
+      def config(incremental_caching):
+        return {
+          'cache.compile.{}'.format(tool_name): {'write_to': [cache_dir], 'read_from': [cache_dir]},
+          'compile.{}'.format(tool_name): {'incremental_caching': incremental_caching},
+        }
+
+      srcfile = os.path.join(src_dir, 'A.java')
+      buildfile = os.path.join(src_dir, 'BUILD')
+      spec = os.path.join(src_dir, ':cachetest')
+      artifact_dir = os.path.join(cache_dir,
+                                  ZincCompile.stable_name(),
+                                  '{}.cachetest'.format(os.path.basename(src_dir)))
+
+      self.create_file(srcfile, """class A {}""")
+      self.create_file(buildfile, """java_library(name='cachetest', sources=['A.java'])""")
+
+
+      # Confirm that the result is one cached artifact.
+      self.run_compile(spec, config(False), workdir, tool_name)
+      clean_artifacts = os.listdir(artifact_dir)
+      self.assertEquals(1, len(clean_artifacts))
+
+      # Modify the file, and confirm that artifacts haven't changed.
+      self.create_file(srcfile, """final class A {}""")
+      self.run_compile(spec, config(False), workdir, tool_name)
+      self.assertEquals(clean_artifacts, os.listdir(artifact_dir))
+
+      # Modify again, this time with incremental and confirm that we have a second artifact.
+      self.create_file(srcfile, """public final class A {}""")
+      self.run_compile(spec, config(True), workdir, tool_name)
+      self.assertEquals(2, len(os.listdir(artifact_dir)))

tests/python/pants_test/backend/jvm/tasks/jvm_compile/java/test_java_compile_integration.py

@@ -177,7 +177,15 @@ def test_java_compile_with_different_resolved_jars_produce_different_artifacts(s
       dotted_path = path_prefix.replace(os.path.sep, '.')
       artifact_dir = os.path.join(cache_dir, ZincCompile.stable_name(),
                                   '{}.jarversionincompatibility'.format(dotted_path))
-      config = {'cache.compile.zinc': {'write_to': [cache_dir], 'read_from': [cache_dir]}}
+      config = {
+          'cache.compile.zinc': {
+            'write_to': [cache_dir],
+            'read_from': [cache_dir],
+          },
+          'compile.zinc': {
+            'incremental_caching': True,
+          },
+      }
 
       pants_run = self.run_pants_with_workdir(['compile.java',
                                                ('{}:only-15-directly'.format(path_prefix))],
@@ -190,7 +198,7 @@ def test_java_compile_with_different_resolved_jars_produce_different_artifacts(s
 
       # Rerun for guava 16
       pants_run = self.run_pants_with_workdir(['compile.java',
-                                               (u'{}:alongside-16'.format(path_prefix)), '-ldebug'],
+                                               (u'{}:alongside-16'.format(path_prefix))],
                                               workdir,
                                               config)
       self.assert_success(pants_run)