Automated rollback of commit 45b308a62f42c2c0bcfe79dcd4046c4025a31059.

*** Reason for rollback ***

Breaks Javascript compilation.

There's currently no way to iterate over a depset of Artifacts and deduplicate by identical paths in Skylark. This means that actions that want to do something once per Artifact in a depset (add a flag to the command line with the path of the Artifact for instance) will have duplicate entries if there are multiple Artifacts with the same path.

This is not a true automated rollback, since I tried to make this as minimal as possible, to avoid merge conflicts and keep some of the benefits of the rolled back CL. In particular, the tests that were changed in the original CL to give artifacts their correct owners did not need to be changed back, since the owners are still correct.

Moreover, this effectively contains rollbacks of unknown commit and https://github.com/bazelbuild/bazel/commit/39d6f89644107a8f7c080c4f4aec8527c1a73954, but keeps test coverage from those CLs as well. Comments added to CL thread where not a clean rollback (there are plenty of files not changed at all: ActionArtifactCycleReporter is the main wart, since it can still handle SkyKeys with Artifact as the argument, but Artifact is no longer the argument to any SkyKey).

RELNOTES: None

*** Original change description ***

Make Artifact#equals take the owner into account for derived artifacts.

Derived artifacts' owners are important because they are used to determine the artifact's generating action. Source artifacts' owners are not used in this way, so I left them alone.

This allows us to get rid of most uses of ArtifactSkyKey. We may be able to delete it entirely in a follow-up.

PiperOrigin-RevId: 201464780

4 files changed, 11 insertions, 38 deletions

diff --git a/src/test/java/com/google/devtools/build/lib/actions/ArtifactTest.java b/src/test/java/com/google/devtools/build/lib/actions/ArtifactTest.java
index e4bd5b9a46..abbd3472c8 100644
--- a/src/test/java/com/google/devtools/build/lib/actions/ArtifactTest.java
+++ b/src/test/java/com/google/devtools/build/lib/actions/ArtifactTest.java
@@ -20,7 +20,6 @@ import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.Iterables;
 import com.google.common.collect.Lists;
-import com.google.common.testing.EqualsTester;
 import com.google.devtools.build.lib.actions.ActionAnalysisMetadata.MiddlemanType;
 import com.google.devtools.build.lib.actions.Artifact.SourceArtifact;
 import com.google.devtools.build.lib.actions.ArtifactResolver.ArtifactResolverSupplier;
@@ -412,40 +411,6 @@ public class ArtifactTest {
     assertThat(new Artifact(scratch.file("/newRoot/foo"), root).getRoot()).isEqualTo(root);
   }
 
