Alternative _virtual_includes organization to solve MSVC issues

[laramiel]

Description of the feature request:

Currently bazel constructs _virtual_includes paths for libraries in order to place files at known locations or restrict the set of files available by the compiler. These paths look somewhat like this, in the case of protobuf:

<bazel_output_base>/<hash>/execroot/<root workspace>/bazel-out/<config>/bin/external/com_google_protobuf/src/google/protobuf/compiler/objectivec/_virtual_includes/objectivec/google/protobuf/compiler/objectivec/text_format_decode_data.h

These constructed include paths are quite long, as they incorporate both the path to the bazel rule and the path to the file. Unfortunately MSVC uses a MAX_PATH of 260 characters, which leads to hard to discover errors such as:

In order to solve this, I request a feature to make _virtual_includes paths shorter.

If the virtual includes path were constructed more like:

<bazel_output_base>/<hash>/_virtual_includes/<different hash>/objectivec/google/protobuf/compiler/objectivec/text_format_decode_data.h

Then this problem would be essentially eliminated. The exact path construction doesn't matter, as long as it can eliminate much of the lengths of deep bazel rules.

This may require a rule context feature which, in essence, constructs a virtual path which encodes the entire build / workspace / package config.

Of course, links could be maintained from the current virtual include to that hash.

What underlying problem are you trying to solve with this feature?

Windows, but more importantly MSVC cl.exe, limits filename lengths to MAX_PATH (260) characters: https://learn.microsoft.com/en-us/windows/win32/fileio/maximum-file-path-limitation https://developercommunity.visualstudio.com/t/clexe-compiler-driver-cannot-handle-long-file-path/975889

The bazel workspace and _virtual_includes repository construction runs into this limit. This has been a recurring issue in some windows builds as there are multiple ways to trigger it:

• C++ object files path is too long for some projects #15961
• Resolve Windows long path issue during C++ compilation #4149
• src build on windows not working protocolbuffers/protobuf#12947

The organization of the constructed virtual includes directories exacerbates this limit.

fatal error C1083: Cannot open include file: 'google/protobuf/compiler/objectivec/text_format_decode_data.h': No such file or directory

Workarounds involve setting the output_base startup flag, such as --output_base=C:\Out, which is not a universal solution, or reorganizing the directory structure within a repository, which cannot always be done on mature projects.

Which operating system are you running Bazel on?

Windows + MSVC (any version)

Any other information, logs, or outputs that you want to share?

For my specific build, which includes protobuf, this is an output of the @params file as supporting evidence. Here many of the bazel constructed paths are >130 characters, leaving 130 total for both the --bazel_root and the actual path.

If the bazel root is 'C:/users/laramiel/_bazel_laramiel/12345678', (which is 42 characters), that leaves at most 80 characters for the file + path in a bazel repository.

