Automated rollback of commit 8181f0abe6591bc14957e6a941a6556fb040ca9f.
*** Reason for rollback ***
b/198749645
*** Original change description ***
Add a new `FileSystem` method to get a `SeekableByteChannel` for writing files.
Add `FileSystem::createSeekableChannel` which allows us to create a `SeekableByteChannel` for file writes. The channel is opened in truncated mode, meaning that it is not suitable for reading existing files.
PiperOrigin-RevId: 394690266
diff --git a/src/main/java/com/google/devtools/build/lib/vfs/AbstractFileSystem.java b/src/main/java/com/google/devtools/build/lib/vfs/AbstractFileSystem.java
index 1ff1435..8a4e53d 100644
--- a/src/main/java/com/google/devtools/build/lib/vfs/AbstractFileSystem.java
+++ b/src/main/java/com/google/devtools/build/lib/vfs/AbstractFileSystem.java
@@ -14,13 +14,8 @@
//
package com.google.devtools.build.lib.vfs;
-import static java.nio.file.StandardOpenOption.CREATE;
import static java.nio.file.StandardOpenOption.READ;
-import static java.nio.file.StandardOpenOption.TRUNCATE_EXISTING;
-import static java.nio.file.StandardOpenOption.WRITE;
-import com.google.common.collect.ImmutableSet;
-import com.google.common.collect.Sets;
import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe;
import com.google.devtools.build.lib.profiler.Profiler;
import com.google.devtools.build.lib.profiler.ProfilerTask;
@@ -32,11 +27,8 @@
import java.io.InputStream;
import java.io.OutputStream;
import java.nio.channels.ReadableByteChannel;
-import java.nio.channels.SeekableByteChannel;
import java.nio.file.Files;
-import java.nio.file.OpenOption;
-import java.nio.file.Paths;
-import java.nio.file.StandardOpenOption;
+import java.util.EnumSet;
/** This class implements the FileSystem interface using direct calls to the UNIX filesystem. */
@ThreadSafe
@@ -91,42 +83,21 @@
}
}
- private static final ImmutableSet<StandardOpenOption> READABLE_BYTE_CHANNEL_OPEN_OPTIONS =
- Sets.immutableEnumSet(READ);
- private static final ImmutableSet<ProfilerTask> READABLE_BYTE_CHANNEL_PROFILER_TASKS =
- Sets.immutableEnumSet(ProfilerTask.VFS_OPEN, ProfilerTask.VFS_READ);
- private static final ImmutableSet<StandardOpenOption> READ_WRITE_BYTE_CHANNEL_OPEN_OPTIONS =
- Sets.immutableEnumSet(READ, WRITE, CREATE, TRUNCATE_EXISTING);
- private static final ImmutableSet<ProfilerTask> READ_WRITE_BYTE_CHANNEL_PROFILER_TASKS =
- Sets.immutableEnumSet(ProfilerTask.VFS_OPEN, ProfilerTask.VFS_READ, ProfilerTask.VFS_WRITE);
-
@Override
protected ReadableByteChannel createReadableByteChannel(PathFragment path) throws IOException {
- return createSeekableByteChannelInternal(
- path, READABLE_BYTE_CHANNEL_OPEN_OPTIONS, READABLE_BYTE_CHANNEL_PROFILER_TASKS);
- }
-
- @Override
- protected SeekableByteChannel createReadWriteByteChannel(PathFragment path) throws IOException {
- return createSeekableByteChannelInternal(
- path, READ_WRITE_BYTE_CHANNEL_OPEN_OPTIONS, READ_WRITE_BYTE_CHANNEL_PROFILER_TASKS);
- }
-
- private static SeekableByteChannel createSeekableByteChannelInternal(
- PathFragment path,
- ImmutableSet<? extends OpenOption> options,
- ImmutableSet<ProfilerTask> profilerTasks)
- throws IOException {
- String name = path.getPathString();
- if (!profiler.isActive() || profilerTasks.stream().noneMatch(profiler::isProfiling)) {
- return Files.newByteChannel(Paths.get(name), options);
- }
- long startTime = Profiler.nanoTimeMaybe();
- try {
- // Currently, we do not proxy SeekableByteChannel for profiling.
- return Files.newByteChannel(Paths.get(name), options);
- } finally {
- profiler.logSimpleTask(startTime, ProfilerTask.VFS_OPEN, name);
+ final String name = path.toString();
+ if (profiler.isActive()
+ && (profiler.isProfiling(ProfilerTask.VFS_READ)
+ || profiler.isProfiling(ProfilerTask.VFS_OPEN))) {
+ long startTime = Profiler.nanoTimeMaybe();
+ try {
+ // Currently, we do not proxy ReadableByteChannel for profiling.
+ return Files.newByteChannel(java.nio.file.Paths.get(name), EnumSet.of(READ));
+ } finally {
+ profiler.logSimpleTask(startTime, ProfilerTask.VFS_OPEN, name);
+ }
+ } else {
+ return Files.newByteChannel(java.nio.file.Paths.get(name));
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/vfs/BUILD b/src/main/java/com/google/devtools/build/lib/vfs/BUILD
index 2f1c648..bcd1791 100644
--- a/src/main/java/com/google/devtools/build/lib/vfs/BUILD
+++ b/src/main/java/com/google/devtools/build/lib/vfs/BUILD
@@ -59,7 +59,6 @@
"//third_party:caffeine",
"//third_party:guava",
"//third_party:jsr305",
- "//third_party/protobuf:protobuf_java",
],
)
diff --git a/src/main/java/com/google/devtools/build/lib/vfs/FileSystem.java b/src/main/java/com/google/devtools/build/lib/vfs/FileSystem.java
index 67a225b..283e87b 100644
--- a/src/main/java/com/google/devtools/build/lib/vfs/FileSystem.java
+++ b/src/main/java/com/google/devtools/build/lib/vfs/FileSystem.java
@@ -28,7 +28,6 @@
import java.io.InputStreamReader;
import java.io.OutputStream;
import java.nio.channels.ReadableByteChannel;
-import java.nio.channels.SeekableByteChannel;
import java.nio.file.FileAlreadyExistsException;
import java.util.Collection;
import java.util.Iterator;
@@ -721,16 +720,6 @@
}
/**
- * Returns a {@link SeekableByteChannel} for writing to a file at provided path.
- *
- * <p>Truncates the target file, therefore it cannot be used to read already existing files.
- * Please use {@link #createReadableByteChannel} to get a {@linkplain ReadableByteChannel channel}
- * for reads instead.
- */
- protected abstract SeekableByteChannel createReadWriteByteChannel(PathFragment path)
- throws IOException;
-
- /**
* Creates an OutputStream accessing the file denoted by path.
*
* @throws IOException if there was an error opening the file for writing
diff --git a/src/main/java/com/google/devtools/build/lib/vfs/Path.java b/src/main/java/com/google/devtools/build/lib/vfs/Path.java
index bfc4161..71d21c6 100644
--- a/src/main/java/com/google/devtools/build/lib/vfs/Path.java
+++ b/src/main/java/com/google/devtools/build/lib/vfs/Path.java
@@ -32,7 +32,6 @@
import java.io.OutputStream;
import java.io.Serializable;
import java.nio.channels.ReadableByteChannel;
-import java.nio.channels.SeekableByteChannel;
import java.util.ArrayList;
import java.util.Collection;
import javax.annotation.Nullable;
@@ -791,21 +790,10 @@
}
/**
- * Opens the file denoted by this path, following symbolic links, for reading and writing and
- * returns a file channel for it.
- *
- * <p>Truncates the file, therefore it cannot be used to read already existing files. Please use
- * {@link #createReadableByteChannel} to get a {@linkplain ReadableByteChannel channel} for reads
- * instead.
- */
- public SeekableByteChannel createReadWriteByteChannel() throws IOException {
- return fileSystem.createReadWriteByteChannel(asFragment());
- }
-
- /**
* Returns a java.io.File representation of this path.
*
- * <p>Caveat: the result may be useless if this path's getFileSystem() is not the UNIX filesystem.
+ * <p>Caveat: the result may be useless if this path's getFileSystem() is not
+ * the UNIX filesystem.
*/
public File getPathFile() {
return new File(getPathString());
diff --git a/src/main/java/com/google/devtools/build/lib/vfs/PathTransformingDelegateFileSystem.java b/src/main/java/com/google/devtools/build/lib/vfs/PathTransformingDelegateFileSystem.java
index 063ecc6..3070239 100644
--- a/src/main/java/com/google/devtools/build/lib/vfs/PathTransformingDelegateFileSystem.java
+++ b/src/main/java/com/google/devtools/build/lib/vfs/PathTransformingDelegateFileSystem.java
@@ -19,7 +19,6 @@
import java.io.InputStream;
import java.io.OutputStream;
import java.nio.channels.ReadableByteChannel;
-import java.nio.channels.SeekableByteChannel;
import java.util.Collection;
/**
@@ -179,11 +178,6 @@
}
@Override
- protected SeekableByteChannel createReadWriteByteChannel(PathFragment path) throws IOException {
- return delegateFs.createReadWriteByteChannel(toDelegatePath(path));
- }
-
- @Override
protected OutputStream getOutputStream(PathFragment path, boolean append) throws IOException {
return delegateFs.getOutputStream(toDelegatePath(path), append);
}
diff --git a/src/main/java/com/google/devtools/build/lib/vfs/inmemoryfs/InMemoryFileSystem.java b/src/main/java/com/google/devtools/build/lib/vfs/inmemoryfs/InMemoryFileSystem.java
index 39eb14f..097f7fa 100644
--- a/src/main/java/com/google/devtools/build/lib/vfs/inmemoryfs/InMemoryFileSystem.java
+++ b/src/main/java/com/google/devtools/build/lib/vfs/inmemoryfs/InMemoryFileSystem.java
@@ -35,7 +35,6 @@
import java.io.InputStream;
import java.io.OutputStream;
import java.nio.channels.ReadableByteChannel;
-import java.nio.channels.SeekableByteChannel;
import java.util.ArrayDeque;
import java.util.ArrayList;
import java.util.Collection;
@@ -612,12 +611,6 @@
}
@Override
- protected synchronized SeekableByteChannel createReadWriteByteChannel(PathFragment path) {
- // It's feasible to implement, but so far it is not needed.
- throw new UnsupportedOperationException("Not implemented");
- }
-
- @Override
protected synchronized byte[] getFastDigest(PathFragment path) throws IOException {
return statFile(path).getFastDigest();
}
diff --git a/src/test/java/com/google/devtools/build/lib/vfs/BUILD b/src/test/java/com/google/devtools/build/lib/vfs/BUILD
index 662b976..49defcd 100644
--- a/src/test/java/com/google/devtools/build/lib/vfs/BUILD
+++ b/src/test/java/com/google/devtools/build/lib/vfs/BUILD
@@ -66,8 +66,10 @@
"//src/test/java/com/google/devtools/build/lib/testutil:TestThread",
"//src/test/java/com/google/devtools/build/lib/testutil:TestUtils",
"//src/test/java/com/google/devtools/build/lib/vfs/util",
+ "//third_party:caffeine",
"//third_party:guava",
"//third_party:guava-testlib",
+ "//third_party:jsr305",
"//third_party:junit4",
"//third_party:mockito",
"//third_party:truth",
@@ -88,7 +90,6 @@
"//third_party:guava",
"//third_party:junit4",
"//third_party:truth",
- "@com_google_testparameterinjector//:testparameterinjector",
],
)
@@ -146,6 +147,5 @@
"//third_party:guava",
"//third_party:junit4",
"//third_party:truth",
- "@com_google_testparameterinjector//:testparameterinjector",
],
)
diff --git a/src/test/java/com/google/devtools/build/lib/vfs/FileSystemTest.java b/src/test/java/com/google/devtools/build/lib/vfs/FileSystemTest.java
index 09fc304..b4f6f60 100644
--- a/src/test/java/com/google/devtools/build/lib/vfs/FileSystemTest.java
+++ b/src/test/java/com/google/devtools/build/lib/vfs/FileSystemTest.java
@@ -14,9 +14,7 @@
//
package com.google.devtools.build.lib.vfs;
-import static com.google.common.base.Preconditions.checkArgument;
import static com.google.common.truth.Truth.assertThat;
-import static java.nio.charset.StandardCharsets.ISO_8859_1;
import static java.nio.charset.StandardCharsets.UTF_8;
import static org.junit.Assert.assertThrows;
import static org.junit.Assert.fail;
@@ -28,9 +26,6 @@
import com.google.devtools.build.lib.unix.FileStatus;
import com.google.devtools.build.lib.unix.NativePosixFiles;
import com.google.devtools.build.lib.util.Fingerprint;
-import com.google.testing.junit.testparameterinjector.TestParameter;
-import com.google.testing.junit.testparameterinjector.TestParameter.TestParameterValuesProvider;
-import com.google.testing.junit.testparameterinjector.TestParameterInjector;
import java.io.File;
import java.io.FileNotFoundException;
import java.io.IOException;
@@ -38,10 +33,9 @@
import java.io.OutputStream;
import java.nio.ByteBuffer;
import java.nio.channels.ReadableByteChannel;
-import java.nio.channels.SeekableByteChannel;
-import java.nio.channels.WritableByteChannel;
import java.nio.charset.StandardCharsets;
import java.nio.file.FileAlreadyExistsException;
+import java.util.Collection;
import java.util.concurrent.CountDownLatch;
import java.util.concurrent.atomic.AtomicBoolean;
import java.util.regex.Matcher;
@@ -50,13 +44,17 @@
import org.junit.Before;
import org.junit.Test;
import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+import org.junit.runners.Parameterized.Parameter;
+import org.junit.runners.Parameterized.Parameters;
/**
* This class handles the generic tests that any filesystem must pass.
*
- * <p>Each filesystem-test should inherit from this class, thereby obtaining all the tests.
+ * <p>Each filesystem-test should inherit from this class, thereby obtaining
+ * all the tests.
*/
-@RunWith(TestParameterInjector.class)
+@RunWith(Parameterized.class)
public abstract class FileSystemTest {
private long savedTime;
@@ -71,16 +69,19 @@
protected Path xNonEmptyDirectoryFoo;
protected Path xEmptyDirectory;
- @TestParameter(valuesProvider = DigestHashFunctionsProvider.class)
- public DigestHashFunction digestHashFunction;
-
- private static final class DigestHashFunctionsProvider implements TestParameterValuesProvider {
- @Override
- public ImmutableList<?> provideValues() {
- return DigestHashFunction.getPossibleHashFunctions().asList();
- }
+ @Parameters(name = "{index}: digestHashFunction={0}")
+ public static Collection<DigestHashFunction[]> hashFunctions() {
+ // TODO(b/112537387): Remove the array-ification and return Collection<DigestHashFunction>. This
+ // is possible in Junit4.12, but 4.11 requires the array. Bazel 0.18 will have Junit4.12, so
+ // this can change then.
+ return DigestHashFunction.getPossibleHashFunctions()
+ .stream()
+ .map(dhf -> new DigestHashFunction[] {dhf})
+ .collect(ImmutableList.toImmutableList());
}
+ @Parameter public DigestHashFunction digestHashFunction;
+
@Before
public final void createDirectories() throws Exception {
testFS = getFreshFileSystem(digestHashFunction);
@@ -1320,74 +1321,6 @@
}
}
- @Test
- public void testCreateReadWriteByteChannelWrite(@TestParameter boolean overwrite)
- throws Exception {
- String text = "hello";
- Path file = overwrite ? xFile : xNothing;
- try (SeekableByteChannel channel = file.createReadWriteByteChannel()) {
- writeToChannelAsLatin1(channel, text);
- assertThat(channel.position()).isEqualTo(text.length());
- }
-
- assertThat(FileSystemUtils.readContent(file, ISO_8859_1)).isEqualTo("hello");
- }
-
- @Test
- public void testCreateReadWriteByteChannelWriteAfterSeek() throws Exception {
- try (SeekableByteChannel channel = xNothing.createReadWriteByteChannel()) {
- writeToChannelAsLatin1(channel, "01234567890");
- channel.position(5);
- writeToChannelAsLatin1(channel, "hello!");
- assertThat(channel.position()).isEqualTo(5 + "hello!".length());
- }
-
- assertThat(FileSystemUtils.readContent(xNothing, ISO_8859_1)).isEqualTo("01234hello!");
- }
-
- @Test
- public void testCreateReadWriteByteChannelRead(@TestParameter({"0", "5", "12"}) int seekPosition)
- throws Exception {
- String text = "hello there!";
- try (SeekableByteChannel channel = xNothing.createReadWriteByteChannel()) {
- writeToChannelAsLatin1(channel, text);
- channel.position(seekPosition);
-
- String read = readAllAsString(channel, text.length() - seekPosition);
- assertThat(channel.position()).isEqualTo(text.length());
- assertThat(read).isEqualTo(text.substring(seekPosition));
- }
- }
-
- private static void writeToChannelAsLatin1(WritableByteChannel channel, String text)
- throws IOException {
- byte[] bytes = text.getBytes(ISO_8859_1);
- ByteBuffer buffer = ByteBuffer.wrap(bytes);
- int toWrite = bytes.length;
- while (toWrite > 0) {
- toWrite -= channel.write(buffer);
- }
- assertThat(toWrite).isEqualTo(0);
- assertThat(buffer.remaining()).isEqualTo(0);
- }
-
- private static String readAllAsString(ReadableByteChannel channel, int expectedSize)
- throws IOException {
- checkArgument(expectedSize >= 0, "negative expected size: %s", expectedSize);
- // +1 to make sure we can observe EOF -- Channel::read will always return 0 for a full buffer.
- ByteBuffer buffer = ByteBuffer.allocate(expectedSize + 1);
- int totalRead = 0;
- for (; ; ) {
- int read = channel.read(buffer);
- if (read == -1) {
- assertThat(totalRead).isEqualTo(expectedSize);
- return new String(buffer.array(), 0, expectedSize, ISO_8859_1);
- }
- totalRead += read;
- assertThat(buffer.position()).isEqualTo(totalRead);
- }
- }
-
private static int readFromChannel(ReadableByteChannel channel, ByteBuffer buffer, int expected)
throws IOException {
int numRead = 0;
diff --git a/src/test/java/com/google/devtools/build/lib/vfs/inmemoryfs/InMemoryFileSystemTest.java b/src/test/java/com/google/devtools/build/lib/vfs/inmemoryfs/InMemoryFileSystemTest.java
index 827e3f2..e1654a0 100644
--- a/src/test/java/com/google/devtools/build/lib/vfs/inmemoryfs/InMemoryFileSystemTest.java
+++ b/src/test/java/com/google/devtools/build/lib/vfs/inmemoryfs/InMemoryFileSystemTest.java
@@ -26,7 +26,6 @@
import com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.build.lib.vfs.PathFragment;
import com.google.devtools.build.lib.vfs.SymlinkAwareFileSystemTest;
-import com.google.testing.junit.testparameterinjector.TestParameter;
import java.io.BufferedReader;
import java.io.IOException;
import java.io.InputStreamReader;
@@ -65,8 +64,8 @@
}
/**
- * Tests concurrent creation of a substantial tree hierarchy including files, directories,
- * symlinks, file contents, and permissions.
+ * Tests concurrent creation of a substantial tree hierarchy including
+ * files, directories, symlinks, file contents, and permissions.
*/
@Test
public void testConcurrentTreeConstruction() throws Exception {
@@ -411,19 +410,4 @@
assertThat(symlink.getxattr("some.xattr")).isNull();
}
-
- // createSeekableByteChannel is not supported by InMemoryFileSystem.
- @Test
- @Override
- public void testCreateReadWriteByteChannelWrite(@TestParameter boolean overwrite) {}
-
- // createSeekableByteChannel is not supported by InMemoryFileSystem.
- @Test
- @Override
- public void testCreateReadWriteByteChannelWriteAfterSeek() {}
-
- // createSeekableByteChannel is not supported by InMemoryFileSystem.
- @Test
- @Override
- public void testCreateReadWriteByteChannelRead(@TestParameter({"0"}) int seekPosition) {}
}
