public_deps transitivity and `gn check`

[Nico Weber]

Hi gn-dev,

Here's a question about dependency checking and public_deps. It's general, but it's motivated by re2 and absl, so I'll talk about this to hopefully make the discussion easier to follow.

re2 uses absl in its public headers. That means that targets that include re2 headers need to have -I flags matching the include paths used in re2. This is currently done by re2 having a public_dep on absl.

My mental model for public_deps is that it's for deps that are used in the public interface of a library, and this works, and looks like the right approach to me.

However, it does have the effect that any client of re2 can now include absl headers without having to have a dep on absl itself. `gn check` documentation explicitly mentions this behavior [1].

On one hand, that's understandable behavior: If you can't use re2 without using absl, depending on re2 is "enough" to use abls. On the other hand, for e.g. C++ headers, it's generally considered poor form to rely on some random header to include <vector> – cc files including that random header are supposed to include <vector> as well.

I'm writing this because someone sent me a CL to replace re2's public_dep on absl with a dep on absl, combined with using all_dependent_configs to push absl's include paths to deps of re2. That seems like a huge hack to me. Their argument is that in some other environment, their re2 doesn't publicly depend on absl, and so code using absl without an explicit dep on absl breaks in that other environment.

I'm not sure what exactly my question is! "What do you think of this situation", I suppose. Am I wrong about dislinking all_dependent_configs here? Should `gn check` be less transitive somehow? Something else?

Thanks,

Nico

1:

 https://chromium.googlesource.com/experimental/chromium/src/+/refs/wip/bajones/webvr/tools/gn/docs/check.md

"""
Public dependencies work transitively, so listing a target as a public dependency also exposes that target's public dependencies.
"""

[Brett Wilson]

I think all_dependent_configs is a bad hack and should be removed. It exists only because there was a GYP feature with that name and it wasn't practical to block on fixing every use-case as part of the GYP -> GN transition. I think the main legitimate use now is sometimes you need to inject linker flags into whatever links you without necessarily requiring the path to the linking target to be public.

The ambiguity on the header usage is unfortunate but I think unavoidable. Sometimes you want targets that depend on you to be able to use headers from your public_headers, and sometimes you don't, depending on how your targets are laid out and what they mean. I think this is too subtle to worry about most of the time, and in practice it's impossible to change anyway. I couldn't even get rid of all_dependent_configs!

Brett

[Philipp Wollermann]

Hi,

Bazel's C++ rules have a feature called "layering check" that works together with clang to ensure that C++ code can only include header files of libraries that are declared as direct dependencies in the BUILD file, and not any header that might be on the include path due to transitive dependencies.

I don't know how exactly it works and whether we could implement the same logic in GN, but maybe it would be an interesting feature to look into?

Here is a blog post I found that describes it in nice detail: https://maskray.me/blog/2022-09-25-layering-check-with-clang

And this is the original pull request: https://github.com/bazelbuild/bazel/pull/11440

Hope this is helpful!

Cheers,

Philipp

To unsubscribe from this group and stop receiving emails from it, send an email to gn-dev+un...@chromium.org.

[danakj]

On Fri, Sep 6, 2024 at 10:50 AM Brett Wilson <bre...@chromium.org> wrote:

I think all_dependent_configs is a bad hack and should be removed. It exists only because there was a GYP feature with that name and it wasn't practical to block on fixing every use-case as part of the GYP -> GN transition. I think the main legitimate use now is sometimes you need to inject linker flags into whatever links you without necessarily requiring the path to the linking target to be public.

Yeah we use it to add `ldflags` or `rustflags = [ --sysroot... ]` so it does have its uses beyond this header hack.

[Dirk Pranke]

On Fri, Sep 6, 2024 at 7:56 AM 'Philipp Wollermann' via gn-dev <gn-...@chromium.org> wrote:

Hi,

Bazel's C++ rules have a feature called "layering check" that works together with clang to ensure that C++ code can only include header files of libraries that are declared as direct dependencies in the BUILD file, and not any header that might be on the include path due to transitive dependencies.

I don't know how exactly it works and whether we could implement the same logic in GN, but maybe it would be an interesting feature to look into?

Here is a blog post I found that describes it in nice detail: https://maskray.me/blog/2022-09-25-layering-check-with-clang

And this is the original pull request: https://github.com/bazelbuild/bazel/pull/11440

The blog post looks like it was all in the context of C++ modules, though. Do you know if this can work w/o using modules?

-- Dirk

[Mirko Bonadei]

I'm not sure what exactly my question is! "What do you think of this situation", I suppose. Am I wrong about dislinking all_dependent_configs here? Should `gn check` be less transitive somehow? Something else?

I did look into this in 2017. While I find public_deps (and public_configs) really neat for propagating compiler flags only where they are needed (so we get nice and short compiler invocations), I also find it not intuitive and sometimes problematic for the build graph health. Let me give some examples:

- I have seen situations where removing a public_dep ended up with "gn check" complaining because one or more targets were getting away without declaring a dependency since they were getting it through another target. This can lead to failures far away from the place where the dependency is removed.
 
- If you want to avoid to use public_deps for a public header dependency, you should remove the #include from the public headers of your library. To do so you should use forward declarations but they are not always allowed by style guides (this can be an issue for 3rd party dependencies).

- GN itself doesn't enforce the distinction between "deps" and "public_deps", leaving it to the user to decide which one to use (this helped in WebRTC, where we had to use "deps" to avoid issues with layering check when we translate the build files to Bazel, but it also means that both styles are possible).

