Remove path from `InstrumentationOutputFactory#createInstrumentatio…

…nOutput()`

Local path to the output can be derived from `destination` and `relativeTo` type. So it is not necessary to pass in the `Path` anymore.

Some test coverage is also added in `InstrumentationOutputFactoryTest`.

PiperOrigin-RevId: 693876782
Change-Id: I86dd0c95b07f1544e71da6cfb96d6e6ea8d1fdde

src/main/java/com/google/devtools/build/lib/buildtool/ExecutionTool.java

@@ -850,9 +850,8 @@ private ExplanationHandler installExplanationHandler(
               .getInstrumentationOutputFactory()
               .createInstrumentationOutput(
                   /* name= */ "explain",
-                  /* redirectDestination= */ explanationPath,
+                  /* destination= */ explanationPath,
                   DestinationRelativeTo.WORKSPACE_OR_HOME,
-                  getWorkspace().getRelative(explanationPath),
                   env,
                   getReporter(),
                   /* append= */ null,
@@ -1048,10 +1047,6 @@ private Reporter getReporter() {
     return env.getReporter();
   }
 
-  private Path getWorkspace() {
-    return env.getWorkspace();
-  }
-
   private Path getExecRoot() {
     return env.getExecRoot();
   }

src/main/java/com/google/devtools/build/lib/runtime/BlazeRuntime.java

@@ -374,7 +374,6 @@ ProfilerStartedEvent initProfiler(
                     profileName,
                     PathFragment.create(profileName),
                     DestinationRelativeTo.OUTPUT_BASE,
-                    workspace.getOutputBase().getRelative(profileName),
                     env,
                     eventHandler,
                     /* append= */ null,
@@ -394,15 +393,13 @@ ProfilerStartedEvent initProfiler(
               commandOptions.profilePath.toString().endsWith(".gz")
                   ? Format.JSON_TRACE_FILE_COMPRESSED_FORMAT
                   : Format.JSON_TRACE_FILE_FORMAT;
-          var profilePath = workspace.getWorkspace().getRelative(commandOptions.profilePath);
           profile =
               instrumentationOutputFactory.createInstrumentationOutput(
                   (format == Format.JSON_TRACE_FILE_COMPRESSED_FORMAT)
                       ? "command.profile.gz"
                       : "command.profile.json",
-                  /* redirectDestination= */ commandOptions.profilePath,
+                  /* destination= */ commandOptions.profilePath,
                   DestinationRelativeTo.WORKSPACE_OR_HOME,
-                  profilePath,
                   env,
                   eventHandler,
                   /* append= */ false,

src/main/java/com/google/devtools/build/lib/runtime/InstrumentationOutputFactory.java

@@ -88,13 +88,10 @@ public enum DestinationRelativeTo {
    * @param append Whether to open the {@link LocalInstrumentationOutput} file in append mode
    * @param internal Whether the {@link LocalInstrumentationOutput} file is a Bazel internal file.
    */
-  // TODO: b/331203854 - Remove the path parameter. Local instrumentation output should be able to
-  // figure out the path based on the destination string as well.
   public InstrumentationOutput createInstrumentationOutput(
       String name,
-      PathFragment redirectDestination,
+      PathFragment destination,
       DestinationRelativeTo destinationRelativeTo,
-      Path path,
       CommandEnvironment env,
       EventHandler eventHandler,
       @Nullable Boolean append,
@@ -108,7 +105,7 @@ public InstrumentationOutput createInstrumentationOutput(
         return redirectInstrumentationOutputBuilderSupplier
             .get()
             .setName(name)
-            .setDestination(redirectDestination)
+            .setDestination(destination)
             .setDestinationRelatedToType(destinationRelativeTo)
             .setOptions(env.getOptions())
             .build();
@@ -118,10 +115,16 @@ public InstrumentationOutput createInstrumentationOutput(
               "Redirecting to write Instrumentation Output on a different machine is not"
                   + " supported. Defaulting to writing output locally."));
     }
+
+    // Since PathFragmentConverter for flag value replaces prefixed `~/` with user's home path, the
+    // destination is either an absolute path or a path relative to output_base/workspace.
     return localInstrumentationOutputBuilderSupplier
         .get()
         .setName(name)
-        .setPath(path)
+        .setPath(
+            destinationRelativeTo.equals(DestinationRelativeTo.OUTPUT_BASE)
+                ? env.getOutputBase().getRelative(destination)
+                : env.getWorkspace().getRelative(destination))
         .setAppend(append)
         .setInternal(internal)
         .build();

src/main/java/com/google/devtools/build/lib/runtime/LocalInstrumentationOutput.java

@@ -15,6 +15,7 @@
 
 import static com.google.common.base.Preconditions.checkNotNull;
 
+import com.google.common.annotations.VisibleForTesting;
 import com.google.devtools.build.lib.buildtool.BuildResult.BuildToolLogCollection;
 import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.lib.vfs.PathFragment;
@@ -68,6 +69,11 @@ public OutputStream createOutputStream() throws IOException {
     return path.getOutputStream();
   }
 
+  @VisibleForTesting
+  public Path getPath() {
+    return path;
+  }
+
   /** Builder for {@link LocalInstrumentationOutput}. */
   public static class Builder implements InstrumentationOutputBuilder {
     private String name;

src/test/java/com/google/devtools/build/lib/runtime/InstrumentationOutputFactoryTest.java

@@ -20,12 +20,11 @@
 import com.google.devtools.build.lib.buildeventstream.BuildEventArtifactUploader;
 import com.google.devtools.build.lib.buildtool.util.BuildIntegrationTestCase;
 import com.google.devtools.build.lib.events.Event;
+import com.google.devtools.build.lib.events.EventHandler;
 import com.google.devtools.build.lib.events.EventKind;
 import com.google.devtools.build.lib.events.ExtendedEventHandler;
 import com.google.devtools.build.lib.runtime.InstrumentationOutputFactory.DestinationRelativeTo;
-import com.google.devtools.build.lib.vfs.DigestHashFunction;
 import com.google.devtools.build.lib.vfs.PathFragment;
-import com.google.devtools.build.lib.vfs.inmemoryfs.InMemoryFileSystem;
 import com.google.errorprone.annotations.CanIgnoreReturnValue;
 import com.google.testing.junit.testparameterinjector.TestParameter;
 import com.google.testing.junit.testparameterinjector.TestParameterInjector;
@@ -62,16 +61,20 @@ public void testInstrumentationOutputFactory_cannotCreateFactorIfBepSupplierUnse
         factoryBuilder::build);
   }
 
-  @Test
-  public void testInstrumentationOutputFactory_successfullyCreateLocalOutputWithConvenientLink()
-      throws Exception {
+  private static InstrumentationOutputFactory createInstrumentationOutputFactory() {
     InstrumentationOutputFactory.Builder factoryBuilder =
         new InstrumentationOutputFactory.Builder();
     factoryBuilder.setLocalInstrumentationOutputBuilderSupplier(
         LocalInstrumentationOutput.Builder::new);
     factoryBuilder.setBuildEventArtifactInstrumentationOutputBuilderSupplier(
         BuildEventArtifactInstrumentationOutput.Builder::new);
-    InstrumentationOutputFactory outputFactory = factoryBuilder.build();
+    return factoryBuilder.build();
+  }
+
+  @Test
+  public void testInstrumentationOutputFactory_successfullyCreateLocalOutputWithConvenientLink()
+      throws Exception {
+    InstrumentationOutputFactory outputFactory = createInstrumentationOutputFactory();
 
     CommandEnvironment env = runtimeWrapper.newCommand();
     InstrumentationOutput output =
@@ -85,6 +88,66 @@ public void testInstrumentationOutputFactory_successfullyCreateLocalOutputWithCo
     assertThat(env.getWorkspace().getRelative("link-to-output").isSymbolicLink()).isTrue();
   }
 
+  @Test
+  public void testInstrumentationOutputFactory_localRelativeToOutputBase() throws Exception {
+    InstrumentationOutputFactory outputFactory = createInstrumentationOutputFactory();
+
+    CommandEnvironment env = runtimeWrapper.newCommand();
+    InstrumentationOutput output =
+        outputFactory.createInstrumentationOutput(
+            /* name= */ "output-baseoutput",
+            PathFragment.create("output-baseoutput"),
+            DestinationRelativeTo.OUTPUT_BASE,
+            env,
+            mock(EventHandler.class),
+            /* append= */ null,
+            /* internal= */ null);
+
+    assertThat(output).isInstanceOf(LocalInstrumentationOutput.class);
+    assertThat(((LocalInstrumentationOutput) output).getPath())
+        .isEqualTo(env.getOutputBase().getRelative("output-baseoutput"));
+  }
+
+  @Test
+  public void testInstrumentationOutputFactory_localAbsolutePath() throws Exception {
+    InstrumentationOutputFactory outputFactory = createInstrumentationOutputFactory();
+
+    CommandEnvironment env = runtimeWrapper.newCommand();
+    InstrumentationOutput output =
+        outputFactory.createInstrumentationOutput(
+            /* name= */ "output-absolute",
+            PathFragment.create("/tmp/absolute-path-output"),
+            DestinationRelativeTo.WORKSPACE_OR_HOME,
+            env,
+            mock(EventHandler.class),
+            /* append= */ null,
+            /* internal= */ null);
+
+    assertThat(output).isInstanceOf(LocalInstrumentationOutput.class);
+    assertThat(((LocalInstrumentationOutput) output).getPath())
+        .isEqualTo(env.getRuntime().getFileSystem().getPath("/tmp/absolute-path-output"));
+  }
+
+  @Test
+  public void testInstrumentationOutputFactory_localRelativeToWorkspace() throws Exception {
+    InstrumentationOutputFactory outputFactory = createInstrumentationOutputFactory();
+
+    CommandEnvironment env = runtimeWrapper.newCommand();
+    InstrumentationOutput output =
+        outputFactory.createInstrumentationOutput(
+            /* name= */ "output-ws-relative",
+            PathFragment.create("workspace-output"),
+            DestinationRelativeTo.WORKSPACE_OR_HOME,
+            env,
+            mock(EventHandler.class),
+            /* append= */ null,
+            /* internal= */ null);
+
+    assertThat(output).isInstanceOf(LocalInstrumentationOutput.class);
+    assertThat(((LocalInstrumentationOutput) output).getPath())
+        .isEqualTo(env.getWorkspace().getRelative("workspace-output"));
+  }
+
   @Test
   public void testInstrumentationOutputFactory_successfulFactoryCreation(
       @TestParameter boolean injectRedirectOutputBuilderSupplier,
@@ -138,11 +201,8 @@ public void handle(Event event) {
     var instrumentationOutput =
         outputFactory.createInstrumentationOutput(
             /* name= */ "local",
-            /* redirectDestination= */ PathFragment.create("/file"),
+            /* destination= */ PathFragment.create("/file"),
             DestinationRelativeTo.WORKSPACE_OR_HOME,
-            createRedirectOutput && injectRedirectOutputBuilderSupplier
-                ? null
-                : new InMemoryFileSystem(DigestHashFunction.SHA256).getPath("/file"),
             env,
             eventHandler,
             /* append= */ null,