Fix DefaultSyscallCache for normalizing filesystems by fmeum · Pull Request #26842 · bazelbuild/bazel

fmeum · 2025-08-27T16:19:59Z

DefaultSyscallCache includes an optimization that answers a query for a file's type via a cached readdir result. This optimization didn't work correctly on macOS and Windows for various reasons: it hardcoded whether a file system on these OSes is case-insensitive, didn't reencode strings properly and also ignored Unicode normalization.

This is fixed by only making assumptions about the case mapping and normalization behavior of ASCII strings, which still allows this optimization to apply to the common case of a BUILD file existence check in a directory with only ASCII file names.

Along the way and since any OS now begins with an exact search in the list of Dirents, migrate to CompactSortedDirents for reduced peak memory usage.

fmeum · 2025-08-28T21:07:19Z

@meteorcloudy for macOS and Windows, @tjgq for Unicode :-)

Copilot

Pull Request Overview

This PR fixes the DefaultSyscallCache optimization for normalizing filesystems on macOS and Windows by addressing case-insensitive and Unicode normalization issues. The fix ensures that cached readdir results work correctly for file type queries across different filesystems.

Key changes:

Replace hardcoded filesystem case sensitivity assumptions with ASCII-only matching for case-insensitive operations
Migrate from raw collections to CompactSortedDirents for improved memory efficiency
Update all filesystem implementations to use the new mayBeCaseOrNormalizationInsensitive() method

Reviewed Changes

Copilot reviewed 19 out of 19 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
CompactSortedDirents.java	Extract compact directory entry implementation into separate class
DefaultSyscallCache.java	Implement improved case/normalization handling logic for cached directory lookups
FileSystem.java	Replace `isFilePathCaseSensitive()` with `mayBeCaseOrNormalizationInsensitive()` method
Various FileSystem implementations	Update all filesystem classes to implement the new method
Test files	Add comprehensive tests for case/normalization consistency across filesystems

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

src/main/java/com/google/devtools/build/lib/skyframe/CompactSortedDirents.java

src/test/java/com/google/devtools/build/lib/vfs/FileSystemTest.java

fmeum · 2025-08-29T09:41:22Z

@meteorcloudy Can we have Copilot bypass the cla check? That's a bit of a gotcha

meteorcloudy · 2025-08-29T09:41:30Z

Copilot needs to sign CLA 😅..

meteorcloudy · 2025-08-29T09:42:07Z

Yeah, I don't think that's possible for me to do, it's controlled by the internal policy.

meteorcloudy · 2025-08-29T09:44:02Z

I think you'll have to squash the changes and remove Copilot as an author..

fmeum · 2025-08-29T12:52:20Z

@meteorcloudy That worked

meteorcloudy

LGTM, but please wait for @tjgq to also take a look

iancha1992 · 2025-09-15T22:57:51Z

@fmeum Could you please resolve the conflicts? Thanks!

# Conflicts: # src/test/java/com/google/devtools/build/lib/windows/WindowsFileSystemTest.java

fmeum · 2025-09-16T07:47:59Z

@iancha1992 Done

meteorcloudy · 2025-09-19T08:14:12Z

src/main/java/com/google/devtools/build/lib/vfs/ReadonlyFileSystem.java

    return false;
  }

-  @Override


This is removed accidentally? It's causing some test failures internally, but I can add this back.
Should be

@Override public boolean mayBeCaseOrNormalizationInsensitive() { return false; }

right?

Ah, yes, please do!

