Add FileSystem#createDirectoryAndParents. A native implementation of this (instead of using FileSystemUtils, which can only use public interfaces) should be more efficient and more easy to make correct. In particular, it should allow removing FileSystemUtils#createDirectoriesAndParents, which has poor thread safety characteristics. The latter method has a lot of logic that forces certain unnatural atomicity guarantees on createDirectory, and it also has logic that is conditional on sub-string content of exception messages. PiperOrigin-RevId: 179819623
diff --git a/src/main/java/com/google/devtools/build/lib/unix/NativePosixFiles.java b/src/main/java/com/google/devtools/build/lib/unix/NativePosixFiles.java index 781467d..238ef4d 100644 --- a/src/main/java/com/google/devtools/build/lib/unix/NativePosixFiles.java +++ b/src/main/java/com/google/devtools/build/lib/unix/NativePosixFiles.java
@@ -242,6 +242,15 @@ throws IOException; /** + * Implements (effectively) mkdir -p. + * + * @param path the directory to recursively create. + * @param mode the mode with which to create the directories. + * @throws IOException if the directory creation failed for any reason. + */ + public static native void mkdirs(String path, int mode) throws IOException; + + /** * Native wrapper around POSIX opendir(2)/readdir(3)/closedir(3) syscall. * * @param path the directory to read.
diff --git a/src/main/java/com/google/devtools/build/lib/unix/UnixFileSystem.java b/src/main/java/com/google/devtools/build/lib/unix/UnixFileSystem.java index 7b82dcc..0af17cf 100644 --- a/src/main/java/com/google/devtools/build/lib/unix/UnixFileSystem.java +++ b/src/main/java/com/google/devtools/build/lib/unix/UnixFileSystem.java
@@ -325,6 +325,11 @@ } @Override + public void createDirectoryAndParents(Path path) throws IOException { + NativePosixFiles.mkdirs(path.toString(), 0777); + } + + @Override protected void createSymbolicLink(Path linkPath, PathFragment targetFragment) throws IOException { synchronized (linkPath) {
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 fef88b8..e914663 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
@@ -257,6 +257,12 @@ public abstract boolean createDirectory(Path path) throws IOException; /** + * Creates all directories up to the path. See {@link Path#createDirectoryAndParents} for + * specification. + */ + public abstract void createDirectoryAndParents(Path path) throws IOException; + + /** * Returns the size in bytes of the file denoted by {@code path}. See {@link * Path#getFileSize(Symlinks)} for specification. *
diff --git a/src/main/java/com/google/devtools/build/lib/vfs/JavaIoFileSystem.java b/src/main/java/com/google/devtools/build/lib/vfs/JavaIoFileSystem.java index dca8b95..4477275 100644 --- a/src/main/java/com/google/devtools/build/lib/vfs/JavaIoFileSystem.java +++ b/src/main/java/com/google/devtools/build/lib/vfs/JavaIoFileSystem.java
@@ -258,6 +258,12 @@ } } + @Override + public void createDirectoryAndParents(Path path) throws IOException { + java.nio.file.Path nioPath = getNioPath(path); + Files.createDirectories(nioPath); + } + private boolean linkExists(File file) { String shortName = file.getName(); File parentFile = file.getParentFile();
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 1d3947d..06a884b 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
@@ -1026,6 +1026,19 @@ return fileSystem.createDirectory(this); } + /** + * Ensures that the directory with the name of the current path and all its ancestor directories + * exist. + * + * <p>Does not return whether the directory already existed or was created by some other + * concurrent call to this method. + * + * @throws IOException if the directory creation failed for any reason + */ + public void createDirectoryAndParents() throws IOException { + fileSystem.createDirectoryAndParents(this); + } + /** Prefer to use {@link #createSymbolicLink(FileSystem, Path)}. */ @Deprecated public void createSymbolicLink(Path target) throws IOException {
diff --git a/src/main/java/com/google/devtools/build/lib/vfs/ReadonlyFileSystem.java b/src/main/java/com/google/devtools/build/lib/vfs/ReadonlyFileSystem.java index bfcc4f9..938f7a7 100644 --- a/src/main/java/com/google/devtools/build/lib/vfs/ReadonlyFileSystem.java +++ b/src/main/java/com/google/devtools/build/lib/vfs/ReadonlyFileSystem.java
@@ -17,20 +17,21 @@ import java.io.OutputStream; /** - * An abstract partial implementation of FileSystem for read-only - * implementations. + * An abstract partial implementation of FileSystem for read-only implementations. * * <p>Any ReadonlyFileSystem does not support the following: + * * <ul> - * <li>{@link #createDirectory(Path)}</li> - * <li>{@link #createSymbolicLink(Path, PathFragment)}</li> - * <li>{@link #delete(Path)}</li> - * <li>{@link #getOutputStream(Path)}</li> - * <li>{@link #renameTo(Path, Path)}</li> - * <li>{@link #setExecutable(Path, boolean)}</li> - * <li>{@link #setLastModifiedTime(Path, long)}</li> - * <li>{@link #setWritable(Path, boolean)}</li> + * <li>{@link #createDirectory(Path)} + * <li>{@link #createSymbolicLink(Path, PathFragment)} + * <li>{@link #delete(Path)} + * <li>{@link #getOutputStream(Path)} + * <li>{@link #renameTo(Path, Path)} + * <li>{@link #setExecutable(Path, boolean)} + * <li>{@link #setLastModifiedTime(Path, long)} + * <li>{@link #setWritable(Path, boolean)} * </ul> + * * The above calls will always result in an {@link IOException}. */ public abstract class ReadonlyFileSystem extends AbstractFileSystem { @@ -91,6 +92,11 @@ } @Override + public void createDirectoryAndParents(Path path) throws IOException { + throw modificationException(); + } + + @Override protected void createSymbolicLink(Path linkPath, PathFragment targetFragment) throws IOException { throw modificationException(); }
diff --git a/src/main/java/com/google/devtools/build/lib/vfs/ReadonlyFileSystemWithCustomStat.java b/src/main/java/com/google/devtools/build/lib/vfs/ReadonlyFileSystemWithCustomStat.java index de5daca..f9ca4eb 100644 --- a/src/main/java/com/google/devtools/build/lib/vfs/ReadonlyFileSystemWithCustomStat.java +++ b/src/main/java/com/google/devtools/build/lib/vfs/ReadonlyFileSystemWithCustomStat.java
@@ -76,6 +76,11 @@ } @Override + public void createDirectoryAndParents(Path path) throws IOException { + throw modificationException(); + } + + @Override protected void createSymbolicLink(Path linkPath, PathFragment targetFragment) throws IOException { throw modificationException(); }
diff --git a/src/main/java/com/google/devtools/build/lib/vfs/UnionFileSystem.java b/src/main/java/com/google/devtools/build/lib/vfs/UnionFileSystem.java index 824e71d..569357b 100644 --- a/src/main/java/com/google/devtools/build/lib/vfs/UnionFileSystem.java +++ b/src/main/java/com/google/devtools/build/lib/vfs/UnionFileSystem.java
@@ -206,6 +206,13 @@ } @Override + public void createDirectoryAndParents(Path path) throws IOException { + checkModifiable(path); + FileSystem delegate = getDelegate(path); + delegate.createDirectoryAndParents(path); + } + + @Override protected long getFileSize(Path path, boolean followSymlinks) throws IOException { path = followSymlinks ? internalResolveSymlink(path) : path; FileSystem delegate = getDelegate(path);
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 ff6d88a..0008423 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
@@ -14,6 +14,7 @@ package com.google.devtools.build.lib.vfs.inmemoryfs; import com.google.common.base.Preconditions; +import com.google.common.collect.Lists; import com.google.devtools.build.lib.clock.Clock; import com.google.devtools.build.lib.clock.JavaClock; import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe; @@ -624,6 +625,22 @@ } @Override + public synchronized void createDirectoryAndParents(Path path) throws IOException { + List<Path> subdirs = new ArrayList<>(); + for (; !path.isRootDirectory(); path = path.getParentDirectory()) { + if (path.isDirectory()) { + break; + } else if (path.exists()) { + throw new IOException("Not a directory: " + path); + } + subdirs.add(path); + } + for (Path subdir : Lists.reverse(subdirs)) { + subdir.createDirectory(); + } + } + + @Override protected void createSymbolicLink(Path path, PathFragment targetFragment) throws IOException { if (path.equals(getRootDirectory())) {
diff --git a/src/main/native/unix_jni.cc b/src/main/native/unix_jni.cc index 395dcc1..571e22b 100644 --- a/src/main/native/unix_jni.cc +++ b/src/main/native/unix_jni.cc
@@ -81,28 +81,28 @@ * are replaced by '?'. Must be followed by a call to * ReleaseStringLatin1Chars. */ -static const char *GetStringLatin1Chars(JNIEnv *env, jstring jstr) { - jint len = env->GetStringLength(jstr); - const jchar *str = env->GetStringCritical(jstr, NULL); - if (str == NULL) { - return NULL; - } +static char *GetStringLatin1Chars(JNIEnv *env, jstring jstr) { + jint len = env->GetStringLength(jstr); + const jchar *str = env->GetStringCritical(jstr, NULL); + if (str == NULL) { + return NULL; + } - char *result = reinterpret_cast<char *>(malloc(len + 1)); - if (result == NULL) { - env->ReleaseStringCritical(jstr, str); - ::PostException(env, ENOMEM, "Out of memory in GetStringLatin1Chars"); - return NULL; - } - - for (int i = 0; i < len; i++) { - jchar unicode = str[i]; // (unsigned) - result[i] = unicode <= 0x00ff ? unicode : '?'; - } - - result[len] = 0; + char *result = reinterpret_cast<char *>(malloc(len + 1)); + if (result == NULL) { env->ReleaseStringCritical(jstr, str); - return result; + ::PostException(env, ENOMEM, "Out of memory in GetStringLatin1Chars"); + return NULL; + } + + for (int i = 0; i < len; i++) { + jchar unicode = str[i]; // (unsigned) + result[i] = unicode <= 0x00ff ? unicode : '?'; + } + + result[len] = 0; + env->ReleaseStringCritical(jstr, str); + return result; } /** @@ -536,6 +536,88 @@ return result; } +/* + * Class: com.google.devtools.build.lib.unix.NativePosixFiles + * Method: mkdirs + * Signature: (Ljava/lang/String;I)V + * Throws: java.io.IOException + */ +extern "C" JNIEXPORT void JNICALL +Java_com_google_devtools_build_lib_unix_NativePosixFiles_mkdirs(JNIEnv *env, + jclass clazz, + jstring path, + int mode) { + char *path_chars = GetStringLatin1Chars(env, path); + portable_stat_struct statbuf; + int len; + char *p; + + // First, check if the directory already exists and early-out. + if (portable_stat(path_chars, &statbuf) == 0) { + if (!S_ISDIR(statbuf.st_mode)) { + // Exists but is not a directory. + ::PostFileException(env, ENOTDIR, path_chars); + } + goto cleanup; + } else if (errno != ENOENT) { + ::PostFileException(env, errno, path_chars); + goto cleanup; + } + + // Find the first directory that already exists and leave a pointer just past + // it. + len = strlen(path_chars); + p = path_chars + len - 1; + for (; p > path_chars; --p) { + if (*p == '/') { + *p = 0; + int res = portable_stat(path_chars, &statbuf); + *p = '/'; + if (res == 0) { + // Exists and must be a directory, or the initial stat would have failed + // with ENOTDIR. + break; + } else if (errno != ENOENT) { + ::PostFileException(env, errno, path_chars); + goto cleanup; + } + } + } + // p now points at the '/' after the last directory that exists. + // Successively create each directory + for (const char *end = path_chars + len; p < end; ++p) { + if (*p == '/') { + *p = 0; + int res = ::mkdir(path_chars, mode); + *p = '/'; + // EEXIST is fine, just means we're racing to create the directory. + // Note that somebody could have raced to create a file here, but that + // will get handled by a ENOTDIR by a subsequent mkdir call. + if (res != 0 && errno != EEXIST) { + ::PostFileException(env, errno, path_chars); + goto cleanup; + } + } + } + if (::mkdir(path_chars, mode) != 0) { + if (errno != EEXIST) { + ::PostFileException(env, errno, path_chars); + goto cleanup; + } + if (portable_stat(path_chars, &statbuf) != 0) { + ::PostFileException(env, errno, path_chars); + goto cleanup; + } + if (!S_ISDIR(statbuf.st_mode)) { + // Exists but is not a directory. + ::PostFileException(env, ENOTDIR, path_chars); + goto cleanup; + } + } +cleanup: + ReleaseStringLatin1Chars(path_chars); +} + static jobject NewDirents(JNIEnv *env, jobjectArray names, jbyteArray types) {
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 278a288..ba4b4e8 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
@@ -20,6 +20,7 @@ import com.google.common.base.Preconditions; import com.google.common.io.BaseEncoding; +import com.google.devtools.build.lib.testutil.MoreAsserts; import com.google.devtools.build.lib.testutil.TestUtils; import com.google.devtools.build.lib.unix.NativePosixFiles; import com.google.devtools.build.lib.util.Fingerprint; @@ -144,7 +145,7 @@ if (workingPath.exists()) { removeEntireDirectory(workingPath.getPathFile()); // uses java.io.File! } - FileSystemUtils.createDirectoryAndParents(workingPath); + workingPath.createDirectoryAndParents(); } /** @@ -477,46 +478,55 @@ @Test public void testCreateDirectories() throws Exception { Path newPath = absolutize("new-dir/sub/directory"); - assertThat(FileSystemUtils.createDirectoryAndParents(newPath)).isTrue(); - } - - @Test - public void testCreateDirectoriesIsDirectory() throws Exception { - Path newPath = absolutize("new-dir/sub/directory"); - FileSystemUtils.createDirectoryAndParents(newPath); + newPath.createDirectoryAndParents(); assertThat(newPath.isDirectory()).isTrue(); } @Test public void testCreateDirectoriesIsNotFile() throws Exception { Path newPath = absolutize("new-dir/sub/directory"); - FileSystemUtils.createDirectoryAndParents(newPath); + newPath.createDirectoryAndParents(); assertThat(newPath.isFile()).isFalse(); } @Test public void testCreateDirectoriesIsNotSymbolicLink() throws Exception { Path newPath = absolutize("new-dir/sub/directory"); - FileSystemUtils.createDirectoryAndParents(newPath); + newPath.createDirectoryAndParents(); assertThat(newPath.isSymbolicLink()).isFalse(); } @Test public void testCreateDirectoriesIsEmpty() throws Exception { Path newPath = absolutize("new-dir/sub/directory"); - FileSystemUtils.createDirectoryAndParents(newPath); + newPath.createDirectoryAndParents(); assertThat(newPath.getDirectoryEntries()).isEmpty(); } @Test public void testCreateDirectoriesIsOnlyChildInParent() throws Exception { Path newPath = absolutize("new-dir/sub/directory"); - FileSystemUtils.createDirectoryAndParents(newPath); + newPath.createDirectoryAndParents(); assertThat(newPath.getParentDirectory().getDirectoryEntries()).hasSize(1); assertThat(newPath.getParentDirectory().getDirectoryEntries()).containsExactly(newPath); } @Test + public void testCreateAlreadyExistingDirectorySucceeds() throws Exception { + Path newPath = absolutize("new-dir"); + newPath.createDirectory(); + newPath.createDirectoryAndParents(); + assertThat(newPath.isDirectory()).isTrue(); + } + + @Test + public void testCreateDirectoryAtFileFails() throws Exception { + Path newPath = absolutize("file"); + FileSystemUtils.createEmptyFile(newPath); + MoreAsserts.assertThrows(IOException.class, newPath::createDirectoryAndParents); + } + + @Test public void testCreateEmptyFileIsEmpty() throws Exception { Path newPath = xEmptyDirectory.getChild("new-file"); FileSystemUtils.createEmptyFile(newPath);