Android tools: remove mtime-modifications

The Android tools no longer modify output file
mtimes in hopes of achievening better action
cache hits.

Modifying the mtimes was confusing Bazel and
causing correctness bugs.

Modifying the mtimes is unnecessary because Bazel
is smart about picking up filesystem changes and
observes more signals than just the mtime, though
as the corresponding bug shows it's sadly not
bullet-proof.

Fixes https://github.com/bazelbuild/bazel/issues/4734

Change-Id: I4aa8abf29486841ba8133f927e2816d7f85881fe

Closes #4848.

Change-Id: I0615fae1f20d786771d742705ab4a6ddf7f2306e
PiperOrigin-RevId: 189183742

5 files changed, 45 insertions, 47 deletions

diff --git a/src/test/java/com/google/devtools/build/android/RClassGeneratorActionTest.java b/src/test/java/com/google/devtools/build/android/RClassGeneratorActionTest.java
index e03348cdaa..7e1d8741ca 100644
--- a/src/test/java/com/google/devtools/build/android/RClassGeneratorActionTest.java
+++ b/src/test/java/com/google/devtools/build/android/RClassGeneratorActionTest.java
@@ -23,11 +23,12 @@ import java.io.IOException;
 import java.nio.charset.StandardCharsets;
 import java.nio.file.Files;
 import java.nio.file.Path;
-import java.nio.file.attribute.FileTime;
+import java.time.Instant;
 import java.util.Collections;
 import java.util.List;
 import java.util.zip.ZipEntry;
 import java.util.zip.ZipFile;
+import org.junit.Assert;
 import org.junit.Before;
 import org.junit.Test;
 import org.junit.runner.RunWith;
@@ -123,7 +124,6 @@ public class RClassGeneratorActionTest {
             .toArray(new String[0]));
 
     assertThat(Files.exists(jarPath)).isTrue();
-    assertThat(Files.getLastModifiedTime(jarPath)).isEqualTo(FileTime.fromMillis(0));
 
     try (ZipFile zip = new ZipFile(jarPath.toFile())) {
       List<? extends ZipEntry> zipEntries = Collections.list(zip.entries());
@@ -144,6 +144,7 @@ public class RClassGeneratorActionTest {
               "com/google/app/R$string.class",
               "com/google/app/R.class",
               "META-INF/MANIFEST.MF");
+      ZipMtimeAsserter.assertEntries(zipEntries);
     }
   }
 
@@ -175,7 +176,6 @@ public class RClassGeneratorActionTest {
             .toArray(new String[0]));
 
     assertThat(Files.exists(jarPath)).isTrue();
-    assertThat(Files.getLastModifiedTime(jarPath)).isEqualTo(FileTime.fromMillis(0));
 
     try (ZipFile zip = new ZipFile(jarPath.toFile())) {
       List<? extends ZipEntry> zipEntries = Collections.list(zip.entries());
@@ -190,6 +190,7 @@ public class RClassGeneratorActionTest {
               "com/google/bar/R$drawable.class",
               "com/google/bar/R.class",
               "META-INF/MANIFEST.MF");
+      ZipMtimeAsserter.assertEntries(zipEntries);
     }
   }
 
@@ -219,7 +220,6 @@ public class RClassGeneratorActionTest {
     ).toArray(new String[0]));
 
     assertThat(Files.exists(jarPath)).isTrue();
-    assertThat(Files.getLastModifiedTime(jarPath)).isEqualTo(FileTime.fromMillis(0));
 
     try (ZipFile zip = new ZipFile(jarPath.toFile())) {
       List<? extends ZipEntry> zipEntries = Collections.list(zip.entries());
@@ -233,6 +233,7 @@ public class RClassGeneratorActionTest {
               "com/google/app/R$string.class",
               "com/google/app/R.class",
               "META-INF/MANIFEST.MF");
+      ZipMtimeAsserter.assertEntries(zipEntries);
     }
   }
 
@@ -244,12 +245,12 @@ public class RClassGeneratorActionTest {
     ).toArray(new String[0]));
 
     assertThat(Files.exists(jarPath)).isTrue();
