Use AccumulateClassNames.getClassNames() to determine whether to run dx.
Summary:
Previously, `DexProducedFromJavaLibraryThatContainsClassFiles` would store a
tiny bit of metadata to determine whether its dependent `AccumulateClassNames`
found any class files. Now we just ask the `AccumulateClassNames` directly.
Test Plan: Sandcastle builds.
diff --git a/src/com/facebook/buck/android/DexProducedFromJavaLibraryThatContainsClassFiles.java b/src/com/facebook/buck/android/DexProducedFromJavaLibraryThatContainsClassFiles.java
index e485218..d917476 100644
--- a/src/com/facebook/buck/android/DexProducedFromJavaLibraryThatContainsClassFiles.java
+++ b/src/com/facebook/buck/android/DexProducedFromJavaLibraryThatContainsClassFiles.java
@@ -27,19 +27,14 @@
import com.facebook.buck.rules.BuildRuleType;
import com.facebook.buck.rules.Buildable;
import com.facebook.buck.rules.BuildableContext;
-import com.facebook.buck.rules.OnDiskBuildInfo;
import com.facebook.buck.step.AbstractExecutionStep;
-import com.facebook.buck.step.CompositeStep;
-import com.facebook.buck.step.ConditionalStep;
import com.facebook.buck.step.ExecutionContext;
import com.facebook.buck.step.Step;
-import com.facebook.buck.step.fs.FileExistsAndIsNotEmptyStep;
import com.facebook.buck.step.fs.MkdirStep;
import com.facebook.buck.step.fs.RmStep;
import com.facebook.buck.util.BuckConstant;
+import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Preconditions;
-import com.google.common.base.Supplier;
-import com.google.common.base.Suppliers;
import com.google.common.collect.ImmutableList;
import java.io.IOException;
@@ -66,30 +61,16 @@
*/
public class DexProducedFromJavaLibraryThatContainsClassFiles extends AbstractBuildable {
- /**
- * Key used with {@link OnDiskBuildInfo} to identify whether this {@link Buildable} has
- * generated a {@code .dex} files. The only expected value to be associated with this key is
- * {@code "true"}.
- */
- private static final String HAS_DEX_OUTPUT_METADATA = "HAS_DEX_OUTPUT";
-
private final BuildTarget buildTarget;
private final AccumulateClassNames javaLibraryWithClassesList;
- /** This {@link Supplier} will be defined and determined after this buildable is built. */
- @Nullable
- private Supplier<Boolean> hasOutputFile;
-
- private DexProducedFromJavaLibraryThatContainsClassFiles(BuildTarget buildTarget,
+ @VisibleForTesting
+ DexProducedFromJavaLibraryThatContainsClassFiles(BuildTarget buildTarget,
AccumulateClassNames javaLibraryWithClassesList) {
this.buildTarget = Preconditions.checkNotNull(buildTarget);
this.javaLibraryWithClassesList = Preconditions.checkNotNull(javaLibraryWithClassesList);
}
- public BuildTarget getBuildTarget() {
- return buildTarget;
- }
-
@Override
public Iterable<String> getInputsToCompareToOutput() {
// The deps of this rule already capture all of the inputs that should affect the cache key.
@@ -106,31 +87,24 @@
// Make sure that the buck-out/gen/ directory exists for this.buildTarget.
steps.add(new MkdirStep(getPathToDex().getParent()));
- // Check whether the list of class files in the JavaLibraryRule is empty.
- FileExistsAndIsNotEmptyStep fileExistsStep = new FileExistsAndIsNotEmptyStep(
- Paths.get(javaLibraryWithClassesList.getPathToOutputFile()));
- hasOutputFile = fileExistsStep;
- steps.add(fileExistsStep);
+ if (!javaLibraryWithClassesList.getClassNames().isEmpty()) {
+ // To be conservative, use --force-jumbo for these intermediate .dex files so that they can be
+ // merged into a final classes.dex that uses jumbo instructions.
+ JavaLibraryRule javaLibraryRuleToDex = javaLibraryWithClassesList.getJavaLibraryRule();
+ DxStep dx = new DxStep(getPathToDex().toString(),
+ Collections.singleton(Paths.get(javaLibraryRuleToDex.getPathToOutputFile())),
+ EnumSet.of(DxStep.Option.NO_OPTIMIZE, DxStep.Option.FORCE_JUMBO));
+ steps.add(dx);
- // To be conservative, use --force-jumbo for these intermediate .dex files so that they can be
- // merged into a final classes.dex that uses jumbo instructions.
- JavaLibraryRule javaLibraryRuleToDex = javaLibraryWithClassesList.getJavaLibraryRule();
- DxStep dx = new DxStep(getPathToDex().toString(),
- Collections.singleton(Paths.get(javaLibraryRuleToDex.getPathToOutputFile())),
- EnumSet.of(DxStep.Option.NO_OPTIMIZE, DxStep.Option.FORCE_JUMBO));
- AbstractExecutionStep recordArtifactStep = new AbstractExecutionStep("record dx success") {
- @Override
- public int execute(ExecutionContext context) {
- buildableContext.recordArtifact(getPathToDex().getFileName());
- buildableContext.addMetadata(HAS_DEX_OUTPUT_METADATA, "true");
- return 0;
- }
- };
- CompositeStep dxAndStore = new CompositeStep(ImmutableList.of(dx, recordArtifactStep));
-
- // Make sure that there are .class files to dex before running dx.
- ConditionalStep runDxIfThereAreClassFiles = new ConditionalStep(fileExistsStep, dxAndStore);
- steps.add(runDxIfThereAreClassFiles);
+ AbstractExecutionStep recordArtifactStep = new AbstractExecutionStep("record_dx_success") {
+ @Override
+ public int execute(ExecutionContext context) {
+ buildableContext.recordArtifact(getPathToDex().getFileName());
+ return 0;
+ }
+ };
+ steps.add(recordArtifactStep);
+ }
return steps.build();
}
@@ -152,14 +126,7 @@
public boolean hasOutput() {
// TODO(mbolin): Assert that this Buildable has been built. Currently, there is no way to do
// that from a Buildable (but there is from an AbstractCachingBuildRule).
- Preconditions.checkNotNull(hasOutputFile);
- return hasOutputFile.get();
- }
-
- @Override
- protected void initializeFromDisk(OnDiskBuildInfo onDiskBuildInfo) {
- boolean hasOutput = "true".equals(onDiskBuildInfo.getValue(HAS_DEX_OUTPUT_METADATA).orNull());
- this.hasOutputFile = Suppliers.ofInstance(hasOutput);
+ return !javaLibraryWithClassesList.getClassNames().isEmpty();
}
public static Builder newPreDexBuilder(AbstractBuildRuleBuilderParams params) {
diff --git a/src/com/facebook/buck/step/fs/FileExistsAndIsNotEmptyStep.java b/src/com/facebook/buck/step/fs/FileExistsAndIsNotEmptyStep.java
deleted file mode 100644
index 9a72c01..0000000
--- a/src/com/facebook/buck/step/fs/FileExistsAndIsNotEmptyStep.java
+++ /dev/null
@@ -1,68 +0,0 @@
-/*
- * Copyright 2013-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.step.fs;
-
-import com.facebook.buck.step.AbstractExecutionStep;
-import com.facebook.buck.step.ConditionalStep;
-import com.facebook.buck.step.ExecutionContext;
-import com.facebook.buck.util.ProjectFilesystem;
-import com.facebook.buck.util.TriState;
-import com.google.common.base.Preconditions;
-import com.google.common.base.Supplier;
-
-import java.io.IOException;
-import java.nio.file.Path;
-
-/**
- * Step that verifies that a file of non-zero length exists at the specified path. The side effect
- * of this step is that its {@link #get()} method will return a boolean indicating whether the
- * file exists. This is designed to be used with a {@link ConditionalStep}.
- */
-public class FileExistsAndIsNotEmptyStep extends AbstractExecutionStep
- implements Supplier<Boolean> {
-
- private final Path pathToFile;
- private TriState fileExistsAndIsNotEmpty = TriState.UNSPECIFIED;
-
- public FileExistsAndIsNotEmptyStep(Path pathToFile) {
- super("test -s " + pathToFile);
- this.pathToFile = Preconditions.checkNotNull(pathToFile);
- }
-
- @Override
- public Boolean get() {
- Preconditions.checkState(fileExistsAndIsNotEmpty.isSet());
- return fileExistsAndIsNotEmpty.asBoolean();
- }
-
- @Override
- public int execute(ExecutionContext context) {
- ProjectFilesystem filesystem = context.getProjectFilesystem();
- boolean exists = filesystem.exists(pathToFile.toString());
-
- try {
- fileExistsAndIsNotEmpty = TriState.forBooleanValue(exists
- && filesystem.getFileSize(pathToFile) > 0);
- } catch (IOException e) {
- context.logError(e, "Failed to get size for file: %s.", pathToFile);
- return 1;
- }
-
- return 0;
- }
-
-}
diff --git a/test/com/facebook/buck/android/DexProducedFromJavaLibraryThatContainsClassFilesTest.java b/test/com/facebook/buck/android/DexProducedFromJavaLibraryThatContainsClassFilesTest.java
new file mode 100644
index 0000000..c18b4a5
--- /dev/null
+++ b/test/com/facebook/buck/android/DexProducedFromJavaLibraryThatContainsClassFilesTest.java
@@ -0,0 +1,167 @@
+/*
+ * Copyright 2013-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.easymock.EasyMock.expect;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNull;
+import static org.junit.Assert.assertTrue;
+
+import com.facebook.buck.java.AccumulateClassNames;
+import com.facebook.buck.java.JavaLibraryRule;
+import com.facebook.buck.model.BuildTarget;
+import com.facebook.buck.rules.BuildContext;
+import com.facebook.buck.rules.BuildableContext;
+import com.facebook.buck.step.ExecutionContext;
+import com.facebook.buck.step.Step;
+import com.facebook.buck.step.TestExecutionContext;
+import com.facebook.buck.testutil.MoreAsserts;
+import com.facebook.buck.util.AndroidPlatformTarget;
+import com.facebook.buck.util.ProjectFilesystem;
+import com.google.common.base.Optional;
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableSortedMap;
+import com.google.common.collect.Iterables;
+import com.google.common.hash.HashCode;
+
+import org.easymock.EasyMockSupport;
+import org.junit.Test;
+
+import java.io.File;
+import java.io.IOException;
+import java.nio.file.Paths;
+import java.util.List;
+
+public class DexProducedFromJavaLibraryThatContainsClassFilesTest extends EasyMockSupport {
+
+ @Test
+ public void testGetBuildStepsWhenThereAreClassesToDex() throws IOException {
+ JavaLibraryRule javaLibraryRule = createMock(JavaLibraryRule.class);
+ expect(javaLibraryRule.getPathToOutputFile()).andReturn("buck-out/gen/foo/bar.jar");
+
+ AccumulateClassNames accumulateClassNames = createMock(AccumulateClassNames.class);
+ expect(accumulateClassNames.getClassNames()).andReturn(
+ ImmutableSortedMap.of("com/example/Foo", HashCode.fromString("cafebabe")));
+ expect(accumulateClassNames.getJavaLibraryRule()).andReturn(javaLibraryRule);
+
+ BuildContext context = createMock(BuildContext.class);
+ BuildableContext buildableContext = createMock(BuildableContext.class);
+
+ replayAll();
+
+ BuildTarget buildTarget = new BuildTarget("//foo", "bar");
+ DexProducedFromJavaLibraryThatContainsClassFiles preDex =
+ new DexProducedFromJavaLibraryThatContainsClassFiles(buildTarget, accumulateClassNames);
+ List<Step> steps = preDex.getBuildSteps(context, buildableContext);
+
+ verifyAll();
+ resetAll();
+
+ AndroidPlatformTarget androidPlatformTarget = createMock(AndroidPlatformTarget.class);
+ expect(androidPlatformTarget.getDxExecutable()).andReturn(new File("/usr/bin/dx"));
+
+ ProjectFilesystem projectFilesystem = createMock(ProjectFilesystem.class);
+ expect(projectFilesystem.resolve(Paths.get("buck-out/gen/foo")))
+ .andReturn(Paths.get("/home/user/buck-out/gen/foo"));
+ expect(projectFilesystem.resolve(Paths.get("buck-out/gen/foo/bar.dex.jar")))
+ .andReturn(Paths.get("/home/user/buck-out/gen/foo/bar.dex.jar"));
+ expect(projectFilesystem.resolve(Paths.get("buck-out/gen/foo/bar.jar")))
+ .andReturn(Paths.get("/home/user/buck-out/gen/foo/bar.jar"));
+ replayAll();
+
+ ExecutionContext executionContext = TestExecutionContext
+ .newBuilder()
+ .setAndroidPlatformTarget(Optional.of(androidPlatformTarget))
+ .setProjectFilesystem(projectFilesystem)
+ .build();
+
+ String expectedDxCommand = "/usr/bin/dx" +
+ " --dex --no-optimize --force-jumbo --output buck-out/gen/foo/bar.dex.jar " +
+ "/home/user/buck-out/gen/foo/bar.jar";
+ MoreAsserts.assertSteps("Generate bar.dex.jar.",
+ ImmutableList.of(
+ "rm -f /home/user/buck-out/gen/foo/bar.dex.jar",
+ "mkdir -p /home/user/buck-out/gen/foo",
+ expectedDxCommand,
+ "record_dx_success"),
+ steps,
+ executionContext);
+
+ verifyAll();
+ }
+
+ @Test
+ public void testGetBuildStepsWhenThereAreNoClassesToDex() throws IOException {
+ AccumulateClassNames accumulateClassNames = createMock(AccumulateClassNames.class);
+ expect(accumulateClassNames.getClassNames()).andReturn(
+ ImmutableSortedMap.<String, HashCode>of());
+
+ BuildContext context = createMock(BuildContext.class);
+ BuildableContext buildableContext = createMock(BuildableContext.class);
+
+ replayAll();
+
+ BuildTarget buildTarget = new BuildTarget("//foo", "bar");
+ DexProducedFromJavaLibraryThatContainsClassFiles preDex =
+ new DexProducedFromJavaLibraryThatContainsClassFiles(buildTarget, accumulateClassNames);
+ List<Step> steps = preDex.getBuildSteps(context, buildableContext);
+
+ verifyAll();
+ resetAll();
+
+ ProjectFilesystem projectFilesystem = createMock(ProjectFilesystem.class);
+ expect(projectFilesystem.resolve(Paths.get("buck-out/gen/foo")))
+ .andReturn(Paths.get("/home/user/buck-out/gen/foo"));
+ expect(projectFilesystem.resolve(Paths.get("buck-out/gen/foo/bar.dex.jar")))
+ .andReturn(Paths.get("/home/user/buck-out/gen/foo/bar.dex.jar"));
+ replayAll();
+
+ ExecutionContext executionContext = TestExecutionContext
+ .newBuilder()
+ .setProjectFilesystem(projectFilesystem)
+ .build();
+
+ MoreAsserts.assertSteps("Do not generate a .dex.jar file.",
+ ImmutableList.of(
+ "rm -f /home/user/buck-out/gen/foo/bar.dex.jar",
+ "mkdir -p /home/user/buck-out/gen/foo"),
+ steps,
+ executionContext);
+
+ verifyAll();
+ }
+
+ @Test
+ public void testObserverMethods() {
+ AccumulateClassNames accumulateClassNames = createMock(AccumulateClassNames.class);
+ expect(accumulateClassNames.getClassNames())
+ .andReturn(ImmutableSortedMap.of("com/example/Foo", HashCode.fromString("cafebabe")))
+ .anyTimes();
+
+ replayAll();
+
+ BuildTarget buildTarget = new BuildTarget("//foo", "bar");
+ DexProducedFromJavaLibraryThatContainsClassFiles preDexWithClasses =
+ new DexProducedFromJavaLibraryThatContainsClassFiles(buildTarget, accumulateClassNames);
+ assertNull(preDexWithClasses.getPathToOutputFile());
+ assertTrue(Iterables.isEmpty(preDexWithClasses.getInputsToCompareToOutput()));
+ assertEquals(Paths.get("buck-out/gen/foo/bar.dex.jar"), preDexWithClasses.getPathToDex());
+ assertTrue(preDexWithClasses.hasOutput());
+
+ verifyAll();
+ }
+}