-  @Test
-  public void hashCodeAndEquals() throws IOException {
-    Path execRoot = scratch.getFileSystem().getPath("/");
-    ArtifactRoot root = ArtifactRoot.asDerivedRoot(execRoot, scratch.dir("/newRoot"));
-    ArtifactOwner firstOwner = () -> Label.parseAbsoluteUnchecked("//bar:bar");
-    ArtifactOwner secondOwner = () -> Label.parseAbsoluteUnchecked("//foo:foo");
-    Artifact derived1 = new Artifact(root, PathFragment.create("newRoot/shared"), firstOwner);
-    Artifact derived2 = new Artifact(root, PathFragment.create("newRoot/shared"), secondOwner);
-    ArtifactRoot sourceRoot = ArtifactRoot.asSourceRoot(Root.fromPath(root.getRoot().asPath()));
-    Artifact source1 = new SourceArtifact(sourceRoot, PathFragment.create("shared"), firstOwner);
-    Artifact source2 = new SourceArtifact(sourceRoot, PathFragment.create("shared"), secondOwner);
-    new EqualsTester()
-        .addEqualityGroup(derived1)
-        .addEqualityGroup(derived2)
-        .addEqualityGroup(source1, source2)
-        .testEquals();
-    assertThat(derived1.hashCode()).isEqualTo(derived2.hashCode());
-    assertThat(derived1.hashCode()).isNotEqualTo(source1.hashCode());
-    assertThat(source1.hashCode()).isEqualTo(source2.hashCode());
-    Artifact.OwnerlessArtifactWrapper wrapper1 = new Artifact.OwnerlessArtifactWrapper(derived1);
-    Artifact.OwnerlessArtifactWrapper wrapper2 = new Artifact.OwnerlessArtifactWrapper(derived2);
-    Artifact.OwnerlessArtifactWrapper wrapper3 = new Artifact.OwnerlessArtifactWrapper(source1);
-    Artifact.OwnerlessArtifactWrapper wrapper4 = new Artifact.OwnerlessArtifactWrapper(source2);
-    new EqualsTester()
-        .addEqualityGroup(wrapper1, wrapper2)
-        .addEqualityGroup(wrapper3, wrapper4)
-        .testEquals();
-    Path path1 = derived1.getPath();
-    Path path2 = derived2.getPath();
-    Path path3 = source1.getPath();
-    Path path4 = source2.getPath();
-    new EqualsTester().addEqualityGroup(path1, path2, path3, path4).testEquals();
-  }
-
   private Artifact createDirNameArtifact() throws Exception {
     return new Artifact(
         scratch.file("/aaa/bbb/ccc/ddd"),
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/ActionTemplateExpansionFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/ActionTemplateExpansionFunctionTest.java
index 8b32d520f9..c8f9da1a7d 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/ActionTemplateExpansionFunctionTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/ActionTemplateExpansionFunctionTest.java
@@ -32,6 +32,7 @@ import com.google.devtools.build.lib.actions.Artifact.TreeFileArtifact;
 import com.google.devtools.build.lib.actions.ArtifactOwner;
 import com.google.devtools.build.lib.actions.ArtifactPrefixConflictException;
 import com.google.devtools.build.lib.actions.ArtifactRoot;
+import com.google.devtools.build.lib.actions.ArtifactSkyKey;
 import com.google.devtools.build.lib.actions.FileArtifactValue;
 import com.google.devtools.build.lib.actions.MutableActionGraph.ActionConflictException;
 import com.google.devtools.build.lib.actions.util.ActionsTestUtil;
@@ -261,7 +262,9 @@ public final class ActionTemplateExpansionFunctionTest extends FoundationTestCas
     }
     @Override
     public SkyValue compute(SkyKey skyKey, Environment env) {
-      return Preconditions.checkNotNull(artifactValueMap.get(skyKey));
+      ArtifactSkyKey artifactSkyKey = (ArtifactSkyKey) skyKey.argument();
+      Artifact artifact = artifactSkyKey.getArtifact();
+      return Preconditions.checkNotNull(artifactValueMap.get(artifact));
     }
 
     @Override
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/ArtifactFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/ArtifactFunctionTest.java
index b6e5a369b8..8dc33e22a7 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/ArtifactFunctionTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/ArtifactFunctionTest.java
@@ -152,7 +152,10 @@ public class ArtifactFunctionTest extends ArtifactFunctionTestCase {
     actions.add(action);
     file(input2.getPath(), "contents");
     file(input1.getPath(), "source contents");
-    evaluate(Iterables.toArray(ImmutableSet.of(input2, input1, input2, tree), SkyKey.class));
+    evaluate(
+        Iterables.toArray(
+            ArtifactSkyKey.mandatoryKeys(ImmutableSet.of(input2, input1, input2, tree)),
+            SkyKey.class));
     SkyValue value = evaluateArtifactValue(output);
     assertThat(((AggregatingArtifactValue) value).getInputs())
         .containsExactly(
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/RecursiveFilesystemTraversalFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/RecursiveFilesystemTraversalFunctionTest.java
index 488c9378d0..b75009d447 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/RecursiveFilesystemTraversalFunctionTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/RecursiveFilesystemTraversalFunctionTest.java
@@ -896,8 +896,10 @@ public final class RecursiveFilesystemTraversalFunctionTest extends FoundationTe
     @Override
     public SkyValue compute(SkyKey skyKey, Environment env)
         throws SkyFunctionException, InterruptedException {
+      ArtifactSkyKey artifactKey = (ArtifactSkyKey) skyKey.argument();
+      Artifact artifact = artifactKey.getArtifact();
       try {
-        return FileArtifactValue.create(((Artifact) skyKey).getPath());
+        return FileArtifactValue.create(artifact.getPath());
       } catch (IOException e) {
         throw new SkyFunctionException(e, Transience.PERSISTENT){};
       }