### What does this PR do? Upgrades the Bazel toolchain from 8.6.0 to 9.0.1. ### Motivation 9.0.1 ships two upstream fixes that directly benefit our build: - bazelbuild/bazel#28606: NPE when `--repo_env` is set to an env var that has no value — contributed by Datadog (Joseph Gette). - bazelbuild/bazel#26842: `DefaultSyscallCache` incorrectly treated `BUILD` files and `build` directories as the same entry on case-insensitive / normalizing filesystems (e.g. Linux container on a macOS host), causing spurious build failures. ### Describe how you validated your changes `bazel test //...` passes. ### Additional Notes Changes required: - `.bazelversion`: 8.6.0 → 9.0.1 - `MODULE.bazel`: - `protobuf` 29.1 → 33.4 (resolved version in Bazel 9 dep graph) - `apple_support` 1.24.1 → 1.24.2 - `libarchive` 3.8.1 → 3.8.1.bcr.2, whose transitive `bzip2` dep (pinned to 1.0.8.bcr.4 via `single_version_override`) ships a Bazel 9-compatible BUILD file with explicit `load()` statements - `gcc_toolchain`: add patch 0005 (see below) - `bazel/toolchains/mingw/toolchain.bzl`: drop `provides = [CcToolchainConfigInfo]`; `CcToolchainConfigInfo` was never imported so it evaluated to `None`, which Bazel 9 rejects - `bazel/patches/gcc-toolchain/0005-fix-bazel-9-compat.patch`: same fix for `gcc_toolchain`'s `cc_toolchain_config.bzl`; also adds explicit `cc_common` load and `cc_library` to the generated BUILD template, both required by Bazel 9

### What does this PR do? Upgrades the Bazel toolchain from 8.6.0 to 9.0.1. ### Motivation 9.0.1 ships two upstream fixes that directly benefit our build: - bazelbuild/bazel#28606: NPE when `--repo_env` is set to an env var that has no value — contributed by Datadog (Joseph Gette). - bazelbuild/bazel#26842: `DefaultSyscallCache` incorrectly treated `BUILD` files and `build` directories as the same entry on case-insensitive / normalizing filesystems (e.g. Linux container on a macOS host), causing spurious build failures. ### Describe how you validated your changes `bazel test //...` passes. ### Additional Notes Changes required: - `.bazelversion`: 8.6.0 → 9.0.1 - `MODULE.bazel`: - `protobuf` 29.1 → 33.4: `bazel_tools@_` (Bazel 9's built-in module) requires `protobuf@33.4`; MVS propagates it to all deps - `apple_support` 1.24.1 → 1.24.2: same root cause (`bazel_tools@_`) - `libarchive` 3.8.1 → 3.8.1.bcr.2: `3.8.1` pulls in `bzip2@1.0.8.bcr.1` which lacks the `:bz2` target; `.bcr.2` requires `bzip2@1.0.8.bcr.3` which has it - `gcc_toolchain`: add patch 0005 (see below) - `bazel/toolchains/mingw/toolchain.bzl`: drop `provides = [CcToolchainConfigInfo]`; `CcToolchainConfigInfo` was never imported so it evaluated to `None`, which Bazel 9 rejects - `bazel/patches/gcc-toolchain/0005-fix-bazel-9-compat.patch`: same fix for `gcc_toolchain`'s `cc_toolchain_config.bzl`; also adds explicit `cc_common` load and `cc_library` to the generated BUILD template, both required by Bazel 9 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

### What does this PR do? Upgrades the Bazel toolchain from 8.6.0 to 9.0.1. ### Motivation 9.0.1 ships two upstream fixes that directly benefit our build: - bazelbuild/bazel#28606: NPE when `--repo_env` is set to an env var that has no value — contributed by Datadog (Joseph Gette). - bazelbuild/bazel#26842: `DefaultSyscallCache` incorrectly treated `BUILD` files and `build` directories as the same entry on case-insensitive / normalizing filesystems (e.g. Linux container on a macOS host), causing spurious build failures. ### Describe how you validated your changes `bazel test //...` passes. ### Additional Notes Groundwork landed in advance: - #47716: Bump `rules_cc` with explicit loads for Bazel 9 - #47745: Fix `rules_cc` leftovers from #47716 for Bazel 9 - #47982: Add explicit `cc_static_library` import for Bazel 9 - #48016: Add explicit `py_binary` import for Bazel 9 - #48071: Bump `rules_foreign_cc` for Bazel 9 fixes - #48082: Bump `rules_python` to 1.9.0 and fix misuses spotted on Windows - #48183: Fix Python hermetic toolchain check for Bazel 9 - #48186: Disable `repo_contents_cache` when in-workspace for Bazel 9 - #48228: Bump `protobuf` Bazel dep to 34.1

@JSGette

