In BuildRuleResolver, assert that a rule is added only once. Summary: This seems to have uncovered a bug in AndroidBinaryRuleTest. Test Plan: Sandcastle builds.
diff --git a/src/com/facebook/buck/rules/BuildRuleResolver.java b/src/com/facebook/buck/rules/BuildRuleResolver.java index 0b07b8f..bbafe5e 100644 --- a/src/com/facebook/buck/rules/BuildRuleResolver.java +++ b/src/com/facebook/buck/rules/BuildRuleResolver.java
@@ -61,7 +61,11 @@ public <T extends BuildRule> T buildAndAddToIndex(BuildRuleBuilder<T> builder) { T buildRule = builder.build(this); - buildRuleIndex.put(buildRule.getBuildTarget(), buildRule); + BuildRule oldValue = buildRuleIndex.put(buildRule.getBuildTarget(), buildRule); + if (oldValue != null) { + throw new IllegalStateException("A build rule for this target has already been created: " + + buildRule.getBuildTarget()); + } return buildRule; } }
diff --git a/test/com/facebook/buck/android/AndroidBinaryRuleTest.java b/test/com/facebook/buck/android/AndroidBinaryRuleTest.java index 97d434c..12d8cc3 100644 --- a/test/com/facebook/buck/android/AndroidBinaryRuleTest.java +++ b/test/com/facebook/buck/android/AndroidBinaryRuleTest.java
@@ -16,7 +16,6 @@ package com.facebook.buck.android; -import static com.facebook.buck.android.FilterResourcesStep.ResourceFilter; import static com.facebook.buck.util.BuckConstant.BIN_DIR; import static com.facebook.buck.util.BuckConstant.GEN_DIR; import static org.easymock.EasyMock.createMock; @@ -27,6 +26,7 @@ import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; +import com.facebook.buck.android.FilterResourcesStep.ResourceFilter; import com.facebook.buck.dalvik.ZipSplitter; import com.facebook.buck.java.JavaLibraryRule; import com.facebook.buck.java.Keystore; @@ -379,17 +379,6 @@ .setBuildTarget(libraryOnebuildTarget); if (!Strings.isNullOrEmpty(resDirectory) || !Strings.isNullOrEmpty(assetDirectory)) { - BuildTarget resourceOnebuildTarget = BuildTargetFactory.newInstance(buildTarget); - AndroidResourceRule androidResourceRule = ruleResolver.buildAndAddToIndex( - AndroidResourceRule.newAndroidResourceRuleBuilder(new FakeAbstractBuildRuleBuilderParams()) - .setAssetsDirectory(assetDirectory) - .setRes(resDirectory) - .setBuildTarget(resourceOnebuildTarget)); - - androidLibraryRuleBuilder.addDep(androidResourceRule.getBuildTarget()); - } - - if (!Strings.isNullOrEmpty(resDirectory) || !Strings.isNullOrEmpty(assetDirectory)) { BuildTarget resourceOnebuildTarget = BuildTargetFactory.newInstance(buildTarget + "_resources"); AndroidResourceRule androidResourceRule = ruleResolver.buildAndAddToIndex( @@ -439,6 +428,7 @@ .getInputsToCompareToOutput()); SourcePath proguardConfig = new FileSourcePath("java/src/com/facebook/proguard.cfg"); + androidBinaryRuleBuilder.setBuildTarget(new BuildTarget("//java/src/com/facebook", "app2")); androidBinaryRuleBuilder.setProguardConfig(Optional.of(proguardConfig)); MoreAsserts.assertListEquals( "getInputsToCompareToOutput() should include Proguard config, if present.", @@ -454,12 +444,13 @@ @Test public void testGetUnsignedApkPath() { BuildRuleResolver ruleResolver = new BuildRuleResolver(); + BuildTarget keystoreTarget = addKeystoreRule(ruleResolver); AndroidBinaryRule ruleInRootDirectory = ruleResolver.buildAndAddToIndex( AndroidBinaryRule.newAndroidBinaryRuleBuilder(new FakeAbstractBuildRuleBuilderParams()) .setBuildTarget(BuildTargetFactory.newInstance("//:fb4a")) .setManifest("AndroidManifest.xml") - .setKeystore(addKeystoreRule(ruleResolver)) + .setKeystore(keystoreTarget) .setTarget("Google Inc.:Google APIs:16")); assertEquals(GEN_DIR + "/fb4a.apk", ruleInRootDirectory.getApkPath()); @@ -467,7 +458,7 @@ AndroidBinaryRule.newAndroidBinaryRuleBuilder(new FakeAbstractBuildRuleBuilderParams()) .setBuildTarget(BuildTargetFactory.newInstance("//java/com/example:fb4a")) .setManifest("AndroidManifest.xml") - .setKeystore(addKeystoreRule(ruleResolver)) + .setKeystore(keystoreTarget) .setTarget("Google Inc.:Google APIs:16")); assertEquals(GEN_DIR + "/java/com/example/fb4a.apk", ruleInNonRootDirectory.getApkPath()); }
diff --git a/test/com/facebook/buck/rules/BuildRuleResolverTest.java b/test/com/facebook/buck/rules/BuildRuleResolverTest.java new file mode 100644 index 0000000..f9da646 --- /dev/null +++ b/test/com/facebook/buck/rules/BuildRuleResolverTest.java
@@ -0,0 +1,61 @@ +/* + * 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.rules; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.fail; + +import com.facebook.buck.java.DefaultJavaLibraryRule; +import com.facebook.buck.model.BuildTarget; +import com.facebook.buck.util.ProjectFilesystem; + +import org.junit.Test; + +import java.io.File; + +public class BuildRuleResolverTest { + + @Test + public void testBuildAndAddToIndexRejectsDuplicateBuildTarget() { + BuildRuleResolver buildRuleResolver = new BuildRuleResolver(); + + ProjectFilesystem projectFilesystem = new ProjectFilesystem(new File(".")); + RuleKeyBuilderFactory ruleKeyBuilderFactory = new FakeRuleKeyBuilderFactory(); + AbstractBuildRuleBuilderParams params = new DefaultBuildRuleBuilderParams(projectFilesystem, + ruleKeyBuilderFactory); + + DefaultJavaLibraryRule.Builder builder1 = DefaultJavaLibraryRule.newJavaLibraryRuleBuilder( + params); + BuildTarget buildTarget = new BuildTarget("//foo", "bar"); + builder1.setBuildTarget(buildTarget); + buildRuleResolver.buildAndAddToIndex(builder1); + + DefaultJavaLibraryRule.Builder builder2 = DefaultJavaLibraryRule.newJavaLibraryRuleBuilder( + params); + builder2.setBuildTarget(buildTarget); + + // A BuildRuleResolver should allow only one entry for a BuildTarget. + try { + buildRuleResolver.buildAndAddToIndex(builder2); + fail("Should throw IllegalStateException."); + } catch (IllegalStateException e) { + assertEquals( + "A build rule for this target has already been created: " + buildTarget, + e.getMessage()); + } + } +}
diff --git a/test/com/facebook/buck/rules/RuleKeyTest.java b/test/com/facebook/buck/rules/RuleKeyTest.java index 727c6f1..473d50e 100644 --- a/test/com/facebook/buck/rules/RuleKeyTest.java +++ b/test/com/facebook/buck/rules/RuleKeyTest.java
@@ -50,7 +50,8 @@ */ @Test public void testRuleKeyDependsOnDeps() throws IOException { - BuildRuleResolver ruleResolver = new BuildRuleResolver(); + BuildRuleResolver ruleResolver1 = new BuildRuleResolver(); + BuildRuleResolver ruleResolver2 = new BuildRuleResolver(); ProjectFilesystem projectFilesystem = new ProjectFilesystem(new File(".")); final FileHashCache fileHashCache = new DefaultFileHashCache(projectFilesystem, new TestConsole()); @@ -64,9 +65,11 @@ }); // Create a dependent build rule, //src/com/facebook/buck/cli:common. - ruleResolver.buildAndAddToIndex( - DefaultJavaLibraryRule.newJavaLibraryRuleBuilder(builderParams) - .setBuildTarget(BuildTargetFactory.newInstance("//src/com/facebook/buck/cli:common"))); + DefaultJavaLibraryRule.Builder commonJavaLibraryRuleBuilder = DefaultJavaLibraryRule + .newJavaLibraryRuleBuilder(builderParams) + .setBuildTarget(BuildTargetFactory.newInstance("//src/com/facebook/buck/cli:common")); + ruleResolver1.buildAndAddToIndex(commonJavaLibraryRuleBuilder); + ruleResolver2.buildAndAddToIndex(commonJavaLibraryRuleBuilder); // Create a java_library() rule with no deps. DefaultJavaLibraryRule.Builder javaLibraryBuilder = DefaultJavaLibraryRule @@ -78,12 +81,12 @@ // TODO(mbolin): Update RuleKey.Builder.setVal(File) to use a ProjectFilesystem so that file // access can be mocked appropriately during a unit test. .addSrc("src/com/facebook/buck/cli/Main.java"); - DefaultJavaLibraryRule libraryNoCommon = ruleResolver.buildAndAddToIndex( + DefaultJavaLibraryRule libraryNoCommon = ruleResolver1.buildAndAddToIndex( javaLibraryBuilder); // Create the same java_library() rule, but with a dep on //src/com/facebook/buck/cli:common. javaLibraryBuilder.addDep(BuildTargetFactory.newInstance("//src/com/facebook/buck/cli:common")); - DefaultJavaLibraryRule libraryWithCommon = ruleResolver.buildAndAddToIndex( + DefaultJavaLibraryRule libraryWithCommon = ruleResolver2.buildAndAddToIndex( javaLibraryBuilder); // Assert that the RuleKeys are distinct.