/Ibazel-out/x64_windows-fastbuild/bin/external/com_google_protobuf/src/google/protobuf/compiler/objectivec/_virtual_includes/objectivec
/Ibazel-out/x64_windows-fastbuild/bin/external/com_google_protobuf/src/google/protobuf/compiler/objectivec/_virtual_includes/line_consumer
/Ibazel-out/x64_windows-fastbuild/bin/external/com_google_protobuf/src/google/protobuf/_virtual_includes/protobuf_nowkt
/Ibazel-out/x64_windows-fastbuild/bin/external/com_google_protobuf/src/google/protobuf/_virtual_includes/protobuf_lite
/Ibazel-out/x64_windows-fastbuild/bin/external/com_google_protobuf/src/google/protobuf/_virtual_includes/arena
/Ibazel-out/x64_windows-fastbuild/bin/external/com_google_protobuf/src/google/protobuf/_virtual_includes/arena_align
/Ibazel-out/x64_windows-fastbuild/bin/external/com_google_protobuf/src/google/protobuf/stubs/_virtual_includes/lite
/Ibazel-out/x64_windows-fastbuild/bin/external/com_google_protobuf/src/google/protobuf/_virtual_includes/port_def
/Ibazel-out/x64_windows-fastbuild/bin/external/com_google_protobuf/src/google/protobuf/_virtual_includes/arena_allocation_policy
/Ibazel-out/x64_windows-fastbuild/bin/external/com_google_protobuf/src/google/protobuf/_virtual_includes/arena_config
/Ibazel-out/x64_windows-fastbuild/bin/external/com_google_protobuf/src/google/protobuf/_virtual_includes/arena_cleanup
/Ibazel-out/x64_windows-fastbuild/bin/external/com_google_protobuf/src/google/protobuf/_virtual_includes/string_block
/Ibazel-out/x64_windows-fastbuild/bin/external/com_google_protobuf/src/google/protobuf/_virtual_includes/varint_shuffle
/Ibazel-out/x64_windows-fastbuild/bin/external/com_google_protobuf/src/google/protobuf/io/_virtual_includes/io
/Ibazel-out/x64_windows-fastbuild/bin/external/com_google_protobuf/src/google/protobuf/io/_virtual_includes/io_win32
/Ibazel-out/x64_windows-fastbuild/bin/external/com_google_protobuf/src/google/protobuf/io/_virtual_includes/gzip_stream
/Ibazel-out/x64_windows-fastbuild/bin/external/com_google_protobuf/src/google/protobuf/stubs/_virtual_includes/stubs
/Ibazel-out/x64_windows-fastbuild/bin/external/com_google_protobuf/src/google/protobuf/io/_virtual_includes/printer
/Ibazel-out/x64_windows-fastbuild/bin/external/com_google_protobuf/src/google/protobuf/io/_virtual_includes/zero_copy_sink
/Ibazel-out/x64_windows-fastbuild/bin/external/com_google_protobuf/src/google/protobuf/io/_virtual_includes/tokenizer
/Ibazel-out/x64_windows-fastbuild/bin/external/com_google_protobuf/src/google/protobuf/compiler/_virtual_includes/code_generator
/Ibazel-out/x64_windows-fastbuild/bin/external/com_google_protobuf/src/google/protobuf/compiler/objectivec/_virtual_includes/names
/Ibazel-out/x64_windows-fastbuild/bin/external/com_google_protobuf/src/google/protobuf/compiler/objectivec/_virtual_includes/names_internal
/Ibazel-out/x64_windows-fastbuild/bin/external/com_google_protobuf/src/google/protobuf/_virtual_includes/descriptor_legacy

[laramiel]

This isn't just a C++ issue; any rules which use similar virtual directories can have the same issue. For example proto, etc.

[laramiel]

This may also affect building gRPC. See, for example, this proto filename, which is > 100 bytes. The package name and any virtual include directories could easily be more.

https://github.com/envoyproxy/data-plane-api/blob/main/contrib/envoy/extensions/matching/input_matchers/hyperscan/v3alpha/hyperscan.proto

https://github.com/envoyproxy/data-plane-api/blob/main/envoy/extensions/load_balancing_policies/client_side_weighted_round_robin/v3/client_side_weighted_round_robin.proto

len(envoy/extensions/load_balancing_policies/client_side_weighted_round_robin/v3/client_side_weighted_round_robin.proto) is 115.

[laramiel]

... this has been tagged as 'help wanted'.

Suppose that I started here:

bazel/src/main/starlark/builtins_bzl/common/cc/cc_compilation_helper.bzl

Line 116 in 1027d6f

if not original_header.path == include_path:

And tried to replace:

<bazel_output_base>/<hash>/execroot/<root workspace>/bazel-out/<config>/bin/external/com_google_protobuf/src/google/protobuf/compiler/objectivec/_virtual_includes/objectivec/google/protobuf/compiler/objectivec/text_format_decode_data.h

with