### What does this PR do? Upgrade `bazel` from 8.6.0 to 9.0.1. ### Motivation Bazel 9.0.1 ships upstream fixes that directly benefit our build & developer experience: - bazelbuild/bazel#26842: `BUILD` files and `build` directories were incorrectly treated as the same entry on case-insensitive filesystems (e.g., Linux container running on a macOS host), causing spurious build failures (esp. for `rloader` we had to `gazelle`-exclude because of that), - bazelbuild/bazel#27695: a contribution of ours (also available in 8.6.0 as bazelbuild/bazel#28367) - nice to have when sharing a folder between a Linux host and a Windows VM through `virtiofs`, - bazelbuild/bazel#28640: another contribution of ours (@JSGette) - critical because it what preventing us from switching to Bazel 9 (ADMS config). ### Describe how you validated your changes `bazel test //...` passes. ### Additional Notes Groundwork landed in advance: - #47716 - #47745 - #47982 - #48016 - #48071 - #48082 - #48183 - #48186 - #48200 (>= Bazel 9 min) - #48201 (>= Bazel 9 min) - #48228 (>= Bazel 9 min) Co-authored-by: regis.desgroppes <regis.desgroppes@datadoghq.com>

@JSGette

### What does this PR do? Upgrade `bazel` from 8.6.0 to 9.0.1. ### Motivation Bazel 9.0.1 ships upstream fixes that directly benefit our build & developer experience: - bazelbuild/bazel#26842: `BUILD` files and `build` directories were incorrectly treated as the same entry on case-insensitive filesystems (e.g., Linux container running on a macOS host), causing spurious build failures (esp. for `rloader` we had to `gazelle`-exclude because of that), - bazelbuild/bazel#27695: a contribution of ours (also available in 8.6.0 as bazelbuild/bazel#28367) - nice to have when sharing a folder between a Linux host and a Windows VM through `virtiofs`, - bazelbuild/bazel#28640: another contribution of ours (@JSGette) - critical because it what preventing us from switching to Bazel 9 (ADMS config). ### Describe how you validated your changes `bazel test //...` passes. ### Additional Notes Groundwork landed in advance: - #47716 - #47745 - #47982 - #48016 - #48071 - #48082 - #48183 - #48186 - #48200 (>= Bazel 9 min) - #48201 (>= Bazel 9 min) - #48228 (>= Bazel 9 min) Co-authored-by: regis.desgroppes <regis.desgroppes@datadoghq.com>

### What does this PR do? Configure the `//:gazelle` target with `-build_file_name=BUILD.bazel`. ### Motivation When `gazelle` runs inside a Linux container on macOS via Docker Desktop, the workspace is typically mounted on a case-insensitive filesystem where `BUILD` and `build` are indistinguishable. This causes spurious errors such as: ```sh bazel run //:gazelle; echo $? ... gazelle: open /dda/.gitlab/windows/build: no such file or directory open /dda/rtloader/build: no such file or directory 1 ``` Whereas the cause strictly resides in a filesystem layer/driver (docker/desktop-feedback#251, recently imported from docker/for-mac#7218), portable tools have been taking countermeasures. Bazel 9, for instance, revamped their filesystem cache to better accomodate case-insensitive filesystems (bazelbuild/bazel#26842), which led us to switch to it (#47742), but that only covers Bazel's own I/O, not the processes it spawns, of which `gazelle`. For that purpose, Gazelle happens to explicitly recommend setting its `build_file_name` directive to `BUILD.bazel`: > If a repository contains files named `build` that aren't related to Bazel, it may help to set this to `"BUILD.bazel"`, especially on case-insensitive file systems. ### Additional Notes For consistency with the narrowed scope, this change renames `compliance/rules/BUILD` to `compliance/rules/BUILD.bazel` (the only plain `BUILD` file still present in the tree).

