Make SplitZipStep use proguard mapping on builds with obfuscation.
Summary:
Previously, we had some hacks in place to make zip splitting work
properly on builds with obfuscated class names, but they weren't
scalable. Now the split-zip step pulls in the proguard class name
mapping and uses that to apply primary dex policies more accurately.
diff --git a/src/com/facebook/buck/android/AndroidBinaryRule.java b/src/com/facebook/buck/android/AndroidBinaryRule.java
index 74d9fad..e802db5 100644
--- a/src/com/facebook/buck/android/AndroidBinaryRule.java
+++ b/src/com/facebook/buck/android/AndroidBinaryRule.java
@@ -1041,6 +1041,11 @@
final Optional<String> secondaryInputsDir;
if (shouldSplitDex()) {
+ Optional<Path> proguardMappingFile = Optional.absent();
+ if (packageType.isBuildWithObfuscation()) {
+ proguardMappingFile = Optional.of(getPathForProGuardDirectory().resolve("mapping.txt"));
+ }
+
// DexLibLoader expects that metadata.txt and secondary jar files are under this dir
// in assets.
String magicSecondaryDexSubdir = "assets/secondary-program-dex-jars";
@@ -1073,6 +1078,7 @@
primaryJarPath,
secondaryZipDir,
"secondary-%d.jar",
+ proguardMappingFile,
primaryDexSubstrings,
primaryDexClassesFile.transform(sourcePathResolver),
dexSplitMode.getDexSplitStrategy(),
diff --git a/src/com/facebook/buck/android/ProguardMapping.java b/src/com/facebook/buck/android/ProguardMapping.java
new file mode 100644
index 0000000..2bdc6c0
--- /dev/null
+++ b/src/com/facebook/buck/android/ProguardMapping.java
@@ -0,0 +1,50 @@
+/*
+ * Copyright 2012-present Facebook, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License"); you may
+ * not use this file except in compliance with the License. You may obtain
+ * a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ */
+
+package com.facebook.buck.android;
+
+import com.google.common.collect.ImmutableMap;
+
+import java.util.Map;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
+
+/**
+ * Parser for ProGuard-generated mapping files. Currently only handles class mapping.
+ */
+public class ProguardMapping {
+ private static Pattern CLASS_LINE_PATTERN = Pattern.compile("([\\w.$]+) -> ([\\w.$]+):");
+
+ public static Map<String, String> readClassMapping(Iterable<String> lines) {
+ ImmutableMap.Builder<String, String> classMappingBuilder = ImmutableMap.builder();
+
+ for (String line : lines) {
+ if (line.charAt(0) == ' ') {
+ // This is a member mapping, which we don't handle yet.
+ continue;
+ }
+
+ Matcher matcher = CLASS_LINE_PATTERN.matcher(line);
+ if (!matcher.matches()) {
+ throw new IllegalArgumentException("Invalid line in proguard mapping: " + line);
+ }
+
+ classMappingBuilder.put(matcher.group(1), matcher.group(2));
+ }
+
+ return classMappingBuilder.build();
+ }
+}
diff --git a/src/com/facebook/buck/android/SplitZipStep.java b/src/com/facebook/buck/android/SplitZipStep.java
index d3b85d1..637b77b 100644
--- a/src/com/facebook/buck/android/SplitZipStep.java
+++ b/src/com/facebook/buck/android/SplitZipStep.java
@@ -26,11 +26,13 @@
import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Charsets;
import com.google.common.base.Function;
+import com.google.common.base.Functions;
import com.google.common.base.Joiner;
import com.google.common.base.Optional;
import com.google.common.base.Preconditions;
import com.google.common.base.Predicate;
import com.google.common.collect.FluentIterable;
+import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
import com.google.common.hash.Hashing;
import com.google.common.io.Files;
@@ -41,12 +43,15 @@
import java.nio.file.Path;
import java.util.Collection;
import java.util.Collections;
+import java.util.Map;
import java.util.Set;
import java.util.regex.Matcher;
import java.util.regex.Pattern;
import java.util.zip.ZipEntry;
import java.util.zip.ZipFile;
+import javax.annotation.Nullable;
+
/**
* Split zipping tool designed to divide input code blobs into a set of output jar files such that
* none will exceed the DexOpt LinearAlloc limit or the dx method limit when passed through
@@ -71,6 +76,7 @@
private final String primaryJarPath;
private final String secondaryJarDir;
private final String secondaryJarPattern;
+ private final Optional<Path> proguardMappingFile;
private final ImmutableSet<String> primaryDexSubstrings;
private final Optional<Path> primaryDexClassesFile;
private final ZipSplitter.DexSplitStrategy dexSplitStrategy;
@@ -101,6 +107,7 @@
String primaryJarPath,
String secondaryJarDir,
String secondaryJarPattern,
+ Optional<Path> proguardMappingFile,
Set<String> primaryDexSubstrings,
Optional<Path> primaryDexClassesFile,
ZipSplitter.DexSplitStrategy dexSplitStrategy,
@@ -113,6 +120,7 @@
this.primaryJarPath = Preconditions.checkNotNull(primaryJarPath);
this.secondaryJarDir = Preconditions.checkNotNull(secondaryJarDir);
this.secondaryJarPattern = Preconditions.checkNotNull(secondaryJarPattern);
+ this.proguardMappingFile = Preconditions.checkNotNull(proguardMappingFile);
this.primaryDexSubstrings = ImmutableSet.copyOf(primaryDexSubstrings);
this.primaryDexClassesFile = Preconditions.checkNotNull(primaryDexClassesFile);
this.dexSplitStrategy = Preconditions.checkNotNull(dexSplitStrategy);
@@ -168,6 +176,7 @@
@VisibleForTesting
Predicate<String> createRequiredInPrimaryZipPredicate(ExecutionContext context)
throws IOException {
+ final Function<String, String> deobfuscate = createProguardDeobfuscator(context);
final ImmutableSet<String> primaryDexClassNames = getPrimaryDexClassNames(context);
return new Predicate<String>() {
@@ -181,12 +190,16 @@
return true;
}
- if (primaryDexClassNames.contains(classFileName)) {
+ // Drop the ".class" suffix and deobfuscate the class name before we apply our checks.
+ String internalClassName = Preconditions.checkNotNull(
+ deobfuscate.apply(classFileName.replaceAll("\\.class$", "")));
+
+ if (primaryDexClassNames.contains(internalClassName)) {
return true;
}
for (String substr : SplitZipStep.this.primaryDexSubstrings) {
- if (classFileName.contains(substr)) {
+ if (internalClassName.contains(substr)) {
return true;
}
}
@@ -195,6 +208,10 @@
};
}
+ /**
+ * Construct a {@link Set} of internal class names that should go into the primary dex.
+ * @return the Set.
+ */
private ImmutableSet<String> getPrimaryDexClassNames(ExecutionContext context)
throws IOException {
if (!primaryDexClassesFile.isPresent()) {
@@ -218,16 +235,40 @@
return !line.isEmpty() && !(line.charAt(0) == '#');
}
})
- .transform(new Function<String, String>() {
- @Override
- public String apply(String line) {
- // Append a ".class" so the input file can just contain the internal class name.
- return line + ".class";
- }
- })
.toSet();
}
+ /**
+ * Create a {@link Function} that will deobfuscate internal class names for this build.
+ * @return the Function.
+ */
+ private Function<String, String> createProguardDeobfuscator(ExecutionContext context)
+ throws IOException {
+ if (!proguardMappingFile.isPresent()) {
+ return Functions.identity();
+ }
+
+ Map<String, String> rawProguardMap = ProguardMapping.readClassMapping(
+ context.getProjectFilesystem().readLines(proguardMappingFile.get()));
+
+ ImmutableMap.Builder<String, String> internalNameBuilder = ImmutableMap.builder();
+ for (Map.Entry<String, String> entry : rawProguardMap.entrySet()) {
+ internalNameBuilder.put(
+ entry.getValue().replace('.', '/'),
+ entry.getKey().replace('.', '/'));
+ }
+ final Map<String, String> deobfuscator = internalNameBuilder.build();
+
+ return new Function<String, String>() {
+ @Nullable
+ @Override
+ public String apply(@Nullable String input) {
+ return deobfuscator.get(input);
+ }
+ };
+
+ }
+
@VisibleForTesting
static void writeMetaList(
BufferedWriter writer,
diff --git a/test/com/facebook/buck/android/ProguardMappingTest.java b/test/com/facebook/buck/android/ProguardMappingTest.java
new file mode 100644
index 0000000..4bf900d
--- /dev/null
+++ b/test/com/facebook/buck/android/ProguardMappingTest.java
@@ -0,0 +1,48 @@
+/*
+ * Copyright 2012-present Facebook, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License"); you may
+ * not use this file except in compliance with the License. You may obtain
+ * a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ */
+
+package com.facebook.buck.android;
+
+import static org.junit.Assert.assertEquals;
+
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMap;
+
+import org.junit.Test;
+
+import java.util.Map;
+
+public class ProguardMappingTest {
+ @Test
+ public void testBasicParse() {
+ Map<String, String> mapping = ProguardMapping.readClassMapping(ImmutableList.of(
+ "foo.bar.Baz -> foo.bar.a:",
+ " member -> x",
+ "foo.bar.Baz$Qux -> foo.bar.Baz$Qux:"
+ ));
+ assertEquals(mapping, ImmutableMap.of(
+ "foo.bar.Baz", "foo.bar.a",
+ "foo.bar.Baz$Qux", "foo.bar.Baz$Qux"
+ ));
+ }
+
+ @Test(expected = IllegalArgumentException.class)
+ public void testInternalNameError() {
+ ProguardMapping.readClassMapping(ImmutableList.of(
+ "foo/bar/Baz -> foo/bar/a:"
+ ));
+ }
+}
diff --git a/test/com/facebook/buck/android/SplitZipStepTest.java b/test/com/facebook/buck/android/SplitZipStepTest.java
index bc3dd00..13a5b9c 100644
--- a/test/com/facebook/buck/android/SplitZipStepTest.java
+++ b/test/com/facebook/buck/android/SplitZipStepTest.java
@@ -101,6 +101,7 @@
/* primaryJarPath */ "",
/* secondaryJarDir */ "",
/* secondaryJarPattern */ "",
+ /* proguardMappingFile */ Optional.<Path>absent(),
/* primaryDexSubstrings */ ImmutableSet.of("List"),
Optional.of(primaryDexClassesFile),
ZipSplitter.DexSplitStrategy.MAXIMIZE_PRIMARY_DEX_SIZE,
@@ -152,6 +153,79 @@
}
@Test
+ public void testRequiredInPrimaryZipPredicateWithProguard() throws IOException {
+ Path proguardMappingFile = Paths.get("the/mapping.txt");
+ Path primaryDexClassesFile = Paths.get("the/manifest.txt");
+ SplitZipStep splitZipStep = new SplitZipStep(
+ /* inputPathsToSplit */ ImmutableSet.<String>of(),
+ /* secondaryJarMetaPath */ "",
+ /* primaryJarPath */ "",
+ /* secondaryJarDir */ "",
+ /* secondaryJarPattern */ "",
+ /* proguardMappingFile */ Optional.of(proguardMappingFile),
+ /* primaryDexSubstrings */ ImmutableSet.of("/primary/", "x/"),
+ Optional.of(primaryDexClassesFile),
+ ZipSplitter.DexSplitStrategy.MAXIMIZE_PRIMARY_DEX_SIZE,
+ DexStore.JAR,
+ /* pathToReportDir */ "",
+ /* useLinearAllocSplitDex */ true,
+ /* linearAllocHardLimit */ 4 * 1024 * 1024);
+ List<String> linesInMappingFile = ImmutableList.of(
+ "foo.bar.MappedPrimary -> foo.bar.a:",
+ "foo.bar.MappedSecondary -> foo.bar.b:",
+ "foo.bar.UnmappedPrimary -> foo.bar.UnmappedPrimary:",
+ "foo.bar.UnmappedSecondary -> foo.bar.UnmappedSecondary:",
+ "foo.primary.MappedPackage -> x.a:",
+ "foo.secondary.MappedPackage -> x.b:",
+ "foo.primary.UnmappedPackage -> foo.primary.UnmappedPackage:"
+ );
+ List<String> linesInManifestFile = ImmutableList.of(
+ // Actual primary dex classes.
+ "foo/bar/MappedPrimary",
+ "foo/bar/UnmappedPrimary",
+ // Red herrings!
+ "foo/bar/b",
+ "x/b"
+ );
+
+ ProjectFilesystem projectFilesystem = EasyMock.createMock(ProjectFilesystem.class);
+ EasyMock.expect(projectFilesystem.readLines(primaryDexClassesFile))
+ .andReturn(linesInManifestFile);
+ EasyMock.expect(projectFilesystem.readLines(proguardMappingFile))
+ .andReturn(linesInMappingFile);
+ ExecutionContext context = EasyMock.createMock(ExecutionContext.class);
+ EasyMock.expect(context.getProjectFilesystem()).andReturn(projectFilesystem).anyTimes();
+ EasyMock.replay(projectFilesystem, context);
+
+ Predicate<String> requiredInPrimaryZipPredicate = splitZipStep
+ .createRequiredInPrimaryZipPredicate(context);
+ assertTrue(
+ "Mapped class from primary list should be in primary.",
+ requiredInPrimaryZipPredicate.apply("foo/bar/a.class"));
+ assertTrue(
+ "Unmapped class from primary list should be in primary.",
+ requiredInPrimaryZipPredicate.apply("foo/bar/UnmappedPrimary.class"));
+ assertTrue(
+ "Mapped class from substring should be in primary.",
+ requiredInPrimaryZipPredicate.apply("x/a.class"));
+ assertTrue(
+ "Unmapped class from substring should be in primary.",
+ requiredInPrimaryZipPredicate.apply("foo/primary/UnmappedPackage.class"));
+ assertFalse(
+ "Mapped class with obfuscated name match should not be in primary.",
+ requiredInPrimaryZipPredicate.apply("foo/bar/b.class"));
+ assertFalse(
+ "Unmapped class name should not randomly be in primary.",
+ requiredInPrimaryZipPredicate.apply("foo/bar/UnmappedSecondary.class"));
+ assertFalse(
+ "Map class with obfuscated name substring should not be in primary.",
+ requiredInPrimaryZipPredicate.apply("x/b.class"));
+
+ EasyMock.verify(projectFilesystem, context);
+ }
+
+
+ @Test
public void testClassFilePattern() {
assertTrue(SplitZipStep.classFilePattern.matcher(
"com/facebook/orca/threads/ParticipantInfo.class").matches());
