-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Make virtual include paths shorter #26005
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
64d1009
to
5c92e46
Compare
@@ -1071,12 +1072,12 @@ public void testIncludeManglingSmoke() throws Exception { | |||
ConfiguredTarget lib = getConfiguredTarget("//third_party/a"); | |||
CcCompilationContext ccCompilationContext = lib.get(CcInfo.PROVIDER).getCcCompilationContext(); | |||
assertThat(ActionsTestUtil.prettyArtifactNames(ccCompilationContext.getDeclaredIncludeSrcs())) | |||
.containsExactly("third_party/a/_virtual_includes/a/lib/b/c.h", "third_party/a/v1/b/c.h"); | |||
.containsExactly("_v_inc/207132b2/lib/b/c.h", "third_party/a/v1/b/c.h"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this hash stable? Will we see test failures (and one-off updates) in otherwise unrelated CLs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the hash function is stable, input is basically the target path and name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a way to only have this happen for Windows? I doubt it, but the loss in expressiveness of file paths is unfortunate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would be a use case for ctx.exec_platform_has_constraint
. But since clang-cl
doesn't have this issue, it could be more appropriate to match on the compiler
attribute of the toolchain or, even better, a new hash_virtual_includes
feature that the MSVC toolchain sets.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think a generic hash_virtual_includes
feature makes sense as needing this support is a property of the toolchain itself, rather than the target (or exec) platform (should another toolchain run into this issue it would be available), and MSVC can add it.
@@ -21,7 +21,7 @@ load(":common/paths.bzl", "paths") | |||
|
|||
cc_internal = _builtins.internal.cc_internal | |||
|
|||
_VIRTUAL_INCLUDES_DIR = "_virtual_includes" | |||
_VIRTUAL_INCLUDES_DIR = "_v_inc" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ISTM the important piece is hashing the potentially long set of path fragments that could accumulate.
Saving 11 chars on the constant piece is hopefully less relevant?
inc
is clear enough, I suppose... maybe this could be "_virtual_inc"
for everything, and then use a feature to control the hashing of path fragments, optionally?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I only rarely work on Windows builds, but even I got into situations where every single character counts. What do you think of also gating this abbreviation behind the new feature and giving it a more generic name that doesn't promise a particular scheme, say shorten_virtual_includes
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll defer to folks who have more MSVC experience than I do. I mostly work with clang-cl
for windows builds.
I would certainly prefer to keep the more descriptive root when path length is not an issue (i.e. gate it behind a common feature as you suggest)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Saving 11 chars on the constant piece is hopefully less relevant?
The path length limit for MSVC is 260, 11 chars is almost 4% of that, I would say every char is precious on windows unfortunately.
I really like the shorten_virtual_includes
idea, will give it a try.
@comius has pointed out this approach depends on the Another approach to solve this problem is to have a C++ wrapper for the MSVC toolchain to shorten paths that look too long.
Cons:
|
Fixes bazelbuild#18683 RELNOTES[INC]: Virtual include header files are linked under `bin/_v_inc/<hash of target path>` instead of `bin/<target package path>/_virtual_include/<target name>`. This shortens the virtual include paths which is critical for mitigating long path issue with MSVC on Windows. Closes bazelbuild#26005. PiperOrigin-RevId: 754930166 Change-Id: Ie1e5bad06a9f945c0cf71c5e122d444e527f7ca6
4a0d65c
to
7841b51
Compare
Fixes #18683
RELNOTES: Virtual include header files are linked under
bin/_v_inc/<hash of target path>
instead ofbin/<target package path>/_virtual_include/<target name>
. This shortens the virtual include paths which is critical for mitigating long path issue with MSVC on Windows.