From 7e1719daa0ea11d03a5a52974ce3fa24bf688d9a Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Fri, 14 Apr 2023 08:28:49 -0700 Subject: [PATCH] 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. Since Java (N)IO doesn't have support for reading file ctimes on Windows, a new method backed by a native implementation is added to `WindowsFileOperation`. Adding a call to this function to `stat` uncovered previously silent bugs where Unix-style `PathFragment`s were created on Windows: 1. Bzlmod's `createLocalRepoSpec` did not correctly extract the path from a registry's `file://` URI on Windows. 2. `--package_path` isn't usable with absolute paths on Windows as it splits on `:`. Since the flag is deprecated, this commit fixes the tests rather than the implementation. Fixes #14723 Closes #18003. PiperOrigin-RevId: 524297459 Change-Id: I96bfc0210e2f71bf8603c7b7cc5eb06a04048c85 --- .../lib/skyframe/ActionMetadataHandler.java | 3 +- .../build/lib/unix/UnixFileSystem.java | 5 +- .../devtools/build/lib/vfs/DigestUtils.java | 6 ++ .../devtools/build/lib/vfs/FileStatus.java | 12 +++ .../vfs/inmemoryfs/InMemoryContentInfo.java | 16 +++ .../lib/windows/WindowsFileOperations.java | 30 ++++++ .../build/lib/windows/WindowsFileSystem.java | 11 ++- src/main/native/windows/file-jni.cc | 23 +++++ src/main/native/windows/file.cc | 49 ++++++++++ src/main/native/windows/file.h | 17 ++++ .../skyframe/ActionMetadataHandlerTest.java | 5 +- .../integration/execution_phase_tests.sh | 98 +++++++++++++++++++ .../shell/integration/loading_phase_test.sh | 8 +- 13 files changed, 273 insertions(+), 10 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 52464a9d342500..0d494b6cff6fd4 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 @@ -665,7 +665,8 @@ private static FileArtifactValue fileArtifactValueFromStat( } private static void setPathReadOnlyAndExecutableIfFile(Path path) throws IOException { - if (path.isFile(Symlinks.NOFOLLOW)) { + FileStatus stat = path.statIfFound(Symlinks.NOFOLLOW); + if (stat != null && stat.isFile() && stat.getPermissions() != 0555) { setPathReadOnlyAndExecutable(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 f392d10800748d..3885121968ffa5 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 @@ -98,7 +98,10 @@ public long getNodeId() { return status.getInodeNumber(); } - int getPermissions() { return status.getPermissions(); } + @Override + public int getPermissions() { + return status.getPermissions(); + } @Override public String toString() { return status.toString(); } 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 d3841ea8ad0306..a428193edfa5c6 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 @@ -54,6 +54,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; @@ -70,6 +73,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(); } @@ -84,6 +88,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; } @@ -94,6 +99,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/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/WindowsFileOperations.java b/src/main/java/com/google/devtools/build/lib/windows/WindowsFileOperations.java index 25cba40bd0d3a4..7b8e691ce4500b 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,25 @@ 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 30afecc1c2415f..2c4eda788c0f22 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 @@ -202,6 +203,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; diff --git a/src/main/native/windows/file-jni.cc b/src/main/native/windows/file-jni.cc index d920b0825021ba..4ed2470c4029df 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..f20831e09bcfff 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,54 @@ 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. 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 267093cbd1ab71..075956f3ed8f79 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 @@ -300,9 +300,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.getMetadata(artifact).getSize()).isEqualTo(10); - assertThat(chmodCalls).containsExactly(outputPath); + // The handler should not have chmodded the file as it already has the correct permission. + assertThat(chmodCalls).isEmpty(); } @Test diff --git a/src/test/shell/integration/execution_phase_tests.sh b/src/test/shell/integration/execution_phase_tests.sh index 4c70716a003b58..1846edf53dacd8 100755 --- a/src/test/shell/integration/execution_phase_tests.sh +++ b/src/test/shell/integration/execution_phase_tests.sh @@ -431,4 +431,102 @@ EOF true # reset the last exit code so the test won't be considered failed } + +# 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)" +} + +# 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." diff --git a/src/test/shell/integration/loading_phase_test.sh b/src/test/shell/integration/loading_phase_test.sh index d4f83765522f63..644d46b2cbdaee 100755 --- a/src/test/shell/integration/loading_phase_test.sh +++ b/src/test/shell/integration/loading_phase_test.sh @@ -292,24 +292,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"