-    assertThat(Files.getLastModifiedTime(jarPath)).isEqualTo(FileTime.fromMillis(0));
 
     try (ZipFile zip = new ZipFile(jarPath.toFile())) {
       List<? extends ZipEntry> zipEntries = Collections.list(zip.entries());
       Iterable<String> entries = getZipFilenames(zipEntries);
       assertThat(entries).containsExactly("META-INF/MANIFEST.MF");
+      ZipMtimeAsserter.assertEntries(zipEntries);
     }
   }
 
@@ -285,7 +286,6 @@ public class RClassGeneratorActionTest {
             .toArray(new String[0]));
 
     assertThat(Files.exists(jarPath)).isTrue();
-    assertThat(Files.getLastModifiedTime(jarPath)).isEqualTo(FileTime.fromMillis(0));
 
     try (ZipFile zip = new ZipFile(jarPath.toFile())) {
       List<? extends ZipEntry> zipEntries = Collections.list(zip.entries());
@@ -299,6 +299,7 @@ public class RClassGeneratorActionTest {
               "com/custom/er/R$string.class",
               "com/custom/er/R.class",
               "META-INF/MANIFEST.MF");
+      ZipMtimeAsserter.assertEntries(zipEntries);
     }
   }
 
@@ -323,12 +324,12 @@ public class RClassGeneratorActionTest {
             .toArray(new String[0]));
 
     assertThat(Files.exists(jarPath)).isTrue();
-    assertThat(Files.getLastModifiedTime(jarPath)).isEqualTo(FileTime.fromMillis(0));
 
     try (ZipFile zip = new ZipFile(jarPath.toFile())) {
       List<? extends ZipEntry> zipEntries = Collections.list(zip.entries());
       Iterable<String> entries = getZipFilenames(zipEntries);
       assertThat(entries).containsExactly("META-INF/MANIFEST.MF");
+      ZipMtimeAsserter.assertEntries(zipEntries);
     }
   }
 
@@ -349,4 +350,36 @@ public class RClassGeneratorActionTest {
           }
         });
   }
+
+  private static final class ZipMtimeAsserter {
+    private static final long ZIP_EPOCH = Instant.parse("1980-01-01T00:00:00Z").getEpochSecond();
+    private static final long ZIP_EPOCH_PLUS_ONE_DAY =
+        Instant.parse("1980-01-02T00:00:00Z").getEpochSecond();
+
+    public static void assertEntry(ZipEntry e) {
+      // getLastModifiedTime().toMillis() returns milliseconds, Instant.getEpochSecond() returns
+      // seconds.
+      long mtime = e.getLastModifiedTime().toMillis() / 1000;
+      // The ZIP epoch is the same as the MS-DOS epoch, 1980-01-01T00:00:00Z.
+      // AndroidResourceOutputs.ZipBuilder sets this to most of its entries, except for .class files
+      // for which the ZipBuilder increments the timestamp by 2 seconds.
+      // We don't care about the details of this logic and asserting exact timestamps would couple
+      // the test to the code too tightly, so here we only assert that the timestamp is on
+      // 1980-01-01, ignoring the exact time.
+      // AndroidResourceOutputs.ZipBuilde sets the ZIP epoch (same as the MS-DOS epoch,
+      // 1980-01-01T00:00:00Z) as the timestamp for all of its entries (except .class files, for
+      // which it sets a timestamp 2 seconds later than the DOS epoch).
+      // We don't care about the exact timestamps though, only that they are stable, so let's just
+      // assert that they are all on the day of 1980-01-01.
+      if (mtime < ZIP_EPOCH || mtime > ZIP_EPOCH_PLUS_ONE_DAY) {
+        Assert.fail(String.format("e=(%s) mtime=(%s)", e.getName(), e.getLastModifiedTime()));
+      }
+    }
+
+    public static void assertEntries(Iterable<? extends ZipEntry> entries) throws Exception {
+      for (ZipEntry e : entries) {
+        assertEntry(e);
+      }
+    }
+  }
 }
diff --git a/src/tools/android/java/com/google/devtools/build/android/AndroidResourceOutputs.java b/src/tools/android/java/com/google/devtools/build/android/AndroidResourceOutputs.java
index a4e9344c91..b277e010e2 100644
--- a/src/tools/android/java/com/google/devtools/build/android/AndroidResourceOutputs.java
+++ b/src/tools/android/java/com/google/devtools/build/android/AndroidResourceOutputs.java
@@ -30,8 +30,8 @@ import java.nio.file.Files;
 import java.nio.file.Path;
 import java.nio.file.SimpleFileVisitor;
 import java.nio.file.attribute.BasicFileAttributes;
