Script for ensuring correct Envoy include paths

[oschaaf]

Let's see if the format check in CI flags the current state as a failure with this.

Signed-off-by: Otto van der Schaaf oschaaf@we-amp.com

[oschaaf]

While the ci script seems to do what it is supposed to do in CI, referencing envoy includes via external/envoy/source/... unfortunately fails, which is sad. It seems that when we have @repo=envoy in a target, bazel won't set up these includes for Envoy

[htuch]

@oschaaf how do the existing external/envoy references work?

Do we need to fork or do some surgery on envoy_cc_library to avoid this include path masking? It seems the external/<dep> path is common, see https://docs.bazel.build/versions/master/cpp-use-cases.html#adding-include-paths.

[oschaaf]

So envoy_cc_test_library is doing something different compared to envoy_cc_library when it comes to exporting includes, which allows #include external/envoy/... to work.
I think the following line in the definition of enovy_cc_library might be relevant:

include_prefix = envoy_include_prefix(native.package_name())

and then

def envoy_include_prefix(path):
    if path.startswith("source/") or path.startswith("include/"):
        return "/".join(path.split("/")[1:])
    return None

I propose adding a flag to envoy_cc_library that would allow us to opt-out of having it manipulate these prefixes for us. I think that is a simple change, and voids the need for a CI check in NH to enforce the right include paths. The downside is that nearly all #include paths in NH would have to be updated, but that is a mechanical change.

[oschaaf]

@htuch maybe something along the lines of https://github.com/envoyproxy/envoy/compare/master...oschaaf:include-prefixes?expand=1 ?

[oschaaf]

After chatting, we have found a way forward with this with a different modification to envoy_cc_library [1]. The discussed approach, where we'll export foo_with_external_headers separately, will probably mean we will need this script to check for correct envoy includes, so requesting a review to start preparing this for merge.

[1]

def envoy_cc_library(
.... # we add a parallel native.cc_library
    native.cc_library(
        name = name + "_with_external_headers",
        hdrs = hdrs,
        copts = envoy_copts(repository) + copts,
        visibility = visibility,
        deps = [":" + name],
        strip_include_prefix = strip_include_prefix,

/assign htuch

[htuch]

Thanks, this is exactly what we were after I reckon.

[htuch]

Does this match the style in https://github.com/envoyproxy/nighthawk/blob/master/test/integration/integration_test.py? Ideally same style, check/fix_format applies.

[oschaaf]

I ran it through fix fomat to resolve, ptal?

[oschaaf]

For context, linking the PR to Envoy which would allows us to disambiguate the headers:
envoyproxy/envoy#8005

[oschaaf]

Ready for another look
/assign htuch

If we're happy with the way this looks, I propose adding the actual include changes here to make the new CI format check happy?

[htuch]

Looks good, thanks. Just a few comments.
/wait

[oschaaf]

Status:

• Updated the Envoy dependency to the revision that has envoy_cc_library: add export of foo_with_external_headers envoy#8005
• I had to include fixes to accommodate for changes in Envoy to this rather large change. Fortunately they are low-complexity
• Include paths are disambiguated modulo filesystem_impl.h & thread_impl.h which play by different rules. Those will be taken on in a follow up.
• Header ordering is reconfigured to minimize the diff yet take advantage of the new paths for more clarity.
• CI is all green
• Review comments addressed

/assign htuch
Ready for another pass!

[htuch]

Thanks!