deps vs. exported_deps for c++?

[zjturner]

Semantically I understand the difference, but I'm curious what the thinking is about best practices? Consider the following example. We'll assume for this example that b/b.h looks like this:

// b.h
#include "a/a.h"

cxx_library(
    name="a",
    public_include_directories=["a/include"],
    headers=["a/include/a/a.h"],
cxx_library(
    name="b",
    public_include_directories=["b/include"],
    headers=["b/include/b/b.h"],
    deps=["//:a"],
#    Is this preferable?
#    exported_deps=["//:a"]
cxx_library(
    name="c",
    public_include_directories=["c/include"],
    headers=["c/include/c/c.h"],
    deps=[
        "//:b",

        # This is required unless we uncomment the exported dep in b
        "//:a",
    ]

So this is more of a philisophical question. If we use exported_deps, targets carry their dependencies with them. It makes maintenance a lot easier. Adding a dependency from c to b requires doing exactly that: Add a dependency to b. If we use deps instead requires a potentially never-ending cycle of "build / interpret-error-message / add-dependency", which is difficult to scale. Worse though is that if we aren't using exported_deps, removing the dependency from b to a requires you to remember that you need to also remove it from c. And I don't think buck2 has the ability to warn you about superfluous dependencies (I think this isn't a solvable problem in general).

But I know there's argument for trying to avoid exported_deps, and this methodology is used at scale (I think google internally operates like this?), the thinking being that requireing it to be explicit leads to better code hygiene and more awareness of your dependency chains.

Curious to hear others' thoughts and what your experience has been at scale. My gut tells me you want to be using exported_deps because of the maintenance on large scale projects.

[cjhopman]

My recommendation has been that if you are exposing something from your dependencies in a way that your dependents are going to need it, then you should export that dependency. I think trying to avoid exported_deps entirely would be too painful for the small value it gives you. I could see that calculus being different if you had a system that could automatically keep your deps up-to-date based on the contents of the sources.

[zjturner]

Do you think buck2 could gain the ability to report unneeded dependencies? It seems possible in limited scenarios. For example, if the only thing a dependency exports is public include directories, and buck2's internal dependency analysis determines that no file from that path is ever loaded, it could warn (or even error) that there is an unnecessary dependency.

From here, there may be opt-in ways to take this further. For example, maybe there could be something like buck2 check-deps <target-pattern>. Maybe it looks for exported preprocessor definitions that don't alter the generated output by compiling both ways and checking the hash.

Just thinking out loud though, it might trigger too infrequently to be worth the development effort.

[zjturner]

Another kind of warning / error that might be useful. If B has an exported_dep of A, and C has a dep of B, then C doesn't need to explicitly specify a dep on A to make it work, even if C directly (not just transitively) relies on A. Is there any way to detect this during analysis and require that C explicitly specify the dep? Example:

cxx_library(
    name = "a",
    visibility = ["PUBLIC"],
    public_include_directories=["a"],
    headers=["a/a/a.h"]
)

cxx_library(
    name = "b",
    visibility = ["PUBLIC"],
    public_include_directories=["b"],
    headers=["b/b/b.h"],
    exported_deps=["//:a"]
)

cxx_library(
    name = "c",
    visibility = ["PUBLIC"],
    public_include_directories=["c"],
    headers=["c/c/c.h"],
    exported_deps=["//:b"]
)

cxx_binary(
    name = "d",
    visibility = ["PUBLIC"],
    include_directories=["d"],
    headers=["d/d/d.h"],
    srcs=["d/d.cpp"],
    deps=["//:c"]
)

And here are a, b, c, and d source files:

// a.h
using MyType = int;

// b.h
#include "a/a.h"

// c.h
#include "b/b.h"

// C should be required to explicitly export a, in case the dependency on b
// is ever refactored out.
#include "a/a.h"

// d.h
#include "c/c.h"

// d.cpp
#include "d/d.h"

// d should be required to explicitly depend on a, since it directly includes a header.
#include "a/a.h"

int main(MyType argc, char **argv)
{
}

In theory, I think buck2 has enough information to be able to enforce this. But I tested this out, and if I run buck2 build //:d right now, everything works fine. Do you see any value in making buck2 error in this example (is it even possible)?

[andyfriesen]

It's quite easy for BXL to traverse the set of targets and introspect them.

It would be entirely feasible to implement a sort of linter atop that.