add %virtual_includes% includes substitution

[werkt]

use of include_prefix prevents the facility of includes from supplying
the virtual_includes prefix without a substitution pattern. The
system_include_paths configuration list will contain, for each member of
includes, the _virtual_includes root in place of the string
%virtual_includes%.

[werkt]

+@ulfjack

[lberki]

(sorry for the late reply!)

I'm not exactly a big fan -- this adds another little macro substitution in the BUILD language. At the very least, this should be implemented using the $(VARIABLE) template variable substitution mechanism and there should be a test case that showcases what this commit does.

Is there a good reason for both includes and include_prefix / strip_include_prefix to coexist? If there is, is it possible to make them work together "by default", i.e. make it so that the appropriate directory structure is created under _virtual_includes so that the header files are available at the right location without magic like this?

[werkt]

The usecase for the combination is to remove some warnings that are suppressed under -isystem inclusion that are not otherwise distinguishable. They all work 'by default' together, but using include_prefix specifically prevents the use of includes to provide -isystem parameterization.

I don't care what does the substitution, and I agree about the test. I'll try to find some time to work it up with the $(VARIABLE) mechanism and with verification.

[lberki]

I'm afraid I don't understand the "remove some warnings... that are not otherwise distinguishable" part :( Do you have an example?

[werkt]

Explaining this use case is going to be difficult, but here goes:

We are building 3rd party libraries that provide headers as a part of their interface that use deprecated features of C++ standard libraries (my canonical example is apache avro, using std::auto_ptr from libstdc++).
The headers as distributed with the source archives are not prefixed with a common include path, so in order to separate them from the global include namespace (with paths that could collide with it), we use include_prefix to substitute an avro path ahead of the headers.
This prefixing reflects a typical 'installation' of avro as well, corresponding to its example usage code.
When we include avro headers without specifying -isystem for its include hierarchy, the declarations of std::auto_ptr based methods, for example, trigger compiler warnings (that we promote to errors with a global -Werror C++ compile semantic).
When we include avro headers by specifying -isystem (through includes, and via include_prefix and my substitution), these deprecated warnings (and others) are suppressed and our compiles can proceed as normal.
We don't want to prevent these warnings (promoted to errors) for other uses of these deprecated interfaces in our 1st party code, so disabling the warning or all promotion is out of the question.
This suppression occurs per http://clang.llvm.org/docs/UsersManual.html#controlling-diagnostics-in-system-headers.

This substitution closes the gap for include_prefixed headers in cc_library that would otherwise have already been available to includes, and prevents a loss of control over this command line facility with respect to compile actions.

[lberki]

Well, this is indeed difficult to understand :) Let me see if I understand this correctly: what you want to do is to be able to say e.g. in avro_sources/BUILD:

cc_library(name, "avro", include_prefix="avro", includes="src")

Then use e.g. avro_sources/src/foo/bar.h like this:

#include <avro/foo/bar.h>

Am I correct? How about making include_prefix and strip_include_prefix affect what includes instead? (I'm not sure if this is possible, I'd need to look at our internal code to see if anything would break, but I'm cautiously optimistic)

[werkt]

You've got it right there, sorry I couldn't put it more succinctly.

You can do that change if you like, but I would probably be loathe to have the automatic system include mechanism (#include <...>) available without control in the same way. Also, the includes option is dual-purpose, for both consumers and providers, so provide the mechanism automatically without being able to specify the virtual include prefix for one special member of the includes list, makes me concerned for even more implicit mess

[werkt]

You've identified includes incorrectly there as a single string parameter, which fits the provider frame of reference of avro, but not the consumer's reference, as includes is also allowed to occur. Not sure if that was clear in my post.

[chsigg]

I have a similar use case. I would like CUDA headers to be quote-included as virtual headers (#include "third_party/gpus/cuda/include/cuda_runtime.h"). The CUDA headers themselves and Eigen include CUDA headers as '#include <cuda.h>'. So I need to combine both 'includes' and 'include_prefix'.

I think the following approach with two dependent header targets works:

filegroup(
  name = "cuda-includes",
  srcs = [
    "cuda/include/cuda.h", 
    "cuda/include/cuda_runtime.h",  #include <cuda.h>
  ],
)

cc_library(
  name = "cuda_virtual_headers",
  hdrs = [":cuda-includes"],
  include_prefix = "third_party/gpu",
)

cc_library(
  name = "cuda_headers",
  textual_hdrs = [":cuda-includes"],
  includes = ["cuda/include"],
  visibility = ["//visibility:private"],
  deps = [":cuda_virtual_headers"],
)

cc_library(
  name = "some_tf_op_kernel_gpu",
  srcs = ["some_tf_op_kernel_gpu.cc"], #include "third_party/gpus/cuda/include/cuda_runtime.h"
  deps = [":cuda_headers"],
)

[meisterT]

Closing for lack of activity. If you want to pick it up again, please comment.