-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Bazel + MSVC C++ = missing dependency declarations [culprit #5136] #5640
Comments
I can reproduce the issue with Bazel from master branch and VS 2017 15.7 and Clang on Windows (master). @derekmauro I have some idea, but I will need your help to verify it. Can you do the following on Linux machine with GCC/Clang?
|
Output from step 3:
|
Thanks for the console output. It disprove my guess that the include flags From what I can see in Bazel's java code, Bazel handles MSVC's |
@meteorcloudy @mhlopko Can you take a look? |
@rongjiecomputer Thanks for the help, I'll take it from here. |
@derekmauro I've debugged on this and found the root cause. Let me explain how this issue happened on Windows and not on Linux.
@dslomov Is there anyway to override |
Not right now, but there will be once experimental_enable_repo_mapping is fully finished and #3115 is fixed. |
@dslomov Thanks! Then I'll assign this to you. |
FYI @laszlocsomor, this is the similar bug you encountered recently. |
/cc @mhlopko This issue happens when you have the same header file under |
@mhlopko, @meteorcloudy : I think the culprit is that every action runs in the same execution root. I believe the single execution root is the reason the compilation action has |
And I believe we could solve this problem if all build actions corresponding to rules in external repos would use |
@laszlocsomor What if you have a cc_library |
If |
@rongjiecomputer Yes, that would be wrong if the actual file you want to include is in your main repo instead of external repo. I think the only way to make it right is sandbox, this way it can keep the compiler from accidentally discovering undeclared include files. (although there could still be header file conflicts between declared dependencies). |
@meteorcloudy : Indeed you're right. I also forgot to consider Update: fixed repo paths. |
Correction: the symlink would have to be |
Right, for example when a library depends on both |
I think the include scanner could catch this error and suggest a fix ("Did you mean to include external/repo/foo/x.h instead of foo/x.h?"). Bazel knows the allowed set of included headers, so the include scanner could detect if an illegally included header file exists under one of the external repos. |
I agree. |
After in-person discussion with @mhlopko and @meteorcloudy, it became clear that including files via "external/reponame" is in fact the wrong approach. When a header file ( The author of |
Avoid include collision inside the Bazel source tree when a rule depends on @bazel_tools//tools/cpp/runfiles and includes "tools/cpp/runfiles/runfiles.h", but they end up getting "tools/cpp/runfiles/runfiles.h" from the main repo. Change-Id: Ic0fa23045b25cf9b1987780f9e752a54fa32be45 Details: bazelbuild#5640 (comment)
Avoid include collision inside the Bazel source tree when a rule depends on @bazel_tools//tools/cpp/runfiles and includes "tools/cpp/runfiles/runfiles.h", but they end up getting "tools/cpp/runfiles/runfiles.h" from the main repo. Change-Id: Ic0fa23045b25cf9b1987780f9e752a54fa32be45 Details: bazelbuild#5640 (comment)
Avoid include collision inside the Bazel source tree when a rule depends on @bazel_tools//tools/cpp/runfiles and includes "tools/cpp/runfiles/runfiles.h", but they end up getting "tools/cpp/runfiles/runfiles.h" from the main repo. Change-Id: Ic0fa23045b25cf9b1987780f9e752a54fa32be45 Details: #5640 (comment) Closes #6023. Change-Id: Ic0fa23045b25cf9b1987780f9e752a54fa32be45 PiperOrigin-RevId: 210878008
Thabk you for the suggestion. googletest is a standalone repo which is
serving thousands of users. We can't just merge it with abseil .
Looks like windows sandboxing would br the answer .
Thanks again
G
…On Fri, Nov 9, 2018, 05:03 László Csomor ***@***.*** wrote:
@gennadiycivil <https://github.com/gennadiycivil> : The build works on
Linux because of sandboxing. If you disable sandboxing with
--spawn_strategy=standalone, the build breaks on Linux too:
$ bazel build --define absl=1 absl/... --spawn_strategy=standalone
Starting local Bazel server and connecting to it...
INFO: Analysed 204 targets (42 packages loaded, 1882 targets configured).
INFO: Found 204 targets...
ERROR: /usr/local/google/home/laszlocsomor/stuff/abseil-cpp/absl/time/internal/cctz/BUILD.bazel:79:1: undeclared inclusion(s) in rule '//absl/time/internal/cctz:time_zone_format_test':
this rule is missing dependency declarations for the following files included by 'absl/time/internal/cctz/src/time_zone_format_test.cc':
'absl/strings/string_view.h'
'absl/base/config.h'
'absl/base/policy_checks.h'
'absl/base/internal/throw_delegate.h'
'absl/base/macros.h'
'absl/base/port.h'
'absl/base/attributes.h'e
'absl/base/optimization.h'
'absl/types/optional.h'
'absl/utility/utility.h'
'absl/base/internal/inline_variable.h'
'absl/base/internal/identity.h'
'absl/base/internal/invoke.h'
'absl/meta/type_traits.h'
'absl/memory/memory.h'
'absl/types/bad_optional_access.h'
'absl/types/variant.h'
'absl/types/internal/variant.h'
'absl/types/bad_variant_access.h'
INFO: Elapsed time: 8.772s, Critical Path: 4.63s, Remote (0.00% of the time): [queue: 0.00%, setup: 0.00%, process: 0.00%]
INFO: 83 processes: 83 local.
FAILED: Build did NOT complete successfully
We can fix this problem in one of two ways:
- implement sandboxing on Windows
- merge the Abseil and GoogleTest repos and remove external repo
references to each other
In fact I just did that (merging the repos):
1. I looked at the com_google_googletest rule in Abseil's WORKSPACE,
downloaded the zip file, extracted it somewhere, copied everything except
for the WORKSPACE file into the Abseil source tree.
2. Removed the com_google_googletest from Abseil's WORKSPACE.
3. Removed the strings @com_google_googletest and @com_google_absl
from all BUILD* files.
After that I could successfully bazel build --define absl=1 absl/... on
Windows.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#5640 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AMJSMkukP7o75kTLNAAfcPpAaDYAbDCCks5utVLbgaJpZM4VWlmm>
.
|
Inspired by @laszlocsomor's solution: In a I believe (*) is how most Bazel users consume Abseil and GoogleTest (main repo is user's own code). This solution should enable Abseil team to continue their work without breaking existing Abseil and GoogleTest users. |
Thank you for this suggestion!
I am a bit hazy for some reason.... could you please point me to the actual
workspace file to look at?
Thanks again
G
…On Fri, Nov 9, 2018 at 9:37 AM Loo Rong Jie ***@***.***> wrote:
Inspired by @laszlocsomor <https://github.com/laszlocsomor>'s solution:
In a WORKSPACE, specify Abseil and GoogleTest as [new_]local_repository
or [new_]http_archive (so neither Abseil nor GoogleTest is the main repo
__main__) (*), then run bazel test --define absl=1
@com_google_absl//absl/base/.... All tests pass.
I believe (*) is how most Bazel users consume Abseil and GoogleTest.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#5640 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AMJSMrSmlWMgJYIrcwSO0LymG25SPtUuks5utZMZgaJpZM4VWlmm>
.
|
Sure thing! See https://github.com/rongjiecomputer/bazel-issue/tree/master/abseil EDIT: fix
|
Sandboxing on Windows would solve this problem. |
Lack of sandboxing is the culprit of this bug, and @rongjiecomputer is working on #5136 to address that. Closing this in favor of #5136. |
Actually, let's keep this open until sandoxing is done. |
[Example failure](https://oss-fuzz-build-logs.storage.googleapis.com/log-8eabbac0-bb8f-4f90-a840-f23efe427e0e.txt) This only occurs on the fuzzers for some reason, not on Travis CI. Error is similar to bazelbuild/bazel#5640. Change spawn strategy to `sandboxed` to work around this. Not sure why it was `local` to begin with (other than a slight performance improvement). Tested locally to ensure this spawn strategy will work. ``` python infra/helper.py build_fuzzers --sanitizer address --engine libfuzzer --architecture x86_64 esp-v2 ``` Signed-off-by: Teju Nareddy <nareddyt@google.com>
[Example failure](https://oss-fuzz-build-logs.storage.googleapis.com/log-8eabbac0-bb8f-4f90-a840-f23efe427e0e.txt) This only occurs on the fuzzers for some reason, not on Travis CI. Error is similar to bazelbuild/bazel#5640. Change spawn strategy to `sandboxed` to work around this. Not sure why it was `local` to begin with (other than a slight performance improvement). Tested locally to ensure this spawn strategy will work. ``` python infra/helper.py build_fuzzers --sanitizer address --engine libfuzzer --architecture x86_64 esp-v2 ``` Signed-off-by: Teju Nareddy <nareddyt@google.com>
Thank you for contributing to the Bazel repository! This issue has been marked as stale since it has not had any activity in the last 2.5 years. It will be closed in the next 14 days unless any other activity occurs or one of the following labels is added: "not stale", "awaiting-bazeler". Please reach out to the triage team ( |
This issue has been automatically closed due to inactivity. If you're still interested in pursuing this, please reach out to the triage team ( |
Description of the problem / feature request:
We are getting a spurious
this rule is missing dependency declarations for the following files included by ...
when using MSVC to build on Windows. I say it is spurious because the same command works correctly on Linux and OSX, and I've verified the BUILD rule is correct.Bugs: what's the simplest, easiest way to reproduce this bug? Please provide a minimal example if possible.
On Windows + MSVC:
The
--define absl=1
part makes Google Test use Abseil. So Abseil tests uses Google Test, which in turn uses the Abseil libraries. I'm not sure if the circular project dependency has anything to do with this. Everything works as expected without--define absl=1
.Output on my machine:
It's interesting that I see both
Compiling absl/base/internal/spinlock.cc;
andexternal/com_google_absl/absl/base/internal/low_level_alloc.cc;
(note theexternal/com_google_absl
prefix). It is as if Bazel doesn't understand that the root WORKSPACE, namedcom_google_absl
, isn't the same Abseil that Google Test depends on, even though it depends on Abseil under the namecom_google_absl
. A Linux build never shows theexternal/com_google_absl
in the output.What operating system are you running Bazel on?
Windows 2008 R2 on GCP. I also tried it on a Kokoro instance (not sure of the Windows version of that) and got the same result.
What's the output of
bazel info release
?What's the output of
git remote get-url origin ; git rev-parse master ; git rev-parse HEAD
?Have you found anything relevant by searching the web?
#5087 is possibly the same issue, but I'm not sure. That issue refers to remote workers.
The text was updated successfully, but these errors were encountered: