From 5b1ce4d5d7568ecacf02c63c30a9cc7ce7ef24d3 Mon Sep 17 00:00:00 2001
From: Dmitry Lomov <dslomov@google.com>
Date: Wed, 30 May 2018 04:34:08 -0700
Subject: Fix `equals()` and `hashCode()` for artifacts: artifacts of different
 classes are not equal.

Also validate that there are no tree and file artifacts with the same exec path.

Fixes #4668.

Closes #5284.

Change-Id: Id97c0407a476a5bfc697b4ca7b858e3d0c0f8c75
PiperOrigin-RevId: 198540425
---
 .../build/lib/analysis/util/AnalysisTestUtil.java  | 12 +++++-
 .../build/lib/analysis/util/BuildViewTestCase.java | 45 ++++++++++++++++++++++
 .../devtools/build/lib/packages/RuleClassTest.java |  2 +-
 .../build/lib/rules/android/AndroidBinaryTest.java |  2 +-
 .../build/lib/skylark/SkylarkIntegrationTest.java  | 24 ++++++++++++
 5 files changed, 82 insertions(+), 3 deletions(-)

(limited to 'src/test/java/com')

diff --git a/src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisTestUtil.java b/src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisTestUtil.java
index a86066d1a5..51c0fcf6e3 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisTestUtil.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisTestUtil.java
@@ -193,6 +193,11 @@ public final class AnalysisTestUtil {
       return original.getOrphanArtifacts();
     }
 