<bazel_output_base>/<hash>/execroot/<root workspace>/bazel-out/<config>/bin/external/com_google_protobuf/_virtual_includes/<hash>/objectivec/google/protobuf/compiler/objectivec/text_format_decode_data.h

Where else should I look for potential collisions?

[laramiel]

I suppose that in theory, if there were a reasonable built-in hash, it could look a bit like this:

diff --git a/src/main/starlark/builtins_bzl/common/cc/cc_compilation_helper.bzl b/src/main/starlark/builtins_bzl/common/cc/cc_compilation_helper.bzl
index c9f1f2a3e8..4f79da3d74 100644
--- a/src/main/starlark/builtins_bzl/common/cc/cc_compilation_helper.bzl
+++ b/src/main/starlark/builtins_bzl/common/cc/cc_compilation_helper.bzl
@@ -20,7 +20,6 @@ load(":common/paths.bzl", "paths")

 cc_internal = _builtins.internal.cc_internal

-_VIRTUAL_INCLUDES_DIR = "_virtual_includes"

 def _include_dir(directory, repo_path, sibling_repo_layout):
     if sibling_repo_layout:
@@ -45,6 +44,21 @@ def _repo_relative_path(artifact):
 def _enabled(feature_configuration, feature_name):
     return cc_common.is_enabled(feature_configuration = feature_configuration, feature_name = feature_name)

