From 0ce9540a8e11e92fe5389056c06a0b49e2bf479f Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Thu, 6 Apr 2023 07:55:52 +0200 Subject: [PATCH 1/7] Use ctime in file digest cache key File digests are now additionally keyed by ctime for supported file system implementations. Since Bazel has a non-zero default for `--cache_computed_file_digests`, this may be required for correctness in cases where different files have identical mtime and inode number. For example, this can happen on Linux when files are extracted from a tar file with fixed mtime and are then replaced with `mv`, which preserves inodes. --- .../devtools/build/lib/vfs/DigestUtils.java | 6 ++ .../integration/execution_phase_tests.sh | 56 +++++++++++++++++++ 2 files changed, 62 insertions(+) diff --git a/src/main/java/com/google/devtools/build/lib/vfs/DigestUtils.java b/src/main/java/com/google/devtools/build/lib/vfs/DigestUtils.java index 6c2f2b5393a5b1..77bd98d39ce3f5 100644 --- a/src/main/java/com/google/devtools/build/lib/vfs/DigestUtils.java +++ b/src/main/java/com/google/devtools/build/lib/vfs/DigestUtils.java @@ -46,6 +46,9 @@ private static class CacheKey { /** File system identifier of the file (typically the inode number). */ private final long nodeId; + /** Last change time of the file. */ + private final long changeTime; + /** Last modification time of the file. */ private final long modifiedTime; @@ -62,6 +65,7 @@ private static class CacheKey { public CacheKey(Path path, FileStatus status) throws IOException { this.path = path.asFragment(); this.nodeId = status.getNodeId(); + this.changeTime = status.getLastChangeTime(); this.modifiedTime = status.getLastModifiedTime(); this.size = status.getSize(); } @@ -76,6 +80,7 @@ public boolean equals(Object object) { CacheKey key = (CacheKey) object; return path.equals(key.path) && nodeId == key.nodeId + && changeTime == key.changeTime && modifiedTime == key.modifiedTime && size == key.size; } @@ -86,6 +91,7 @@ public int hashCode() { int result = 17; result = 31 * result + path.hashCode(); result = 31 * result + Longs.hashCode(nodeId); + result = 31 * result + Longs.hashCode(changeTime); result = 31 * result + Longs.hashCode(modifiedTime); result = 31 * result + Longs.hashCode(size); return result; diff --git a/src/test/shell/integration/execution_phase_tests.sh b/src/test/shell/integration/execution_phase_tests.sh index 7dcf63a8e7b3f3..7baf363e41f473 100755 --- a/src/test/shell/integration/execution_phase_tests.sh +++ b/src/test/shell/integration/execution_phase_tests.sh @@ -369,4 +369,60 @@ function test_track_directory_crossing_package() { >& "$TEST_log" || fail "Expected success" expect_log "WARNING: Directory artifact foo/dir crosses package boundary into" } + +# Regression test for https://github.com/bazelbuild/bazel/issues/14723 +function test_fixed_mtime_move_detected_as_change() { + mkdir -p pkg + cat > pkg/BUILD <<'EOF' +load("rules.bzl", "my_expand") + +genrule( + name = "my_templates", + srcs = ["template_archive.tar"], + outs = ["template1"], + cmd = "tar -C $(RULEDIR) -xf $<", +) + +my_expand( + name = "expand1", + input = "template1", + output = "expanded1", + to_sub = {"test":"foo"} +) +EOF + cat > pkg/rules.bzl <<'EOF' +def _my_expand_impl(ctx): + ctx.actions.expand_template( + template = ctx.file.input, + output = ctx.outputs.output, + substitutions = ctx.attr.to_sub + ) + +my_expand = rule( + implementation = _my_expand_impl, + attrs = { + "input": attr.label(allow_single_file=True), + "output": attr.output(), + "to_sub" : attr.string_dict(), + } +) +EOF + + echo "test : alpha" > template1 + touch -t 197001010000 template1 + tar -cf pkg/template_archive_alpha.tar template1 + + echo "test : delta" > template1 + touch -t 197001010000 template1 + tar -cf pkg/template_archive_delta.tar template1 + + mv pkg/template_archive_alpha.tar pkg/template_archive.tar + bazel build //pkg:expand1 || fail "Expected success" + assert_equals "foo : alpha" "$(cat bazel-bin/pkg/expanded1)" + + mv pkg/template_archive_delta.tar pkg/template_archive.tar + bazel build //pkg:expand1 || fail "Expected success" + assert_equals "foo : delta" "$(cat bazel-bin/pkg/expanded1)" +} + run_suite "Integration tests of ${PRODUCT_NAME} using the execution phase." From 8479bf0cd610e01ac9f964cc4ad6ac4676cd2ac2 Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Thu, 6 Apr 2023 16:27:52 +0200 Subject: [PATCH 2/7] Only chmod files in ActionMetadataHandler if necessary This ensures that the ctime of the files doesn't change if no permission change is necessary, thereby improving the hit rate of the digest cache. --- .../lib/skyframe/ActionMetadataHandler.java | 3 ++- .../devtools/build/lib/unix/UnixFileSystem.java | 3 ++- .../devtools/build/lib/vfs/FileStatus.java | 12 ++++++++++++ .../lib/vfs/inmemoryfs/InMemoryContentInfo.java | 16 ++++++++++++++++ .../build/lib/windows/WindowsFileSystem.java | 6 ++++++ 5 files changed, 38 insertions(+), 2 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandler.java b/src/main/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandler.java index d3f3b7c78b7fd5..3fd606f5c4511a 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandler.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandler.java @@ -727,7 +727,8 @@ private static FileArtifactValue fileArtifactValueFromStat( } private void setPathPermissionsIfFile(Path path) throws IOException { - if (path.isFile(Symlinks.NOFOLLOW)) { + FileStatus stat = path.stat(Symlinks.NOFOLLOW); + if (stat.isFile() && stat.getPermissions() != outputPermissions.getPermissionsMode()) { setPathPermissions(path); } } 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 ced6405b1b5012..249fe92706bb78 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 @@ -120,7 +120,8 @@ public long getNodeId() { return status.getInodeNumber(); } - int getPermissions() { + @Override + public int getPermissions() { return status.getPermissions(); } diff --git a/src/main/java/com/google/devtools/build/lib/vfs/FileStatus.java b/src/main/java/com/google/devtools/build/lib/vfs/FileStatus.java index f6112beaf89a2a..5827576381cd9a 100644 --- a/src/main/java/com/google/devtools/build/lib/vfs/FileStatus.java +++ b/src/main/java/com/google/devtools/build/lib/vfs/FileStatus.java @@ -85,4 +85,16 @@ public interface FileStatus { * ought to cause the node ID of b to change, but appending / modifying b should not. */ long getNodeId() throws IOException; + + /** + * Returns the file's permissions in POSIX format (e.g. 0755) if possible without performing + * additional IO, otherwise (or if unsupported by the file system) returns -1. + * + *

If accurate group and other permissions aren't available, the returned value should attempt + * to mimic a umask of 022 (i.e. read and execute permissions extend to group and other, write + * does not). + */ + default int getPermissions() { + return -1; + } } diff --git a/src/main/java/com/google/devtools/build/lib/vfs/inmemoryfs/InMemoryContentInfo.java b/src/main/java/com/google/devtools/build/lib/vfs/inmemoryfs/InMemoryContentInfo.java index 5686308aeab5dc..51b9b65f580aad 100644 --- a/src/main/java/com/google/devtools/build/lib/vfs/inmemoryfs/InMemoryContentInfo.java +++ b/src/main/java/com/google/devtools/build/lib/vfs/inmemoryfs/InMemoryContentInfo.java @@ -120,6 +120,22 @@ public long getNodeId() { return System.identityHashCode(this); } + @Override + public final int getPermissions() { + int permissions = 0; + // Emulate the default umask of 022. + if (isReadable) { + permissions |= 0444; + } + if (isWritable) { + permissions |= 0200; + } + if (isExecutable) { + permissions |= 0111; + } + return permissions; + } + @Override public final InMemoryContentInfo inode() { return this; diff --git a/src/main/java/com/google/devtools/build/lib/windows/WindowsFileSystem.java b/src/main/java/com/google/devtools/build/lib/windows/WindowsFileSystem.java index 30afecc1c2415f..1ab7602ed80cdf 100644 --- a/src/main/java/com/google/devtools/build/lib/windows/WindowsFileSystem.java +++ b/src/main/java/com/google/devtools/build/lib/windows/WindowsFileSystem.java @@ -202,6 +202,12 @@ public long getNodeId() { // TODO(bazel-team): Consider making use of attributes.fileKey(). return -1; } + + @Override + public int getPermissions() { + // Files on Windows are implicitly readable and executable. + return 0555 | (attributes.isReadOnly() ? 0 : 0200); + } }; return status; From 4eefe9813a9b0ee0f27ddaa7cefc7b093591f75d Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Thu, 6 Apr 2023 21:52:08 +0200 Subject: [PATCH 3/7] Update test --- .../build/lib/skyframe/ActionMetadataHandlerTest.java | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandlerTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandlerTest.java index 7b8be7e77751b4..115e7b75eec945 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandlerTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandlerTest.java @@ -271,9 +271,10 @@ public void resettingOutputs() throws Exception { // Reset this output, which will make the handler stat the file again. handler.resetOutputs(ImmutableList.of(artifact)); - chmodCalls.clear(); // Permit a second chmod call for the artifact. + chmodCalls.clear(); assertThat(handler.getOutputMetadata(artifact).getSize()).isEqualTo(10); - assertThat(chmodCalls).containsExactly(outputPath, 0555); + // The handler should not have chmodded the file as it already has the correct permission. + assertThat(chmodCalls).containsExactly(); } @Test From 94039a195e417bd030dbfadb966c9c55ce4fa8fe Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Sun, 9 Apr 2023 11:01:47 +0200 Subject: [PATCH 4/7] Add further test --- .../integration/execution_phase_tests.sh | 42 +++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/src/test/shell/integration/execution_phase_tests.sh b/src/test/shell/integration/execution_phase_tests.sh index 7baf363e41f473..214e5641673f06 100755 --- a/src/test/shell/integration/execution_phase_tests.sh +++ b/src/test/shell/integration/execution_phase_tests.sh @@ -425,4 +425,46 @@ EOF assert_equals "foo : delta" "$(cat bazel-bin/pkg/expanded1)" } +# Regression test for https://github.com/bazelbuild/bazel/issues/14723 +function test_fixed_mtime_source_file() { + mkdir -p pkg + cat > pkg/BUILD <<'EOF' +load("rules.bzl", "my_expand") + +my_expand( + name = "expand1", + input = "template1", + output = "expanded1", + to_sub = {"test":"foo"} +) +EOF + cat > pkg/rules.bzl <<'EOF' +def _my_expand_impl(ctx): + ctx.actions.expand_template( + template = ctx.file.input, + output = ctx.outputs.output, + substitutions = ctx.attr.to_sub + ) + +my_expand = rule( + implementation = _my_expand_impl, + attrs = { + "input": attr.label(allow_single_file=True), + "output": attr.output(), + "to_sub" : attr.string_dict(), + } +) +EOF + + echo "test : alpha" > pkg/template1 + touch -t 197001010000 pkg/template1 + bazel build //pkg:expand1 || fail "Expected success" + assert_equals "foo : alpha" "$(cat bazel-bin/pkg/expanded1)" + + echo "test : delta" > pkg/template1 + touch -t 197001010000 pkg/template1 + bazel build //pkg:expand1 || fail "Expected success" + assert_equals "foo : delta" "$(cat bazel-bin/pkg/expanded1)" +} + run_suite "Integration tests of ${PRODUCT_NAME} using the execution phase." From 3a8661258aae7432794366a74f563b5419037deb Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Sun, 9 Apr 2023 01:00:53 +0200 Subject: [PATCH 5/7] Implement ctime on Windows --- .../lib/windows/WindowsFileOperations.java | 32 ++++++++++++ .../build/lib/windows/WindowsFileSystem.java | 5 +- src/main/native/windows/file-jni.cc | 23 ++++++++ src/main/native/windows/file.cc | 52 +++++++++++++++++++ src/main/native/windows/file.h | 17 ++++++ 5 files changed, 127 insertions(+), 2 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/windows/WindowsFileOperations.java b/src/main/java/com/google/devtools/build/lib/windows/WindowsFileOperations.java index 25cba40bd0d3a4..058b53c57d032a 100644 --- a/src/main/java/com/google/devtools/build/lib/windows/WindowsFileOperations.java +++ b/src/main/java/com/google/devtools/build/lib/windows/WindowsFileOperations.java @@ -15,7 +15,9 @@ package com.google.devtools.build.lib.windows; import com.google.devtools.build.lib.jni.JniLoader; +import java.io.FileNotFoundException; import java.io.IOException; +import java.nio.file.AccessDeniedException; /** File operations on Windows. */ public class WindowsFileOperations { @@ -82,6 +84,12 @@ public Status getStatus() { // IS_SYMLINK_OR_JUNCTION_ERROR = 1; private static final int IS_SYMLINK_OR_JUNCTION_DOES_NOT_EXIST = 2; + // Keep GET_CHANGE_TIME_* values in sync with src/main/native/windows/file.cc. + private static final int GET_CHANGE_TIME_SUCCESS = 0; + // private static final int GET_CHANGE_TIME_ERROR = 1; + private static final int GET_CHANGE_TIME_DOES_NOT_EXIST = 2; + private static final int GET_CHANGE_TIME_ACCESS_DENIED = 3; + // Keep CREATE_JUNCTION_* values in sync with src/main/native/windows/file.h. private static final int CREATE_JUNCTION_SUCCESS = 0; // CREATE_JUNCTION_ERROR = 1; @@ -114,6 +122,9 @@ public Status getStatus() { private static native int nativeIsSymlinkOrJunction( String path, boolean[] result, String[] error); + private static native int nativeGetChangeTime(String path, boolean followReparsePoints, + long[] result, String[] error); + private static native boolean nativeGetLongPath(String path, String[] result, String[] error); private static native int nativeCreateJunction(String name, String target, String[] error); @@ -143,6 +154,27 @@ public static boolean isSymlinkOrJunction(String path) throws IOException { throw new IOException(String.format("Cannot tell if '%s' is link: %s", path, error[0])); } + /** + * Returns the time at which the file was last changed, including metadata changes. + */ + public static long getLastChangeTime(String path, boolean followReparsePoints) + throws IOException { + long[] result = new long[]{0}; + String[] error = new String[]{null}; + switch (nativeGetChangeTime(asLongPath(path), followReparsePoints, result, error)) { + case GET_CHANGE_TIME_SUCCESS: + return result[0]; + case GET_CHANGE_TIME_DOES_NOT_EXIST: + throw new FileNotFoundException(path); + case GET_CHANGE_TIME_ACCESS_DENIED: + throw new AccessDeniedException(path); + default: + // This is GET_CHANGE_TIME_ERROR (1). The JNI code puts a custom message in 'error[0]'. + break; + } + throw new IOException(String.format("Cannot get last change time of '%s': %s", path, error[0])); + } + /** * Returns the long path associated with the input `path`. * diff --git a/src/main/java/com/google/devtools/build/lib/windows/WindowsFileSystem.java b/src/main/java/com/google/devtools/build/lib/windows/WindowsFileSystem.java index 1ab7602ed80cdf..0a21a404c26afd 100644 --- a/src/main/java/com/google/devtools/build/lib/windows/WindowsFileSystem.java +++ b/src/main/java/com/google/devtools/build/lib/windows/WindowsFileSystem.java @@ -156,6 +156,8 @@ protected FileStatus stat(PathFragment path, boolean followSymlinks) throws IOEx } final boolean isSymbolicLink = !followSymlinks && fileIsSymbolicLink(file); + final long lastChangeTime = WindowsFileOperations.getLastChangeTime( + getNioPath(path).toString(), followSymlinks); FileStatus status = new FileStatus() { @Override @@ -193,8 +195,7 @@ public long getLastModifiedTime() throws IOException { @Override public long getLastChangeTime() { - // This is the best we can do with Java NIO... - return attributes.lastModifiedTime().toMillis(); + return lastChangeTime; } @Override diff --git a/src/main/native/windows/file-jni.cc b/src/main/native/windows/file-jni.cc index d920b0825021ba..30ed47e9c77d88 100644 --- a/src/main/native/windows/file-jni.cc +++ b/src/main/native/windows/file-jni.cc @@ -62,6 +62,29 @@ Java_com_google_devtools_build_lib_windows_WindowsFileOperations_nativeIsSymlink return static_cast(result); } +extern "C" JNIEXPORT jint JNICALL +Java_com_google_devtools_build_lib_windows_WindowsFileOperations_nativeGetChangeTime( + JNIEnv* env, jclass clazz, jstring path, jboolean follow_reparse_points, + jlongArray result_holder, jobjectArray error_msg_holder) { + std::wstring wpath(bazel::windows::GetJavaWstring(env, path)); + std::wstring error; + jlong ctime = 0; + int result = bazel::windows::GetChangeTime( + wpath.c_str(), follow_reparse_points, reinterpret_cast(&ctime), + &error); + if (result == bazel::windows::GetChangeTimeResult::kSuccess) { + env->SetLongArrayRegion(result_holder, 0, 1, &ctime); + } else { + if (!error.empty() && CanReportError(env, error_msg_holder)) { + ReportLastError( + bazel::windows::MakeErrorMessage( + WSTR(__FILE__), __LINE__, L"nativeGetChangeTime", wpath, error), + env, error_msg_holder); + } + } + return static_cast(result); +} + extern "C" JNIEXPORT jboolean JNICALL Java_com_google_devtools_build_lib_windows_WindowsFileOperations_nativeGetLongPath( JNIEnv* env, jclass clazz, jstring path, jobjectArray result_holder, diff --git a/src/main/native/windows/file.cc b/src/main/native/windows/file.cc index 72d3e4a2263fc9..6b47579a60bce1 100644 --- a/src/main/native/windows/file.cc +++ b/src/main/native/windows/file.cc @@ -21,6 +21,7 @@ #include #include // uint8_t #include +#include #include #include @@ -119,6 +120,57 @@ int IsSymlinkOrJunction(const WCHAR* path, bool* result, wstring* error) { } } +int GetChangeTime(const WCHAR* path, bool follow_reparse_points, + int64_t* result, wstring* error) { + if (!IsAbsoluteNormalizedWindowsPath(path)) { + if (error) { + *error = + MakeErrorMessage(WSTR(__FILE__), __LINE__, L"GetChangeTime", + path, L"expected an absolute Windows path"); + } + return GetChangeTimeResult::kError; + } + + AutoHandle handle; + DWORD flags = FILE_FLAG_BACKUP_SEMANTICS; + if (!follow_reparse_points) { + flags |= FILE_FLAG_OPEN_REPARSE_POINT; + } + handle = CreateFileW( + path, 0, FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE, nullptr, + OPEN_EXISTING, flags, nullptr); + + if (!handle.IsValid()) { + DWORD err = GetLastError(); + if (err == ERROR_FILE_NOT_FOUND || err == ERROR_PATH_NOT_FOUND) { + return GetChangeTimeResult::kDoesNotExist; + } else if (err == ERROR_ACCESS_DENIED) { + return GetChangeTimeResult::kAccessDenied; + } + if (error) { + *error = MakeErrorMessage(WSTR(__FILE__), __LINE__, L"CreateFileW", path, + err); + } + return GetChangeTimeResult::kError; + } + + FILE_BASIC_INFO info; + if (!GetFileInformationByHandleEx(handle, + FileBasicInfo, + (LPVOID) &info, + sizeof(FILE_BASIC_INFO))) { + DWORD err = GetLastError(); + if (error) { + *error = MakeErrorMessage(WSTR(__FILE__), __LINE__, + L"GetFileInformationByHandleEx", path, err); + } + return GetChangeTimeResult::kError; + } + + *result = info.ChangeTime.QuadPart; + return GetChangeTimeResult::kSuccess; +} + wstring GetLongPath(const WCHAR* path, unique_ptr* result) { if (!IsAbsoluteNormalizedWindowsPath(path)) { return MakeErrorMessage(WSTR(__FILE__), __LINE__, L"GetLongPath", path, diff --git a/src/main/native/windows/file.h b/src/main/native/windows/file.h index 590137fc842107..2850357ffc6088 100644 --- a/src/main/native/windows/file.h +++ b/src/main/native/windows/file.h @@ -82,6 +82,16 @@ struct IsSymlinkOrJunctionResult { }; }; +// Keep in sync with j.c.g.devtools.build.lib.windows.WindowsFileOperations +struct GetChangeTimeResult { + enum { + kSuccess = 0, + kError = 1, + kDoesNotExist = 2, + kAccessDenied = 3, + }; +}; + // Keep in sync with j.c.g.devtools.build.lib.windows.WindowsFileOperations struct DeletePathResult { enum { @@ -136,6 +146,13 @@ struct ReadSymlinkOrJunctionResult { // see http://superuser.com/a/343079. In Bazel we only ever create junctions. int IsSymlinkOrJunction(const WCHAR* path, bool* result, wstring* error); +// Retrieves the FILETIME at which `path` was last changed, including metadata. +// +// `path` should be an absolute, normalized, Windows-style path, with "\\?\" +// prefix if it's longer than MAX_PATH. +int GetChangeTime(const WCHAR* path, bool follow_reparse_points, + int64_t* result, wstring* error); + // Computes the long version of `path` if it has any 8dot3 style components. // Returns the empty string upon success, or a human-readable error message upon // failure. From 086093cbbdae92eb14447d57158750310b54bd62 Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Sun, 9 Apr 2023 22:48:30 +0200 Subject: [PATCH 6/7] Fix sources of Unix-style paths on Windows --- .../com/google/devtools/build/lib/bazel/bzlmod/BUILD | 1 + .../build/lib/bazel/bzlmod/IndexRegistry.java | 11 ++++++++++- src/test/shell/integration/loading_phase_test.sh | 8 ++++---- 3 files changed, 15 insertions(+), 5 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BUILD b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BUILD index 218bfdf05f9202..89c0d585946e1c 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BUILD +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BUILD @@ -77,6 +77,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/bazel/repository/downloader", "//src/main/java/com/google/devtools/build/lib/cmdline", "//src/main/java/com/google/devtools/build/lib/events", + "//src/main/java/com/google/devtools/build/lib/util:os", "//src/main/java/com/google/devtools/build/lib/vfs:pathfragment", "//third_party:gson", "//third_party:guava", diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/IndexRegistry.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/IndexRegistry.java index 6cb6fc0b453c48..8656979cea06e7 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/IndexRegistry.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/IndexRegistry.java @@ -24,6 +24,7 @@ import com.google.devtools.build.lib.bazel.repository.downloader.DownloadManager; import com.google.devtools.build.lib.cmdline.RepositoryName; import com.google.devtools.build.lib.events.ExtendedEventHandler; +import com.google.devtools.build.lib.util.OS; import com.google.devtools.build.lib.vfs.PathFragment; import com.google.gson.FieldNamingPolicy; import com.google.gson.Gson; @@ -176,7 +177,15 @@ private RepoSpec createLocalPathRepoSpec( path = moduleBase + "/" + path; if (!PathFragment.isAbsolute(moduleBase)) { if (uri.getScheme().equals("file")) { - path = uri.getPath() + "/" + path; + if (uri.getPath().isEmpty() || !uri.getPath().startsWith("/")) { + throw new IOException( + String.format( + "Provided non absolute local registry path for module %s: %s", + key, uri.getPath())); + } + // Unix: file:///tmp --> /tmp + // Windows: file:///C:/tmp --> C:/tmp + path = uri.getPath().substring(OS.getCurrent() == OS.WINDOWS ? 1 : 0) + "/" + path; } else { throw new IOException(String.format("Provided non local registry for module %s", key)); } diff --git a/src/test/shell/integration/loading_phase_test.sh b/src/test/shell/integration/loading_phase_test.sh index da44fb8589b24c..15313edaddbf6e 100755 --- a/src/test/shell/integration/loading_phase_test.sh +++ b/src/test/shell/integration/loading_phase_test.sh @@ -289,24 +289,24 @@ function test_incremental_deleting_package_roots() { local -r pkg="${FUNCNAME}" mkdir -p "$pkg" || fail "could not create \"$pkg\"" - local other_root=$TEST_TMPDIR/other_root/${WORKSPACE_NAME} + local other_root=other_root/${WORKSPACE_NAME} mkdir -p $other_root/$pkg/a touch $other_root/WORKSPACE echo 'sh_library(name="external")' > $other_root/$pkg/a/BUILD mkdir -p $pkg/a echo 'sh_library(name="internal")' > $pkg/a/BUILD - bazel query --package_path=$other_root:. $pkg/a:all >& $TEST_log \ + bazel query --package_path=%workspace%/$other_root:. $pkg/a:all >& $TEST_log \ || fail "Expected success" expect_log "//$pkg/a:external" expect_not_log "//$pkg/a:internal" rm -r $other_root - bazel query --package_path=$other_root:. $pkg/a:all >& $TEST_log \ + bazel query --package_path=%workspace%/$other_root:. $pkg/a:all >& $TEST_log \ || fail "Expected success" expect_log "//$pkg/a:internal" expect_not_log "//$pkg/a:external" mkdir -p $other_root - bazel query --package_path=$other_root:. $pkg/a:all >& $TEST_log \ + bazel query --package_path=%workspace%/$other_root:. $pkg/a:all >& $TEST_log \ || fail "Expected success" expect_log "//$pkg/a:internal" expect_not_log "//$pkg/a:external" From e19dae7fa8529b5a3af30276e5504cd9c0aece7f Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Thu, 13 Apr 2023 22:39:56 +0200 Subject: [PATCH 7/7] Use statIfFound --- .../devtools/build/lib/skyframe/ActionMetadataHandler.java | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandler.java b/src/main/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandler.java index 3fd606f5c4511a..5c31012432e5fb 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandler.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandler.java @@ -727,8 +727,9 @@ private static FileArtifactValue fileArtifactValueFromStat( } private void setPathPermissionsIfFile(Path path) throws IOException { - FileStatus stat = path.stat(Symlinks.NOFOLLOW); - if (stat.isFile() && stat.getPermissions() != outputPermissions.getPermissionsMode()) { + FileStatus stat = path.statIfFound(Symlinks.NOFOLLOW); + if (stat != null && stat.isFile() + && stat.getPermissions() != outputPermissions.getPermissionsMode()) { setPathPermissions(path); } }