-import java.nio.file.attribute.FileTime;
 import java.util.ArrayList;
+import java.util.Calendar;
 import java.util.Collection;
 import java.util.GregorianCalendar;
 import java.util.List;
@@ -56,7 +56,7 @@ public class AndroidResourceOutputs {
 
     // The earliest date representable in a zip file, 1-1-1980 (the DOS epoch).
     private static final long ZIP_EPOCH =
-        new GregorianCalendar(1980, 01, 01, 0, 0).getTimeInMillis();
+        new GregorianCalendar(1980, Calendar.JANUARY, 01, 0, 0).getTimeInMillis();
 
     private final ZipOutputStream zip;
 
@@ -299,8 +299,6 @@ public class AndroidResourceOutputs {
     try {
       Files.createDirectories(manifestOut.getParent());
       Files.copy(provider.getManifest(), manifestOut);
-      // Set to the epoch for caching purposes.
-      Files.setLastModifiedTime(manifestOut, FileTime.fromMillis(0L));
     } catch (IOException e) {
       throw new RuntimeException(e);
     }
@@ -332,8 +330,6 @@ public class AndroidResourceOutputs {
         // outputs. This state occurs when there are no resource directories.
         Files.createFile(rOutput);
       }
-      // Set to the epoch for caching purposes.
-      Files.setLastModifiedTime(rOutput, FileTime.fromMillis(0L));
     } catch (IOException e) {
       throw new RuntimeException(e);
     }
@@ -349,8 +345,6 @@ public class AndroidResourceOutputs {
         visitor.writeEntries();
         visitor.writeManifestContent();
       }
-      // Set to the epoch for caching purposes.
-      Files.setLastModifiedTime(classJar, FileTime.fromMillis(0L));
     } catch (IOException e) {
       throw new RuntimeException(e);
     }
@@ -366,8 +360,6 @@ public class AndroidResourceOutputs {
         Files.walkFileTree(root, visitor);
         visitor.writeEntries();
       }
-      // Set to the epoch for caching purposes.
-      Files.setLastModifiedTime(archive, FileTime.fromMillis(0L));
     } catch (IOException e) {
       throw new RuntimeException(e);
     }
@@ -383,8 +375,6 @@ public class AndroidResourceOutputs {
         Files.walkFileTree(generatedSourcesRoot, visitor);
         visitor.writeEntries();
       }
-      // Set to the epoch for caching purposes.
-      Files.setLastModifiedTime(srcJar, FileTime.fromMillis(0L));
     } catch (IOException e) {
       throw new RuntimeException(e);
     }
diff --git a/src/tools/android/java/com/google/devtools/build/android/AndroidResourceProcessor.java b/src/tools/android/java/com/google/devtools/build/android/AndroidResourceProcessor.java
index 2ff8330046..a72d1d243a 100644
--- a/src/tools/android/java/com/google/devtools/build/android/AndroidResourceProcessor.java
+++ b/src/tools/android/java/com/google/devtools/build/android/AndroidResourceProcessor.java
@@ -57,7 +57,6 @@ import java.nio.charset.StandardCharsets;
 import java.nio.file.DirectoryStream;
 import java.nio.file.Files;
 import java.nio.file.Path;
-import java.nio.file.attribute.FileTime;
 import java.util.ArrayList;
 import java.util.Collection;
 import java.util.List;
@@ -343,24 +342,11 @@ public class AndroidResourceProcessor {
           dependencyData, customPackageForR, androidManifest, sourceOut);
     }
     // Reset the output date stamps.
-    if (proguardOut != null) {
-      Files.setLastModifiedTime(proguardOut, FileTime.fromMillis(0L));
-    }
-    if (mainDexProguardOut != null) {
-      Files.setLastModifiedTime(mainDexProguardOut, FileTime.fromMillis(0L));
-    }
     if (packageOut != null) {
-      Files.setLastModifiedTime(packageOut, FileTime.fromMillis(0L));
       if (!splits.isEmpty()) {
-        Iterable<Path> splitFilenames = findAndRenameSplitPackages(packageOut, splits);
-        for (Path splitFilename : splitFilenames) {
-          Files.setLastModifiedTime(splitFilename, FileTime.fromMillis(0L));
-        }
+        renameSplitPackages(packageOut, splits);
       }
     }
