[Nexthop] Extend buck_cmake_dep_checker to catch file-list inconsistencies.#1300
Open
benoit-nexthop wants to merge 1 commit into
Open
[Nexthop] Extend buck_cmake_dep_checker to catch file-list inconsistencies.#1300benoit-nexthop wants to merge 1 commit into
benoit-nexthop wants to merge 1 commit into
Conversation
1f12ea3 to
2a98433
Compare
Adds --check-files mode to detect .cpp files present in cmake targets but
missing from BUCK (and vice versa for pre-commit mode), catching the class
of mistake where a file is added to one build system but not the other.
New flags:
--check-files cmake → BUCK direction; files in cmake must
also appear in some BUCK target
--check-files --from-commit REF [--to-commit REF]
Bidirectional check for files changed in the
given commit range; any touched .cpp must be
in both build systems
Design choices to keep false-positive rate at zero:
- Same-name 1:1 target pairing with >0 shared .cpp as confidence gate
(excludes macro/glob-generated Buck targets invisible to the parser)
- Files under oss/ or facebook/ path segments silently skipped (OSS shims)
- Only paths starting with fboss/ considered (excludes bgp++ sibling clone)
- Global corpus check: a file is only flagged if absent from the *entire*
other build system, not just the paired target
Also fixes two pre-existing parser bugs:
- cpp_library/thrift_library blocks with comments between '(' and 'name ='
were silently skipped; fixed by extracting the block first then searching
for name= inside it (21 additional targets now parsed)
- bgp++ directory was polluting the Buck corpus with duplicate paths
Adds a commented-out pre-commit hook in .pre-commit-config.yaml for when
the existing violations are resolved.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
2a98433 to
c977254
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Extends
fboss/oss/scripts/buck_cmake_dep_checker.pywith two new check modes and several bug fixes.New flags
--check-filesand--check-sortedcan be combined; they share one parse pass:The pre-commit hook in
.pre-commit-config.yamlis updated to use this combined invocation.Design choices (file-list check)
fboss2_config_lib) while BUCK uses hyphens (fboss2-config-lib); both sides are normalized before matching so the confidence gate works correctlyheaderslists are included in the global corpus (some.cppfiles live underheadersin BUCK, notsrcs); previously these appeared as false-positive "missing from BUCK" entriesoss/orfacebook/path segments silently skipped (OSS shims)fboss/consideredAlso fixes a pre-existing parser bug
cpp_library/thrift_libraryblocks with comments between(andname =were silently skipped; fixed by extracting the block first then searching forname=inside it (21 additional targets now parsed)Test Plan
Tested against #1140 (
config/delete tunnel ip-in-ip). With--from-commit, output is scoped to the 6 build files touched by that commit:The
CmdConfigTunnelIpInIpTest.cpp(T) was appended afterCmdConfigVlanDefaultTest.cpp(V); alphabetically T < V so the test file should appear first.Also validated against #1095 where
--check-files --from-commitcorrectly flagged two integration test files added toBUCKbut missing from cmake: