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.

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

Windows: Use of strip_include_prefix causes "missing dependency declarations" errors #6337

Closed
mumbleskates opened this issue Oct 9, 2018 · 11 comments
Assignees
Labels
P4 This is either out of scope or we don't have bandwidth to review a PR. (No assignee) team-Rules-CPP Issues for C++ rules type: bug

Comments

@mumbleskates
Copy link

Description of the problem / feature request:

On Windows, inclusion of any header file from a rule with strip_include_prefix set will cause a "missing dependency declaration" error to be thrown, breaking the build.

Feature requests: what underlying problem are you trying to solve with this feature?

I'm trying to use Bazel as it is documented and as it works in Linux. strip_include_prefix is an essential tool for describing the build processes of third-party repositories that are not natively set up in Bazel (i.e., most of them). The main alternative is to modify every file that includes another file by forking or by rewriting it with genrules, both of which levy a major maintenance burden on the developer on top of already-tedious BUILD-writing.

Bugs: what's the simplest, easiest way to reproduce this bug? Please provide a minimal example if possible.

Make the following directory tree:

bazel_bug_reproduction
  |-  WORKSPACE
  |-  BUILD
  \-  dir
       |-  a.cc
       |-  a.h
       \-  b.h

WORKSPACE, a.h, b.h:
(empty files)

BUILD:

cc_library(
    name = "a",
    srcs = ["dir/a.cc"],
    hdrs = ["dir/a.h"],
    strip_include_prefix = "dir",
    deps = [
        ":b",
    ],
)

cc_library(
    name = "b",
    hdrs = ["dir/b.h"],
    strip_include_prefix = "dir",
)

a.cc:

#include "a.h"
#include "b.h"

In the directory, run bazel build .... On Linux, this will probably succeed. On Windows, you are likely to receive an error like:

ERROR: O:/docs/repos/bazel-test/BUILD:1:1: undeclared inclusion(s) in rule '//:a':
this rule is missing dependency declarations for the following files included by 'dir/a.cc':
  'dir/a.h'
  'dir/b.h'

What operating system are you running Bazel on?

Windows 10 build 1803

What's the output of bazel info release?

release 0.17.2

Have you found anything relevant by searching the web?

Yes, more issues describing the same thing, that have gone unanswered for a long time: #3828

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

This still works fine in Linux by comparison:

mumbles@ubuntu ~/repos $ mkdir -p bazel-test/dir
mumbles@ubuntu ~/repos $ cd bazel-test/
mumbles@ubuntu ~/repos/bazel-test $ touch WORKSPACE
mumbles@ubuntu ~/repos/bazel-test $ cat > BUILD << EOF
> cc_library(
>     name = "a",
>     srcs = ["dir/a.cc"],
>     hdrs = ["dir/a.h"],
>     strip_include_prefix = "dir",
>     deps = [
>         ":b",
>     ],
> )
> 
> cc_library(
>     name = "b",
>     hdrs = ["dir/b.h"],
>     strip_include_prefix = "dir",
> )
> EOF
mumbles@ubuntu ~/repos/bazel-test $ touch dir/a.h
mumbles@ubuntu ~/repos/bazel-test $ touch dir/b.h
mumbles@ubuntu ~/repos/bazel-test $ cat > dir/a.cc << EOF
> #include "a.h"
> #include "b.h"
> EOF
mumbles@ubuntu ~/repos/bazel-test $ ls
BUILD  dir  WORKSPACE
mumbles@ubuntu ~/repos/bazel-test $ bazel build ...
Starting local Bazel server and connecting to it...
INFO: Analysed 2 targets (7 packages loaded).
INFO: Found 2 targets...
INFO: Elapsed time: 3.693s, Critical Path: 0.24s
INFO: 3 processes: 3 linux-sandbox.
INFO: Build completed successfully, 8 total actions
mumbles@ubuntu ~/repos/bazel-test $ bazel info release
release 0.17.2
@Nurdok
Copy link

Nurdok commented Oct 21, 2019

@hlopko this is still an issue with 1.0.0. Can you please take a look? Is there a workaround available? This feature is critical for running on Windows, IMO this should be P1 if there's no workaround.

@skeptic-monkey
Copy link
Contributor

I'am trying to make bazel rules for google/glog compatible with windows and I face the same issue.
This is annoying, as the only workaround I found is to use the includes argument of cc_library. But it kinda pollutes the compilation flags of all projects depending on it, which is not something we should do.