-    if (publicResourcesOut != null && Files.exists(publicResourcesOut)) {
-      Files.setLastModifiedTime(publicResourcesOut, FileTime.fromMillis(0L));
-    }
     return new MergedAndroidData(resourceDir, assetsDir, androidManifest);
   }
 
@@ -623,8 +609,8 @@ public class AndroidResourceProcessor {
     }
   }
 
-  /** Finds aapt's split outputs and renames them according to the input flags. */
-  private Iterable<Path> findAndRenameSplitPackages(Path packageOut, Iterable<String> splits)
+  /** Renames aapt's split outputs according to the input flags. */
+  private void renameSplitPackages(Path packageOut, Iterable<String> splits)
       throws UnrecognizedSplitsException, IOException {
     String prefix = packageOut.getFileName().toString() + "_";
     // The regex java string literal below is received as [\\{}\[\]*?] by the regex engine,
@@ -641,16 +627,13 @@ public class AndroidResourceProcessor {
     }
     Map<String, String> outputs =
         SplitConfigurationFilter.mapFilenamesToSplitFlags(filenameSuffixes.build(), splits);
-    ImmutableList.Builder<Path> outputPaths = new ImmutableList.Builder<>();
     for (Map.Entry<String, String> splitMapping : outputs.entrySet()) {
       Path resultPath = packageOut.resolveSibling(prefix + splitMapping.getValue());
-      outputPaths.add(resultPath);
       if (!splitMapping.getKey().equals(splitMapping.getValue())) {
         Path sourcePath = packageOut.resolveSibling(prefix + splitMapping.getKey());
         Files.move(sourcePath, resultPath);
       }
     }
-    return outputPaths.build();
   }
 
   /** A logger that will print messages to a target OutputStream. */
diff --git a/src/tools/android/java/com/google/devtools/build/android/ManifestMergerAction.java b/src/tools/android/java/com/google/devtools/build/android/ManifestMergerAction.java
index a7301f1030..3fb09d2dff 100644
--- a/src/tools/android/java/com/google/devtools/build/android/ManifestMergerAction.java
+++ b/src/tools/android/java/com/google/devtools/build/android/ManifestMergerAction.java
@@ -34,7 +34,6 @@ import java.nio.file.FileSystems;
 import java.nio.file.Files;
 import java.nio.file.Path;
 import java.nio.file.StandardCopyOption;
-import java.nio.file.attribute.FileTime;
 import java.util.Map;
 import java.util.Map.Entry;
 import java.util.logging.Logger;
@@ -217,9 +216,6 @@ public class ManifestMergerAction {
       if (!mergedManifest.equals(options.manifestOutput)) {
         Files.copy(options.manifest, options.manifestOutput, StandardCopyOption.REPLACE_EXISTING);
       }
-
-      // Set to the epoch for caching purposes.
-      Files.setLastModifiedTime(options.manifestOutput, FileTime.fromMillis(0L));
     } catch (AndroidManifestProcessor.ManifestProcessingException e) {
       System.exit(1);
     } catch (Exception e) {
diff --git a/src/tools/android/java/com/google/devtools/build/android/dexer/DexBuilder.java b/src/tools/android/java/com/google/devtools/build/android/dexer/DexBuilder.java
index eadc42532f..ceb41e6ff2 100644
--- a/src/tools/android/java/com/google/devtools/build/android/dexer/DexBuilder.java
+++ b/src/tools/android/java/com/google/devtools/build/android/dexer/DexBuilder.java
@@ -138,8 +138,6 @@ class DexBuilder {
         executor.shutdown();
       }
     }
-    // Use input's timestamp for output file so the output file is stable.
-    Files.setLastModifiedTime(options.outputZip, Files.getLastModifiedTime(options.inputJar));
   }
 
   /**
@@ -225,8 +223,6 @@ class DexBuilder {
           new Dexing(context, optionsParser.getOptions(DexingOptions.class)),
           dexCache);
     }
-    // Use input's timestamp for output file so the output file is stable.
-    Files.setLastModifiedTime(options.outputZip, Files.getLastModifiedTime(options.inputJar));
   }
 
   private static ZipOutputStream createZipOutputStream(Path path) throws IOException {