Skip to content
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.

Sign up for GitHub

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

Jump to bottom

virtual includes can confuse inclusion checking #3828

Closed
benjaminp opened this issue Sep 28, 2017 · 2 comments
Closed

virtual includes can confuse inclusion checking #3828

benjaminp opened this issue Sep 28, 2017 · 2 comments
Labels
P3 We're not considering working on this, but happy to review a PR. (No assignee) team-Rules-CPP Issues for C++ rules type: bug

Comments

@benjaminp
Copy link
Collaborator

Consider the error building this simple library:

$ mkdir lib
$ touch WORKSPACE lib/header.h
$ echo '#include "header.h"' > lib/impl.c
$ cat BUILD
cc_library(
    name = 'mylib',
    hdrs = ["lib/header.h"],
    srcs = ["lib/impl.c"],
    strip_include_prefix = 'lib',
)
$ bazel build --spawn_strategy=standalone :mylib
ERROR: BUILD:2:1: undeclared inclusion(s) in rule '//:mylib':
this rule is missing dependency declarations for the following files included by 'lib/impl.c':
  'lib/header.h'.
Target //:mylib failed to build
Use --verbose_failures to see the command lines of failed build steps.

The problem seems to be that inclusion checking is expecting header.h to always be included as bazel-out/local-fastbuild/bin/_virtual_includes/mylib/header.h. That doesn't happen in this example because the quoted-include in impl.c ends up using the sibling header.h rather than the one in _virtual_includes. Indeed, we can fix this error by putting "lib/header.h" into the library's srcs. Interestingly, this example works in the sandbox because lib/header.h isn't placed in the sandbox, so the expected virtual header is included.

It's probably the case that libraries should be including their own exported headers with angle brackets rather than quotes. However, strip_include_prefix is mostly useful for third-party code, which can't be forced to follow any particular conventions.
@katre katre added category: rules > C++ P2 We'll consider working on this in future. (Assignee optional) labels Sep 28, 2017
@mumbleskates mumbleskates mentioned this issue Oct 9, 2018
Closed
@hlopko hlopko added team-Rules-CPP Issues for C++ rules and removed category: rules > C++ labels Oct 11, 2018
@ozio85 ozio85 mentioned this issue Dec 13, 2018
Closed
@ozio85 ozio85 mentioned this issue Jan 3, 2019
Closed
@jmillikin jmillikin mentioned this issue Jan 23, 2019
Closed
@baskus
Copy link

baskus commented Feb 11, 2019

Is this being looked at? 😃

@aaliddell aaliddell mentioned this issue Aug 1, 2019
Open
@hlopko hlopko removed their assignment Dec 6, 2019
@matthewvon
Copy link

fyi: seeing same problem in build of Facebook's rocksdb. They moved everything to using "#pragma once" and both clang and msvc on Windows fail.

@shrouxm shrouxm mentioned this issue Nov 16, 2020
Open
@c-mita c-mita added P3 We're not considering working on this, but happy to review a PR. (No assignee) type: bug and removed P2 We'll consider working on this in future. (Assignee optional) labels Nov 24, 2020
copybara-service bot pushed a commit to tensorflow/tensorflow that referenced this issue Dec 24, 2020
@mihaimaruseac @tensorflower-gardener
Bump TF's minimum Bazel version to 3.7.2.
3e9fccb 
Bug fixed: bazelbuild/bazel#3828

We need this because we are working on integrating TF with a different project that should remove the breakages caused by LLVM integration

