Skip to content

Commit

Permalink
Expand macros in redex_extra_args
Browse files Browse the repository at this point in the history 
Summary:
redex_extra_args could contain macros like $(location ...).
The existing redex support from buck does not expand those macros in redex_extra_args, but simply passes them through to redex.
This diff addresses this issue.

Test Plan: Please see the updated AndroidBinaryIntegrationTest.

Reviewed By: marcinkosiba

fbshipit-source-id: 62f07ed
  • Loading branch information
@thezhangwei @facebook-github-bot
thezhangwei authored and facebook-github-bot committed Mar 7, 2017
1 parent d7f9741 commit cdcfc4c
Show file tree
Hide file tree
Showing 10 changed files with 118 additions and 56 deletions.
1 change: 0 additions & 1 deletion docs/rule/android_binary.soy
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,6 @@ but the following conditions must hold:
</p>
<ul>
<li><code>redex = True</code> must be set on the <code>android_binary()</code>.
<li><code>package_type = 'release'</code> must be set on the <code>android_binary()</code>.
</ul>

<p>
Expand Down
3 changes: 2 additions & 1 deletion src/com/facebook/buck/android/AndroidBinary.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -612,7 +612,8 @@ public ImmutableList<Step> getBuildSteps(
apkToRedexAndAlign,
redexedApk,
keystoreProperties,
proguardConfigDir
proguardConfigDir,
context.getSourcePathResolver()
);
steps.addAll(redexSteps);
}
Expand Down
72 changes: 47 additions & 25 deletions src/com/facebook/buck/android/AndroidBinaryDescription.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@
import com.facebook.buck.model.Flavor;
import com.facebook.buck.model.Flavored;
import com.facebook.buck.model.HasTests;
import com.facebook.buck.model.MacroException;
import com.facebook.buck.parser.NoSuchBuildTargetException;
import com.facebook.buck.rules.AbstractDescriptionArg;
import com.facebook.buck.rules.BuildRule;
Expand All @@ -53,6 +54,7 @@
import com.facebook.buck.rules.SourcePathRuleFinder;
import com.facebook.buck.rules.TargetGraph;
import com.facebook.buck.rules.Tool;
import com.facebook.buck.rules.args.MacroArg;
import com.facebook.buck.rules.coercer.BuildConfigFields;
import com.facebook.buck.rules.coercer.ManifestEntries;
import com.facebook.buck.rules.macros.ExecutableMacroExpander;
Expand All @@ -62,7 +64,6 @@
import com.facebook.buck.util.MoreCollectors;
import com.facebook.buck.util.RichStream;
import com.facebook.infer.annotation.SuppressFieldNotInitialized;
import com.google.common.base.Joiner;
import com.google.common.base.Preconditions;
import com.google.common.base.Suppliers;
import com.google.common.collect.ImmutableList;
Expand All @@ -72,7 +73,6 @@
import com.google.common.collect.Ordering;
import com.google.common.util.concurrent.ListeningExecutorService;

