Consolidate cmake error targets to match BUCK fboss-error

benoit-nexthop · benoit-nexthop · commit df09fd7f7be5 · 2026-01-24T01:26:02.000+01:00
The BUCK build has a single `fboss-error` target containing: - `FbossError.h` - `FbossHwUpdateError.h` - `SysError.h` The cmake build previously had two separate targets: - `error` (only `FbossError.h`) - `fboss_error` (`FbossError.h` and `SysError.h`) This caused divergence between the two build systems. Changes made: 1. Remove the `error` target from `cmake/Agent.cmake` 2. Update `fboss_error` to include all three headers to match BUCK: - `FbossError.h` - `FbossHwUpdateError.h` (was missing) - `SysError.h` 3. Update `fboss_error` dependencies to match BUCK exactly: - `fboss_cpp2` (matches `//fboss/agent/if:fboss-cpp2-types`) - `Folly::folly` (matches `//folly:conv` and `//folly/logging:logging`) - Remove `fboss_types` (not in BUCK dependencies) - Plot twist: add`switch_config_cpp2` dependency to `logging_util` target. This was previously provided transitively via `fboss_error` -> `fboss_types` -> `switch_config_cpp2`, but removing `fboss_types` from `fboss_error` broke that chain. The `logging_util` target directly includes `switch_config_types.h` so it needs to declare this dependency explicitly. 4. Replace all 23 instances of `error` dependency with `fboss_error` across 16 cmake files 5. Add missing `fboss_error` dependency to `external_phy` target (required in the BUCK file but was missing in cmake) Additionally, two more fixes had to be fixed to address all the remaining issue with `system_scale_test_utils` found by the script, and those were resolved once again by matching BUCK files: - Remove bundled source files (`PortFlapHelper.cpp`, `MacLearningFloodHelper.cpp`) - Add dependencies on existing `port_flap_helper` and `mac_learning_flood_helper` targets (defined in `AgentTestUtils.cmake`) Oh and a few more errors that sneaked into the code more recently and that I missed in facebook#833 because I was working off a fork a few days old: 1. In `cmake/AgentState.cmake`, the `state` library was missing a dependency on `agent_features` (from Buck dep: `//fboss/agent:agent_features`) 2. In `cmake/fsdb/FsdbIf.cmake`, the `thriftpath_lib` library was missing a dependency on `fsdb_utils` (from Buck dep: `//fboss/fsdb/common:utils`) 3. In `cmake/fsdb/FsdbTests.cmake`, the `thriftpath_test_cpp2` library was missing a dependency on `common_cpp2` (from Buck dep: `//fboss/agent/if:common`) Bonus: add duplicate cmake target detection to `buck_cmake_dep_checker.py`, because while fixing that last error I inadvertently made the mistake of creating a duplicate rule in a cmake file that was defined in another. Now the script doesn't report any errors anymore and could potentially be used in CI to ensure no further differences between the BUCK build and the cmake build are introduced.
diff --git a/cmake/Agent.cmake b/cmake/Agent.cmake
@@ -148,7 +148,7 @@ add_library(utils
 target_link_libraries(utils
   agent_dir_util
   asic_utils
-  error
+  fboss_error
   ctrl_cpp2
   hw_switch_fb303_stats
   load_agent_config
@@ -260,7 +260,7 @@ add_library(hw_switch_thrift_client_table
 )
 
 target_link_libraries(hw_switch_thrift_client_table
-  error
+  fboss_error
   fboss_types
   hw_ctrl_cpp2
   Folly::folly
@@ -432,15 +432,6 @@ set(core_libs
 
 target_link_libraries(core ${core_libs})
 
-add_library(error
-  fboss/agent/FbossError.h
-)
-
-target_link_libraries(error
-  ctrl_cpp2
-  Folly::folly
-)
-
 add_library(thrifthandler_utils
   fboss/agent/ThriftHandlerUtils.cpp
 )
@@ -491,12 +482,12 @@ target_link_libraries(fboss_event_base
 
 add_library(fboss_error
   fboss/agent/FbossError.h
+  fboss/agent/FbossHwUpdateError.h
   fboss/agent/SysError.h
 )
 
 target_link_libraries(fboss_error
   fboss_cpp2
-  fboss_types
   Folly::folly
 )
 
@@ -514,7 +505,7 @@ target_link_libraries(platform_base
   agent_config_cpp2
   agent_dir_util
   ctrl_cpp2
-  error
+  fboss_error
   fboss_event_base
   fboss_types
   Folly::folly
diff --git a/cmake/AgentHw.cmake b/cmake/AgentHw.cmake
@@ -84,7 +84,7 @@ target_link_libraries(hw_switch_fb303_stats
 
 target_link_libraries(hw_fb303_stats
   counter_utils
-  error
+  fboss_error
   fb303::fb303
   Folly::folly
   switch_config_cpp2
diff --git a/cmake/AgentHwSaiApi.cmake b/cmake/AgentHwSaiApi.cmake
@@ -18,6 +18,7 @@ add_library(logging_util
 target_link_libraries(logging_util
   fboss_cpp2
   fboss_error
+  switch_config_cpp2
   Folly::folly
 )
 
diff --git a/cmake/AgentHwSaiDiag.cmake b/cmake/AgentHwSaiDiag.cmake
@@ -11,7 +11,7 @@ add_library(diag_shell
 target_link_libraries(diag_shell
   sai_repl
   python_repl
-  error
+  fboss_error
   sai_switch
   sai_tracer
   Folly::folly
@@ -32,7 +32,7 @@ add_library(python_repl
 target_include_directories(python_repl PRIVATE Python3::Python)
 
 target_link_libraries(python_repl
-  error
+  fboss_error
   Folly::folly
   Python3::Python
   # Python3::Python requires forkpty and openpty provided by libutil.
@@ -46,7 +46,7 @@ add_library(sai_repl
 )
 
 target_link_libraries(sai_repl
-  error
+  fboss_error
   sai_api
   Folly::folly
 )
diff --git a/cmake/AgentHwSwitchAsics.cmake b/cmake/AgentHwSwitchAsics.cmake
@@ -32,7 +32,7 @@ add_library(switch_asics
 
 target_link_libraries(switch_asics
   agent_features
-  error
+  fboss_error
   fboss_cpp2
   fboss_types
   phy_cpp2
diff --git a/cmake/AgentPacket.cmake b/cmake/AgentPacket.cmake
@@ -44,7 +44,7 @@ add_library(pktutil
 )
 
 target_link_libraries(pktutil
-  error
+  fboss_error
   Folly::folly
 )
 
diff --git a/cmake/AgentPlatformsCommon.cmake b/cmake/AgentPlatformsCommon.cmake
@@ -11,7 +11,7 @@ add_library(platform_mapping
 
 target_link_libraries(platform_mapping
   agent_features
-  error
+  fboss_error
   fboss_config_utils
   platform_config_cpp2
   state
@@ -23,7 +23,7 @@ add_library(platform_mapping_utils
 )
 
 target_link_libraries(platform_mapping_utils
-  error
+  fboss_error
   minipack_platform_mapping
   elbert_platform_mapping
   yamp_platform_mapping
diff --git a/cmake/AgentPlatformsCommonUtils.cmake b/cmake/AgentPlatformsCommonUtils.cmake
@@ -13,7 +13,7 @@ add_library(wedge_led_utils
 )
 
 target_link_libraries(wedge_led_utils
-  error
+  fboss_error
   ctrl_cpp2
   fboss_types
   transceiver_cpp2
diff --git a/cmake/AgentRib.cmake b/cmake/AgentRib.cmake
@@ -13,7 +13,7 @@ add_library(standalone_rib
 target_link_libraries(standalone_rib
   network_to_route_map
   address_utils
-  error
+  fboss_error
   fboss_event_base
   fboss_types
   switch_config_cpp2
diff --git a/cmake/AgentState.cmake b/cmake/AgentState.cmake
@@ -118,7 +118,8 @@ add_library(state
 
 target_link_libraries(state
   address_utils
-  error
+  agent_features
+  fboss_error
   platform_config_cpp2
   switch_config_cpp2
   switch_state_cpp2
@@ -143,7 +144,7 @@ add_library(state_utils
 )
 
 target_link_libraries(state_utils
-  error
+  fboss_error
   fboss_types
   hwswitch_matcher
   state
@@ -156,7 +157,7 @@ add_library(label_forwarding_action
 )
 
 target_link_libraries(label_forwarding_action
-  error
+  fboss_error
   fboss_cpp2
   Folly::folly
 )
diff --git a/cmake/AgentTest.cmake b/cmake/AgentTest.cmake
@@ -243,8 +243,6 @@ target_link_libraries(linkstate_toggler
 
 add_library(system_scale_test_utils
   fboss/agent/test/utils/SystemScaleTestUtils.cpp
-  fboss/agent/test/utils/PortFlapHelper.cpp
-  fboss/agent/test/utils/MacLearningFloodHelper.cpp
 )
 
 target_link_libraries(system_scale_test_utils
@@ -261,6 +259,8 @@ target_link_libraries(system_scale_test_utils
   route_scale_gen
   qos_test_utils
   trap_packet_utils
+  port_flap_helper
+  mac_learning_flood_helper
   Folly::folly
   Folly::follybenchmark
 )
diff --git a/cmake/Led.cmake b/cmake/Led.cmake
@@ -9,6 +9,6 @@ target_link_libraries(ledIO
   Folly::folly
   led_mapping_cpp2
   led_structs_types_cpp2
-  error
+  fboss_error
   led_utils
 )
diff --git a/cmake/LedService.cmake b/cmake/LedService.cmake
@@ -39,7 +39,7 @@ add_library(led_config
 )
 
 target_link_libraries(led_config
-  error
+  fboss_error
   led_config_cpp2
   Folly::folly
   FBThrift::thriftcpp2
diff --git a/cmake/LedServiceHwTest.cmake b/cmake/LedServiceHwTest.cmake
@@ -12,7 +12,7 @@ target_link_libraries(led_service_hw_test
   ${GTEST}
   ${LIBGMOCK_LIBRARIES}
   fboss_common_init
-  error
+  fboss_error
   fboss_types
   led_manager_lib
   common_file_utils
diff --git a/cmake/LibPhy.cmake b/cmake/LibPhy.cmake
@@ -11,6 +11,7 @@ add_library(external_phy
 target_link_libraries(external_phy
   alert_logger
   ctrl_cpp2
+  fboss_error
   fboss_types
   mdio
   platform_config_cpp2
@@ -77,7 +78,7 @@ add_library(phy_management_base
 
 target_link_libraries(phy_management_base
   external_phy
-  error
+  fboss_error
   platform_mapping
   fboss_config_utils
   Folly::folly
diff --git a/cmake/QsfpService.cmake b/cmake/QsfpService.cmake
@@ -48,7 +48,7 @@ add_library(qsfp_config
 )
 
 target_link_libraries(qsfp_config
-  error
+  fboss_error
   qsfp_config_cpp2
   Folly::folly
   FBThrift::thriftcpp2
diff --git a/cmake/QsfpServiceTestHwTest.cmake b/cmake/QsfpServiceTestHwTest.cmake
@@ -24,7 +24,7 @@ add_library(hw_transceiver_utils
 
 target_link_libraries(hw_transceiver_utils
   Folly::folly
-  error
+  fboss_error
   platform_mapping
   switch_config_cpp2
   transceiver_cpp2
diff --git a/cmake/fsdb/FsdbClientTest.cmake b/cmake/fsdb/FsdbClientTest.cmake
@@ -9,7 +9,7 @@ target_link_libraries(fsdb_client_test
   fsdb_pub_sub
   fsdb_model
   fsdb_test_clients
-  error
+  fboss_error
   ${GTEST}
   ${LIBGMOCK_LIBRARIES}
 )
diff --git a/cmake/fsdb/FsdbIf.cmake b/cmake/fsdb/FsdbIf.cmake
@@ -69,6 +69,7 @@ add_library(thriftpath_lib
 )
 
 target_link_libraries(thriftpath_lib
+  fsdb_utils
   switch_config_cpp2
   fsdb_oper_cpp2
   FBThrift::thriftcpp2
diff --git a/cmake/fsdb/FsdbTests.cmake b/cmake/fsdb/FsdbTests.cmake
@@ -9,6 +9,8 @@ add_fbthrift_cpp_library(
   OPTIONS
     json
     reflection
+  DEPENDS
+    common_cpp2
 )
 
 add_library(fsdb_test_server
diff --git a/fboss/oss/scripts/buck_cmake_dep_checker.py b/fboss/oss/scripts/buck_cmake_dep_checker.py
@@ -206,6 +206,20 @@ def __init__(self, repo_root: str):
         self.repo_root = Path(repo_root)
         self.targets: dict[str, CmakeTarget] = {}  # name -> target
         self.variables: dict[str, list[str]] = {}  # cmake variable -> values
+        self.duplicate_errors: list[str] = []  # List of duplicate target errors
+
+    def _add_target(self, name: str, target: CmakeTarget):
+        """Add a target, reporting error if it already exists"""
+        if name in self.targets:
+            existing = self.targets[name]
+            error = (
+                f"ERROR: Duplicate cmake target '{name}' defined in:\n"
+                f"  First:  {existing.file_path}\n"
+                f"  Second: {target.file_path}"
+            )
+            self.duplicate_errors.append(error)
+            print(error)
+        self.targets[name] = target
 
     def parse_all(self) -> dict[str, CmakeTarget]:
         """Parse all cmake files in cmake/ directory and CMakeLists.txt"""
@@ -292,7 +306,7 @@ def _parse_add_library(self, content: str, file_path: str):
             target = CmakeTarget(
                 name=name, target_type="library", srcs=srcs, file_path=file_path
             )
-            self.targets[name] = target
+            self._add_target(name, target)
 
     def _parse_add_executable(self, content: str, file_path: str):
         """Parse add_executable calls"""
@@ -305,7 +319,7 @@ def _parse_add_executable(self, content: str, file_path: str):
             target = CmakeTarget(
                 name=name, target_type="executable", srcs=srcs, file_path=file_path
             )
-            self.targets[name] = target
+            self._add_target(name, target)
 
     def _parse_add_fbthrift_cpp_library(self, content: str, file_path: str):
         """Parse add_fbthrift_cpp_library calls"""
@@ -329,7 +343,7 @@ def _parse_add_fbthrift_cpp_library(self, content: str, file_path: str):
                 thrift_deps=deps,
                 file_path=file_path,
             )
-            self.targets[name] = target
+            self._add_target(name, target)
 
     def _extract_thrift_depends(self, block: str) -> list[str]:
         """Extract DEPENDS from a thrift cmake block"""
@@ -757,8 +771,9 @@ def main():
 
     print_report(missing, analyzer.matches, args.verbose)
 
-    # Return non-zero if there are missing dependencies
-    return 1 if missing else 0
+    # Return non-zero if there are missing dependencies or duplicate targets
+    has_duplicates = len(analyzer.cmake_parser.duplicate_errors) > 0
+    return 1 if missing or has_duplicates else 0
 
 
 if __name__ == "__main__":

Original file line number	Diff line number	Diff line change
`@@ -18,6 +18,7 @@ add_library(logging_util`
`18`	`18`	`target_link_libraries(logging_util`
`19`	`19`	`fboss_cpp2`
`20`	`20`	`fboss_error`
	`21`	`+ switch_config_cpp2`
`21`	`22`	`Folly::folly`
`22`	`23`	`)`
`23`	`24`
Original file line number	Diff line number	Diff line change
`@@ -44,7 +44,7 @@ add_library(pktutil`
`44`	`44`	`)`
`45`	`45`
`46`	`46`	`target_link_libraries(pktutil`
`47`		`- error`
	`47`	`+ fboss_error`
`48`	`48`	`Folly::folly`
`49`	`49`	`)`
`50`	`50`
Original file line number	Diff line number	Diff line change
`@@ -13,7 +13,7 @@ add_library(wedge_led_utils`
`13`	`13`	`)`
`14`	`14`
`15`	`15`	`target_link_libraries(wedge_led_utils`
`16`		`- error`
	`16`	`+ fboss_error`
`17`	`17`	`ctrl_cpp2`
`18`	`18`	`fboss_types`
`19`	`19`	`transceiver_cpp2`