@drigz
Copy link
Contributor

drigz commented Nov 20, 2019

Workaround: add a separate cc_library that doesn't use strip_include_prefix, eg:

    cc_library(
        name = "windows_glog_headers",
        hdrs = glob(["src/windows/glog/*.h"]),
        strip_include_prefix = "src/windows",
        deps = [":strip_include_prefix_hack"],
    )

    # Workaround https://github.com/bazelbuild/bazel/issues/6337 by declaring
    # the dependencies without strip_include_prefix.
    cc_library(
        name = "strip_include_prefix_hack",
        hdrs = glob(["src/windows/*.h"]),
    )

google/glog@1bb0133#diff-71fb65f6a0f5906fd8a4c156e6f77a7dR130

@hlopko
Copy link
Member

hlopko commented Nov 29, 2019

I'm afraid I won't be able to work on this any time soon. Maybe @meteorcloudy will have a free afternoon? :)

@hlopko hlopko assigned meteorcloudy and unassigned hlopko Nov 29, 2019
@meteorcloudy
Copy link
Member

Hmm. this is another "include" issue that's caused by not having sandbox on Windows. Same root cause as #5640

To summarize, the reason is a.h, b.h, a.cc are all in the same dir. During compiling time, a.h and b.h will be visible without sandbox. So the compiler always finds them first before a.h and b.h in the virtual directory we created to strip the prefix.

You can reproduce the same issue by disabling sandbox on Linux:

pcloudy@pcloudy:~/workspace/my_tests/debug-6337
$ bazel build //...
Starting local Bazel server and connecting to it...
INFO: Analyzed 2 targets (13 packages loaded, 49 targets configured).
INFO: Found 2 targets...
INFO: Elapsed time: 2.713s, Critical Path: 0.22s
INFO: 3 processes: 3 linux-sandbox.
INFO: Build completed successfully, 8 total actions
pcloudy@pcloudy:~/workspace/my_tests/debug-6337
$ bazel clean 
INFO: Starting clean (this may take a while). Consider using --async if the clean takes more than several minutes.
pcloudy@pcloudy:~/workspace/my_tests/debug-6337
$ bazel build //... --spawn_strategy=standalone
INFO: Analyzed 2 targets (13 packages loaded, 49 targets configured).
INFO: Found 2 targets...
ERROR: /usr/local/google/home/pcloudy/workspace/my_tests/debug-6337/BUILD:1:1: undeclared inclusion(s) in rule '//:a':
this rule is missing dependency declarations for the following files included by 'dir/a.cc':
  'dir/a.h'
  'dir/b.h'
INFO: Elapsed time: 0.379s, Critical Path: 0.03s
INFO: 0 processes.
FAILED: Build did NOT complete successfully

There's really no good fix from Bazel side except implementing sandbox on Windows, which is not currently a priority.

But users should be able to workaround by not having the headers they want to strip in the same directory as the source files.

@mumbleskates
Copy link
Author

"not having the headers they want to strip in the same directory as the source files" is a poor solution that does not work for many use cases, because much of the time the entire reason for using strip_include_prefixes is because the code is from a third party, such as a submodule.

I'm not sure what makes implementing the entire sandbox easier than detecting this aliasing between the file in . vs the file in the virtual // path, but complexity of the fix doesn't make it less of a blocker for using bazel cross-platform.

@meteorcloudy
Copy link
Member

I'm not sure what makes implementing the entire sandbox easier than detecting this aliasing between the file in . vs the file in the virtual // path

It's because even if you don't specify /I., the compiler will still look into the . directory. So we have to remove dir/a.h and dir/b.h to allow compiler to pick headers from the virtual path.

pcloudy@PCLOUDY0-W C:\src\tmp\xrc6ifik\execroot\__main__
$ cl.exe /Ibazel-out/x64_windows-fastbuild/bin /Ibazel-out/x64_windows-fastbuild/bin/_virtual_includes/a /Ibazel-out/x64_windows-fastbuild/bin/_virtual_includes/b /showIncludes /Fobazel-out/x64_windows-fastbuild/bin/_objs/a/a.obj /c dir/a.cc
Microsoft (R) C/C++ Optimizing Compiler Version 19.00.24234.1 for x64
Copyright (C) Microsoft Corporation.  All rights reserved.

a.cc
Note: including file: c:\src\tmp\xrc6ifik\execroot\__main__\dir\a.h
Note: including file: c:\src\tmp\xrc6ifik\execroot\__main__\dir\b.h

