Skip to content

Commit 33b514b

Browse files
coeuvrecopybara-github
coeuvre
authored and
copybara-github
committed
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
1 parent e008462 commit 33b514b

File tree

3 files changed

+144
-8
lines changed

3 files changed

+144
-8
lines changed

‎src/main/java/com/google/devtools/build/lib/remote/RemoteActionFileSystem.java‎

Lines changed: 29 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -250,11 +250,27 @@ protected ReadableByteChannel createReadableByteChannel(PathFragment path) throw
250250

251251
@Override
252252
public void setLastModifiedTime(PathFragment path, long newTime) throws IOException {
253-
RemoteFileArtifactValue m = getRemoteMetadata(path);
254-
if (m == null) {
253+
FileNotFoundException remoteException = null;
254+
try {
255+
// We can't set mtime for a remote file, set mtime of in-memory file node instead.
256+
remoteOutputTree.setLastModifiedTime(path, newTime);
257+
} catch (FileNotFoundException e) {
258+
remoteException = e;
259+
}
260+
261+
FileNotFoundException localException = null;
262+
try {
255263
super.setLastModifiedTime(path, newTime);
264+
} catch (FileNotFoundException e) {
265+
localException = e;
256266
}
257-
remoteOutputTree.setLastModifiedTime(path, newTime);
267+
268+
if (remoteException == null || localException == null) {
269+
return;
270+
}
271+
272+
localException.addSuppressed(remoteException);
273+
throw localException;
258274
}
259275

260276
@Override
@@ -383,8 +399,16 @@ protected void createSymbolicLink(PathFragment linkPath, PathFragment targetFrag
383399

384400
@Override
385401
protected long getLastModifiedTime(PathFragment path, boolean followSymlinks) throws IOException {
386-
FileStatus stat = stat(path, followSymlinks);
387-
return stat.getLastModifiedTime();
402+
try {
403+
// We can't get mtime for a remote file, use mtime of in-memory file node instead.
404+
return remoteOutputTree
405+
.getPath(path)
406+
.getLastModifiedTime(followSymlinks ? Symlinks.FOLLOW : Symlinks.NOFOLLOW);
407+
} catch (FileNotFoundException e) {
408+
// Intentionally ignored
409+
}
410+
411+
return super.getLastModifiedTime(path, followSymlinks);
388412
}
389413

390414
@Override

‎src/test/java/com/google/devtools/build/lib/remote/BuildWithoutTheBytesIntegrationTestBase.java‎

Lines changed: 26 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -76,6 +76,32 @@ public void outputsAreNotDownloaded() throws Exception {
7676
assertOutputsDoNotExist("//:foobar");
7777
}
7878

79+
@Test
80+
public void disableRunfiles_buildSuccessfully() throws Exception {
81+
// Disable on Windows since it fails for unknown reasons.
82+
// TODO(chiwang): Enable it on windows.
83+
if (OS.getCurrent() == OS.WINDOWS) {
84+
return;
85+
}
86+
write(
87+
"BUILD",
88+
"genrule(",
89+
" name = 'foo',",
90+
" cmd = 'echo foo > $@',",
91+
" outs = ['foo.data'],",
92+
")",
93+
"sh_test(",
94+
" name = 'foobar',",
95+
" srcs = ['test.sh'],",
96+
" data = [':foo'],",
97+
")");
98+
write("test.sh");
99+
getWorkspace().getRelative("test.sh").setExecutable(true);
100+
addOptions("--build_runfile_links", "--enable_runfiles=no");
101+
102+
buildTarget("//:foobar");
103+
}
104+
79105
@Test
80106
public void downloadOutputsWithRegex() throws Exception {
81107
write(
@@ -108,7 +134,6 @@ public void downloadOutputsWithRegex_treeOutput_regexMatchesTreeFile() throws Ex
108134
if (OS.getCurrent() == OS.WINDOWS) {
109135
return;
110136
}
111-
112137
writeOutputDirRule();
113138
write(
114139
"BUILD",
@@ -331,7 +356,6 @@ public void dynamicExecution_stdoutIsReported() throws Exception {
331356
if (OS.getCurrent() == OS.WINDOWS) {
332357
return;
333358
}
334-
335359
addOptions("--internal_spawn_scheduler");
336360
addOptions("--strategy=Genrule=dynamic");
337361
addOptions("--experimental_local_execution_delay=9999999");
@@ -527,7 +551,6 @@ public void treeOutputsFromLocalFileSystem_works() throws Exception {
527551
if (OS.getCurrent() == OS.WINDOWS) {
528552
return;
529553
}
530-
531554
// Test that tree artifact generated locally can be consumed by other actions.
532555
// See https://github.com/bazelbuild/bazel/issues/16789
533556

‎src/test/java/com/google/devtools/build/lib/remote/RemoteActionFileSystemTestBase.java‎

Lines changed: 89 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -826,4 +826,93 @@ public void chmod_localFileAndRemoteFile_changeLocal() throws IOException {
826826
assertThat(getLocalFileSystem(actionFs).getPath(path).isWritable()).isFalse();
827827
assertThat(getLocalFileSystem(actionFs).getPath(path).isExecutable()).isTrue();
828828
}
829+
830+
@Test
831+
public void getLastModifiedTime_fileDoesNotExist_throwError() throws IOException {
832+
var actionFs = createActionFileSystem();
833+
var path = getOutputPath("file");
834+
835+
assertThrows(FileNotFoundException.class, () -> actionFs.getPath(path).getLastModifiedTime());
836+
}
837+
838+
@Test
839+
public void getLastModifiedTime_onlyRemoteFile_returnRemote() throws IOException {
840+
var actionFs = createActionFileSystem();
841+
var path = getOutputPath("file");
842+
injectRemoteFile(actionFs, path, "remote-content");
843+
844+
var mtime = actionFs.getPath(path).getLastModifiedTime();
845+
846+
assertThat(mtime).isEqualTo(getRemoteFileSystem(actionFs).getPath(path).getLastModifiedTime());
847+
}
848+
849+
@Test
850+
public void getLastModifiedTime_onlyLocalFile_returnLocal() throws IOException {
851+
var actionFs = createActionFileSystem();
852+
var path = getOutputPath("file");
853+
writeLocalFile(actionFs, path, "local-content");
854+
855+
var mtime = actionFs.getPath(path).getLastModifiedTime();
856+
857+
assertThat(mtime).isEqualTo(getLocalFileSystem(actionFs).getPath(path).getLastModifiedTime());
858+
}
859+
860+
@Test
861+
public void getLastModifiedTime_localAndRemoteFile_returnRemote() throws IOException {
862+
var actionFs = createActionFileSystem();
863+
var path = getOutputPath("file");
864+
injectRemoteFile(actionFs, path, "remote-content");
865+
writeLocalFile(actionFs, path, "local-content");
866+
867+
var mtime = actionFs.getPath(path).getLastModifiedTime();
868+
869+
assertThat(mtime).isEqualTo(getRemoteFileSystem(actionFs).getPath(path).getLastModifiedTime());
870+
}
871+
872+
@Test
873+
public void setLastModifiedTime_fileDoesNotExist_throwError() throws IOException {
874+
var actionFs = createActionFileSystem();
875+
var path = getOutputPath("file");
876+
877+
assertThrows(FileNotFoundException.class, () -> actionFs.getPath(path).setLastModifiedTime(0));
878+
}
879+
880+
@Test
881+
public void setLastModifiedTime_onlyRemoteFile_successfullySet() throws IOException {
882+
var actionFs = createActionFileSystem();
883+
var path = getOutputPath("file");
884+
injectRemoteFile(actionFs, path, "remote-content");
885+
assertThat(actionFs.getPath(path).getLastModifiedTime()).isNotEqualTo(0);
886+
887+
actionFs.getPath(path).setLastModifiedTime(0);
888+
889+
assertThat(actionFs.getPath(path).getLastModifiedTime()).isEqualTo(0);
890+
}
891+
892+
@Test
893+
public void setLastModifiedTime_onlyLocalFile_successfullySet() throws IOException {
894+
var actionFs = createActionFileSystem();
895+
var path = getOutputPath("file");
896+
writeLocalFile(actionFs, path, "local-content");
897+
assertThat(actionFs.getPath(path).getLastModifiedTime()).isNotEqualTo(0);
898+
899+
actionFs.getPath(path).setLastModifiedTime(0);
900+
901+
assertThat(actionFs.getPath(path).getLastModifiedTime()).isEqualTo(0);
902+
}
903+
904+
@Test
905+
public void setLastModifiedTime_localAndRemoteFile_changeBoth() throws IOException {
906+
var actionFs = createActionFileSystem();
907+
var path = getOutputPath("file");
908+
injectRemoteFile(actionFs, path, "remote-content");
909+
writeLocalFile(actionFs, path, "local-content");
910+
assertThat(getLocalFileSystem(actionFs).getPath(path).getLastModifiedTime()).isNotEqualTo(0);
911+
assertThat(getRemoteFileSystem(actionFs).getPath(path).getLastModifiedTime()).isNotEqualTo(0);
912+
913+
actionFs.getPath(path).setLastModifiedTime(0);
914+
915+
assertThat(getLocalFileSystem(actionFs).getPath(path).getLastModifiedTime()).isEqualTo(0);
916+
assertThat(getRemoteFileSystem(actionFs).getPath(path).getLastModifiedTime()).isEqualTo(0);
917+
}
829918
}

0 commit comments

Comments
 (0)