Skip to content

bazel: replace -layering_check overrides with scoped private_hdrs #10489

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
maliberty wants to merge 3 commits into
base: master
Choose a base branch
from
Draft

bazel: replace -layering_check overrides with scoped private_hdrs #10489

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
98b6213
bazel: replace -layering_check overrides with scoped private_hdrs
maliberty May 22, 2026
af79957
bazel: break dst :private_hdrs ↔ :dst cycle with leaf :dst_hdrs target
maliberty May 23, 2026
210ab6d
bazel: break gpl/rsz private_hdrs ↔ main-lib header cycle with leaf h…
maliberty May 23, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/cts/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ cc_library(
includes = [
"src",
],
visibility = ["//visibility:private"],
visibility = ["//src/cts:__subpackages__"],
deps = [
"//src/sta:opensta_lib",
"@boost.container_hash",
Expand Down
2 changes: 1 addition & 1 deletion src/cts/test/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -235,9 +235,9 @@ filegroup(
cc_test(
name = "cts_unittest",
srcs = ["cts_unittest.cc"],
features = ["-layering_check"], # TODO: accesses private headers
deps = [
"//src/cts",
"//src/cts:private_hdrs",
"//src/utl",
"@googletest//:gtest",
"@googletest//:gtest_main",
Expand Down
4 changes: 0 additions & 4 deletions src/dft/test/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,6 @@ filegroup(
cc_test(
name = "dft_scan_architect_unittest",
srcs = ["cpp/TestScanArchitect.cpp"],
features = ["-layering_check"], # TODO: includes private headers
deps = [
"//src/dft/src/architect",
"//src/dft/src/config",
Expand All @@ -90,7 +89,6 @@ cc_test(
cc_test(
name = "TestScanArchitect",
srcs = ["cpp/TestScanArchitect.cpp"],
features = ["-layering_check"], # TODO: includes private headers
deps = [
"//src/dft/src/architect",
"//src/dft/src/config",
Expand All @@ -106,7 +104,6 @@ cc_test(
"cpp/ScanCellMock.hh",
"cpp/TestScanArchitectHeuristic.cpp",
],
features = ["-layering_check"], # TODO: includes private headers
deps = [
"//src/dft/src/architect",
"//src/dft/src/cells",
Expand All @@ -127,7 +124,6 @@ cc_test(
"cpp/ScanCellMock.hh",
"cpp/TestScanArchitectHeuristic.cpp",
],
features = ["-layering_check"], # TODO: includes private headers
deps = [
"//src/dft/src/architect",
"//src/dft/src/cells",
Expand Down
2 changes: 1 addition & 1 deletion src/drt/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,7 @@ cc_library(
"src/ta/FlexTA.h",
],
includes = ["src"],
visibility = ["//visibility:private"],
visibility = ["//src/drt:__subpackages__"],
deps = [
":base_types",
":db_hdrs",
Expand Down
7 changes: 2 additions & 5 deletions src/drt/test/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,14 +162,11 @@ cc_test(
"fixture.h",
"gcTest.cpp",
],
features = ["-layering_check"], # TODO: acceses private headers
includes = [
"../src",
],
deps = [
"//src/drt", # build_cleaner: keep for private headers
"//src/drt",
"//src/drt:base_types",
"//src/drt:db_hdrs",
"//src/drt:drt_private_hdrs",
"//src/odb/src/db",
"//src/utl",
"@googletest//:gtest",
Expand Down
55 changes: 48 additions & 7 deletions src/dst/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,31 +10,72 @@ package(
features = ["layering_check"],
)

# Public API headers. No dependencies on anything inside src/dst/, so this
# target sits at the bottom of the dst dependency graph and both
# :private_hdrs and :dst can depend on it without forming a cycle.
cc_library(
name = "dst_hdrs",
hdrs = [
"include/dst/BalancerJobDescription.h",
"include/dst/BroadcastJobDescription.h",
"include/dst/Distributed.h",
"include/dst/JobCallBack.h",
"include/dst/JobMessage.h",
],
includes = ["include"],
deps = [
"@boost.asio",
"@boost.serialization",
],
)

cc_library(
name = "private_hdrs",
hdrs = [
"src/BalancerConnection.h",
"src/LoadBalancer.h",
"src/Worker.h",
"src/WorkerConnection.h",
Copy link
Copy Markdown
Collaborator

@hzeller hzeller May 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member Author

@maliberty maliberty May 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps the general pattern is

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

Copy link
Copy Markdown
Member Author

@maliberty maliberty May 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude said the same

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

Copy link
Copy Markdown
Collaborator

@hzeller hzeller May 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member Author

@maliberty maliberty May 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Collaborator

@hzeller hzeller May 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member Author

@maliberty maliberty May 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Collaborator

@hzeller hzeller May 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added in PR #10531

],
includes = ["src"],
visibility = ["//src/dst:__subpackages__"],
deps = [
":dst_hdrs",
"//src/utl",
"@abseil-cpp//absl/base:core_headers",
"@abseil-cpp//absl/synchronization",
"@boost.asio",
"@boost.smart_ptr",
] + select({
"@platforms//os:macos": ["@boost.thread//:thread_mac"],
"//conditions:default": ["@boost.thread//:thread_posix"],
}),
)

cc_library(
name = "dst",
srcs = [
"src/BalancerConnection.cc",
"src/BalancerConnection.h",
"src/Distributed.cc",
"src/JobMessage.cc",
"src/LoadBalancer.cc",
"src/LoadBalancer.h",
"src/Worker.cc",
"src/Worker.h",
"src/WorkerConnection.cc",
"src/WorkerConnection.h",
],
# The public hdrs are also listed in :dst_hdrs; they are duplicated here
# so that external consumers depending on //src/dst continue to satisfy
# layering_check without needing a separate //src/dst:dst_hdrs dep.
hdrs = [
"include/dst/BalancerJobDescription.h",
"include/dst/BroadcastJobDescription.h",
"include/dst/Distributed.h",
"include/dst/JobCallBack.h",
"include/dst/JobMessage.h",
],
includes = [
"include",
],
includes = ["include"],
deps = [
":dst_hdrs",
":private_hdrs",
"//src/odb/src/db",
"//src/utl",
"@abseil-cpp//absl/base:core_headers",
Expand Down
14 changes: 3 additions & 11 deletions src/dst/test/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,14 +23,11 @@ cc_test(
"cpp/TestBalancer.cc",
"cpp/stubs.cpp",
],
features = ["-layering_check"], # TODO: includes private headers
includes = [
"../src",
],
deps = [
":test_helpers",
"//:ord",
"//src/dst",
"//src/dst:private_hdrs",
"//src/utl",
"@boost.asio",
"@boost.bind",
Expand All @@ -52,13 +49,11 @@ cc_binary(
"cpp/TestDistributed.cc",
"cpp/stubs.cpp",
],
includes = [
"../src",
],
deps = [
":test_helpers",
"//:ord",
"//src/dst",
"//src/dst:private_hdrs",
"//src/utl",
"@boost.asio",
"@boost.bind",
Expand All @@ -76,14 +71,11 @@ cc_test(
"cpp/TestWorker.cc",
"cpp/stubs.cpp",
],
features = ["-layering_check"], # TODO: includes private headers
includes = [
"../src",
],
deps = [
":test_helpers",
"//:ord",
"//src/dst",
"//src/dst:private_hdrs",
"//src/utl",
"@boost.asio",
"@boost.bind",
Expand Down
14 changes: 13 additions & 1 deletion src/gpl/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,20 +14,31 @@ package(
features = ["layering_check"],
)

# Public API headers. Leaf target with no deps on anything inside src/gpl/,
# so :gpl_private_hdrs can depend on it without forming a cycle back through
# :gpl. The same hdrs are also listed in :gpl for layering_check.
cc_library(
name = "gpl_hdrs",
hdrs = ["include/gpl/Replace.h"],
includes = ["include"],
)

# Headers owned by :gpl but also consumed by :ui. Kept package-private so
# nothing outside //src/gpl can depend on them.
cc_library(
name = "gpl_private_hdrs",
hdrs = [
"src/fft.h",
"src/nesterovBase.h",
"src/nesterovPlace.h",
"src/placerBase.h",
"src/point.h",
"src/routeBase.h",
],
includes = ["src"],
visibility = ["//visibility:private"],
visibility = ["//src/gpl:__subpackages__"],
deps = [
":gpl_hdrs",
"//src/odb/src/db",
"//src/utl",
"@boost.unordered",
Expand Down Expand Up @@ -69,6 +80,7 @@ cc_library(
"include",
],
deps = [
":gpl_hdrs",
":gpl_private_hdrs",
"//src/dbSta",
"//src/dbSta:dbNetwork",
Expand Down
2 changes: 1 addition & 1 deletion src/gpl/test/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,10 +138,10 @@ PY_TESTS = [
cc_test(
name = "gpl_fft_unittest",
srcs = ["fft_test.cc"],
features = ["-layering_check"], # TODO: includes private headers
linkstatic = True, # TODO: remove once deps define all symbols
deps = [
"//src/gpl",
"//src/gpl:gpl_private_hdrs",
"@googletest//:gtest",
"@googletest//:gtest_main",
"@spdlog",
Expand Down
74 changes: 60 additions & 14 deletions src/rsz/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,64 @@ package(
features = ["layering_check"],
)

# Public API headers. Leaf target with no deps on anything inside src/rsz/,
# so :private_hdrs can depend on it without forming a cycle back through
# :rsz. The same hdrs are also listed in :rsz for layering_check.
cc_library(
name = "rsz_hdrs",
hdrs = [
"include/rsz/OdbCallBack.hh",
"include/rsz/Resizer.hh",
],
includes = ["include"],
deps = [
"//src/dbSta",
"//src/dbSta:SpefWriter",
"//src/dbSta:dbNetwork",
"//src/dpl",
"//src/est",
"//src/grt",
"//src/odb/src/db",
"//src/sta:opensta_lib",
"//src/stt",
"//src/utl",
],
)

cc_library(
name = "private_hdrs",
hdrs = [
"src/DelayEstimator.hh",
"src/MoveCommitter.hh",
"src/MoveTracker.hh",
"src/OptimizerTypes.hh",
"src/RepairSetupContext.hh",
"src/RepairTargetCollector.hh",
"src/move/MoveCandidate.hh",
"src/move/MoveGenerator.hh",
"src/move/VtSwapMtCandidate.hh",
"src/policy/OptimizationPolicy.hh",
"src/policy/SetupLegacyBase.hh",
"src/policy/SetupLegacyMtPolicy.hh",
"src/policy/SetupLegacyPolicy.hh",
"src/policy/SetupMt1Policy.hh",
Copy link
Copy Markdown
Collaborator

@hzeller hzeller May 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member Author

@maliberty maliberty May 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Collaborator

@hzeller hzeller May 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

],
includes = [
"src",
"src/move",
"src/policy",
],
visibility = ["//src/rsz:__subpackages__"],
deps = [
":rsz_hdrs",
"//src/dbSta",
"//src/dbSta:dbNetwork",
"//src/odb/src/db",
"//src/sta:opensta_lib",
"//src/utl",
],
)

cc_library(
name = "rsz",
srcs = [
Expand All @@ -20,18 +78,14 @@ cc_library(
"src/ConcreteSwapArithModules.cc",
"src/ConcreteSwapArithModules.hh",
"src/DelayEstimator.cc",
"src/DelayEstimator.hh",
"src/DelayEstimatorReporter.cc",
"src/DelayEstimatorReporter.hh",
"src/MoveCommitter.cc",
"src/MoveCommitter.hh",
"src/MoveTracker.cc",
"src/MoveTracker.hh",
"src/OdbCallBack.cc",
"src/Optimizer.cc",
"src/Optimizer.hh",
"src/OptimizerTypes.cc",
"src/OptimizerTypes.hh",
"src/PreChecks.cc",
"src/PreChecks.hh",
"src/Rebuffer.cc",
Expand All @@ -42,9 +96,7 @@ cc_library(
"src/RepairDesign.hh",
"src/RepairHold.cc",
"src/RepairHold.hh",
"src/RepairSetupContext.hh",
"src/RepairTargetCollector.cc",
"src/RepairTargetCollector.hh",
"src/Resizer.cc",
"src/ResizerObserver.hh",
"src/SwapArithModules.hh",
Expand All @@ -61,9 +113,7 @@ cc_library(
"src/move/MeasuredVtSwapGenerator.cc",
"src/move/MeasuredVtSwapGenerator.hh",
"src/move/MoveCandidate.cc",
"src/move/MoveCandidate.hh",
"src/move/MoveGenerator.cc",
"src/move/MoveGenerator.hh",
"src/move/RerouteCandidate.cc",
"src/move/RerouteCandidate.hh",
"src/move/RerouteGenerator.cc",
Expand Down Expand Up @@ -101,27 +151,21 @@ cc_library(
"src/move/VtSwapGenerator.cc",
"src/move/VtSwapGenerator.hh",
"src/move/VtSwapMtCandidate.cc",
"src/move/VtSwapMtCandidate.hh",
"src/move/VtSwapMtGenerator.cc",
"src/move/VtSwapMtGenerator.hh",
"src/policy/MeasuredVtSwapPolicy.cc",
"src/policy/MeasuredVtSwapPolicy.hh",
"src/policy/OptimizationPolicy.cc",
"src/policy/OptimizationPolicy.hh",
"src/policy/SetupCritVtSwapPolicy.cc",
"src/policy/SetupCritVtSwapPolicy.hh",
"src/policy/SetupDirectionalPolicy.cc",
"src/policy/SetupDirectionalPolicy.hh",
"src/policy/SetupLastGaspPolicy.cc",
"src/policy/SetupLastGaspPolicy.hh",
"src/policy/SetupLegacyBase.cc",
"src/policy/SetupLegacyBase.hh",
"src/policy/SetupLegacyMtPolicy.cc",
"src/policy/SetupLegacyMtPolicy.hh",
"src/policy/SetupLegacyPolicy.cc",
"src/policy/SetupLegacyPolicy.hh",
"src/policy/SetupMt1Policy.cc",
"src/policy/SetupMt1Policy.hh",
"src/policy/SetupReroutePolicy.cc",
"src/policy/SetupReroutePolicy.hh",
"src/policy/SetupTnsPolicy.cc",
Expand All @@ -141,6 +185,8 @@ cc_library(
],
linkopts = ["-pthread"],
deps = [
":private_hdrs",
":rsz_hdrs",
"//src/dbSta",
"//src/dbSta:SpefWriter",
"//src/dbSta:dbNetwork",
Expand Down
Loading
Loading