Reland "Adaptresources when using repackaging"
This reverts commit a1b0f3b0d0e4271c7c076c89aa4b3fde45217e2c.
Change-Id: I35eff01f77489e9e572deb3cb505411c3b9360be
diff --git a/src/main/java/com/android/tools/r8/dex/ResourceAdapter.java b/src/main/java/com/android/tools/r8/dex/ResourceAdapter.java
index 84459b4..29987e7 100644
--- a/src/main/java/com/android/tools/r8/dex/ResourceAdapter.java
+++ b/src/main/java/com/android/tools/r8/dex/ResourceAdapter.java
@@ -304,7 +304,8 @@
if (getClassNameSeparator() != '/') {
javaPackage = javaPackage.replace(getClassNameSeparator(), '/');
}
- String minifiedJavaPackage = namingLens.lookupPackageName(javaPackage);
+ String packageName = appView.graphLens().lookupPackageName(javaPackage);
+ String minifiedJavaPackage = namingLens.lookupPackageName(packageName);
if (!javaPackage.equals(minifiedJavaPackage)) {
outputRangeFromInput(outputFrom, from);
outputJavaType(
diff --git a/src/main/java/com/android/tools/r8/graph/GraphLens.java b/src/main/java/com/android/tools/r8/graph/GraphLens.java
index 8f6530e..7135cfc 100644
--- a/src/main/java/com/android/tools/r8/graph/GraphLens.java
+++ b/src/main/java/com/android/tools/r8/graph/GraphLens.java
@@ -363,6 +363,8 @@
return false;
}
+ public abstract String lookupPackageName(String pkg);
+
public abstract DexType lookupClassType(DexType type);
public abstract DexType lookupType(DexType type);
@@ -697,6 +699,11 @@
}
@Override
+ public String lookupPackageName(String pkg) {
+ return pkg;
+ }
+
+ @Override
public final DexType lookupType(DexType type) {
if (type.isPrimitiveType() || type.isVoidType() || type.isNullValueType()) {
return type;
@@ -813,6 +820,11 @@
}
@Override
+ public String lookupPackageName(String pkg) {
+ return pkg;
+ }
+
+ @Override
public DexType lookupType(DexType type) {
return type;
}
diff --git a/src/main/java/com/android/tools/r8/repackaging/Repackaging.java b/src/main/java/com/android/tools/r8/repackaging/Repackaging.java
index cdfa649..056abce 100644
--- a/src/main/java/com/android/tools/r8/repackaging/Repackaging.java
+++ b/src/main/java/com/android/tools/r8/repackaging/Repackaging.java
@@ -33,9 +33,11 @@
import com.google.common.collect.HashBiMap;
import java.util.ArrayList;
import java.util.Collection;
+import java.util.HashMap;
import java.util.HashSet;
import java.util.Iterator;
import java.util.List;
+import java.util.Map;
import java.util.Set;
import java.util.concurrent.ExecutionException;
import java.util.concurrent.ExecutorService;
@@ -130,11 +132,13 @@
}
BiMap<DexType, DexType> mappings = HashBiMap.create();
+ Map<String, String> packageMappings = new HashMap<>();
Set<String> seenPackageDescriptors = new HashSet<>();
ProgramPackageCollection packages =
SortedProgramPackageCollection.createWithAllProgramClasses(appView);
- processPackagesInDesiredLocation(packages, mappings, seenPackageDescriptors);
- processRemainingPackages(packages, mappings, seenPackageDescriptors, executorService);
+ processPackagesInDesiredLocation(packages, mappings, packageMappings, seenPackageDescriptors);
+ processRemainingPackages(
+ packages, mappings, packageMappings, seenPackageDescriptors, executorService);
mappings.entrySet().removeIf(entry -> entry.getKey() == entry.getValue());
if (mappings.isEmpty()) {
return null;
@@ -146,7 +150,7 @@
new ArrayList<>(
repackagingTreeFixer.fixupClasses(appView.appInfo().classesWithDeterministicOrder()));
appBuilder.replaceProgramClasses(newProgramClasses);
- RepackagingLens lens = lensBuilder.build(appView);
+ RepackagingLens lens = lensBuilder.build(appView, packageMappings);
new AnnotationFixer(lens).run(appBuilder.getProgramClasses());
return lens;
}
@@ -191,6 +195,7 @@
private void processPackagesInDesiredLocation(
ProgramPackageCollection packages,
BiMap<DexType, DexType> mappings,
+ Map<String, String> packageMappings,
Set<String> seenPackageDescriptors) {
// For each package that is already in the desired location, record all the classes from the
// package in the mapping for collision detection.
@@ -208,6 +213,7 @@
for (DexProgramClass alreadyRepackagedClass : pkg) {
processClass(alreadyRepackagedClass, pkg, newPackageDescriptor, mappings);
}
+ packageMappings.put(pkg.getPackageDescriptor(), newPackageDescriptor);
seenPackageDescriptors.add(newPackageDescriptor);
iterator.remove();
}
@@ -217,6 +223,7 @@
private void processRemainingPackages(
ProgramPackageCollection packages,
BiMap<DexType, DexType> mappings,
+ Map<String, String> packageMappings,
Set<String> seenPackageDescriptors,
ExecutorService executorService)
throws ExecutionException {
@@ -228,13 +235,20 @@
repackagingConfiguration.getNewPackageDescriptor(pkg, seenPackageDescriptors);
assert !pkg.getPackageDescriptor().equals(newPackageDescriptor);
- Iterable<DexProgramClass> classesToRepackage =
+ Collection<DexProgramClass> classesToRepackage =
computeClassesToRepackage(pkg, executorService);
for (DexProgramClass classToRepackage : classesToRepackage) {
processClass(classToRepackage, pkg, newPackageDescriptor, mappings);
}
-
seenPackageDescriptors.add(newPackageDescriptor);
+ // Package remapping is used for adapting resources. If we cannot repackage all classes in
+ // a package then we put in the original descriptor to ensure that resources are not
+ // rewritten.
+ packageMappings.put(
+ pkg.getPackageDescriptor(),
+ classesToRepackage.size() == pkg.classesInPackage().size()
+ ? newPackageDescriptor
+ : pkg.getPackageDescriptor());
// TODO(b/165783399): Investigate if repackaging can lead to different dynamic dispatch. See,
// for example, CrossPackageInvokeSuperToPackagePrivateMethodTest.
}
@@ -269,12 +283,12 @@
classToRepackage, outerClass, newPackageDescriptor, mappings));
}
- private Iterable<DexProgramClass> computeClassesToRepackage(
+ private Collection<DexProgramClass> computeClassesToRepackage(
ProgramPackage pkg, ExecutorService executorService) throws ExecutionException {
RepackagingConstraintGraph constraintGraph = new RepackagingConstraintGraph(appView, pkg);
boolean canRepackageAllClasses = constraintGraph.initializeGraph();
if (canRepackageAllClasses) {
- return pkg;
+ return pkg.classesInPackage();
}
constraintGraph.populateConstraints(executorService);
return constraintGraph.computeClassesToRepackage();
diff --git a/src/main/java/com/android/tools/r8/repackaging/RepackagingConstraintGraph.java b/src/main/java/com/android/tools/r8/repackaging/RepackagingConstraintGraph.java
index ab5b01b..dd6af4a 100644
--- a/src/main/java/com/android/tools/r8/repackaging/RepackagingConstraintGraph.java
+++ b/src/main/java/com/android/tools/r8/repackaging/RepackagingConstraintGraph.java
@@ -18,6 +18,7 @@
import com.android.tools.r8.utils.WorkList;
import com.google.common.collect.Sets;
import java.util.ArrayList;
+import java.util.Collection;
import java.util.IdentityHashMap;
import java.util.List;
import java.util.Map;
@@ -172,7 +173,7 @@
}
}
- public Iterable<DexProgramClass> computeClassesToRepackage() {
+ public Collection<DexProgramClass> computeClassesToRepackage() {
WorkList<Node> worklist = WorkList.newIdentityWorkList(pinnedNodes);
while (worklist.hasNext()) {
Node pinnedNode = worklist.next();
diff --git a/src/main/java/com/android/tools/r8/repackaging/RepackagingLens.java b/src/main/java/com/android/tools/r8/repackaging/RepackagingLens.java
index 63b8d06..e6d16eb 100644
--- a/src/main/java/com/android/tools/r8/repackaging/RepackagingLens.java
+++ b/src/main/java/com/android/tools/r8/repackaging/RepackagingLens.java
@@ -18,16 +18,19 @@
import com.android.tools.r8.utils.collections.MutableBidirectionalOneToOneMap;
import com.google.common.collect.BiMap;
import com.google.common.collect.HashBiMap;
+import java.util.Map;
public class RepackagingLens extends NestedGraphLens {
private final BiMap<DexType, DexType> originalTypes;
+ private final Map<String, String> packageRenamings;
private RepackagingLens(
AppView<AppInfoWithLiveness> appView,
BidirectionalOneToOneMap<DexField, DexField> newFieldSignatures,
BidirectionalOneToOneMap<DexMethod, DexMethod> originalMethodSignatures,
- BiMap<DexType, DexType> originalTypes) {
+ BiMap<DexType, DexType> originalTypes,
+ Map<String, String> packageRenamings) {
super(
originalTypes.inverse(),
originalMethodSignatures.getInverseOneToOneMap().getForwardMap(),
@@ -36,6 +39,12 @@
appView.graphLens(),
appView.dexItemFactory());
this.originalTypes = originalTypes;
+ this.packageRenamings = packageRenamings;
+ }
+
+ @Override
+ public String lookupPackageName(String pkg) {
+ return packageRenamings.getOrDefault(getPrevious().lookupPackageName(pkg), pkg);
}
@Override
@@ -97,10 +106,11 @@
originalTypes.put(to, from);
}
- public RepackagingLens build(AppView<AppInfoWithLiveness> appView) {
+ public RepackagingLens build(
+ AppView<AppInfoWithLiveness> appView, Map<String, String> packageRenamings) {
assert !originalTypes.isEmpty();
return new RepackagingLens(
- appView, newFieldSignatures, originalMethodSignatures, originalTypes);
+ appView, newFieldSignatures, originalMethodSignatures, originalTypes, packageRenamings);
}
}
}
diff --git a/src/test/java/com/android/tools/r8/naming/AdaptResourceFileContentsTest.java b/src/test/java/com/android/tools/r8/naming/AdaptResourceFileContentsTest.java
index fb89853..3727423 100644
--- a/src/test/java/com/android/tools/r8/naming/AdaptResourceFileContentsTest.java
+++ b/src/test/java/com/android/tools/r8/naming/AdaptResourceFileContentsTest.java
@@ -338,6 +338,7 @@
static class B extends A {
+ @Override
public void method() {
System.out.println("In B.method()");
super.method();
diff --git a/src/test/java/com/android/tools/r8/naming/AdaptResourceFileNamesTest.java b/src/test/java/com/android/tools/r8/naming/AdaptResourceFileNamesTest.java
index 6a01e83..85c8459 100644
--- a/src/test/java/com/android/tools/r8/naming/AdaptResourceFileNamesTest.java
+++ b/src/test/java/com/android/tools/r8/naming/AdaptResourceFileNamesTest.java
@@ -4,6 +4,7 @@
package com.android.tools.r8.naming;
+import static com.android.tools.r8.DiagnosticsMatcher.diagnosticMessage;
import static org.hamcrest.CoreMatchers.containsString;
import static org.hamcrest.MatcherAssert.assertThat;
import static org.junit.Assert.assertEquals;
@@ -11,50 +12,51 @@
import static org.junit.Assert.assertNotNull;
import static org.junit.Assert.assertTrue;
-import com.android.tools.r8.CompilationFailedException;
import com.android.tools.r8.DataDirectoryResource;
import com.android.tools.r8.DataEntryResource;
import com.android.tools.r8.DataResourceConsumer;
import com.android.tools.r8.DataResourceProvider.Visitor;
-import com.android.tools.r8.R8Command;
-import com.android.tools.r8.StringConsumer;
+import com.android.tools.r8.R8FullTestBuilder;
+import com.android.tools.r8.R8TestBuilder;
+import com.android.tools.r8.R8TestCompileResult;
+import com.android.tools.r8.TestParameters;
+import com.android.tools.r8.TestParametersCollection;
+import com.android.tools.r8.ThrowableConsumer;
import com.android.tools.r8.ToolHelper;
import com.android.tools.r8.origin.Origin;
import com.android.tools.r8.shaking.forceproguardcompatibility.ProguardCompatibilityTestBase;
-import com.android.tools.r8.utils.AndroidApp;
import com.android.tools.r8.utils.ArchiveResourceProvider;
import com.android.tools.r8.utils.DataResourceConsumerForTesting;
import com.android.tools.r8.utils.FileUtils;
-import com.android.tools.r8.utils.KeepingDiagnosticHandler;
import com.android.tools.r8.utils.StringUtils;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import java.io.File;
-import java.io.IOException;
import java.nio.file.Path;
import java.nio.file.Paths;
import java.util.HashSet;
import java.util.List;
import java.util.Map;
import java.util.Set;
+import java.util.function.Consumer;
import java.util.stream.Collectors;
-import org.junit.Before;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.Parameterized;
+import org.junit.runners.Parameterized.Parameters;
@RunWith(Parameterized.class)
public class AdaptResourceFileNamesTest extends ProguardCompatibilityTestBase {
- private Backend backend;
+ private final TestParameters parameters;
- @Parameterized.Parameters(name = "Backend: {0}")
- public static Backend[] data() {
- return ToolHelper.getBackends();
+ @Parameters(name = "{0}")
+ public static TestParametersCollection data() {
+ return getTestParameters().withAllRuntimesAndApiLevels().build();
}
- public AdaptResourceFileNamesTest(Backend backend) {
- this.backend = backend;
+ public AdaptResourceFileNamesTest(TestParameters parameters) {
+ this.parameters = parameters;
}
private static final Path CF_DIR =
@@ -63,15 +65,6 @@
Paths.get(ToolHelper.EXAMPLES_BUILD_DIR)
.resolve("adaptresourcefilenames" + FileUtils.JAR_EXTENSION);
- private KeepingDiagnosticHandler diagnosticsHandler;
- private ClassNameMapper mapper = null;
-
- @Before
- public void reset() {
- diagnosticsHandler = new KeepingDiagnosticHandler();
- mapper = null;
- }
-
private static String getProguardConfig(
boolean enableAdaptResourceFileNames, String adaptResourceFileNamesPathFilter) {
String adaptResourceFilenamesRule;
@@ -86,7 +79,6 @@
return String.join(
System.lineSeparator(),
adaptResourceFilenamesRule,
- "-keeppackagenames adaptresourcefilenames**",
"-keep class adaptresourcefilenames.TestClass {",
" public static void main(...);",
"}");
@@ -114,30 +106,38 @@
}
@Test
- public void testEnabled() throws Exception {
+ public void testEnabled() throws Throwable {
DataResourceConsumerForTesting dataResourceConsumer = new DataResourceConsumerForTesting();
compileWithR8(
getProguardConfigWithNeverInline(true, null),
dataResourceConsumer,
- ToolHelper.consumeString(this::checkR8Renamings));
- // Check that the generated resources have the expected names.
- for (DataEntryResource dataResource : getOriginalDataResources()) {
- assertNotNull(
- "Resource not renamed as expected: " + dataResource.getName(),
- dataResourceConsumer.get(getExpectedRenamingFor(dataResource.getName(), mapper)));
- }
+ R8TestBuilder::enableProguardTestOptions,
+ mapper -> {
+ checkR8Renamings(mapper);
+ // Check that the generated resources have the expected names.
+ for (DataEntryResource dataResource : getOriginalDataResources()) {
+ ImmutableList<String> object =
+ dataResourceConsumer.get(getExpectedRenamingFor(dataResource.getName(), mapper));
+ if (object == null) {
+ object =
+ dataResourceConsumer.get(getExpectedRenamingFor(dataResource.getName(), mapper));
+ }
+ assertNotNull("Resource not renamed as expected: " + dataResource.getName(), object);
+ }
+ });
}
@Test
- public void testEnabledWithFilter() throws Exception {
+ public void testEnabledWithFilter() throws Throwable {
DataResourceConsumerForTesting dataResourceConsumer = new DataResourceConsumerForTesting();
compileWithR8(
getProguardConfigWithNeverInline(true, "**.md"),
dataResourceConsumer,
- ToolHelper.consumeString(this::checkR8Renamings));
+ R8TestBuilder::enableProguardTestOptions,
+ this::checkR8Renamings);
// Check that the generated resources have the expected names.
Map<String, String> expectedRenamings =
- ImmutableMap.of("adaptresourcefilenames/B.md", "adaptresourcefilenames/b.md");
+ ImmutableMap.of("adaptresourcefilenames/B.md", "a/b.md");
for (DataEntryResource dataResource : getOriginalDataResources()) {
assertNotNull(
"Resource not renamed as expected: " + dataResource.getName(),
@@ -147,9 +147,12 @@
}
@Test
- public void testDisabled() throws Exception {
+ public void testDisabled() throws Throwable {
DataResourceConsumerForTesting dataResourceConsumer = new DataResourceConsumerForTesting();
- compileWithR8(getProguardConfigWithNeverInline(false, null), dataResourceConsumer);
+ compileWithR8(
+ getProguardConfigWithNeverInline(false, null),
+ dataResourceConsumer,
+ R8TestBuilder::enableProguardTestOptions);
// Check that none of the resources were renamed.
for (DataEntryResource dataResource : getOriginalDataResources()) {
assertNotNull(
@@ -159,22 +162,25 @@
}
@Test
- public void testCollisionBehavior() throws Exception {
+ public void testCollisionBehavior() throws Throwable {
DataResourceConsumerForTesting dataResourceConsumer = new DataResourceConsumerForTesting();
- compileWithR8(
- getProguardConfigWithNeverInline(true, null),
- dataResourceConsumer,
- ToolHelper.consumeString(this::checkR8Renamings),
- ImmutableList.<DataEntryResource>builder()
- .addAll(getOriginalDataResources())
- .add(
- DataEntryResource.fromBytes(
- new byte[0], "adaptresourcefilenames/b.txt", Origin.unknown()))
- .build());
- assertEquals(1, diagnosticsHandler.warnings.size());
- assertThat(
- diagnosticsHandler.warnings.get(0).getDiagnosticMessage(),
- containsString("Resource 'adaptresourcefilenames/b.txt' already exists."));
+ R8TestCompileResult compileResult =
+ compileWithR8(
+ getProguardConfigWithNeverInline(true, null),
+ dataResourceConsumer,
+ builder -> {
+ builder.enableProguardTestOptions();
+ builder.allowDiagnosticWarningMessages();
+ },
+ this::checkR8Renamings,
+ ImmutableList.<DataEntryResource>builder()
+ .addAll(getOriginalDataResources())
+ .add(DataEntryResource.fromBytes(new byte[0], "a/b.txt", Origin.unknown()))
+ .build());
+ compileResult.inspectDiagnosticMessages(
+ diagnosticMessages ->
+ diagnosticMessages.assertWarningsMatch(
+ diagnosticMessage(containsString("Resource 'a/b.txt' already exists."))));
assertEquals(getOriginalDataResources().size(), dataResourceConsumer.size());
}
@@ -243,64 +249,59 @@
.count());
}
- private AndroidApp compileWithR8(String proguardConfig, DataResourceConsumer dataResourceConsumer)
- throws CompilationFailedException, IOException {
- return compileWithR8(proguardConfig, dataResourceConsumer, null);
- }
-
- private AndroidApp compileWithR8(
+ private void compileWithR8(
String proguardConfig,
DataResourceConsumer dataResourceConsumer,
- StringConsumer proguardMapConsumer)
- throws CompilationFailedException, IOException {
- return compileWithR8(
- proguardConfig, dataResourceConsumer, proguardMapConsumer, getOriginalDataResources());
+ ThrowableConsumer<R8FullTestBuilder> builderConsumer)
+ throws Throwable {
+ compileWithR8(proguardConfig, dataResourceConsumer, builderConsumer, null);
}
- private AndroidApp compileWithR8(
+ private void compileWithR8(
String proguardConfig,
DataResourceConsumer dataResourceConsumer,
- StringConsumer proguardMapConsumer,
+ ThrowableConsumer<R8FullTestBuilder> builderConsumer,
+ Consumer<ClassNameMapper> proguardMapConsumer)
+ throws Throwable {
+ compileWithR8(
+ proguardConfig,
+ dataResourceConsumer,
+ builderConsumer,
+ proguardMapConsumer,
+ getOriginalDataResources());
+ }
+
+ private R8TestCompileResult compileWithR8(
+ String proguardConfig,
+ DataResourceConsumer dataResourceConsumer,
+ ThrowableConsumer<R8FullTestBuilder> builderConsumer,
+ Consumer<ClassNameMapper> proguardMapConsumer,
List<DataEntryResource> dataResources)
- throws CompilationFailedException, IOException {
- R8Command command =
- ToolHelper.allowTestProguardOptions(
- ToolHelper.prepareR8CommandBuilder(
- getAndroidApp(dataResources), emptyConsumer(backend), diagnosticsHandler)
- .addProguardConfiguration(ImmutableList.of(proguardConfig), Origin.unknown()))
- .addLibraryFiles(runtimeJar(backend))
- .build();
- return ToolHelper.runR8(
- command,
- options -> {
- options.dataResourceConsumer = dataResourceConsumer;
- options.proguardMapConsumer = proguardMapConsumer;
- });
- }
-
- private void checkR8Renamings(String proguardMap) {
- try {
- // Check that the renamings are as expected. These exact renamings are not important as
- // such, but the test expectations rely on them.
- mapper = ClassNameMapper.mapperFromString(proguardMap);
- assertEquals(
- "adaptresourcefilenames.TestClass",
- mapper.deobfuscateClassName("adaptresourcefilenames.TestClass"));
- assertEquals(
- "adaptresourcefilenames.B", mapper.deobfuscateClassName("adaptresourcefilenames.b"));
- assertEquals(
- "adaptresourcefilenames.B$Inner",
- mapper.deobfuscateClassName("adaptresourcefilenames.a"));
- } catch (IOException e) {
- throw new RuntimeException(e);
+ throws Throwable {
+ R8TestCompileResult compile =
+ testForR8(parameters.getBackend())
+ .addProgramFiles(ToolHelper.getClassFilesForTestDirectory(CF_DIR))
+ .addDataResources(dataResources)
+ .addKeepRules(proguardConfig)
+ .apply(builderConsumer)
+ .setMinApi(parameters.getApiLevel())
+ .addOptionsModification(options -> options.dataResourceConsumer = dataResourceConsumer)
+ .compile();
+ if (proguardMapConsumer != null) {
+ compile.inspectProguardMap(
+ map -> proguardMapConsumer.accept(ClassNameMapper.mapperFromString(map)));
}
+ return compile;
}
- private AndroidApp getAndroidApp(List<DataEntryResource> dataResources) throws IOException {
- AndroidApp.Builder builder = AndroidApp.builder();
- builder.addProgramFiles(ToolHelper.getClassFilesForTestDirectory(CF_DIR));
- dataResources.forEach(builder::addDataResource);
- return builder.build();
+ private void checkR8Renamings(ClassNameMapper mapper) {
+ // Check that the renamings are as expected. These exact renamings are not important as
+ // such, but the test expectations rely on them.
+ assertEquals(
+ "adaptresourcefilenames.TestClass",
+ mapper.deobfuscateClassName("adaptresourcefilenames.TestClass"));
+ assertEquals("adaptresourcefilenames.B", mapper.deobfuscateClassName("a.b"));
+ assertEquals("adaptresourcefilenames.B$Inner", mapper.deobfuscateClassName("a.a"));
}
private static List<DataEntryResource> getOriginalDataResources() {
