bazel: replace -layering_check overrides with scoped private_hdrs

[maliberty]

Tests that needed module-internal headers were disabling layering_check via features = ["-layering_check"]. Mirror the cts pattern instead: a private_hdrs cc_library with visibility = ["//src/:subpackages"] exposes the internal headers to the module's own tests only.

• cts: broaden private_hdrs visibility from private to subpackages; drop the test's -layering_check
• gpl: broaden gpl_private_hdrs visibility, add fft.h, drop flag
• drt: broaden drt_private_hdrs visibility, drop flag + ../src include
• dst: introduce private_hdrs for the four src/*.h files; tests + the distributed binary all depend on it
• rsz: introduce private_hdrs covering the .hh files referenced by TestNestedJournal/TestResizerMt plus their transitive .hh chain; move those .hh files out of :rsz.srcs
• dft: drop stale -layering_check; the four tests already list every cc_library whose headers they include

With this change, no BUILD file in src/ disables layering_check.

Fixes #10478

[gemini-code-assist]

Code Review

This pull request improves build hygiene by enabling layering_check across several modules, including cts, dft, drt, dst, gpl, and rsz. The changes involve creating or exposing private_hdrs libraries and updating test targets to depend on them rather than using relative include paths or disabling layering checks. A review comment identifies that the TestResizer target in src/rsz/test/BUILD is missing the //src/rsz:private_hdrs dependency, which will likely cause a compilation failure after the removal of the layering_check feature and internal include paths.

[gemini-code-assist]

The TestResizer target is missing a dependency on //src/rsz:private_hdrs. Since features = ["-layering_check"] and the internal include path were removed, this test will likely fail to compile if it includes any of the headers now moved to private_hdrs. This target had the same TODO: includes private headers comment as TestNestedJournal and TestResizerMt, which both received the dependency update.

Suggested change

	deps = [
	deps = [
	"//src/rsz:private_hdrs",

[github-actions]

clang-tidy review says "All clean, LGTM! 👍"

[hzeller]

WorkerConnection.h uses dst/JobMessage.h so this makes it a cyclic dependency: private headers needs to depend on :dst because of that but dst already depends on private_hdrs.
(this problem also exists in a bunch of other private_hdrs already in the project, but let's not make the problem worse).

If you invoke bant with -vvv you see for each header that the associated sources and headers include the corresponding library that is needed for that

$(etc/get-bant-path.sh) dwyu src/dst:private_hdrs -vvv

... here the JoMessage is found to be needing //src/dst

src/dst/BUILD:19:10-31:[ src/dst/src/WorkerConnection.h include dependency check (//src/dst:private_hdrs) ]
[...]
src/dst/src/WorkerConnection.h:10:11-42: #include "src/dst/include/dst/JobMessage.h"
src/dst/src/WorkerConnection.h:10:11-42:    | //src/dst

[maliberty]

Perhaps the general pattern is

cpp --x--->priv_h
      |       |
      |       v
      x--> public_h

[maliberty]

Claude said the same

  public_hdrs (include/dst/*.h)   ← visibility //:__subpackages__
          ▲                ▲
          │                │
    private_hdrs ◀──────── dst (srcs *.cc)
     (src/*.h)
     visibility //src/dst:__subpackages__

[hzeller]

Don't think in termss of private headers. That brings you on the wrong path of organizing the project.
If you formulate things in terms of public headers and private implementation, this is already leaning towards a hard to maintain project structure.
Because then, you only create targets to make it compilable, but there is still no structure that is easy to understand.

Instead, think of building small units, each providing their thing and compose the structure from there.
These can be properly named and visibility opened where needed.
That allows proper layering from the ground up.
So make small libraries that each have a meaning (and typically: a unittest). Give it a good name ('private_hdrs' does not say what it is), and structure in as directed acyclic graph that makes the project structure easy to follow and fast to compile.

cc_library(
   name = "foo-lib",
   srcs = [
      "foo-impl.cc",
      "some-util.cc",
      "some-util.h",
   ],
   hdrs = [
     "foo-lib-something.h",
   ],
   # visibility as needed
)

# Put assoicated tests in the same directory right next to the library so that it is
# easy to navigate to.
cc_test(
   name = "foo-lib_test",
   srcs = ["foo-lib_test.cc"],
   deps = [
      ":foo-lib",
   ],
)

# Other, independent libraries that provide the functionality and might layer on top of libs here
cc_library(name = "bar", ...)
cc_test(name = "bar_test", ..., deps = [":bar"])

cc_library(name = "baz", ..., deps = [":foo-lib"])
cc_test(name = "baz_test", ..., deps = [":baz"])

cc_library(
   name = "some-bigger-library-that-we-want-people-to-use-and-uses-these-other-libs",
   srcs = [...],
   hdrs = [...],   # srcs and hdrs can use stuff from foo-lib
   deps = [
     ":foo-lib",
     ":bar",
     ":baz",
    ...
  ],
  visibility = ["//visibililty:public"],
)

It might take a bit to detangle the current project structure, but this is the only easy-to-maintain way going forward.
For an inspiration a look at e.g. verible or xls for projects structured this way.

[maliberty]

The pattern of putting nearly every .cc in its own cc_library feels excessively fine grained and a maintenance burden. I am Inherently skeptical of any statement that there is only one way to accomplish a goal. I'm open to discussions of how to achieve encapsulation but I don't think the granularity must be single files.

[hzeller]

It is typically between 1 and 5 files indeed. It makes maintenance much easier, as it is easy to find all the related things (such as tests and the corresponding code). The automatic dependency management can be done with bant.

[maliberty]

Can you add the bant setup now so I can get some experience with how it works and resolves this concern?

[hzeller]

Added in PR #10531

[hzeller]

SetupLegacyMtPolicy.hh includes Resizer.hh so it would need //src/rsz as dependency
Similar cyclic dependency issue.

[maliberty]

How are you detecting these short of looking at all the includes?

[hzeller]

There is no short, bant actually looks at all includes.
bant dwyu looks at the global project, figures out who offers what includes. Then, with that knowledge goes through each target and looks what includes they use (by grepping for includes essentially) and sees if all of them are either mentioned in the target itself or all the dependencies mentioned. If not, it issues buildozer add commands. Dependencies that are not needed result in buildozer remove commands.

[hzeller]

Looks like some of the files need to be more detangled. There are a bunch of more cyclic dependencies.

//src/foo:private_hdrs -> //src/foo --v
        ^----------------<------------|

[github-actions]

clang-tidy review says "All clean, LGTM! 👍"

[maliberty]

@hzeller I remain unclear on how I can generate a list of circular issues to resolve.

[hzeller]

This is something where using bant and bazel together provides the insights. Use bant to add all the dependencies needed, then bazel-compile will find if there are cyclic dependencies.

For this, I'd go one-by-one. After you have done your splits into the sub-libraries, run bant dwyu on that package path; it emits edits needed to add/remove the dependencies needed according to what the sources in each target use and provide.

$(etc/get-bant-path.sh) dwyu //src/drt/...

To better understand why these edits are needed, use the -vvv verbosity - then you see every header that is included in every source of every target and what possible providers for these libraries are.

The remove edits are typically just helping cleaning up dependencies not needed, so will not create cyclic dependencies. But all necessary add edits could result in cyclic dependencies.

(
To narrow what to focus on, you can use the grep function to just show lines that have add in it

$(etc/get-bant-path.sh) dwyu //src/drt/... -g add

)

Apply the edits (or directly source them using the shell <() feature):

. <($(etc/get-bant-path.sh) dwyu //src/drt/...)

... then build the project with bazel; that then will show when it sees cyclic dependencies.

ERROR: src/drt/BUILD:40:11: in cc_library rule //src/drt:db_hdrs: cycle in dependency graph:
    //src/ppl/test:multi_track_pattern2-tcl_test (2c1c7a4f619531fa5d3f8abef90892eb5fd4e31adde6f895a79861f28b0957fe)
    //:openroad (7a02e6547c93bf0bb42907629ef057a8e45a06b63a711aa74a22fb87320da0b7)
    //:openroad (f8001b1e78a2f1f8de3dbb2ff607dfda22a7a54cc176a0c2a2238eb8ef99ca0f)
    //:openroad_lib (f8001b1e78a2f1f8de3dbb2ff607dfda22a7a54cc176a0c2a2238eb8ef99ca0f)
    //src/drt:drt (f8001b1e78a2f1f8de3dbb2ff607dfda22a7a54cc176a0c2a2238eb8ef99ca0f)
.-> //src/drt:db_hdrs (f8001b1e78a2f1f8de3dbb2ff607dfda22a7a54cc176a0c2a2238eb8ef99ca0f)
|   //src/drt:drt_private_hdrs (f8001b1e78a2f1f8de3dbb2ff607dfda22a7a54cc176a0c2a2238eb8ef99ca0f)
`-- //src/drt:db_hdrs (f8001b1e78a2f1f8de3dbb2ff607dfda22a7a54cc176a0c2a2238eb8ef99ca0f)

So these indicate that further splitting is needed until the build-graph can be represented as a DAG.

After all the fixes, it will possible to run bant on the whole project:

. <($(etc/get-bant-path.sh) dwyu ...)

and the project compiles. Further applications of bant would then result in no edits. Whenever there are changes in the sources, the developer can just run bant dwyu on everything to fix the missing dependencies.
(and we can add it to the CI to automatically).

(If there are issues where bant does the wrong decision, let me know, but it should be rare. )