PiperOrigin-RevId: 348886284
Change-Id: Iad539321f1ba7590e4ee77b93d67eb5401d9fec8
@scentini scentini mentioned this issue Jan 7, 2021
Merged
GMNGeoffrey pushed a commit to google/llvm-bazel that referenced this issue Mar 10, 2021
@scentini
Work around bazelbuild/bazel#3828 (#130)
5d2cd83 
Current Bazel has a bug (bazelbuild/bazel#3828) when using `strip_include_prefix` that causes Bazel to erroneously report a missing dependency declaration.

This bug shows up when trying to `bazel build @llvm-project//llvm:NVPTXCodeGen`:
```
 undeclared inclusion(s) in rule '@llvm-project//llvm:NVPTXCodeGen':
this rule is missing dependency declarations for the following files included by 'external/llvm-project/llvm/lib/Target/NVPTX/NVPTXAsmPrinter.cpp':
  'external/llvm-project/llvm/lib/Target/NVPTX/TargetInfo/NVPTXTargetInfo.h'
```

This has been fixed in bazelbuild/bazel@e3b7e17. This PR is a workaround until we have a Bazel version that contains the fix.
philwo pushed a commit that referenced this issue Apr 19, 2021
@scentini @philwo
When generating a symlink in _virtual_includes, add the original head…
af0edb5 
…er to the 'allowed to use' set too

When a cc_library 'a' makes use of strip_include_prefix, bazel creates a symlink for the hdrs at bazel-out/.../bin/_virtual_includes/a/
Later, bazel passes -I bazel-out/.../bin/_virtual_includes/a/ to the command line to tell the compiler where to look for those headers.

For each header in hdrs, bazel will either put the header artifact itself in a set of 'allowed to use' headers, or, in the case of strip_include_prefix usage, bazel will add the symlink to the header under bazel-out/.../bin/_virtual_includes/a/ as 'allowed to use'.

When searching for headers, the compiler will do the following (Taken from https://gcc.gnu.org/onlinedocs/gcc/Directory-Options.html):

1. For the quote form of the include directive, the directory of the current file is searched first.
2. For the quote form of the include directive, the directories specified by -iquote options are searched in left-to-right order, as they appear on the command line.
3. Directories specified with -I options are scanned in left-to-right order.
...

In the case of the following cc_library:
```
cc_library(
    name = 'foo',
    hdrs = ["lib/foo.h"],
    srcs = ["lib/foo.cc"],
    strip_include_prefix = 'lib',
)
```
if foo.cc includes foo.h as #include "foo.h"

the compiler will find the header in step 1 of the search, thus will use the original header and not the symlink. The compiler will note this in the .d file. Bazel however added the symlink in the list of 'allowed to use' headers, so at some point it will error out with "undeclared inclusion(s) in rule" error due to the discrepancy.

This cl tells bazel to add the original header in the 'allowed to use' headers even when it generates a symlink in the _virtual_includes directory.

Fixes #3828 #6337

RELNOTES:None.
PiperOrigin-RevId: 344240730
lelandjansen pushed a commit to lelandjansen/llvm-bazel that referenced this issue May 2, 2021
@scentini @lelandjansen
Work around bazelbuild/bazel#3828 (google#130)
b104282 
Current Bazel has a bug (bazelbuild/bazel#3828) when using `strip_include_prefix` that causes Bazel to erroneously report a missing dependency declaration.

This bug shows up when trying to `bazel build @llvm-project//llvm:NVPTXCodeGen`:
```
 undeclared inclusion(s) in rule '@llvm-project//llvm:NVPTXCodeGen':
this rule is missing dependency declarations for the following files included by 'external/llvm-project/llvm/lib/Target/NVPTX/NVPTXAsmPrinter.cpp':
  'external/llvm-project/llvm/lib/Target/NVPTX/TargetInfo/NVPTXTargetInfo.h'
```

This has been fixed in bazelbuild/bazel@e3b7e17. This PR is a workaround until we have a Bazel version that contains the fix.
@silvergasp silvergasp mentioned this issue Dec 20, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
P3 We're not considering working on this, but happy to review a PR. (No assignee) team-Rules-CPP Issues for C++ rules type: bug
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
6 participants
@benjaminp @matthewvon @katre @hlopko @baskus @c-mita