+
+def _virtual_dir(package):
+    return "_virtual_%08x" % package.hash()
+
+
+def _virtual_root(repository, virtual_dir, is_sibling_repository_layout):
+    if _is_repository_main(repository) or sibling_repository_layout:
+        return virtual_dir
+
+    if repository.startswith("@"):
+        repository = repository[1:]
+
+    return paths.get_relative(paths.get_relative("external", repository), virtual_dir)
+
+
 def _compute_public_headers(
         actions,
         config,
@@ -103,6 +117,8 @@ def _compute_public_headers(
             virtual_to_original_headers = depset(),
         )

+    virtual_dir = _virtual_dir(label.package)
+
     module_map_headers = []
     virtual_to_original_headers_list = []
     for original_header in public_headers_artifacts:
@@ -114,7 +130,7 @@ def _compute_public_headers(
             include_path = paths.get_relative(include_prefix, include_path)

         if not original_header.path == include_path:
-            virtual_include_dir = paths.join(paths.join(cc_helper.package_source_root(label.workspace_name, label.package, is_sibling_repository_layout), _VIRTUAL_INCLUDES_DIR), label.name)
+            virtual_include_dir = paths.join(_virtual_root(label.workspace_name, virtual_dir, is_sibling_repository_layout), label.name)
             virtual_header = actions.declare_shareable_artifact(paths.join(virtual_include_dir, include_path))
             actions.symlink(
                 output = virtual_header,
@@ -133,7 +149,7 @@ def _compute_public_headers(
     return struct(
         headers = virtual_headers,
         module_map_headers = module_map_headers,
-        virtual_include_path = cc_internal.bin_or_genfiles_relative_to_unique_directory(actions = actions, unique_directory = _VIRTUAL_INCLUDES_DIR),
+        virtual_include_path = cc_internal.bin_or_genfiles_relative_to_unique_directory(actions = actions, unique_directory = virtual_dir),
         virtual_to_original_headers = depset(virtual_to_original_headers_list),
     )

[comius]

This is more protos related.

I don’t think hashing for virtual_includes works though. When header files are included C++ searches them in such directory structure. And if the structure isn’t there, it can’t find them. Or maybe I’m missing somethin?

Virtual includes are only used with include_path and strip_include_path set in proto_library in Bazel 7. In older versions that wasn’t the case and they were almost always used.

[laramiel]

The suffix of the path has to remain, but the constructed space between the repository (/external/com_google_protobuf/) and the virtual includes root (/_virtual_includes/) is constructed by that cc_helper.package_source_root, and can be essentially elided to something like /external/com_google_protobuf/_virtual_includes_for_package_target/.

The above snippet attempts to change only that piece; it might not be quite right, though.

[comius]

Pure Starlark rules allow file declaration only under their own package path. That’s why there’s a long path before _virtual_includes. The reason is, so that file name conflicts are limited to a package.

Native rules have some super powers and can choose more arbitrary path. But that only works because they are “centralized” by being part of Bazel.

There are ways how to make this work in a decentralized manner. At the moment we don’t have any design or implementation for it. We will potentially need it for C++ rules that are using such special powers.

[laramiel]

So the only place the virtual_includes can be created is directly on the package path? Well, it would be nice for a rule to ask the bazel runtime for a private unique directory, otherwise it seems like this windows path issue is going to persist.

[jbms]

This is a pretty serious issue --- it basically means that any use of Bazel with strip_include_prefix is very fragile on Windows and can easily be broken by minor changes that increase filename lengths.

In particular, the length limit is easily triggered by the use of protobuf as a dependency, as is very common, particularly for Google projects, if the output_base is not extremely short.

I understand that setting a very short output_base or output_root is a common mitigation on Windows. However, because it requires placing the output directory outside of the user's home directory, it requires explicit user intervention and can't really be done automatically. TensorStore, for example, invokes Bazel via a bundled Bazelisk automatically when invoking pip install tensorstore in order to build our extension module. Users don't even need to be aware that Bazel is used at all. However, due to this path length issue, the build may fail on Windows depending on the length of their home directory path.

It would be great if a solution to this could be prioritized.

[comius]

Update on a091d90

I spend quite some time to figure out the problem, but now it's obvious. The change can't possibly work, and I can't roll it forward.

What happens is that when include path -I points somewhere into the src directory directly, compiler will pick up additional header files (unless layering check, sandbox are enabled). Before only public headers were copied into _virtual_includes, after the change compiler would see also private headers.

[dpvdberg]

I see the same problem while trying to migrate from C++ visual studio solutions to Bazel in a commercial setting.
The paths are too long, resulting in No such file or directory issues.

Any workaround for this?

[jbms]

Update on a091d90

I spend quite some time to figure out the problem, but now it's obvious. The change can't possibly work, and I can't roll it forward.

What happens is that when include path -I points somewhere into the src directory directly, compiler will pick up additional header files (unless layering check, sandbox are enabled). Before only public headers were copied into _virtual_includes, after the change compiler would see also private headers.

What about still generating a virtual_includes directory but giving it a short (e.g. hash-based) path on Windows? Perhaps in general all of the output base directories on Windows could be given short unnested hash-based paths.

[keith]

This issue is about a lot more than just virtual_includes now right? For example I think this failure is the same case, but not specific to headers:

cl : Command line error D8022 : cannot open 'bazel-out/x64_windows-fastbuild/bin/external/grpc~~grpc_repo_deps_ext~envoy_api/envoy/extensions/load_balancing_policies/client_side_weighted_round_robin/v3/_objs/pkg.upb_minitable/client_side_weighted_round_robin.upb_minitable.obj.params'

[battledash]

This issue is about a lot more than just virtual_includes now right? For example I think this failure is the same case, but not specific to headers:

cl : Command line error D8022 : cannot open 'bazel-out/x64_windows-fastbuild/bin/external/grpc~~grpc_repo_deps_ext~envoy_api/envoy/extensions/load_balancing_policies/client_side_weighted_round_robin/v3/_objs/pkg.upb_minitable/client_side_weighted_round_robin.upb_minitable.obj.params'

We're experiencing this issue with this same envoy file. We're using --nolegacy_external_runfiles, --experimental_sibling_repository_layout, and --output_user_root=C:\bz, which just barely gets around it, but a real fix for this would be very nice to see.

This started occurring after updating our workflow to bzlmod (going from 6.4.0 to 8.0.0) and upgrading to gRPC 1.68.0.