import java.util.Arrays;
import java.util.EnumSet;
import java.util.List;
import java.util.Locale;
Expand Down Expand Up @@ -279,13 +279,23 @@ public <A extends Arg> BuildRule createBuildRule(
.collect(MoreCollectors.toImmutableSortedSet(Ordering.natural()));

SourcePathRuleFinder ruleFinder = new SourcePathRuleFinder(resolver);
Optional<RedexOptions> redexOptions = getRedexOptions(params, resolver, args);

ImmutableSortedSet<BuildRule> redexExtraDeps = redexOptions
.map(a -> a.getRedexExtraArgs()
.stream()
.flatMap(arg -> arg.getDeps(ruleFinder).stream())
.collect(MoreCollectors.toImmutableSortedSet(Ordering.natural()))
).orElse(ImmutableSortedSet.of());

return new AndroidBinary(
params
.copyWithExtraDeps(Suppliers.ofInstance(result.getFinalDeps()))
.appendExtraDeps(
ruleFinder.filterBuildRuleInputs(
result.getPackageableCollection().getProguardConfigs()))
.appendExtraDeps(rulesToExcludeFromDex),
.appendExtraDeps(rulesToExcludeFromDex)
.appendExtraDeps(redexExtraDeps),
ruleFinder,
proGuardConfig.getProguardJarOverride(),
proGuardConfig.getProguardMaxHeapSize(),
Expand All @@ -299,7 +309,7 @@ public <A extends Arg> BuildRule createBuildRule(
args.optimizationPasses,
args.proguardConfig,
args.skipProguard,
getRedexOptions(params.getBuildTarget(), args, resolver),
redexOptions,
compressionMode,
args.cpuFilters,
resourceFilter,
Expand Down Expand Up @@ -386,55 +396,67 @@ public Iterable<BuildTarget> findDepsForTargetFromConstructorArgs(
CellPathResolver cellRoots,
Arg constructorArg) {
ImmutableSet.Builder<BuildTarget> deps = ImmutableSet.builder();
if (constructorArg.redex.orElse(false) &&
getPackageType(constructorArg).isBuildWithObfuscation()) {
if (constructorArg.redex.orElse(false)) {
// If specified, this option may point to either a BuildTarget or a file.
Optional<BuildTarget> redexTarget = buckConfig.getMaybeBuildTarget(
SECTION,
CONFIG_PARAM_REDEX);
if (redexTarget.isPresent()) {
deps.add(redexTarget.get());
}

constructorArg.redexExtraArgs.forEach(a ->
addDepsFromParam(buildTarget, cellRoots, a, deps)
);
}
return deps.build();
}

private void addDepsFromParam(
BuildTarget target,
CellPathResolver cellNames,
String paramValue,
ImmutableSet.Builder<BuildTarget> targets) {
try {
targets.addAll(MACRO_HANDLER.extractParseTimeDeps(target, cellNames, paramValue));
} catch (MacroException e) {
throw new HumanReadableException(e, "%s: %s", target, e.getMessage());
}
}

private Optional<RedexOptions> getRedexOptions(
BuildTarget buildTarget,
Arg arg,
BuildRuleResolver sourcePathResolver) {
BuildRuleParams params,
BuildRuleResolver resolver,
Arg arg) {
boolean redexRequested = arg.redex.orElse(false);
if (!redexRequested) {
return Optional.empty();
}

PackageType packageType = getPackageType(arg);
boolean buildWithObfuscation = packageType.isBuildWithObfuscation();
if (!buildWithObfuscation) {
List<PackageType> supported = Arrays.stream(PackageType.values())
.filter(PackageType::isBuildWithObfuscation)
.collect(Collectors.toList());
throw new HumanReadableException("Requested running ReDex for %s but the package type %s " +
"is not compatible with that options. Consider changing it to %s.",
buildTarget,
packageType,
Joiner.on(", ").join(supported));
}

Optional<Tool> redexBinary =
buckConfig.getTool(SECTION, CONFIG_PARAM_REDEX, sourcePathResolver);
buckConfig.getTool(SECTION, CONFIG_PARAM_REDEX, resolver);
if (!redexBinary.isPresent()) {
throw new HumanReadableException("Requested running ReDex for %s but the path to the tool" +
"has not been specified in the %s.%s .buckconfig section.",
buildTarget,
params.getBuildTarget(),
SECTION,
CONFIG_PARAM_REDEX);
}

java.util.function.Function<String, com.facebook.buck.rules.args.Arg> macroArgFunction =
MacroArg.toMacroArgFunction(
MACRO_HANDLER,
params.getBuildTarget(),
params.getCellRoots(),
resolver)::apply;
List<com.facebook.buck.rules.args.Arg> redexExtraArgs = arg.redexExtraArgs.stream()
.map(macroArgFunction)
.collect(Collectors.toList());

return Optional.of(RedexOptions.builder()
.setRedex(redexBinary.get())
.setRedexConfig(arg.redexConfig)
.setRedexExtraArgs(arg.redexExtraArgs)
.setRedexExtraArgs(redexExtraArgs)
.build());
}

Expand Down
3 changes: 2 additions & 1 deletion src/com/facebook/buck/android/redex/AbstractRedexOptions.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import com.facebook.buck.rules.RuleKeyObjectSink;
import com.facebook.buck.rules.SourcePath;
import com.facebook.buck.rules.Tool;
import com.facebook.buck.rules.args.Arg;
import com.facebook.buck.util.immutables.BuckStyleImmutable;
import com.google.common.collect.ImmutableList;

Expand All @@ -32,7 +33,7 @@
abstract class AbstractRedexOptions implements RuleKeyAppendable {
public abstract Tool getRedex();
public abstract Optional<SourcePath> getRedexConfig();
public abstract ImmutableList<String> getRedexExtraArgs();
public abstract ImmutableList<Arg> getRedexExtraArgs();


@Override
Expand Down
2 changes: 2 additions & 0 deletions src/com/facebook/buck/android/redex/BUCK.autodeps
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
],
"exported_deps" : [
"//src/com/facebook/buck/rules:build_rule",
"//src/com/facebook/buck/rules/args:args",
"//src/com/facebook/buck/util/immutables:immutables",
"//third-party/java/guava:guava"
]
Expand All @@ -19,6 +20,7 @@
"//src/com/facebook/buck/android/redex:options",
"//src/com/facebook/buck/io:io",
"//src/com/facebook/buck/rules:build_rule",
"//src/com/facebook/buck/rules/args:args",
"//src/com/facebook/buck/shell:steps",
"//src/com/facebook/buck/step:step",
"//src/com/facebook/buck/util/immutables:immutables",
Expand Down
18 changes: 12 additions & 6 deletions src/com/facebook/buck/android/redex/ReDexStep.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
import com.facebook.buck.io.ProjectFilesystem;
import com.facebook.buck.rules.SourcePathResolver;
import com.facebook.buck.rules.Tool;
import com.facebook.buck.rules.args.Arg;
import com.facebook.buck.shell.ShellStep;
import com.facebook.buck.step.ExecutionContext;
import com.facebook.buck.step.Step;
Expand All @@ -44,10 +45,11 @@ public class ReDexStep extends ShellStep {
private final Path outputApkPath;
private final Supplier<KeystoreProperties> keystorePropertiesSupplier;
private final Optional<Path> redexConfig;
private final List<String> redexExtraArgs;
private final ImmutableList<Arg> redexExtraArgs;
private final Path proguardMap;
private final Path proguardCommandLine;
private final Path seeds;
private final SourcePathResolver pathResolver;

@VisibleForTesting
ReDexStep(
Expand All @@ -58,10 +60,11 @@ public class ReDexStep extends ShellStep {
Path outputApkPath,
Supplier<KeystoreProperties> keystorePropertiesSupplier,
Optional<Path> redexConfig,
List<String> redexExtraArgs,
ImmutableList<Arg> redexExtraArgs,
Path proguardMap,
Path proguardCommandLine,
Path seeds) {
Path seeds,
SourcePathResolver pathResolver) {
super(workingDirectory);
this.redexBinaryArgs = ImmutableList.copyOf(redexBinaryArgs);
this.redexEnvironmentVariables = ImmutableMap.copyOf(redexEnvironmentVariables);
Expand All @@ -73,6 +76,7 @@ public class ReDexStep extends ShellStep {
this.proguardMap = proguardMap;
this.proguardCommandLine = proguardCommandLine;
this.seeds = seeds;
this.pathResolver = pathResolver;
}

public static ImmutableList<Step> createSteps(
Expand All @@ -82,7 +86,8 @@ public static ImmutableList<Step> createSteps(
Path inputApkPath,
Path outputApkPath,
Supplier<KeystoreProperties> keystorePropertiesSupplier,
Path proguardConfigDir) {
Path proguardConfigDir,
SourcePathResolver pathResolver) {
ImmutableList.Builder<Step> steps = ImmutableList.builder();

Tool redexBinary = redexOptions.getRedex();
Expand All @@ -97,7 +102,8 @@ public static ImmutableList<Step> createSteps(
redexOptions.getRedexExtraArgs(),
proguardConfigDir.resolve("mapping.txt"),
proguardConfigDir.resolve("command-line.txt"),
proguardConfigDir.resolve("seeds.txt"));
proguardConfigDir.resolve("seeds.txt"),
pathResolver);
steps.add(redexStep);

return steps.build();
Expand Down Expand Up @@ -138,7 +144,7 @@ protected ImmutableList<String> getShellCommandInternal(ExecutionContext context

args.add("--out", outputApkPath.toString());

args.addAll(redexExtraArgs);
args.addAll(Arg.stringify(redexExtraArgs, pathResolver));

args.add(inputApkPath.toString());
return args.build();
Expand Down
6 changes: 6 additions & 0 deletions test/com/facebook/buck/android/AndroidBinaryIntegrationTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1009,6 +1009,12 @@ public void testReDexIsCalledAppropriatelyFromAndroidBinary() throws IOException
assertEquals(
"my_param_name={\"foo\": true}",
userData.get("J"));
assertTrue(
"redex_extra_args: -j $(location ...) is not properly expanded!",
userData.get("j").toString().endsWith(".jar"));
assertTrue(
"redex_extra_args: -S $(location ...) is not properly expanded!",
userData.get("S").toString().endsWith(".jar"));
}

@Test
Expand Down
18 changes: 16 additions & 2 deletions test/com/facebook/buck/android/redex/ReDexStepTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,13 @@

import com.facebook.buck.android.AndroidPlatformTarget;
import com.facebook.buck.android.KeystoreProperties;
import com.facebook.buck.rules.BuildRuleResolver;
import com.facebook.buck.rules.DefaultTargetNodeToBuildRuleTransformer;
import com.facebook.buck.rules.SourcePathResolver;
import com.facebook.buck.rules.SourcePathRuleFinder;
import com.facebook.buck.rules.TargetGraph;
import com.facebook.buck.rules.args.Arg;
import com.facebook.buck.rules.args.StringArg;
import com.facebook.buck.step.ExecutionContext;
import com.facebook.buck.step.TestExecutionContext;
import com.google.common.base.Supplier;
Expand Down Expand Up @@ -54,11 +61,17 @@ public void constructorArgsAreUsedToCreateShellCommand() {
keystoreProperties);
Path redexConfigPath = Paths.get("redex/redex-config.json");
Optional<Path> redexConfig = Optional.of(redexConfigPath);
List<String> redexExtraArgs = ImmutableList.of("foo", "bar");
ImmutableList<Arg> redexExtraArgs = ImmutableList.of(
StringArg.of("foo"), StringArg.of("bar")
);
Path proguardMap = Paths.get("buck-out/gen/app/__proguard__/mapping.txt");
Path proguardConfig = Paths.get("app.proguard.config");
Path seeds = Paths.get("buck-out/gen/app/__proguard__/seeds.txt");

SourcePathResolver pathResolver = new SourcePathResolver(new SourcePathRuleFinder(
new BuildRuleResolver(TargetGraph.EMPTY, new DefaultTargetNodeToBuildRuleTransformer())
));

ReDexStep redex = new ReDexStep(
workingDirectory,
redexBinaryArgs,
Expand All @@ -70,7 +83,8 @@ public void constructorArgsAreUsedToCreateShellCommand() {
redexExtraArgs,
proguardMap,
proguardConfig,
seeds
seeds,
pathResolver
);

assertEquals("redex", redex.getShortName());
Expand Down
4 changes: 4 additions & 0 deletions test/com/facebook/buck/android/testdata/android_project/apps/sample/BUCK.fixture
View file
Original file line number Diff line number Diff line change
Expand Up @@ -421,6 +421,10 @@ android_binary(
redex_config = 'redex-config.json',
redex_extra_args = [
'-Jmy_param_name={"foo": true}',
'--redex-binary',
'redex-binary',
'-j $(location //java/com/sample/lib:lib)',
'-S $(location //java/com/sample2:lib)',
],
deps = [
'//java/com/sample/lib:lib',
Expand Down
Loading

0 comments on commit cdcfc4c

Please sign in to comment.