Yes, all_dependent_configs is much more invasive since it pushes the config up all the way to the root (I think this is what Bazel does). On the other hand, it does keep the dependency graph clean (you need to depend on what you use, no other target will expose your dependencies for free) and the propagation of the -I flag is only a workaround to ensure that transitively #including headers can compile.

At the end, it looks like there is no perfect way to handle this problem (either you have perfect compiler commands, or compiler commands with extra flags). This was the outcome when I talked with Brett and Dirk in 2017 (maybe even on this mailing list :) ).

I think that all_dependent_configs might sometimes be OK, especially if it is used at the boundary of the build graph (e.g. for 3rd party dependencies, to avoid that they silently export headers from other 3rd party dependencies).

Mirko

 

[Philipp Wollermann]

Hi Dirk,

On Sat, Sep 7, 2024 at 5:22 AM Dirk Pranke <dpr...@chromium.org> wrote:

Here is a blog post I found that describes it in nice detail: https://maskray.me/blog/2022-09-25-layering-check-with-clang

And this is the original pull request: https://github.com/bazelbuild/bazel/pull/11440

The blog post looks like it was all in the context of C++ modules, though. Do you know if this can work w/o using modules?

IIUC "layering check" is a clever combination of using clang's C++ modules with automatically constructing module maps for each C++ target based on the dependencies declared in BUILD files.

So, C++ modules, without the C++ code having to know that we're using modules. :)

The mapping works like this:

private *.h files in "srcs" => private textual header declarations in cppmap

public *.h files in "hdrs" => textual header declarations in cppmap

other targets in "deps" => use declarations in cppmap and a -fmodule-map-file=${target}.cppmap flags in the generated clang command

Let's say you have these target definitions:

# BUILD

cc_binary(
    name = "a",
    srcs = ["a.cc", "a.h"],
    deps = [":b"],
)

cc_library(
    name = "b",
    hdrs = ["b.h"],
    deps = [":c"],
)

cc_library(
    name = "c",
    srcs = ["c.cc"],
    hdrs = ["c.h"],
)

The targets would get converted to these C++ module maps:

# a.cppmap

module "a" {
  export *
  private textual header "a.h"
  use "b"
}
extern module "b" "b.cppmap"

# b.cppmap

module "b" {
  export *
  textual header "b.h"
  use "c"
}
extern module "c" "c.cppmap"

# c.cppmap

module "c" {
  export *
  textual header "c.h"
}

The sources & headers look like this:

// a.cc

#include "a.h"
int main() { fc(); }

// a.h

#include "c.h"

// b.h

#include "c.h"

// c.cc

void fc() {}

// c.h
void fc();

Our target "a" only depends on the other target "b", but it includes a header file from "c".

Thanks to the generated clang command looking like this:

clang \
    -c a.cc \
    -o a.o \
    -fmodule-name=a \
    -fmodule-map-file=a.cppmap \
    -fmodules-strict-decluse \
    -Wprivate-header \
    -fmodule-map-file=b.cppmap

# Note the missing -fmodule-map-file=c.cppmap due to themissing dependency on "c"

clang will complain that a.cc is including c.h, despite "a" not declaring a direct dependency on "c":

./a.h:1:10: error: module a does not directly depend on a module exporting 'c.h', which is part of indirectly-used module c
    1 | #include "c.h"
      |          ^

This is the "layering check" feature in a nutshell.

Cheers,

Philipp

[Dirk Pranke]

Got it, thanks for explaining!

-- Dirk

[Petr Hosek]

This is similar to what I implemented in https://gn-review.git.corp.google.com/c/gn/+/7401.

Note that some parts of that change were split and landed separately (CL1, CL2, CL3), but not the module map generator which would be helpful here.

[Danil Chapovalov]

I'm not a gn developer, just a gn user who reads documentation.

The original problem is third_party absl library that internally uses includes relative to own root, and thus requires extra include path to compile any cpp file that directly or indirectly includes an absl header.

One solution is to use "all_dependent_configs" variable in absl target [once]. Its documentation list the use case above: 

https://gn.googlesource.com/gn/+/main/docs/reference.md#var_all_dependent_configs

"this capability should generally only be used to add defines and include directories necessary to compile a target's headers”

It is still not a perfect fit, but to me looks better than anything else gn has to offer (I need to propagate config to all targets that indirectly include absl headers, while "all_dependent_configs" propagate it a bit further - to all targets that indirectly depend on absl).

Another solution [currently implemented] is to require each direct on indirect dependency of absl that intentionally or unintentionally expose absl header to depend on the absl via public_deps, or depend on another target that depends on absl via public_deps.

https://gn.googlesource.com/gn/+/main/docs/reference.md#var_public_deps

That does solve the problem because "public_configs that are part of the dependency are forwarded to direct dependents"
but it has several other consequences. The one that causes extra issues is "Public headers in the dependency are usable by dependents (includes do not require a direct dependency or visibility)" - that confuses automated tooling that attempts to add missing dependencies.

In particular because gtest has public_deps on re2 and re2 has public_deps on absl every unittests target assume to already depend on absl and thus tool doesn't add an absl target when a test include an absl header.

Above I see that "all_dependent_configs" should be treated as hack. But documentation doesn't reflect that. 

If gn authors plan to remove "all_dependent_configs", can documentation be updated, at the very least to avoid spreading "all_dependent_configs" variable beyond its current usage?

Is there plan to add a variable that better solves the problem currently listed as  "all_dependent_configs" use case?

[Brett Wilson]

I just uploaded https://gn-review.git.corp.google.com/c/gn/+/17760 to clarify the guidance for this feature.

Thanks for highlighting the problem

Brett