Skip to content

Commit

Permalink
Android tools: remove mtime-modifications
Browse files Browse the repository at this point in the history 
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 #4734

Change-Id: I4aa8abf29486841ba8133f927e2816d7f85881fe

Closes #4848.

Change-Id: I0615fae1f20d786771d742705ab4a6ddf7f2306e
PiperOrigin-RevId: 189183742
  • Loading branch information
@laszlocsomor
laszlocsomor authored and Copybara-Service committed Mar 15, 2018
1 parent 8fb66b3 commit 4c3098c
Show file tree
Hide file tree
Showing 5 changed files with 45 additions and 47 deletions.
47 changes: 40 additions & 7 deletions src/test/java/com/google/devtools/build/android/RClassGeneratorActionTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,11 +23,12 @@
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;
Expand Down Expand Up @@ -123,7 +124,6 @@ public void withBinaryAndLibraries() throws Exception {
.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());
Expand All @@ -144,6 +144,7 @@ public void withBinaryAndLibraries() throws Exception {
"com/google/app/R$string.class",
"com/google/app/R.class",
"META-INF/MANIFEST.MF");
ZipMtimeAsserter.assertEntries(zipEntries);
}
}

Expand Down Expand Up @@ -175,7 +176,6 @@ public void withNoBinaryAndLibraries() throws Exception {
.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());
Expand All @@ -190,6 +190,7 @@ public void withNoBinaryAndLibraries() throws Exception {
"com/google/bar/R$drawable.class",
"com/google/bar/R.class",
"META-INF/MANIFEST.MF");
ZipMtimeAsserter.assertEntries(zipEntries);
}
}

Expand Down Expand Up @@ -219,7 +220,6 @@ public void withBinaryNoLibraries() throws Exception {
).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());
Expand All @@ -233,6 +233,7 @@ public void withBinaryNoLibraries() throws Exception {
"com/google/app/R$string.class",
"com/google/app/R.class",
"META-INF/MANIFEST.MF");
ZipMtimeAsserter.assertEntries(zipEntries);
}
}

Expand All @@ -244,12 +245,12 @@ public void noBinary() throws Exception {
).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);
}
}

Expand Down Expand Up @@ -285,7 +286,6 @@ public void customPackageForR() throws Exception {
.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());
Expand All @@ -299,6 +299,7 @@ public void customPackageForR() throws Exception {
"com/custom/er/R$string.class",
"com/custom/er/R.class",
"META-INF/MANIFEST.MF");
ZipMtimeAsserter.assertEntries(zipEntries);
}
}

Expand All @@ -323,12 +324,12 @@ public void noSymbolsNoRClass() throws Exception {
.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);
}
}

Expand All @@ -349,4 +350,36 @@ public String apply(ZipEntry input) {
}
});
}

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);
}
}
}
}
14 changes: 2 additions & 12 deletions src/tools/android/java/com/google/devtools/build/android/AndroidResourceOutputs.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,8 @@
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;
Expand All @@ -56,7 +56,7 @@ static class ZipBuilder implements Closeable {

// 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;

Expand Down Expand Up @@ -299,8 +299,6 @@ public static void copyManifestToOutput(ManifestContainer provider, Path manifes
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);
}
Expand Down Expand Up @@ -332,8 +330,6 @@ public static void copyRToOutput(Path generatedSourceRoot, Path rOutput, boolean
// 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);
}
Expand All @@ -349,8 +345,6 @@ public static void createClassJar(Path generatedClassesRoot, Path classJar) {
visitor.writeEntries();
visitor.writeManifestContent();
}
// Set to the epoch for caching purposes.
Files.setLastModifiedTime(classJar, FileTime.fromMillis(0L));
} catch (IOException e) {
throw new RuntimeException(e);
}
Expand All @@ -366,8 +360,6 @@ public static void archiveDirectory(Path root, Path archive) {
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);
}
Expand All @@ -383,8 +375,6 @@ public static void createSrcJar(Path generatedSourcesRoot, Path srcJar, boolean
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);
}
Expand Down
23 changes: 3 additions & 20 deletions src/tools/android/java/com/google/devtools/build/android/AndroidResourceProcessor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,6 @@
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;
Expand Down Expand Up @@ -343,24 +342,11 @@ public MergedAndroidData processResources(
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);
}

Expand Down Expand Up @@ -623,8 +609,8 @@ void writeDependencyPackageRJavaFiles(
}
}

/** 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,
Expand All @@ -641,16 +627,13 @@ private Iterable<Path> findAndRenameSplitPackages(Path packageOut, Iterable<Stri
}
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. */
Expand Down
4 changes: 0 additions & 4 deletions src/tools/android/java/com/google/devtools/build/android/ManifestMergerAction.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,6 @@
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;
Expand Down Expand Up @@ -217,9 +216,6 @@ public static void main(String[] args) throws Exception {
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) {
Expand Down
4 changes: 0 additions & 4 deletions src/tools/android/java/com/google/devtools/build/android/dexer/DexBuilder.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -138,8 +138,6 @@ static void buildDexArchive(Options options, Dexing dexing) throws Exception {
executor.shutdown();
}
}
// Use input's timestamp for output file so the output file is stable.
Files.setLastModifiedTime(options.outputZip, Files.getLastModifiedTime(options.inputJar));
}

/**
Expand Down Expand Up @@ -225,8 +223,6 @@ private static void processRequest(
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 {
Expand Down

0 comments on commit 4c3098c

Please sign in to comment.