Fix runfiles creation with MANIFEST when building without the bytes

`getLastModifiedTime` and `setLastModifiedTime` are used by `FileSystemUtils.copyFile` to copy files. When runfiles is disabled, `SymlinkTreeStrategy#createSymlinks` use it to copy MANIFEST file. Fixes bazelbuild#16955. Closes bazelbuild#16972. PiperOrigin-RevId: 494712456 Change-Id: I9a77063f35e1f6e2559c02612790542e996994b8
uber-common · Dec 12, 2022 · 33b514b · 33b514b
1 parent e008462
commit 33b514b
Show file tree

Hide file tree

Showing 3 changed files with 144 additions and 8 deletions.
diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteActionFileSystem.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteActionFileSystem.java
@@ -250,11 +250,27 @@ protected ReadableByteChannel createReadableByteChannel(PathFragment path) throw
 
   @Override
   public void setLastModifiedTime(PathFragment path, long newTime) throws IOException {
-    RemoteFileArtifactValue m = getRemoteMetadata(path);
-    if (m == null) {
+    FileNotFoundException remoteException = null;
+    try {
+      // We can't set mtime for a remote file, set mtime of in-memory file node instead.
+      remoteOutputTree.setLastModifiedTime(path, newTime);
+    } catch (FileNotFoundException e) {
+      remoteException = e;
+    }
+
+    FileNotFoundException localException = null;
+    try {
       super.setLastModifiedTime(path, newTime);
+    } catch (FileNotFoundException e) {
+      localException = e;
     }
-    remoteOutputTree.setLastModifiedTime(path, newTime);
+
+    if (remoteException == null || localException == null) {
+      return;
+    }
+
+    localException.addSuppressed(remoteException);
+    throw localException;
   }
 
   @Override
@@ -383,8 +399,16 @@ protected void createSymbolicLink(PathFragment linkPath, PathFragment targetFrag
 
   @Override
   protected long getLastModifiedTime(PathFragment path, boolean followSymlinks) throws IOException {
-    FileStatus stat = stat(path, followSymlinks);
-    return stat.getLastModifiedTime();
+    try {
+      // We can't get mtime for a remote file, use mtime of in-memory file node instead.
+      return remoteOutputTree
+          .getPath(path)
+          .getLastModifiedTime(followSymlinks ? Symlinks.FOLLOW : Symlinks.NOFOLLOW);
+    } catch (FileNotFoundException e) {
+      // Intentionally ignored
+    }
+
+    return super.getLastModifiedTime(path, followSymlinks);
   }
 
   @Override

diff --git a/...st/java/com/google/devtools/build/lib/remote/BuildWithoutTheBytesIntegrationTestBase.java b/...st/java/com/google/devtools/build/lib/remote/BuildWithoutTheBytesIntegrationTestBase.java
@@ -76,6 +76,32 @@ public void outputsAreNotDownloaded() throws Exception {
     assertOutputsDoNotExist("//:foobar");
   }
 
+  @Test
+  public void disableRunfiles_buildSuccessfully() throws Exception {
+    // Disable on Windows since it fails for unknown reasons.
+    // TODO(chiwang): Enable it on windows.
+    if (OS.getCurrent() == OS.WINDOWS) {
+      return;
+    }
+    write(
+        "BUILD",
+        "genrule(",
+        "  name = 'foo',",
+        "  cmd = 'echo foo > $@',",
+        "  outs = ['foo.data'],",
+        ")",
+        "sh_test(",
+        "  name = 'foobar',",
+        "  srcs = ['test.sh'],",
+        "  data = [':foo'],",
+        ")");
+    write("test.sh");
+    getWorkspace().getRelative("test.sh").setExecutable(true);
+    addOptions("--build_runfile_links", "--enable_runfiles=no");
+
+    buildTarget("//:foobar");
+  }
+
   @Test
   public void downloadOutputsWithRegex() throws Exception {
     write(
@@ -108,7 +134,6 @@ public void downloadOutputsWithRegex_treeOutput_regexMatchesTreeFile() throws Ex
     if (OS.getCurrent() == OS.WINDOWS) {
       return;
     }
-
     writeOutputDirRule();
     write(
         "BUILD",
@@ -331,7 +356,6 @@ public void dynamicExecution_stdoutIsReported() throws Exception {
     if (OS.getCurrent() == OS.WINDOWS) {
       return;
     }
-
     addOptions("--internal_spawn_scheduler");
     addOptions("--strategy=Genrule=dynamic");
     addOptions("--experimental_local_execution_delay=9999999");
@@ -527,7 +551,6 @@ public void treeOutputsFromLocalFileSystem_works() throws Exception {
     if (OS.getCurrent() == OS.WINDOWS) {
       return;
     }
-
     // Test that tree artifact generated locally can be consumed by other actions.
     // See https://github.com/bazelbuild/bazel/issues/16789
 

diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteActionFileSystemTestBase.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteActionFileSystemTestBase.java
@@ -826,4 +826,93 @@ public void chmod_localFileAndRemoteFile_changeLocal() throws IOException {
     assertThat(getLocalFileSystem(actionFs).getPath(path).isWritable()).isFalse();
     assertThat(getLocalFileSystem(actionFs).getPath(path).isExecutable()).isTrue();
   }
+
+  @Test
+  public void getLastModifiedTime_fileDoesNotExist_throwError() throws IOException {
+    var actionFs = createActionFileSystem();
+    var path = getOutputPath("file");
+
+    assertThrows(FileNotFoundException.class, () -> actionFs.getPath(path).getLastModifiedTime());
+  }
+
+  @Test
+  public void getLastModifiedTime_onlyRemoteFile_returnRemote() throws IOException {
+    var actionFs = createActionFileSystem();
+    var path = getOutputPath("file");
+    injectRemoteFile(actionFs, path, "remote-content");
+
+    var mtime = actionFs.getPath(path).getLastModifiedTime();
+
+    assertThat(mtime).isEqualTo(getRemoteFileSystem(actionFs).getPath(path).getLastModifiedTime());
+  }
+
+  @Test
+  public void getLastModifiedTime_onlyLocalFile_returnLocal() throws IOException {
+    var actionFs = createActionFileSystem();
+    var path = getOutputPath("file");
+    writeLocalFile(actionFs, path, "local-content");
+
+    var mtime = actionFs.getPath(path).getLastModifiedTime();
+
+    assertThat(mtime).isEqualTo(getLocalFileSystem(actionFs).getPath(path).getLastModifiedTime());
+  }
+
+  @Test
+  public void getLastModifiedTime_localAndRemoteFile_returnRemote() throws IOException {
+    var actionFs = createActionFileSystem();
+    var path = getOutputPath("file");
+    injectRemoteFile(actionFs, path, "remote-content");
+    writeLocalFile(actionFs, path, "local-content");
+
+    var mtime = actionFs.getPath(path).getLastModifiedTime();
+
+    assertThat(mtime).isEqualTo(getRemoteFileSystem(actionFs).getPath(path).getLastModifiedTime());
+  }
+
+  @Test
+  public void setLastModifiedTime_fileDoesNotExist_throwError() throws IOException {
+    var actionFs = createActionFileSystem();
+    var path = getOutputPath("file");
+
+    assertThrows(FileNotFoundException.class, () -> actionFs.getPath(path).setLastModifiedTime(0));
+  }
+
+  @Test
+  public void setLastModifiedTime_onlyRemoteFile_successfullySet() throws IOException {
+    var actionFs = createActionFileSystem();
+    var path = getOutputPath("file");
+    injectRemoteFile(actionFs, path, "remote-content");
+    assertThat(actionFs.getPath(path).getLastModifiedTime()).isNotEqualTo(0);
+
+    actionFs.getPath(path).setLastModifiedTime(0);
+
+    assertThat(actionFs.getPath(path).getLastModifiedTime()).isEqualTo(0);
+  }
+
+  @Test
+  public void setLastModifiedTime_onlyLocalFile_successfullySet() throws IOException {
+    var actionFs = createActionFileSystem();
+    var path = getOutputPath("file");
+    writeLocalFile(actionFs, path, "local-content");
+    assertThat(actionFs.getPath(path).getLastModifiedTime()).isNotEqualTo(0);
+
+    actionFs.getPath(path).setLastModifiedTime(0);
+
+    assertThat(actionFs.getPath(path).getLastModifiedTime()).isEqualTo(0);
+  }
+
+  @Test
+  public void setLastModifiedTime_localAndRemoteFile_changeBoth() throws IOException {
+    var actionFs = createActionFileSystem();
+    var path = getOutputPath("file");
+    injectRemoteFile(actionFs, path, "remote-content");
+    writeLocalFile(actionFs, path, "local-content");
+    assertThat(getLocalFileSystem(actionFs).getPath(path).getLastModifiedTime()).isNotEqualTo(0);
+    assertThat(getRemoteFileSystem(actionFs).getPath(path).getLastModifiedTime()).isNotEqualTo(0);
+
+    actionFs.getPath(path).setLastModifiedTime(0);
+
+    assertThat(getLocalFileSystem(actionFs).getPath(path).getLastModifiedTime()).isEqualTo(0);
+    assertThat(getRemoteFileSystem(actionFs).getPath(path).getLastModifiedTime()).isEqualTo(0);
+  }
 }