+    @Override
+    public ImmutableSet<Artifact> getTreeArtifactsConflictingWithFiles() {
+      return original.getTreeArtifactsConflictingWithFiles();
+    }
+
     @Override
     public ActionKeyContext getActionKeyContext() {
       return original.getActionKeyContext();
@@ -398,7 +403,12 @@ public final class AnalysisTestUtil {
 
     @Override
     public ImmutableSet<Artifact> getOrphanArtifacts() {
-      return ImmutableSet.<Artifact>of();
+      return ImmutableSet.of();
+    }
+
+    @Override
+    public ImmutableSet<Artifact> getTreeArtifactsConflictingWithFiles() {
+      return ImmutableSet.of();
     }
 
     @Override
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java b/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java
index e687bc260f..360717f5fa 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java
@@ -1141,6 +1141,32 @@ public abstract class BuildViewTestCase extends FoundationTestCase {
     return view.getArtifactFactory().getDerivedArtifact(rootRelativePath, root, owner);
   }
 
+  /**
+   * Gets a tree artifact, creating it if necessary. {@code ArtifactOwner} should be a genuine
+   * {@link ConfiguredTargetKey} corresponding to a {@link ConfiguredTarget}. If called from a test
+   * that does not exercise the analysis phase, the convenience methods {@link
+   * #getBinArtifactWithNoOwner} or {@link #getGenfilesArtifactWithNoOwner} should be used instead.
+   */
+  protected Artifact getTreeArtifact(
+      PathFragment rootRelativePath, ArtifactRoot root, ArtifactOwner owner) {
+    return view.getArtifactFactory().getTreeArtifact(rootRelativePath, root, owner);
+  }
+
+  /**
+   * Gets a Tree Artifact for testing in the subdirectory of the {@link
+   * BuildConfiguration#getBinDirectory} corresponding to the package of {@code owner}. So to
+   * specify a file foo/foo.o owned by target //foo:foo, {@code packageRelativePath} should just be
+   * "foo.o".
+   */
+  protected final Artifact getTreeArtifact(String packageRelativePath, ConfiguredTarget owner) {
+    return getPackageRelativeTreeArtifact(
+        packageRelativePath,
+        getConfiguration(owner).getBinDirectory(RepositoryName.MAIN),
+        ConfiguredTargetKey.of(
+            owner, skyframeExecutor.getConfiguration(reporter, owner.getConfigurationKey())));
+  }
+
+
   /**
    * Gets a derived Artifact for testing with path of the form
    * root/owner.getPackageFragment()/packageRelativePath.
@@ -1154,6 +1180,20 @@ public abstract class BuildViewTestCase extends FoundationTestCase {
         root, owner);
   }
 
+  /**
+   * Gets a tree Artifact for testing with path of the form
+   * root/owner.getPackageFragment()/packageRelativePath.
+   *
+   * @see #getDerivedArtifact(PathFragment, ArtifactRoot, ArtifactOwner)
+   */
+  private Artifact getPackageRelativeTreeArtifact(
+      String packageRelativePath, ArtifactRoot root, ArtifactOwner owner) {
+    return getTreeArtifact(
+        owner.getLabel().getPackageFragment().getRelative(packageRelativePath),
+        root, owner);
+  }
+
+
   /**
    * Gets a derived Artifact for testing in the {@link BuildConfiguration#getBinDirectory}. This
    * method should only be used for tests that do no analysis, and so there is no ConfiguredTarget
@@ -1921,6 +1961,11 @@ public abstract class BuildViewTestCase extends FoundationTestCase {
       throw new UnsupportedOperationException();
     }
 
+    @Override
+    public ImmutableSet<Artifact> getTreeArtifactsConflictingWithFiles() {
+      throw new UnsupportedOperationException();
+    }
+
     @Override
     public ActionKeyContext getActionKeyContext() {
       return actionKeyContext;
diff --git a/src/test/java/com/google/devtools/build/lib/packages/RuleClassTest.java b/src/test/java/com/google/devtools/build/lib/packages/RuleClassTest.java
index c82b2aeb25..b9b2f4c33c 100644
--- a/src/test/java/com/google/devtools/build/lib/packages/RuleClassTest.java
+++ b/src/test/java/com/google/devtools/build/lib/packages/RuleClassTest.java
@@ -894,7 +894,7 @@ public class RuleClassTest extends PackageLoadingTestCase {
         supportsConstraintChecking,
         /*requiredToolchains=*/ ImmutableSet.<Label>of(),
         /*supportsPlatforms=*/ true,
-        ImmutableList.copyOf(attributes));
+        OutputFile.Kind.FILE, ImmutableList.copyOf(attributes));
   }
 
   private static RuleClass createParentRuleClass() {
diff --git a/src/test/java/com/google/devtools/build/lib/rules/android/AndroidBinaryTest.java b/src/test/java/com/google/devtools/build/lib/rules/android/AndroidBinaryTest.java
index 7d41301892..f894e4d32b 100644
--- a/src/test/java/com/google/devtools/build/lib/rules/android/AndroidBinaryTest.java
+++ b/src/test/java/com/google/devtools/build/lib/rules/android/AndroidBinaryTest.java
@@ -705,7 +705,7 @@ public class AndroidBinaryTest extends AndroidBuildViewTestCase {
     assertNoEvents();
 
     SpawnAction splitAction =
-        getGeneratingSpawnAction(getBinArtifact("dexsplits/top", topTarget));
+        getGeneratingSpawnAction(getTreeArtifact("dexsplits/top", topTarget));
     assertThat(splitAction.getArguments()).contains("--main-dex-list");
     assertThat(splitAction.getArguments()).contains("--minimal-main-dex");
     assertThat(ActionsTestUtil.baseArtifactNames(getNonToolInputs(splitAction)))
diff --git a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkIntegrationTest.java b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkIntegrationTest.java
index 51fa9d33f0..877b1a3f24 100644
--- a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkIntegrationTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkIntegrationTest.java
@@ -1658,6 +1658,30 @@ public class SkylarkIntegrationTest extends BuildViewTestCase {
             + " rule 'r' should be created by the same rule.");
   }
 
+  @Test
+  public void testFileAndDirectory() throws Exception {
+    scratch.file(
+        "ext.bzl",
+        "def _extrule(ctx):",
+        "  dir = ctx.actions.declare_directory('foo/bar/baz')",
+        "  ctx.actions.run_shell(",
+        "      outputs = [dir],",
+        "      command = 'mkdir -p ' + dir.path + ' && echo wtf > ' + dir.path + '/wtf.txt')",
+        "",
+        "extrule = rule(",
+        "    _extrule,",
+        "    outputs = {",
+        "      'out': 'foo/bar/baz',",
+        "    },",
+        ")");
+    scratch.file("BUILD", "load(':ext.bzl', 'extrule')", "", "extrule(", "    name = 'test'", ")");
+    reporter.removeHandler(failFastHandler);
+    getConfiguredTarget("//:test");
+    assertContainsEvent("ERROR /workspace/BUILD:3:1: in extrule rule //:test:");
+    assertContainsEvent("he following directories were also declared as files:");
+    assertContainsEvent("foo/bar/baz");
+  }
+
   /**
    * Skylark integration test that forces inlining.
    */
-- 
cgit v1.2.3