Skip to content

Commit

Permalink
Fix runfiles creation with MANIFEST when building without the bytes
Browse files Browse the repository at this point in the history 
`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
  • Loading branch information
@coeuvre @copybara-github
coeuvre authored and copybara-github committed Dec 12, 2022
1 parent e008462 commit 33b514b
Show file tree
Hide file tree
Showing 3 changed files with 144 additions and 8 deletions.
34 changes: 29 additions & 5 deletions src/main/java/com/google/devtools/build/lib/remote/RemoteActionFileSystem.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
29 changes: 26 additions & 3 deletions ...st/java/com/google/devtools/build/lib/remote/BuildWithoutTheBytesIntegrationTestBase.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down Expand Up @@ -108,7 +134,6 @@ public void downloadOutputsWithRegex_treeOutput_regexMatchesTreeFile() throws Ex
if (OS.getCurrent() == OS.WINDOWS) {
return;
}

writeOutputDirRule();
write(
"BUILD",
Expand Down Expand Up @@ -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");
Expand Down Expand Up @@ -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

Expand Down
89 changes: 89 additions & 0 deletions src/test/java/com/google/devtools/build/lib/remote/RemoteActionFileSystemTestBase.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}

0 comments on commit 33b514b

Please sign in to comment.