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

1 files changed, 18 insertions, 51 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/actions/Artifact.java b/src/main/java/com/google/devtools/build/lib/actions/Artifact.java
index f91c5cde5a..a974646d55 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/Artifact.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/Artifact.java
@@ -45,7 +45,6 @@ import com.google.devtools.build.lib.util.FileTypeSet;
 import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.lib.vfs.PathFragment;
 import com.google.devtools.build.skyframe.SkyFunctionName;
-import com.google.devtools.build.skyframe.SkyKey;
 import com.google.protobuf.CodedInputStream;
 import com.google.protobuf.CodedOutputStream;
 import java.io.IOException;
@@ -108,8 +107,7 @@ public class Artifact
         ActionInput,
         FileApi,
         Comparable<Object>,
-        CommandLineItem,
-        SkyKey {
+        CommandLineItem {
 
   /** Compares artifact according to their exec paths. Sorts null values first. */
   @SuppressWarnings("ReferenceEquality")  // "a == b" is an optimization
@@ -228,9 +226,6 @@ public class Artifact
       throw new IllegalArgumentException(
           "it is illegal to create an artifact with an empty execPath");
     }
-    // The ArtifactOwner is not part of this computation because it is very rare that two Artifacts
-    // have the same execPath and different owners, so a collision is fine there. If this is
-    // changed, OwnerlessArtifactWrapper must also be changed.
     this.hashCode = execPath.hashCode() + this.getClass().hashCode() * 13;
     this.root = root;
     this.execPath = execPath;
@@ -458,15 +453,6 @@ public class Artifact
     public SourceArtifact asSourceArtifact() {
       return this;
     }
-
-    /**
-     * SourceArtifacts are compared without their owners, since owners do not affect behavior,
-     * unlike with derived artifacts, whose owners determine their generating actions.
-     */
-    @Override
-    public boolean equals(Object other) {
-      return other instanceof SourceArtifact && equalsWithoutOwner((SourceArtifact) other);
-    }
   }
 
   /**
@@ -649,26 +635,36 @@ public class Artifact
   }
 
   @Override
-  public boolean equals(Object other) {
+  public final boolean equals(Object other) {
     if (!(other instanceof Artifact)) {
       return false;
     }
     if (!getClass().equals(other.getClass())) {
       return false;
     }
+    // We don't bother to check root in the equivalence relation, because we
+    // assume that no root is an ancestor of another one.
     Artifact that = (Artifact) other;
-    return equalsWithoutOwner(that) && owner.equals(that.getArtifactOwner());
+    return Objects.equals(this.execPath, that.execPath) && Objects.equals(this.root, that.root);
   }
 
-  public boolean equalsWithoutOwner(Artifact other) {
-    return Objects.equals(this.execPath, other.execPath) && Objects.equals(this.root, other.root);
+  /**
+   * Compare equality including Artifact owner equality, a notable difference compared to the
+   * {@link #equals(Object)} method of {@link Artifact}.
+   */
+  public static boolean equalWithOwner(@Nullable Artifact first, @Nullable Artifact second) {
+    if (first != null) {
+      return first.equals(second) && first.getArtifactOwner().equals(second.getArtifactOwner());
+    } else {
+      return second == null;
+    }
   }
 
   @Override
   public final int hashCode() {
-    // This is just execPath.hashCode() (along with the class). We cache a copy in the Artifact
-    // object to reduce LLC misses during operations which build a HashSet out of many Artifacts.
-    // This is a slight loss for memory but saves ~1% overall CPU in some real builds.
+    // This is just path.hashCode().  We cache a copy in the Artifact object to reduce LLC misses
+    // during operations which build a HashSet out of many Artifacts.  This is a slight loss for
+    // memory but saves ~1% overall CPU in some real builds.
     return hashCode;
   }
 
@@ -976,33 +972,4 @@ public class Artifact
       printer.append("<generated file " + rootRelativePath + ">");
     }
   }
-
-  /**
-   * A utility class that compares {@link Artifact}s without taking their owners into account.
-   * Should only be used for detecting action conflicts and merging shared action data.
-   */
-  public static class OwnerlessArtifactWrapper {
-    private final Artifact artifact;
-
-    public OwnerlessArtifactWrapper(Artifact artifact) {
-      this.artifact = artifact;
-    }
-
-    @Override
-    public int hashCode() {
-      // Depends on the fact that Artifact#hashCode does not use ArtifactOwner.
-      return artifact.hashCode();
-    }
-
-    @Override
-    public boolean equals(Object obj) {
-      return obj instanceof OwnerlessArtifactWrapper
-          && this.artifact.equalsWithoutOwner(((OwnerlessArtifactWrapper) obj).artifact);
-    }
-  }
-
-  @Override
-  public SkyFunctionName functionName() {
-    return ARTIFACT;
-  }
 }