pcloudy@PCLOUDY0-W C:\src\tmp\xrc6ifik\execroot\__main__
$ del c:\src\tmp\xrc6ifik\execroot\__main__\dir\a.h

pcloudy@PCLOUDY0-W C:\src\tmp\xrc6ifik\execroot\__main__
$ del c:\src\tmp\xrc6ifik\execroot\__main__\dir\b.h

pcloudy@PCLOUDY0-W C:\src\tmp\xrc6ifik\execroot\__main__
$ cl.exe /Ibazel-out/x64_windows-fastbuild/bin /Ibazel-out/x64_windows-fastbuild/bin/_virtual_includes/a /Ibazel-out/x64_windows-fastbuild/bin/_virtual_includes/b /showIncludes /Fobazel-out/x64_windows-fastbuild/bin/_objs/a/a.obj /c dir/a.cc
Microsoft (R) C/C++ Optimizing Compiler Version 19.00.24234.1 for x64
Copyright (C) Microsoft Corporation.  All rights reserved.

a.cc
Note: including file: bazel-out/x64_windows-fastbuild/bin/_virtual_includes/a\a.h
Note: including file: bazel-out/x64_windows-fastbuild/bin/_virtual_includes/b\b.h

But perhaps we can fix this issue by allowing including headers in the source file's directory.

@jbms
Copy link

jbms commented Apr 7, 2020

I can confirm that the workaround suggested by @drigz works well. I've packaged it up as a macro so that you can just use cc_library to cc_library_with_strip_include_prefix in place of cc_library:

https://github.com/google/tensorstore/blob/53ca393768bd9f47937b7a78b33429f3ab1891d5/utils.bzl#L125

# Workaround https://github.com/bazelbuild/bazel/issues/6337 by declaring
# the dependencies without strip_include_prefix.
def cc_library_with_strip_include_prefix(name, hdrs, deps = None, **kwargs):
    strip_include_prefix_name = name + "_strip_include_prefix_hack"
    if deps == None:
        deps = []
    deps = deps + [":" + strip_include_prefix_name]
    native.cc_library(
        name = strip_include_prefix_name,
        hdrs = hdrs,
        visibility = ["//visibility:private"],
    )
    native.cc_library(
        name = name,
        hdrs = hdrs,
        deps = deps,
        **kwargs
    )

However, it would be ideal if Bazel could somehow just make cc_library already have the behavior of this workaround, which presumably wouldn't require any work on a Windows sandbox, etc.

Is there a reason that would be difficult to do?

@mumbleskates
Copy link
Author

It's not a complete fix either, iirc, since sometimes the stripped prefix is needed to get the files inside the folder to include each other correctly.

@c-mita c-mita added P4 This is either out of scope or we don't have bandwidth to review a PR. (No assignee) and removed P2 We'll consider working on this in future. (Assignee optional) labels Nov 23, 2020
bazel-io pushed a commit that referenced this issue Nov 25, 2020
…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
philwo pushed a commit that referenced this issue Apr 19, 2021
…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
@fmeum-ci
Copy link

fmeum-ci commented Dec 9, 2021

@scentini Not sure why this didn't happen automatically, but shouldn't e3b7e17 have closed this issue?

@scentini
Copy link
Contributor

scentini commented Dec 9, 2021

Indeed, thanks for flagging. @meteorcloudy would you do the honors?

copybara-service bot pushed a commit to google/tensorstore that referenced this issue Oct 3, 2022
Eliminate `template_rule` from `utils.bzl`.  This is now available as
`expand_template` from bazel_skylib.

Eliminate `cc_library_with_strip_include_prefix` from `utils.bzl`.
The Bazel bug (bazelbuild/bazel#6337) is
fixed as of Bazel 5.0.

Replace uses of `genrule` with `write_file` from bazel_skylib.

Eliminate unnecssary `constraint_value` -> `config_setting` aliases,
now that Bazel allows constraint_value targets to be used directly in
`select`.

Updates bazelisk.py to fix deprecation warning:
bazelbuild/bazelisk#379

PiperOrigin-RevId: 478621631
Change-Id: I2e647861a78ee456fd44bc6f23223611162a136f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P4 This is either out of scope or we don't have bandwidth to review a PR. (No assignee) team-Rules-CPP Issues for C++ rules type: bug
Projects
None yet
Development

No branches or pull requests