Bazel: includes attribute of cc_* rules gives warning if not in 'third_party'

[AlexTalks]

The includes attribute of the cc_* rules gives a warning such as the following:

WARNING: /.../BUILD:51:14: in includes attribute of cc_library rule //:json: 'include' resolves to 'include' not in 'third_party'. This will be an error in the future.

However, it is an extremely useful feature for external users of Bazel to be able to specify how they would like their dependencies to include their headers.

[AlexTalks]

It might be possible to create a rule that has an attribute similar to includes, that will include directories with -I instead of -isystem. @orphis and I will try this.

[calpeyser]

I don't have a lot of context on the includes pattern and why we discourage it. Perhaps @klimek or @ulfjack can clarify.

[janakdr]

We discourage the includes pattern because files included that way go in
via the -isystem flag, which disables checks in clang and also disables
bazel's checks that you actually had access to that #include-d file in the
first place. From discussion with Alex, he'll see if his workaround works.
But @klimek may have opinions on the workaround.

On Thu, May 19, 2016 at 1:12 PM, Cal Peyser [email protected]
wrote:

I don't have a lot of context on the includes pattern and why we
discourage it. Perhaps @klimek https://github.com/klimek or @ulfjack
https://github.com/ulfjack can clarify.

—
You are receiving this because you were assigned.
Reply to this email directly or view it on GitHub
https://github.com/bazelbuild/bazel/issues/1286#issuecomment-220390814

[r4nt]

I agree that in open source projects a project should be able to specify how its files should be included. We don't have a good solution for that yet. Adding copts will not be reverse transitive, so that won't help. I mean, we could basically add a configuration to bazel to specify how third_party is handled:
a) consider "everything" third_party
b) have a dedicated subdir for third_party dependencies, but have higher standards for the main project
I see both in various projects in the open source C++ world.

[ulfjack]

The question is where we'd allow this to be configured. Probably in the workspace file, but it needs to be applied per-workspace, so we'd need to read that file even for remote workspaces.

[abergmeier]

@janakdr I am not sure why you chose to pass "Isystem" instead of "I" like e.g. CMake. Could you perhaps elaborate what the design decisions were?

[r4nt]

@abergmeier - that is a historical incident based on the specific layout of the codebase we have :( I believe @lberki is hopefully going to resolve the madness.

[lberki]

We are in the process of figuring out how include directories should work. Currently it's pretty much what we need at Google internally, which is why odd error message exists. If we can't come up with reasonable way to specify include directories correctly, I'll just remove these warnings from Bazel.

[kayasoze]

If the existing behavior of includes could be made safe for incremental rebuilds, that would be ideal. Our users build many third-party libraries, but each in their own repo, so moving them all to third_party would be problematic.

[ulfjack]

I'm working to re-enable the checks for -isystem, but that requires an
internal cleanup, which will take days or maybe weeks. I'm not sure if the
clang behavioral differences are also present in upstream clang. If not,
then re-enabling the checks should be sufficient for Bazel, and we could
remove the warning entirely.

On Tue, Jun 14, 2016 at 8:23 PM, kayasoze [email protected] wrote:

If the existing behavior of includes could be made safe for incremental
rebuilds, that would be ideal. Our users build many third-party libraries,
but each in their own repo, so moving them all to third_party would be
problematic.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/bazelbuild/bazel/issues/1286#issuecomment-225971314,
or mute the thread
https://github.com/notifications/unsubscribe/AHA9YWwL2ly6UNaGH6nbxy19tGfzwOvMks5qLvGlgaJpZM4IhuDA
.

[ulfjack]

Assigning to myself for re-enabling the deps checks.

[ulfjack]

Also see #1162.

[ghost]

On Wed, Jun 15, 2016 at 8:47 AM Ulf Adams [email protected] wrote:

I'm working to re-enable the checks for -isystem, but that requires an
internal cleanup, which will take days or maybe weeks. I'm not sure if the
clang behavioral differences are also present in upstream clang.

They are, otherwise you couldn't switch on any warnings in a C++ projects
(due to textual includes).

If not,
then re-enabling the checks should be sufficient for Bazel, and we could
remove the warning entirely.

On Tue, Jun 14, 2016 at 8:23 PM, kayasoze [email protected]
wrote:

If the existing behavior of includes could be made safe for incremental
rebuilds, that would be ideal. Our users build many third-party
libraries,
but each in their own repo, so moving them all to third_party would be
problematic.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<https://github.com/bazelbuild/bazel/issues/1286#issuecomment-225971314
,
or mute the thread
<
https://github.com/notifications/unsubscribe/AHA9YWwL2ly6UNaGH6nbxy19tGfzwOvMks5qLvGlgaJpZM4IhuDA

.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/bazelbuild/bazel/issues/1286#issuecomment-226102240,
or mute the thread
https://github.com/notifications/unsubscribe/AArFcf6GHFGSnW36hhpKTmEsbTdestdmks5qL6AJgaJpZM4IhuDA
.

[PiotrSikora]

Could we increase the priority on this? Also, if there is a plan to re-enable checks and remove this requirement, could we get rid of this warning sooner rather than later? Thanks!

[philbinj]

Any update on this? There was a thread on bazel-discuss on the same issue:
https://groups.google.com/forum/#!topic/bazel-discuss/H9d4gPW2RPo

[lberki]

I just sent out an internal code review to @calpeyser to remove this warning from Bazel (unfortunately, it has to be an internal review because, well, it moves the warning to internal code). We'll see how it will go.

[AlexTalks]

@lberki that's great news! This is our largest set of warnings when using Bazel 😄

[lberki]

...aand, submitted today. I would have totally forgotten it if you hadn't just bumped this thread. The fix should hit Github tomorrow the latest.