Add a sanity check to prevent creation of Artifacts with an empty path

And fix the tests that were doing this.

PiperOrigin-RevId: 179548691

5 files changed, 28 insertions, 23 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/actions/ActionInputHelper.java b/src/main/java/com/google/devtools/build/lib/actions/ActionInputHelper.java
index 49e262fcee..0f2c387102 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/ActionInputHelper.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/ActionInputHelper.java
@@ -72,6 +72,7 @@ public final class ActionInputHelper {
 
     public BasicActionInput(String path) {
       this.path = Preconditions.checkNotNull(path);
+      Preconditions.checkArgument(!path.isEmpty());
     }
 
     @Override
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 7d1a5b79e2..7ecdede085 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
@@ -189,6 +189,10 @@ public class Artifact
       throw new IllegalArgumentException(execPath + ": illegal execPath for " + path
           + " (root: " + root + ")");
     }
+    if (execPath.segmentCount() == 0) {
+      throw new IllegalArgumentException(
+          "it is illegal to create an artifact with an empty execPath");
+    }
     this.hashCode = path.hashCode();
     this.path = path;
     this.root = root;
diff --git a/src/test/java/com/google/devtools/build/lib/actions/MapBasedActionGraphTest.java b/src/test/java/com/google/devtools/build/lib/actions/MapBasedActionGraphTest.java
index 9d65341635..ab0cc4a324 100644
--- a/src/test/java/com/google/devtools/build/lib/actions/MapBasedActionGraphTest.java
+++ b/src/test/java/com/google/devtools/build/lib/actions/MapBasedActionGraphTest.java
@@ -19,7 +19,6 @@ import com.google.common.collect.Sets;
 import com.google.devtools.build.lib.actions.MutableActionGraph.ActionConflictException;
 import com.google.devtools.build.lib.actions.util.ActionsTestUtil.UncheckedActionConflictException;
 import com.google.devtools.build.lib.actions.util.TestAction;
-import com.google.devtools.build.lib.clock.BlazeClock;
 import com.google.devtools.build.lib.concurrent.AbstractQueueVisitor;
 import com.google.devtools.build.lib.concurrent.ErrorClassifier;
 import com.google.devtools.build.lib.vfs.FileSystem;