### What does this PR do? Configure the `//:gazelle` target with `-build_file_name=BUILD.bazel`. ### Motivation When `gazelle` runs inside a Linux container on macOS via Docker Desktop, the workspace is typically mounted on a case-insensitive filesystem where `BUILD` and `build` are indistinguishable. This causes spurious errors such as: ```sh bazel run //:gazelle; echo $? ... gazelle: open /dda/.gitlab/windows/build: no such file or directory open /dda/rtloader/build: no such file or directory 1 ``` Whereas the cause strictly resides in a filesystem layer/driver (docker/desktop-feedback#251, recently imported from docker/for-mac#7218), portable tools have been taking countermeasures. Bazel 9, for instance, revamped their filesystem cache to better accomodate case-insensitive filesystems (bazelbuild/bazel#26842), which led us to switch to it (#47742), but that only covers Bazel's own I/O, not the processes it spawns, of which `gazelle`. For that purpose, Gazelle happens to [explicitly recommend](https://github.com/bazel-contrib/bazel-gazelle/blob/4a7caee10fbd2da9bb059f3ce7c2cac8d0050e53/reference.md?plain=1#L251) setting its `build_file_name` directive to `BUILD.bazel`: > If a repository contains files named `build` that aren't related to Bazel, it may help to set this to `"BUILD.bazel"`, especially on case-insensitive file systems. ### Additional Notes For consistency with the narrowed scope, this change renames `compliance/rules/BUILD` to `compliance/rules/BUILD.bazel` (the only plain `BUILD` file still present in the tree). Co-authored-by: regis.desgroppes <regis.desgroppes@datadoghq.com>

@JSGette

### What does this PR do? Upgrade `bazel` from 8.6.0 to 9.0.1. ### Motivation Bazel 9.0.1 ships upstream fixes that directly benefit our build & developer experience: - bazelbuild/bazel#26842: `BUILD` files and `build` directories were incorrectly treated as the same entry on case-insensitive filesystems (e.g., Linux container running on a macOS host), causing spurious build failures (esp. for `rloader` we had to `gazelle`-exclude because of that), - bazelbuild/bazel#27695: a contribution of ours (also available in 8.6.0 as bazelbuild/bazel#28367) - nice to have when sharing a folder between a Linux host and a Windows VM through `virtiofs`, - bazelbuild/bazel#28640: another contribution of ours (@JSGette) - critical because it what preventing us from switching to Bazel 9 (ADMS config). ### Describe how you validated your changes `bazel test //...` passes. ### Additional Notes Groundwork landed in advance: - #47716 - #47745 - #47982 - #48016 - #48071 - #48082 - #48183 - #48186 - #48200 (>= Bazel 9 min) - #48201 (>= Bazel 9 min) - #48228 (>= Bazel 9 min) Co-authored-by: regis.desgroppes <regis.desgroppes@datadoghq.com>

@JSGette

### What does this PR do? Upgrade `bazel` from 8.6.0 to 9.0.1. ### Motivation Bazel 9.0.1 ships upstream fixes that directly benefit our build & developer experience: - bazelbuild/bazel#26842: `BUILD` files and `build` directories were incorrectly treated as the same entry on case-insensitive filesystems (e.g., Linux container running on a macOS host), causing spurious build failures (esp. for `rloader` we had to `gazelle`-exclude because of that), - bazelbuild/bazel#27695: a contribution of ours (also available in 8.6.0 as bazelbuild/bazel#28367) - nice to have when sharing a folder between a Linux host and a Windows VM through `virtiofs`, - bazelbuild/bazel#28640: another contribution of ours (@JSGette) - critical because it what preventing us from switching to Bazel 9 (ADMS config). ### Describe how you validated your changes `bazel test //...` passes. ### Additional Notes Groundwork landed in advance: - #47716 - #47745 - #47982 - #48016 - #48071 - #48082 - #48183 - #48186 - #48200 (>= Bazel 9 min) - #48201 (>= Bazel 9 min) - #48228 (>= Bazel 9 min) Co-authored-by: regis.desgroppes <regis.desgroppes@datadoghq.com>

@alopezz

