From 33b514b25963452be71a015e08d4e890405b00a3 Mon Sep 17 00:00:00 2001 From: Chi Wang Date: Mon, 12 Dec 2022 06:36:08 -0800 Subject: [PATCH] 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 #16955. Closes #16972. PiperOrigin-RevId: 494712456 Change-Id: I9a77063f35e1f6e2559c02612790542e996994b8 --- .../lib/remote/RemoteActionFileSystem.java | 34 +++++-- ...ildWithoutTheBytesIntegrationTestBase.java | 29 +++++- .../RemoteActionFileSystemTestBase.java | 89 +++++++++++++++++++ 3 files changed, 144 insertions(+), 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 index 7916bb802322ef..4d59c2d73b75b0 100644 --- 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/src/test/java/com/google/devtools/build/lib/remote/BuildWithoutTheBytesIntegrationTestBase.java b/src/test/java/com/google/devtools/build/lib/remote/BuildWithoutTheBytesIntegrationTestBase.java index cdab72bae779bd..9e90fff84b3b2d 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/BuildWithoutTheBytesIntegrationTestBase.java +++ b/src/test/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 index 02317f5d23eea0..638faa1ed228b7 100644 --- 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); + } }