Skip to content

Commit a35f592

Browse files
fmeumfmeum
authored
Use ctime in file digest cache key (bazelbuild#18115)
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 bazelbuild#14723 Closes bazelbuild#18003. PiperOrigin-RevId: 524297459 Change-Id: I96bfc0210e2f71bf8603c7b7cc5eb06a04048c85
1 parent 6ba54ac commit a35f592

File tree

13 files changed

+273
-10
lines changed

13 files changed

+273
-10
lines changed

‎src/main/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandler.java‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -665,7 +665,8 @@ private static FileArtifactValue fileArtifactValueFromStat(
665665
}
666666

667667
private static void setPathReadOnlyAndExecutableIfFile(Path path) throws IOException {
668-
if (path.isFile(Symlinks.NOFOLLOW)) {
668+
FileStatus stat = path.statIfFound(Symlinks.NOFOLLOW);
669+
if (stat != null && stat.isFile() && stat.getPermissions() != 0555) {
669670
setPathReadOnlyAndExecutable(path);
670671
}
671672
}

‎src/main/java/com/google/devtools/build/lib/unix/UnixFileSystem.java‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -98,7 +98,10 @@ public long getNodeId() {
9898
return status.getInodeNumber();
9999
}
100100

101-
int getPermissions() { return status.getPermissions(); }
101+
@Override
102+
public int getPermissions() {
103+
return status.getPermissions();
104+
}
102105

103106
@Override
104107
public String toString() { return status.toString(); }

‎src/main/java/com/google/devtools/build/lib/vfs/DigestUtils.java‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -54,6 +54,9 @@ private static class CacheKey {
5454
/** File system identifier of the file (typically the inode number). */
5555
private final long nodeId;
5656

57+
/** Last change time of the file. */
58+
private final long changeTime;
59+
5760
/** Last modification time of the file. */
5861
private final long modifiedTime;
5962

@@ -70,6 +73,7 @@ private static class CacheKey {
7073
public CacheKey(Path path, FileStatus status) throws IOException {
7174
this.path = path.asFragment();
7275
this.nodeId = status.getNodeId();
76+
this.changeTime = status.getLastChangeTime();
7377
this.modifiedTime = status.getLastModifiedTime();
7478
this.size = status.getSize();
7579
}
@@ -84,6 +88,7 @@ public boolean equals(Object object) {
8488
CacheKey key = (CacheKey) object;
8589
return path.equals(key.path)
8690
&& nodeId == key.nodeId
91+
&& changeTime == key.changeTime
8792
&& modifiedTime == key.modifiedTime
8893
&& size == key.size;
8994
}
@@ -94,6 +99,7 @@ public int hashCode() {
9499
int result = 17;
95100
result = 31 * result + path.hashCode();
96101
result = 31 * result + Longs.hashCode(nodeId);
102+
result = 31 * result + Longs.hashCode(changeTime);
97103
result = 31 * result + Longs.hashCode(modifiedTime);
98104
result = 31 * result + Longs.hashCode(size);
99105
return result;

‎src/main/java/com/google/devtools/build/lib/vfs/FileStatus.java‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -85,4 +85,16 @@ public interface FileStatus {
8585
* ought to cause the node ID of b to change, but appending / modifying b should not.
8686
*/
8787
long getNodeId() throws IOException;
88+
89+
/**
90+
* Returns the file's permissions in POSIX format (e.g. 0755) if possible without performing
91+
* additional IO, otherwise (or if unsupported by the file system) returns -1.
92+
*
93+
* <p>If accurate group and other permissions aren't available, the returned value should attempt
94+
* to mimic a umask of 022 (i.e. read and execute permissions extend to group and other, write
95+
* does not).
96+
*/
97+
default int getPermissions() {
98+
return -1;
99+
}
88100
}

‎src/main/java/com/google/devtools/build/lib/vfs/inmemoryfs/InMemoryContentInfo.java‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -120,6 +120,22 @@ public long getNodeId() {
120120
return System.identityHashCode(this);
121121
}
122122

123+
@Override
124+
public final int getPermissions() {
125+
int permissions = 0;
126+
// Emulate the default umask of 022.
127+
if (isReadable) {
128+
permissions |= 0444;
129+
}
130+
if (isWritable) {
131+
permissions |= 0200;
132+
}
133+
if (isExecutable) {
134+
permissions |= 0111;
135+
}
136+
return permissions;
137+
}
138+
123139
@Override
124140
public final InMemoryContentInfo inode() {
125141
return this;

‎src/main/java/com/google/devtools/build/lib/windows/WindowsFileOperations.java‎

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,9 @@
1515
package com.google.devtools.build.lib.windows;
1616

1717
import com.google.devtools.build.lib.jni.JniLoader;
18+
import java.io.FileNotFoundException;
1819
import java.io.IOException;
20+
import java.nio.file.AccessDeniedException;
1921

2022
/** File operations on Windows. */
2123
public class WindowsFileOperations {
@@ -82,6 +84,12 @@ public Status getStatus() {
8284
// IS_SYMLINK_OR_JUNCTION_ERROR = 1;
8385
private static final int IS_SYMLINK_OR_JUNCTION_DOES_NOT_EXIST = 2;
8486

87+
// Keep GET_CHANGE_TIME_* values in sync with src/main/native/windows/file.cc.
88+
private static final int GET_CHANGE_TIME_SUCCESS = 0;
89+
// private static final int GET_CHANGE_TIME_ERROR = 1;
90+
private static final int GET_CHANGE_TIME_DOES_NOT_EXIST = 2;
91+
private static final int GET_CHANGE_TIME_ACCESS_DENIED = 3;
92+
8593
// Keep CREATE_JUNCTION_* values in sync with src/main/native/windows/file.h.
8694
private static final int CREATE_JUNCTION_SUCCESS = 0;
8795
// CREATE_JUNCTION_ERROR = 1;
@@ -114,6 +122,9 @@ public Status getStatus() {
114122
private static native int nativeIsSymlinkOrJunction(
115123
String path, boolean[] result, String[] error);
116124

125+
private static native int nativeGetChangeTime(
126+
String path, boolean followReparsePoints, long[] result, String[] error);
127+
117128
private static native boolean nativeGetLongPath(String path, String[] result, String[] error);
118129

119130
private static native int nativeCreateJunction(String name, String target, String[] error);
@@ -143,6 +154,25 @@ public static boolean isSymlinkOrJunction(String path) throws IOException {
143154
throw new IOException(String.format("Cannot tell if '%s' is link: %s", path, error[0]));
144155
}
145156

157+
/** Returns the time at which the file was last changed, including metadata changes. */
158+
public static long getLastChangeTime(String path, boolean followReparsePoints)
159+
throws IOException {
160+
long[] result = new long[] {0};
161+
String[] error = new String[] {null};
162+
switch (nativeGetChangeTime(asLongPath(path), followReparsePoints, result, error)) {
163+
case GET_CHANGE_TIME_SUCCESS:
164+
return result[0];
165+
case GET_CHANGE_TIME_DOES_NOT_EXIST:
166+
throw new FileNotFoundException(path);
167+
case GET_CHANGE_TIME_ACCESS_DENIED:
168+
throw new AccessDeniedException(path);
169+
default:
170+
// This is GET_CHANGE_TIME_ERROR (1). The JNI code puts a custom message in 'error[0]'.
171+
break;
172+
}
173+
throw new IOException(String.format("Cannot get last change time of '%s': %s", path, error[0]));
174+
}
175+
146176
/**
147177
* Returns the long path associated with the input `path`.
148178
*

‎src/main/java/com/google/devtools/build/lib/windows/WindowsFileSystem.java‎

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -156,6 +156,8 @@ protected FileStatus stat(PathFragment path, boolean followSymlinks) throws IOEx
156156
}
157157

158158
final boolean isSymbolicLink = !followSymlinks && fileIsSymbolicLink(file);
159+
final long lastChangeTime =
160+
WindowsFileOperations.getLastChangeTime(getNioPath(path).toString(), followSymlinks);
159161
FileStatus status =
160162
new FileStatus() {
161163
@Override
@@ -193,15 +195,20 @@ public long getLastModifiedTime() throws IOException {
193195

194196
@Override
195197
public long getLastChangeTime() {
196-
// This is the best we can do with Java NIO...
197-
return attributes.lastModifiedTime().toMillis();
198+
return lastChangeTime;
198199
}
199200

200201
@Override
201202
public long getNodeId() {
202203
// TODO(bazel-team): Consider making use of attributes.fileKey().
203204
return -1;
204205
}
206+
207+
@Override
208+
public int getPermissions() {
209+
// Files on Windows are implicitly readable and executable.
210+
return 0555 | (attributes.isReadOnly() ? 0 : 0200);
211+
}
205212
};
206213

207214
return status;

‎src/main/native/windows/file-jni.cc‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -62,6 +62,29 @@ Java_com_google_devtools_build_lib_windows_WindowsFileOperations_nativeIsSymlink
6262
return static_cast<jint>(result);
6363
}
6464

65+
extern "C" JNIEXPORT jint JNICALL
66+
Java_com_google_devtools_build_lib_windows_WindowsFileOperations_nativeGetChangeTime(
67+
JNIEnv* env, jclass clazz, jstring path, jboolean follow_reparse_points,
68+
jlongArray result_holder, jobjectArray error_msg_holder) {
69+
std::wstring wpath(bazel::windows::GetJavaWstring(env, path));
70+
std::wstring error;
71+
jlong ctime = 0;
72+
int result =
73+
bazel::windows::GetChangeTime(wpath.c_str(), follow_reparse_points,
74+
reinterpret_cast<int64_t*>(&ctime), &error);
75+
if (result == bazel::windows::GetChangeTimeResult::kSuccess) {
76+
env->SetLongArrayRegion(result_holder, 0, 1, &ctime);
77+
} else {
78+
if (!error.empty() && CanReportError(env, error_msg_holder)) {
79+
ReportLastError(
80+
bazel::windows::MakeErrorMessage(
81+
WSTR(__FILE__), __LINE__, L"nativeGetChangeTime", wpath, error),
82+
env, error_msg_holder);
83+
}
84+
}
85+
return static_cast<jint>(result);
86+
}
87+
6588
extern "C" JNIEXPORT jboolean JNICALL
6689
Java_com_google_devtools_build_lib_windows_WindowsFileOperations_nativeGetLongPath(
6790
JNIEnv* env, jclass clazz, jstring path, jobjectArray result_holder,

‎src/main/native/windows/file.cc‎

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@
2121
#include <WinIoCtl.h>
2222
#include <stdint.h> // uint8_t
2323
#include <versionhelpers.h>
24+
#include <winbase.h>
2425
#include <windows.h>
2526

2627
#include <memory>
@@ -119,6 +120,54 @@ int IsSymlinkOrJunction(const WCHAR* path, bool* result, wstring* error) {
119120
}
120121
}
121122

123+
int GetChangeTime(const WCHAR* path, bool follow_reparse_points,
124+
int64_t* result, wstring* error) {
125+
if (!IsAbsoluteNormalizedWindowsPath(path)) {
126+
if (error) {
127+
*error = MakeErrorMessage(WSTR(__FILE__), __LINE__, L"GetChangeTime",
128+
path, L"expected an absolute Windows path");
129+
}
130+
return GetChangeTimeResult::kError;
131+
}
132+
133+
AutoHandle handle;
134+
DWORD flags = FILE_FLAG_BACKUP_SEMANTICS;
135+
if (!follow_reparse_points) {
136+
flags |= FILE_FLAG_OPEN_REPARSE_POINT;
137+
}
138+
handle = CreateFileW(path, 0,
139+
FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE,
140+
nullptr, OPEN_EXISTING, flags, nullptr);
141+
142+
if (!handle.IsValid()) {
143+
DWORD err = GetLastError();
144+
if (err == ERROR_FILE_NOT_FOUND || err == ERROR_PATH_NOT_FOUND) {
145+
return GetChangeTimeResult::kDoesNotExist;
146+
} else if (err == ERROR_ACCESS_DENIED) {
147+
return GetChangeTimeResult::kAccessDenied;
148+
}
149+
if (error) {
150+
*error =
151+
MakeErrorMessage(WSTR(__FILE__), __LINE__, L"CreateFileW", path, err);
152+
}
153+
return GetChangeTimeResult::kError;
154+
}
155+
156+
FILE_BASIC_INFO info;
157+
if (!GetFileInformationByHandleEx(handle, FileBasicInfo, (LPVOID)&info,
158+
sizeof(FILE_BASIC_INFO))) {
159+
DWORD err = GetLastError();
160+
if (error) {
161+
*error = MakeErrorMessage(WSTR(__FILE__), __LINE__,
162+
L"GetFileInformationByHandleEx", path, err);
163+
}
164+
return GetChangeTimeResult::kError;
165+
}
166+
167+
*result = info.ChangeTime.QuadPart;
168+
return GetChangeTimeResult::kSuccess;
169+
}
170+
122171
wstring GetLongPath(const WCHAR* path, unique_ptr<WCHAR[]>* result) {
123172
if (!IsAbsoluteNormalizedWindowsPath(path)) {
124173
return MakeErrorMessage(WSTR(__FILE__), __LINE__, L"GetLongPath", path,

‎src/main/native/windows/file.h‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -82,6 +82,16 @@ struct IsSymlinkOrJunctionResult {
8282
};
8383
};
8484

85+
// Keep in sync with j.c.g.devtools.build.lib.windows.WindowsFileOperations
86+
struct GetChangeTimeResult {
87+
enum {
88+
kSuccess = 0,
89+
kError = 1,
90+
kDoesNotExist = 2,
91+
kAccessDenied = 3,
92+
};
93+
};
94+
8595
// Keep in sync with j.c.g.devtools.build.lib.windows.WindowsFileOperations
8696
struct DeletePathResult {
8797
enum {
@@ -136,6 +146,13 @@ struct ReadSymlinkOrJunctionResult {
136146
// see http://superuser.com/a/343079. In Bazel we only ever create junctions.
137147
int IsSymlinkOrJunction(const WCHAR* path, bool* result, wstring* error);
138148

149+
// Retrieves the FILETIME at which `path` was last changed, including metadata.
150+
//
151+
// `path` should be an absolute, normalized, Windows-style path, with "\\?\"
152+
// prefix if it's longer than MAX_PATH.
153+
int GetChangeTime(const WCHAR* path, bool follow_reparse_points,
154+
int64_t* result, wstring* error);
155+
139156
// Computes the long version of `path` if it has any 8dot3 style components.
140157
// Returns the empty string upon success, or a human-readable error message upon
141158
// failure.

0 commit comments

Comments
 (0)