…nsistencies (#48279) ### What does this PR do? Remove `gazelle` exclusions that had to be put in place pending a fix for `gazelle` confused by `build` directories on "broken" case-insensitive filesystem stacks (docker/desktop-feedback#251). Doing so, I realized there were inconsistencies in the `gazelle` setup and took the opportunity to fix them. ### Motivation #48455 indeed allows to get rid of the following exclusions: ``` # TODO(agent-build): This exclusion is ready for removal, blocked by issues caused by the interaction of # Bazel with directories named `build` created by the CMake builds. We can remove this once we remove CMake # support or we upgrade to a Bazel version containing a fix: bazelbuild/bazel#26842 # In the meantime, temporarily remove this exclusion locally if regeneration of BUILD files is required # gazelle:exclude rtloader ... # gazelle:exclude .gitlab ``` Nearby inconsistencies in the `gazelle` setup: - `build_tags` is passed by attribute (`build_tags = ALL_BUILD_TAGS`) => OK (**canonical**), - `external` was passed by `args` (`-external=static`) => pass by attribute (`external = "static"`), - `prefix` was passed by directive (`# gazelle:prefix github.com/DataDog/datadog-agent`) => pass by attribute (`prefix = "github.com/DataDog/datadog-agent"`), - `args` happens to be a [compatibility shim](bazel-contrib/bazel-gazelle#62) => use `extra_args` (**canonical**) when there's no corresponding attribute yet: - `build_file_name` was passed by `args` and doesn't have a corresponding attribute yet => pass by `extra_args`, - `strict` was passed from a single GitLab job and doesn't have a corresponding attribute yet => pass by `extra_args`. ### Describe how you validated your changes Kind assistance of @alopezz and/or @JSGette who run Linux containers on their macOS-powered laptop. Co-authored-by: regis.desgroppes <regis.desgroppes@datadoghq.com>

fmeum force-pushed the fix-stat-via-readdir branch 2 times, most recently from 433f608 to a6b60fe Compare August 28, 2025 16:21

fmeum changed the title ~~WIP: Fix DefaultSyscallCache for case-insensitive filesystems~~ Fix DefaultSyscallCache for normalizing filesystems Aug 28, 2025

fmeum marked this pull request as ready for review August 28, 2025 21:06

fmeum requested a review from a team as a code owner August 28, 2025 21:06

fmeum requested review from meteorcloudy and tjgq and removed request for a team August 28, 2025 21:06

github-actions bot added team-Remote-Exec Issues and PRs for the Execution (Remote) team awaiting-review PR is awaiting review from an assigned reviewer labels Aug 28, 2025

meteorcloudy requested a review from Copilot August 29, 2025 09:37

Copilot AI reviewed Aug 29, 2025

View reviewed changes

src/main/java/com/google/devtools/build/lib/skyframe/CompactSortedDirents.java Outdated Show resolved Hide resolved

src/test/java/com/google/devtools/build/lib/vfs/FileSystemTest.java Outdated Show resolved Hide resolved

fmeum force-pushed the fix-stat-via-readdir branch from ba96fc4 to 5c5a656 Compare August 29, 2025 12:52

meteorcloudy approved these changes Sep 2, 2025

View reviewed changes

tjgq approved these changes Sep 15, 2025

View reviewed changes

tjgq added awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally and removed awaiting-review PR is awaiting review from an assigned reviewer labels Sep 15, 2025

Fix DefaultSyscallCache for case-insensitive filesystems

1f42ff5

# Conflicts: # src/test/java/com/google/devtools/build/lib/windows/WindowsFileSystemTest.java

fmeum force-pushed the fix-stat-via-readdir branch from 5c5a656 to 1f42ff5 Compare September 16, 2025 07:47

meteorcloudy reviewed Sep 19, 2025

View reviewed changes

copybara-service bot closed this in 2ea131a Sep 22, 2025

github-actions bot removed the awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally label Sep 22, 2025

rdesgroppes mentioned this pull request Mar 26, 2026

Make gazelle consider only BUILD.bazel files DataDog/datadog-agent#48455

Merged

Conversation

fmeum commented Aug 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fmeum commented Aug 28, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

fmeum commented Aug 29, 2025

Uh oh!

meteorcloudy commented Aug 29, 2025

Uh oh!

meteorcloudy commented Aug 29, 2025

Uh oh!

meteorcloudy commented Aug 29, 2025

Uh oh!

fmeum commented Aug 29, 2025

Uh oh!

meteorcloudy left a comment

Choose a reason for hiding this comment

Uh oh!

iancha1992 commented Sep 15, 2025

Uh oh!

fmeum commented Sep 16, 2025

Uh oh!

meteorcloudy Sep 19, 2025

Choose a reason for hiding this comment

Uh oh!

fmeum Sep 19, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

fmeum commented Aug 27, 2025 •

edited

Loading