Dependency visibility should be enforced

[david-german-tri]

Absent a concept of "visibility" in the build system, we should have a test that does a rudimentary sanity check of #include statements. To at least a first approximation, if CMake visits directory A before directory B, nothing in directory A should #include a file from directory B. (If you know of cases where such a dependency should be allowed, please chime in!)

The implementation of this test doesn't need to be rock-solid. One-off static analysis in Python would be pretty useful.

@jwnimmer-tri, I know this is also of interest to you.

[jwnimmer-tri]

For context, #1983 was the first example of fixing physical layering defects (c.f. Lakos).

[david-german-tri]

@billhoffman Is there a standard way of doing this with CMake?

[jamiesnape]

Not exactly what you want, but with regard to headers, as a start, we have include-what-you-use running on a nightly basis.

https://drake-jenkins.csail.mit.edu/job/linux-clang-nightly-include-what-you-use/

[liangfok]

Nice! It looks like this tool enforces the "weak #include rule" which states that the transitive closure of all #include statements in a file should cover all symbols used in the file. Is this correct?

[david-german-tri]

Great to know that IWYU is running, but it seems tangential to the visibility/physical-layering issue.

[jamiesnape]

You need to include the correct headers first.

[david-german-tri]

I'm not sure what you mean. To get started, all we really need is the ordered list of directories that CMake visits. Is that a thing we can make it output?

[jwnimmer-tri]

As a clarification, by way of example, here are three specific defects that we'd like to find with this kind of enforcement:

• drake/drake/core/test/vector_test.cc should not be allowed to use drake/drake/util/testUtil.h (util can depend on core, but not the other way around)
• drake/drake/util/yaml/yamlUtil.h should not be allowed to use drake/drake/systems/plants/RigidBodyTree.h (systems can depend on util, but not the other way around)
• drake/drake/solvers/qpSpline/splineGeneration.h should not be allowed to use drake/drake/systems/trajectories/PiecewisePolynomial.h (systems can depend on solvers, but not the other way around)

[david-german-tri]

Let's make some time to talk about this one when Kitware visits TRI next week.

[david-german-tri]

From f2f discussion with @bradking and @jwnimmer-tri:

Part 1: Determining the physical ordering automatically. We can replace add_subdirectory with a drake_add_subdirectory macro, which will append to a global property with set_property(GLOBAL.... At the end of CMake execution, the contents of that property are the physical ordering that we want to enforce.

Alternative considered: Develop a separate notation for dependency declaration. Do a topological sort and cycle check on that notation in both CMake and our enforcement script. This may become a first-class feature in an upcoming version of CMake, in which case we can switch to it then.

Part 2: Enforcing the physical ordering. We can write the ordering to a file, and then run a Python script ctest that parses the #includes from every C++ source file and checks them against the physical ordering.

Alternative considered: Get the compiler to dump the #includes. This is more principled on the platforms that support it, but not portable and more work.

Alternative considered: Restructure the Drake tree so that the compiler can enforce physical ordering by only including a -I for permissible subtrees.

Alternative considered: Restructure the Drake libraries so that CMake can enforce physical encoding. This would require making all dependencies (even header-only ones) artificial link dependencies, using a Schwarz counter or similar.

[jwnimmer-tri]

I suggest WONTFIXing this, in lieu of #3129.

[jamiesnape]

I would suggest it anyway. This is not a CMake issue per se.

[jwnimmer-tri]

Right, its not a CMake-specific issue.

However, my premise is that #3129 does not let us even write code that violates dependency layering, so creating a separate tool to enforce it would only be useful if #3129 was too long to wait for, and I don't think it is.

[jamiesnape]

I think you are confusing dealing with legacy code with the build system.

[david-german-tri]

Agree with @jwnimmer-tri, closing as wontfix.