laramiel
commented
1 year ago This isn't just a C++ issue; any rules which use similar virtual directories can have the same issue. For example proto, etc.
Open laramiel opened 1 year ago
laramiel
commented
1 year ago This isn't just a C++ issue; any rules which use similar virtual directories can have the same issue. For example proto, etc.
laramiel
commented
1 year ago 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.
len(envoy/extensions/load_balancing_policies/client_side_weighted_round_robin/v3/client_side_weighted_round_robin.proto) is 115.
laramiel
commented
7 months ago ... this has been tagged as 'help wanted'.
Suppose that I started here:
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
commented
7 months ago 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
commented
7 months ago 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
commented
7 months ago 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
commented
7 months ago 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
commented
7 months ago 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
commented
7 months ago 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
commented
2 months ago Update on https://github.com/bazelbuild/bazel/commit/a091d90cded797ce8f6206b56ffd6f89e27e2c76
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
commented
2 months ago 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
commented
2 months ago Update on https://github.com/bazelbuild/bazel/commit/a091d90cded797ce8f6206b56ffd6f89e27e2c76
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
-Ipoints 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
commented
5 days ago 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'
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.hThese 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.hThen 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:
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 directoryWorkarounds 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.