@@ -37,20 +36,21 @@ import org.junit.runners.JUnit4;
  */
 @RunWith(JUnit4.class)
 public class MapBasedActionGraphTest {
+  private final FileSystem fileSystem = new InMemoryFileSystem();
   private final ActionKeyContext actionKeyContext = new ActionKeyContext();
 
   @Test
   public void testSmoke() throws Exception {
     MutableActionGraph actionGraph = new MapBasedActionGraph(actionKeyContext);
-    FileSystem fileSystem = new InMemoryFileSystem(BlazeClock.instance());
-    Path path = fileSystem.getPath("/root/foo");
-    Artifact output = new Artifact(path, Root.asDerivedRoot(path));
+    Path root = fileSystem.getPath("/root");
+    Path path = root.getRelative("foo");
+    Artifact output = new Artifact(path, Root.asDerivedRoot(root));
     Action action = new TestAction(TestAction.NO_EFFECT,
         ImmutableSet.<Artifact>of(), ImmutableSet.of(output));
     actionGraph.registerAction(action);
     actionGraph.unregisterAction(action);
-    path = fileSystem.getPath("/root/bar");
-    output = new Artifact(path, Root.asDerivedRoot(path));
+    path = root.getRelative("bar");
+    output = new Artifact(path, Root.asDerivedRoot(root));
     Action action2 = new TestAction(TestAction.NO_EFFECT,
         ImmutableSet.<Artifact>of(), ImmutableSet.of(output));
     actionGraph.registerAction(action);
@@ -61,9 +61,9 @@ public class MapBasedActionGraphTest {
   @Test
   public void testNoActionConflictWhenUnregisteringSharedAction() throws Exception {
     MutableActionGraph actionGraph = new MapBasedActionGraph(actionKeyContext);
-    FileSystem fileSystem = new InMemoryFileSystem(BlazeClock.instance());
-    Path path = fileSystem.getPath("/root/foo");
-    Artifact output = new Artifact(path, Root.asDerivedRoot(path));
+    Path root = fileSystem.getPath("/root");
+    Path path = root.getRelative("/root/foo");
+    Artifact output = new Artifact(path, Root.asDerivedRoot(root));
     Action action = new TestAction(TestAction.NO_EFFECT,
         ImmutableSet.<Artifact>of(), ImmutableSet.of(output));
     actionGraph.registerAction(action);
@@ -73,7 +73,7 @@ public class MapBasedActionGraphTest {
     actionGraph.unregisterAction(action);
   }
 
-  private static class ActionRegisterer extends AbstractQueueVisitor {
+  private class ActionRegisterer extends AbstractQueueVisitor {
     private final MutableActionGraph graph = new MapBasedActionGraph(new ActionKeyContext());
     private final Artifact output;
     // Just to occasionally add actions that were already present.
@@ -89,11 +89,11 @@ public class MapBasedActionGraphTest {
           "action-graph-test",
           AbstractQueueVisitor.EXECUTOR_FACTORY,
           ErrorClassifier.DEFAULT);
-      FileSystem fileSystem = new InMemoryFileSystem(BlazeClock.instance());
-      Path path = fileSystem.getPath("/root/foo");
-      output = new Artifact(path, Root.asDerivedRoot(path));
-      allActions.add(new TestAction(TestAction.NO_EFFECT,
-          ImmutableSet.<Artifact>of(), ImmutableSet.of(output)));
+      Path root = fileSystem.getPath("/root");
+      Path path = root.getRelative("foo");
+      output = new Artifact(path, Root.asDerivedRoot(root));
+      allActions.add(new TestAction(
+          TestAction.NO_EFFECT, ImmutableSet.<Artifact>of(), ImmutableSet.of(output)));
     }
 
     private void registerAction(final Action action) {
@@ -130,8 +130,8 @@ public class MapBasedActionGraphTest {
       if (Math.random() < 0.5) {
         action = Iterables.getFirst(allActions, null);
       } else {
-        action = new TestAction(TestAction.NO_EFFECT,
-            ImmutableSet.<Artifact>of(), ImmutableSet.of(output));
+        action = new TestAction(
+            TestAction.NO_EFFECT, ImmutableSet.<Artifact>of(), ImmutableSet.of(output));
         allActions.add(action);
       }
       if (Math.random() < 0.5) {
diff --git a/src/test/java/com/google/devtools/build/lib/runtime/BuildEventStreamerTest.java b/src/test/java/com/google/devtools/build/lib/runtime/BuildEventStreamerTest.java
index 9588c5cbc0..541843b9fc 100644
--- a/src/test/java/com/google/devtools/build/lib/runtime/BuildEventStreamerTest.java
+++ b/src/test/java/com/google/devtools/build/lib/runtime/BuildEventStreamerTest.java
@@ -520,7 +520,7 @@ public class BuildEventStreamerTest extends FoundationTestCase {
 
   private Artifact makeArtifact(String pathString) {
     Path path = outputBase.getRelative(PathFragment.create(pathString));
-    return new Artifact(path, Root.asSourceRoot(path));
+    return new Artifact(path, Root.asSourceRoot(outputBase));
   }
 
   @Test
diff --git a/src/test/java/com/google/devtools/build/lib/runtime/ExperimentalStateTrackerTest.java b/src/test/java/com/google/devtools/build/lib/runtime/ExperimentalStateTrackerTest.java
index 06b3376fbe..9dfdb8ee9f 100644
--- a/src/test/java/com/google/devtools/build/lib/runtime/ExperimentalStateTrackerTest.java
+++ b/src/test/java/com/google/devtools/build/lib/runtime/ExperimentalStateTrackerTest.java
@@ -68,7 +68,7 @@ public class ExperimentalStateTrackerTest extends FoundationTestCase {
 
   private Action mockAction(String progressMessage, String primaryOutput) {
     Path path = outputBase.getRelative(PathFragment.create(primaryOutput));
-    Artifact artifact = new Artifact(path, Root.asSourceRoot(path));
+    Artifact artifact = new Artifact(path, Root.asSourceRoot(outputBase));
 
     Action action = Mockito.mock(Action.class);
     when(action.getProgressMessage()).thenReturn(progressMessage);
@@ -431,7 +431,7 @@ public class ExperimentalStateTrackerTest extends FoundationTestCase {
     ExperimentalStateTracker stateTracker = new ExperimentalStateTracker(clock, 70);
     Action action = mockAction(
         "Building some/very/very/long/path/for/some/library/directory/foo.jar (42 source files)",
-        "/home/user/bazel/out/abcdef/some/very/very/long/path/for/some/library/directory/foo.jar");
+        "some/very/very/long/path/for/some/library/directory/foo.jar");
     Label label =
         Label.parseAbsolute("//some/very/very/long/path/for/some/library/directory:libfoo");
     ActionOwner owner =
@@ -465,7 +465,7 @@ public class ExperimentalStateTrackerTest extends FoundationTestCase {
 
     ManualClock clock = new ManualClock();
     Path path = outputBase.getRelative(PathFragment.create(primaryOutput));
-    Artifact artifact = new Artifact(path, Root.asSourceRoot(path));
+    Artifact artifact = new Artifact(path, Root.asSourceRoot(outputBase));
     ActionExecutionMetadata actionMetadata = Mockito.mock(ActionExecutionMetadata.class);
     when(actionMetadata.getOwner()).thenReturn(Mockito.mock(ActionOwner.class));
     when(actionMetadata.getPrimaryOutput()).thenReturn(artifact);
@@ -493,10 +493,10 @@ public class ExperimentalStateTrackerTest extends FoundationTestCase {
 
     Action foobuildAction = mockAction(
         "Building //src/some/very/long/path/long/long/long/long/long/long/long/foo/foobuild.jar",
-        "//src/some/very/long/path/long/long/long/long/long/long/long/foo:foobuild");
+        "src/some/very/long/path/long/long/long/long/long/long/long/foo/foobuild.jar");
     Action bazbuildAction = mockAction(
         "Building //src/some/very/long/path/long/long/long/long/long/long/long/baz/bazbuild.jar",
-        "//src/some/very/long/path/long/long/long/long/long/long/long/baz:bazbuild");
+        "src/some/very/long/path/long/long/long/long/long/long/long/baz/bazbuild.jar");
 
     Label bartestLabel =
         Label.parseAbsolute(