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

Undeclared headers dependencies doesn't lead to error (Windows) #9965

Closed
maximyurchuk opened this issue Oct 7, 2019 · 7 comments
Closed
Labels
P4 This is either out of scope or we don't have bandwidth to review a PR. (No assignee) stale Issues or PRs that are stale (no activity for 30 days) team-Rules-CPP Issues for C++ rules type: bug

Comments

@maximyurchuk
Copy link

Description of the problem / feature request:

Not all headers in cpp files are checked by Bazel that they are in dependencies (there is no this rule is missing dependency declarations for the following files included by ... error).

Since there is no sanbox on Windows there is should be some adequate way to detect undeclared dependencies.

Moreover bazel invokes cl.exe with /showIncludes option, but undeclared headers are not detected.

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

You can find reproduce code here.

Attention on lib.cc file:

#include <from_another_target/missing_dep.h>  // undeclared dependency
#include <missing_dep_from_another_target.h>  // undeclared dependency
#include "missing_dep.h"                      // undeclared dependency

// This case detected by Bazel
// #include "local_include/missing_dep.h"

int foo;

All these headers are not declared as dependencies.

What operating system are you running Bazel on?

Windows 10, 1903.

What's the output of bazel info release?

C:\git\bazel-issues\missing_headers_check>bazel-latest info release
INFO: Writing tracer profile to 'C:/users/yurchuk/_bazel_yurchuk/ivh3eama/command.profile.gz'
release 1.0.0rc4
@maximyurchuk maximyurchuk changed the title Undeclared headers dependencies doesn't lead to error Undeclared headers dependencies doesn't lead to error (Windows) Oct 7, 2019
@maximyurchuk
Copy link
Author

Also, if I change some of these headers bazel will rebuild dependent library (which has no dependency in BUILD file on these headers). So looks like bazel knows, that dependency exists, but ignores that this dependecy is undeclared.

@maximyurchuk
Copy link
Author

I just add some results of my analysis.

After exploring bazel source code I've found that include paths from another targets dependencies are passed with -isystem option. In other hand there is an option in bazel --strict_system_includes which is disabled by default which makes -isystem headers are less restrictive for verification: bazel just ignore all files from system includes.

Explanation above is appliable for

#include <from_another_target/missing_dep.h>  // undeclared dependency
#include <missing_dep_from_another_target.h>  // undeclared dependency

But if enable --strict_system_includes option, undeclared inclusion

#include "missing_dep.h"                      // undeclared dependency

is still doesn't lead to error. For unknown reason for me empty paths (i.e. headers in workspace root) are ignored in CppCompileAction.java
image

@iirina iirina added area-Windows Windows-specific issues and feature requests untriaged labels Oct 10, 2019
@maximyurchuk
Copy link
Author

iirina added team-Windows

jfyi: actually, issue present on other platforms (at least linux), but only if sandboxing is disabled (spawn_strategy=standalone).

@laszlocsomor
Copy link
Contributor

@maximyurchuk : Thanks for your report, I concluded the same as you. Forwarding the bug to the C++ team.

@maximyurchuk
Copy link
Author

hlopko referenced this issue ...

Please note, that actually this issue consists from 2 different parts (which appeared after some analisys):

  1. header from top level are not checked for inclusions (this issue strongly related with incompatible_validate_top_level_header_inclusions: also validate headers in the top level directory #10047)
  2. headers from most source code is not checked for inclusions on windows. I think this check should exists by default (at least for better bazel experience on windows). It can be fixed for example with --strict_system_includes=True by default or with passing inlcude paths with ordinary includes not system_includes (this issue not related with incompatible_validate_top_level_header_inclusions: also validate headers in the top level directory #10047)

Maybt it's better split issue on two different issues.

@laszlocsomor
Copy link
Contributor

Please note, that actually this issue consists from 2 different parts

Good point.

I can confirm that (2) also appears on Linux with --spawn_strategy=standalone.

bazel-io pushed a commit that referenced this issue Oct 17, 2019
#10047
#9965

With this flag flipped Bazel will also validate header inclusions for top level headers.

RELNOTES: Incompatible flag `--incompatible_validate_top_level_header_inclusions` has been added. See #10047 for details.
PiperOrigin-RevId: 275221690
@scentini scentini added P2 We'll consider working on this in future. (Assignee optional) team-Rules-CPP Issues for C++ rules and removed untriaged team-Rules-CPP Issues for C++ rules labels Oct 28, 2019
@oquenchil oquenchil added P4 This is either out of scope or we don't have bandwidth to review a PR. (No assignee) type: bug and removed P2 We'll consider working on this in future. (Assignee optional) labels Nov 19, 2020
@sgowroji sgowroji added the stale Issues or PRs that are stale (no activity for 30 days) label Feb 13, 2023
@sgowroji
Copy link
Member

Hi there! We're doing a clean up of old issues and will be closing this one. Please reopen if you’d like to discuss anything further. We’ll respond as soon as we have the bandwidth/resources to do so.

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) stale Issues or PRs that are stale (no activity for 30 days) team-Rules-CPP Issues for C++ rules type: bug
Projects
None yet
Development

No branches or pull requests

6 participants