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(); + } +}