docs(bazel): propose consolidating onto a single Bazel module#445
docs(bazel): propose consolidating onto a single Bazel module#445balajinvda wants to merge 12 commits into
Conversation
The repository builds as 19 independent Bazel modules, one per service. That layout is inherited from the pre-monorepo history, when each service was its own repository, and it is the direct cause of several recurring failures rather than a style preference. Documents the evidence measured on the current tree: configuration copied 19 times and drifting, rule versions identical across every module so the isolation is unused, more than 20 unchosen Go dependency version conflicts so the isolation is actively harmful, and 18 registry fetches per full matrix to obtain 3 distinct base images. Proposes one module with services as packages, keeping release granularity per service, and deriving CI scheduling from the build graph instead of a hand-maintained subtree list. Includes phases, costs, the parallel-migration constraint on the cloud-tasks labels an external project builds against, and adjacent cleanups that share the same root cause. Explicit non-goal: splitting the Java tree into standalone modules. It is already in the target shape. Co-authored-by: Balaji Ganesan <bganesan@nvidia.com> Signed-off-by: Balaji Ganesan <bganesan@nvidia.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughExpands the proposal to consolidate first-party service Bazel modules into one root module, adds phased migration and CI controls, and introduces a tested inventory script for measuring Bazel, visibility, toolchain, and OCI configuration. ChangesBazel consolidation
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related issues
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
🌿 Preview your docs: https://nvidia-preview-docs-bazel-consolidation-plan.docs.buildwithfern.com/nvcf |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/dev/bazel-consolidation.md`:
- Around line 132-135: Update the fenced code block containing the nvct_core
Bazel targets to specify an explicit language, using text or another appropriate
language identifier in the opening fence, while leaving the block contents
unchanged.
- Around line 76-89: The CI migration plan in the documentation incorrectly
describes scheduling as static and proposes replacing functionality already
present in the workflow. Update the rationale and the Phase 5 section to
acknowledge the existing changed-file labeling, bazel query rdeps logic, and
full-build fallback, and focus the remaining work on consolidating the
multi-module layout while preserving current CI behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 48412079-3085-42c8-8a47-236d9a5c6126
📒 Files selected for processing (1)
docs/dev/bazel-consolidation.md
|
Thanks -- this is a better plan than what I wrote. I verified every factual claim against main The correction that matteredYou are right that CI already combines changed-file mapping, The revision distinguishes two layers rather than simply retracting, because I think the distinction is load-bearing:
So consolidation does not add Accepted as written
I adopted your phase order over mine. Visibility policy and OCI equivalence gates before any service moves is right, and I had both as afterthoughts. One thing I did not change: the |
Corrects the plan on the points raised in review, and adds tools/ci/bazel-consolidation-inventory so the quoted measurements can be regenerated instead of trusted. The material correction is the CI section. The first draft claimed CI scheduling is a hand-maintained approximation of the dependency graph and proposed replacing it with bazel query rdeps. That was wrong: the workflow already maps changed files to labels, queries reverse dependencies, and falls back to a full build for changes it does not model. The revision distinguishes the two layers, notes that only job selection across the matrix is static, and says the existing hybrid must be extended during every phase rather than replaced at the end. Other corrections: - Refreshed inventory. All 19 service subtrees now pin a Bazel version, 15 on 8.6.0 and 4 on 9.1.1; the previous draft said four were missing. Added a measurement stamp, since an in-flight change invalidated the first draft within hours. - Scoped the proposal: one Bazel module does not mean one Go module, one Cargo workspace, or one lockfile per language. Component go.mod and Cargo workspaces stay; the target is one root MODULE.bazel.lock. - Added label rebasing and visibility as the primary migration risk, with counts: 156 root-relative __subpackages__ declarations, 639 public-visibility occurrences, and zero package_group definitions. - Dropped the claim that dependency conflicts mostly resolve by taking the newer version. Kubernetes families carry conflicting replace directives and toolchains span major versions. - Rust moves into the root module while retaining separate Cargo workspaces and crate hubs; merging them is a later, separately justified optimization. - NVCA gets its own phase (1261 vendored BUILD files with //vendor labels that do not survive a root move), with ESS and BYOO as further exceptions. - Added OCI and stamping equivalence gates before any service moves, and noted the nested-module guard needs an allowlist for the intentional helper and vendored modules. - Qualified the fetch and compile savings: one repository definition and one digest pin, not one fetch across isolated runners. Co-authored-by: Balaji Ganesan <bganesan@nvidia.com> Signed-off-by: Balaji Ganesan <bganesan@nvidia.com>
|
Correction to my previous comment: the revision is PR now shows 2 files, +277: the revised |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tools/ci/bazel-consolidation-inventory`:
- Around line 16-71: The inventory tool lacks focused coverage for its parsing
and filtering behavior. Add fixture-based tests for the vendored-module
exclusion, trailing-comma oci.pull block parsing, and empty/no-match inputs,
covering the relevant commands and embedded Python logic in the inventory
entrypoint; alternatively document in the change why such tests are not
applicable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: e2f77516-6d74-4ca6-b4a1-0d82dc6ee7a3
📒 Files selected for processing (2)
docs/dev/bazel-consolidation.mdtools/ci/bazel-consolidation-inventory
Second review pass. Six findings, all confirmed against the tree before being acted on. The inventory tool had a real bug that made its output wrong rather than merely imprecise. It filtered vendored trees with a git ':(exclude)' pathspec combined with a wildcard positive pathspec, a combination git silently ignores. Every first-party count therefore included nvca's 1261 vendored BUILD files: the public-visibility count was 2721 instead of 639. A second defect used an unanchored '/rules/oci/' pattern that could not match the root-level copy, since that path has no leading slash. Both are now explicit anchored greps over git-tracked files, and both are covered by fixture tests that fail when either defect is reintroduced. The tool now also reads tracked files rather than the working tree, so its measurements correspond to the commit it stamps, warns when the tree is dirty, and computes every measurement into a variable before printing so a failed measurement aborts instead of yielding partial output with a success status. Go SDK provenance is now reported separately. The 1.23.0 pin the plan quoted comes only from a vendored module; first-party code spans 1.25.0 through 1.25.11. OCI pull reporting was scoped to nvcr.io and undercounted. There are 23 declarations across 8 distinct images and 7 digests, not 18 and 3. The one tag-only pull is now named in the output rather than asserted in prose. Doc corrections: - nvsnap is excluded by the non-goals but was counted in the 19 modules converging on the root. The end state is now stated as 18 publicly buildable modules plus nvsnap as an allowlisted exception, and Phase 1 and Phase 8 are scoped accordingly. - The claim that one module makes "which subtrees" a non-question was too broad. Consolidation retires the static module-root list, but affected targets still need routing to Docker-host, Java component, Cargo parity, BYOO generation, and release lanes. Root-scoped Java already routes this way on component_kind and ci_lane. - The phase invariant now requires every declared lane to pass, not only bazel build and test at the root. - Added per-component exit criteria, with root-local //src/... label resolution as the criterion that cannot be waived. - NVCA, ESS, and BYOO were named as special but never scheduled. They are now subphases 6a through 6c, each with an owner and the option to move to the exception allowlist rather than stay open. - 639 public-visibility declarations with zero package_group definitions means a very broad expressed API surface, not the absence of one. - Phase 7 now says to add and retain the Cargo parity lane. GitHub CI has no cargo test lane today. Co-authored-by: Balaji Ganesan <bganesan@nvidia.com> Signed-off-by: Balaji Ganesan <bganesan@nvidia.com>
|
All six addressed in 4 is the one that mattered, and it was worse than reportedYou flagged the inventory tool as needing "a small correctness pass." It had a bug that made its output wrong, not imprecise. Vendor filtering used a git So every first-party count silently included nvca's 1261 vendored BUILD files. The public-visibility count was 2721, not 639. The exclusion works with a literal positive pathspec, which is why it looked fine in isolation. A second defect: Both are now explicit anchored greps over Also fixed as you described: tracked files rather than the working tree, a dirty-tree warning on the stamp line, and every measurement computed into a variable before printing so a failed measurement aborts instead of producing partial output with exit 0. Tests live at Two measurements changed as a result:
The rest
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/dev/bazel-consolidation.md`:
- Around line 159-161: Update the Go-label correctness criterion in the
consolidation document to apply only to in-scope, non-exception components,
while explicitly preserving the allowlisted vendored and nested-module
exceptions such as nvsnap and the NVCA vendored tree. Define the exit evidence
required for those exceptions instead of requiring them to resolve to root-local
//src/... labels.
In `@tools/scripts/test/test-bazel-consolidation-inventory`:
- Around line 40-45: Update make_tempdir and both command-substitution call
sites so the created temporary directory is recorded in TEMP_DIRS in the parent
shell, not a subshell. Preserve the existing EXIT-trap cleanup behavior and
ensure callers still receive the directory path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 7588276e-644c-489f-9e9a-972d744f21ba
📒 Files selected for processing (3)
docs/dev/bazel-consolidation.mdtools/ci/bazel-consolidation-inventorytools/scripts/test/test-bazel-consolidation-inventory
🚧 Files skipped from review as they are similar to previous changes (1)
- tools/ci/bazel-consolidation-inventory
… contract Third review pass. The inventory tool stamped HEAD while reading file contents from the working tree, so an uncommitted edit to a tracked file changed the measurements without changing the stamp. Verified: touching one tracked BUILD.bazel moved the public-visibility count from 639 to 640 with the stamp unchanged. The tool now refuses to run on a dirty tree and offers --allow-dirty, which stamps the output as a working-tree measurement instead of a commit. The fixture suite leaked two temporary directories per run. make_tempdir was called through command substitution, so it ran in a subshell and its append to TEMP_DIRS never reached the parent shell's cleanup trap. It now assigns to a variable. Verified zero leaked directories after the fix. Separately, the tool only scanned paths under src/, so it never read the root BUILD.bazel. That is where the repository's first package_group appears, which means the plan's headline "zero package_group definitions" evidence was measured somewhere it could not observe a change. Now repository-wide, which also moves the public-visibility count from 639 to 644. Both new behaviors have fixture coverage: a modified tracked file must be rejected, and --allow-dirty must both stamp differently and actually observe the uncommitted occurrence. Doc corrections: - BYOO already builds through a declared Bazel genrule. Phase 6c said the generator must become one. The real problem is that the action uses host Go with local and no-sandbox, so it is neither hermetic nor remotely cacheable. - Added an exception contract. Retaining a nested module now requires an owner, the retained CI entry point, the first-party dependency-source strategy, residual correctness risk, and a dated revisit trigger. An entry missing any of these is unfinished work, not an exception. - Reconciled the exception list with the summary: moving NVCA, ESS, or BYOO to the allowlist lowers the count of converging modules and keeps that component's lockfile and module-root CI lane, so the summary is updated when it happens. - Scoped the root-local-label criterion to migrated components, without waiving it for vendored first-party code. NVCA has 80 BUILD files referring to vendored copies of NVCF Go libraries, which is the false-green condition this proposal exists to remove. Co-authored-by: Balaji Ganesan <bganesan@nvidia.com> Signed-off-by: Balaji Ganesan <bganesan@nvidia.com>
e8148d9 to
39aa7e5
Compare
|
All five blockers addressed in Tracking issues opened as requested: #448 (epic, carries the plan), #449 through #452 (phases 1 to 4), #453 through #455 (adjacent cleanup). All labeled 3 and 4 were both real, and 4 was mine to have caughtThe tempdir leak reproduces exactly as described: On the dirty-tree stamp, you are right that That fix exposed something worse that I should have found myself. The tool only scanned paths under New fixture coverage: a modified tracked file must be rejected, and 5, and the BYOO correctionYour BYOO correction is right and I verified it: On the exception contract, I adopted your five evidence fields as a named section, and reconciled it with the summary: moving a component to the allowlist lowers the count of converging modules and keeps that component's lockfile and module-root CI lane, so the summary is updated when it happens. The root-local-label criterion is now scoped to migrated components without being waived for vendored first-party code. I measured 80 NVCA BUILD files referring to vendored NVCF Go libraries rather than your 69, which is pattern-dependent, but the point stands unchanged: that is the false-green condition the proposal exists to remove, so a component can be excepted from moving but not from the correctness goal while claiming to be done. 1 and 2All four commits carry PR body rewritten against |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
docs/dev/bazel-consolidation.md (1)
159-175: 🎯 Functional Correctness | 🟠 MajorDo not make the root-label criterion impossible for exceptions.
This repeats the previous unresolved issue: the text scopes the criterion to migrated components, then says a component may remain unmigrated but cannot be excepted from the correctness goal. That makes retained modules such as
nvsnapunable to satisfy the definition of done. Qualify the criterion for in-scope, non-exception components and define exit evidence for exceptions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/dev/bazel-consolidation.md` around lines 159 - 175, Revise the root-local-label criterion and definition of done so it applies only to in-scope components that are required to migrate, explicitly excluding approved exceptions such as nvsnap. Add concrete exit evidence required for exceptions, such as documented approval, rationale, and tracking of remaining vendored references, while preserving the existing criteria for migrated components.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/dev/bazel-consolidation.md`:
- Around line 132-133: Update the prose sentence in the dependency-bump section
to hyphenate “repository-wide,” preserving the existing meaning and surrounding
wording.
- Around line 190-192: Reconcile the retained-module accounting throughout this
document: explicitly define whether the deliberate helper and vendored modules
join nvsnap as retained exceptions, then update every related summary count,
target end state, and migration criterion consistently across this section and
Phase 1.
---
Duplicate comments:
In `@docs/dev/bazel-consolidation.md`:
- Around line 159-175: Revise the root-local-label criterion and definition of
done so it applies only to in-scope components that are required to migrate,
explicitly excluding approved exceptions such as nvsnap. Add concrete exit
evidence required for exceptions, such as documented approval, rationale, and
tracking of remaining vendored references, while preserving the existing
criteria for migrated components.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: efb9ff90-662e-4ae9-a3df-ec714e660445
📒 Files selected for processing (3)
docs/dev/bazel-consolidation.mdtools/ci/bazel-consolidation-inventorytools/scripts/test/test-bazel-consolidation-inventory
🚧 Files skipped from review as they are similar to previous changes (2)
- tools/scripts/test/test-bazel-consolidation-inventory
- tools/ci/bazel-consolidation-inventory
…rrent evidence Fourth review pass. The three visibility measurements appended `|| true` to the whole pipeline, so a failure to read the files was converted into a silent zero. Confirmed by deleting a tracked BUILD.bazel: the tool reported 640 instead of failing, and exited 0. The first attempt at a fix was wrong and the negative control caught it. Relying on `set -e` to propagate a failed command substitution out of a shell function running as a pipeline stage does not work here; the tool still exited 0. It now checks the read status explicitly, and a single preflight verifies every tracked file the tool will read is present and readable. That covers every measurement rather than only the three that were reported. nvsnap was carried as a standing exception on the grounds that its base image is not publicly pullable. That rationale is stale. The workflow comment still says it, but the code disagrees: nvsnap pins public gcr.io/distroless/static, docker.io/library/ubuntu, and docker.io/library/alpine bases by digest, builds its images on @distroless_static, and its CONTRIBUTING documents a public-input source build. Private NGC is the publish destination, not the base. The plan no longer asserts the exception. All 19 first-party service modules are in scope, and Phase 1 decides nvsnap on current evidence rather than inheriting a comment. The retained-module accounting conflated three categories. The 22 tracked MODULE.bazel files are 19 first-party services, the root, one migration scaffolding module used by five nested modules and retired once they migrate, and one vendored third-party module that is a guard exclusion rather than an architectural decision. Only the first category can hold an exception. Also corrected: - ESS retains its Go workspace while the root Bazel graph ingests its modules. Collapsing it contradicted the scope boundary stated earlier in the document. - Remote caching does not reduce analysis-graph growth. Caching avoids re-executing actions; the analysis phase still processes whatever the invocation asks for, so target selection is the mitigation. Co-authored-by: Balaji Ganesan <bganesan@nvidia.com> Signed-off-by: Balaji Ganesan <bganesan@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/dev/bazel-consolidation.md`:
- Around line 227-235: Update the Phase 1 inventory statement in the opening
paragraph to say “Three of the 22” instead of “Two of the 22,” matching the 19
service modules plus root, migration-scaffolding, and vendored modules described
immediately afterward.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 2131c465-c10d-474b-b392-b8c27bcf3149
📒 Files selected for processing (3)
docs/dev/bazel-consolidation.mdtools/ci/bazel-consolidation-inventorytools/scripts/test/test-bazel-consolidation-inventory
🚧 Files skipped from review as they are similar to previous changes (2)
- tools/ci/bazel-consolidation-inventory
- tools/scripts/test/test-bazel-consolidation-inventory
|
All three blocking findings and all four follow-ups addressed in 1, and a wrong first fixConfirmed by deleting a tracked My first fix was wrong, and the negative control is what caught it. I replaced the blanket The tool now checks the read status explicitly rather than depending on 2, which was the most useful findingYou are right and the evidence is unambiguous. The stale claim originates in a This is the second time in this review cycle that a number or claim I carried forward turned out to be inherited rather than verified, which is the argument for the inventory tool existing at all. 3Adopted. The 22 tracked modules are now presented as four categories with distinct end states: 19 first-party services that converge, the root, Follow-ups
|
…anisms Fifth review pass. Two more false-green paths, both reproduced by injecting the failure and both previously printing a complete 23-line report and exiting 0: - `git status` failing was indistinguishable from a clean tree, because its output was consumed inside a test rather than captured and checked. - `git ls-files` failing in the preflight was discarded by process substitution, leaving an empty file list that every count read as a plausible zero. Rather than patch each call site, the tracked-file list is now enumerated once into a temporary file with its status checked, and every helper reads from that. This removes the whole class: no helper re-invokes git inside a pipeline where a failure becomes an empty result. Test coverage was also incomplete. The missing-file case only exercised the preflight, never count_occurrences, which is where the original masking lived. A tracked symlink pointing at a directory passes the readability check and then fails on read, which reaches count_occurrences directly. Both injected git failures are covered too, asserting not only a non-zero exit but that no measurements are printed. Document corrections: - Migration completion and exception completion contradicted each other. There are now two explicit closure paths: a migrated component satisfies the root module criteria, a retained service exception satisfies the exception contract, falls outside the migrated count, and records residual correctness risk. - The nested-module guard needs three mechanisms rather than one allowlist: exact vendored-path exclusions, a migration ledger that must shrink monotonically, and permanent service exceptions. Requiring a permanent exception contract for every entry from Phase 2 was incompatible with incremental migration, because it cannot distinguish "not yet" from "never". - Added a source-of-truth prerequisite phase. A subtree that is still upstream-authoritative in imports.yaml can have a later import restore its nested module and old labels, silently undoing the migration. stargate is in that position today. - Only the root row on a pull request narrows through rdeps; every per-service row builds its whole workspace. Corrected, and noted as an argument for consolidation rather than against it. - The nvsnap statement is qualified to its current Bazel targets. The legacy Dockerfile path still defaults to a private base, which is the likely origin of the original claim. - Three of the 22 modules are not first-party services, not two. Also repository-wide, a language on the code fence, and Rust toolchain versions rather than major versions. Co-authored-by: Balaji Ganesan <bganesan@nvidia.com> Signed-off-by: Balaji Ganesan <bganesan@nvidia.com>
|
All four blockers and all four small corrections addressed in 1, and why I stopped patching call sitesBoth reproduce exactly as you describe: injecting each failure gave a complete 23-line report and exit 0. I fixed them at the source rather than individually, because the pattern was going to keep recurring. Every helper was re-invoking You are also right that the missing-file test only exercised the preflight. To reach Verified after each fix: 2 and 3Both accepted, and 3 is a correction to my reasoning rather than my wording. I had collapsed "not yet migrated" and "never migrating" into one allowlist, which cannot express incremental migration: nothing would ever force the first category to resolve. The guard now has three mechanisms, with the migration ledger required to shrink monotonically so a phase that adds an entry or omits a target phase fails. For 2, the document now states two closure paths in a table. A retained exception is a legitimate outcome but not a quiet one: it lowers the migrated count, keeps a lockfile and module-root lane alive, and puts a stated correctness risk on the record. 4Good catch, and it is the kind of thing that would have silently undone the work. A subtree that stays upstream-authoritative can have a later import restore its nested module and old labels. Added as a prerequisite phase ahead of the Rust move, requiring either a native ownership cutover or upstream Bazel changes landed and synchronized. That renumbered the final phase, so remaining references now say "the final phase" rather than a number. Small correctionsAll four verified before applying. On the last two:
Also fixed: three of the 22 rather than two, repository-wide, a language on the code fence, and Rust toolchain versions. |
…sure models Sixth review pass. The tool could dirty the checkout it was about to inspect. mktemp honours TMPDIR, and the scratch file was created before the cleanliness check, so running with a TMPDIR inside the repository made a clean tree report as dirty and refuse to run. Reproduced with TMPDIR set to the repository root. The cleanliness check now runs first, and a fixture covers it: a clean tree with TMPDIR inside the repository must produce measurements, must not stamp itself as a working-tree measurement, and must leave no scratch file behind. Reverting the ordering fails that fixture. Two statements still contradicted the closure and guard models added last pass: - The exit criteria said a phase is not complete until every component it touched satisfies the migration criteria, which excluded the retained-exception path that the same document had just introduced. It now reads as the criteria for a migrated component, with phase closure requiring either those criteria or the exception contract. - Phase 1 still required an exception contract for every allowlist entry, which contradicts the three-mechanism guard and would make the guard unusable during the migration itself. Only permanent service exceptions carry a contract. Also corrected: - The exception contract applies to retained first-party service modules. The vendored module is a guard exclusion and carries no contract. - The private nvsnap default is in docker/agent/Dockerfile.app, not the criuv2 variant, which documents public inputs. I named the wrong file. - imports.yaml is not part of the public tree, so the source-of-truth prerequisite now refers to subtrees synchronized from an upstream repository. The stargate cutover requirement is unchanged. - Other CI rows build their full declared scope, which is a whole workspace only for nested modules; a root-scoped Java row builds its subtree of the root graph. - Rust toolchains span releases from 1.91.1 to 1.97.0, not major releases. - One cross-reference pointed below to a contract that is above it. Co-authored-by: Balaji Ganesan <bganesan@nvidia.com> Signed-off-by: Balaji Ganesan <bganesan@nvidia.com>
|
All addressed in 1Reproduced with The check now runs first, and the fixture asserts three things rather than just an exit code: measurements are produced, the stamp does not claim a working-tree measurement, and no scratch file is left in the repository. Reverting the ordering fails it: Worth noting my own first attempt at verifying this was invalid. I tested with a freshly created 2Both contradictions were mine, introduced by the fixes from the previous pass rather than surviving from before, which is the more annoying kind. The exit criteria still said a phase is not complete until every component it touched satisfies the migration criteria, immediately after the same document introduced a second closure path. It now reads as the criteria for a migrated component, with phase closure requiring either those criteria or the exception contract. Phase 1 still demanded a contract for every allowlist entry, which contradicts the three-mechanism guard and would make the guard unusable during the migration. Only permanent service exceptions carry a contract. And the contract now applies to retained first-party service modules, since the vendored module is a guard exclusion with nothing to justify. Factual correctionsAll verified before applying.
|
Seventh review pass. first_party, any, n_of, and the preflight selector all masked grep failures with a bare `|| true`, which tolerates status 2 and above as well as the status 1 that means "no match". Injecting a status-73 failure into the service-module selector produced a count of zero inside an otherwise complete, stamped report. That is the worst failure mode this tool can have. A tool whose entire purpose is to make quoted numbers reproducible must not answer confidently when its own selector is broken. All four now go through a helper that accepts 0 and 1 and propagates anything greater. The preflight selector additionally materializes its output with a checked status rather than reading it through process substitution, which discarded the status a second time. A final guard rejects any measurement that is not numeric before printing, so an empty value fails loudly instead of being formatted as a plausible answer. Regression test added, asserting both a non-zero exit and that no measurements are printed. Restoring the `|| true` fails it. Document corrections: - The ledger is a migration and retirement ledger. rules/oci-destinations is a retirement entry: nothing moves into the root graph, the module disappears once its five consumers no longer need it. Calling it a migration ledger left it unclassified. - Removed the claim that NVCA has 80 BUILD files referencing vendored NVCF libraries. The figure is definition-dependent, and the inventory tool does not generate it, so it cannot be reproduced by a reader. Plausible definitions give 80, 98, or 1211 depending on whether vendored BUILD files and non-NVCF vendored deps are counted. It now says dozens, and says why no number is quoted, which is consistent with the rest of the document requiring reproducible measurements. Co-authored-by: Balaji Ganesan <bganesan@nvidia.com> Signed-off-by: Balaji Ganesan <bganesan@nvidia.com>
|
Fixed in 1Confirmed. All four now go through a helper accepting 0 and 1 and propagating anything greater. The preflight additionally materializes its output with a checked status instead of reading through process substitution, which discarded the status a second time. I also added a final guard rejecting any measurement that is not numeric before printing, so an empty value fails loudly rather than being formatted into a plausible report. Regression test added, asserting non-zero exit and that no measurements print. Restoring the Worth being straight about the pattern here: this is the third pass finding the same DocumentationBoth correct. The ledger is now a migration and retirement ledger. On the NVCA figure, you are right that it is not reproducible, and checking it made that worse rather than better. Plausible definitions give 80, 98, or 1211 depending on whether vendored BUILD files themselves and non-NVCF vendored dependencies are counted. My 80 was one arbitrary choice among several, presented as fact, in a document whose central argument is that quoted numbers must be reproducible. It now says dozens and states why no number is quoted. PR bodyCorrected. The sentence about multiple attempts now sits with the pipeline-status defect it describes, rather than trailing the scratch-file entry. Two process notes, since both cost something: While reproducing your injection I ran My own first reproduction attempt also gave a misleading result: the fake |
…lently Eighth review pass. The .bazelversion aggregation used a raw grep, which returns status 1 when it matches nothing. Under pipefail that aborted the whole run with no output and no message. Zero subtree .bazelversion files is the intended end state, so the tool could not report the very outcome the plan exists to reach. Reproduced against a consolidated-shape repository: rc=1, empty stdout, empty stderr. The same zero-case was present on both go_sdk.download aggregations, which were not reported but would have failed identically in a repository with no Go SDK pins. Fixed together rather than waiting for it to be found separately. All three now use the status-tolerant helper, and an empty distribution is normalized to "none" rather than an empty field, so absence is stated rather than implied by whitespace. Fixture added for the fully consolidated shape: zero nested service modules, zero subtree versions, no Go SDK pins. It asserts a successful report and the normalized values. Reverting to the raw grep fails it. Document corrections: - rules/oci-destinations: "nothing moves into the root graph" was imprecise. What disappears is its standalone module boundary; its package becomes root-owned unless the macro is rehomed into the shared OCI API, which is the better outcome and belongs to the OCI and stamping phase rather than happening by default. - Root-scoped Java rows already share the root graph, so sharing the graph is not by itself what enables narrowing. Only the root row narrows today. - Phase 1 now says migration and retirement ledger, consistently with the guard. Co-authored-by: Balaji Ganesan <bganesan@nvidia.com> Signed-off-by: Balaji Ganesan <bganesan@nvidia.com>
|
Fixed in The blocker, which is the most interesting one so farReproduced exactly: rc=1, empty stdout, empty stderr, against a consolidated-shape repository. This one is worth naming precisely, because it is the inverse of every other defect in this review. The previous seven were the tool answering when it should have failed. This one is the tool failing when it should have answered, and failing in the single case the whole plan is aimed at: a repository with zero nested service modules and zero subtree Bazel versions. The tool could not report success. The cause is the same status-propagation family, from the other direction. I also fixed the same zero-case on both Empty distributions now normalize to Live output on that shape: I have added that condition to #448's definition of done, since it is now the machine-checkable form of "the migration is complete". CleanupsAll three correct. On On the CI rationale, you are right that sharing the root graph is not the distinguishing property, since root-scoped Java rows already share it. Only the root row narrows today. I had implied a causal link that is not there. Phase 1 now says migration and retirement ledger consistently. PR body says status-propagation defects, which is accurate for the whole family rather than for one mechanism within it. |
Ninth review pass. Normalizing an empty parse to "none" fixed the previous defect and introduced a worse one: files that exist but cannot be read as versions were reported as absence. A repository with 19 subtree .bazelversion files whose contents the parser did not understand produced "none (all subtrees use the root version)" in a complete, stamped report. That is the plan's success condition being announced on the strength of a parse failure. Source counts are now computed separately from parse results. "none" is reported only when the source count is zero. Otherwise the parsed count must equal the source count, and a mismatch fails with both numbers rather than reporting an answer. The same treatment covers both go_sdk.download aggregations, where a multiline declaration form previously yielded "none". Negative fixtures added for a malformed .bazelversion and a multiline go_sdk.download, each asserting a non-zero exit and that the value is not reported as absence. The consolidated fixture now contains a root .bazelversion, so the parenthetical "all subtrees use the root version" is actually supported by the fixture rather than asserted. Document correction: the CI section explained that root-scoped Java rows share the root graph and then contradicted it. The explanation is kept; the contradiction is replaced with the accurate point that extending narrowing to other rows is work consolidation enables rather than performs. Co-authored-by: Balaji Ganesan <bganesan@nvidia.com> Signed-off-by: Balaji Ganesan <bganesan@nvidia.com>
|
Fixed in The blockerYou are right, and this is the sharpest version of the problem yet: my previous fix for the zero-case introduced it. Normalizing an empty parse to Source counts are now computed separately from parse results. Both Live counts are unchanged: 15 on 8.6.0, 4 on 9.1.1, first-party Go 1.25.0 through 1.25.11. CorrectionsThe CI section did contradict itself in consecutive sentences. Kept the explanation, replaced the contradiction with the accurate point that extending narrowing to the other rows is work consolidation enables rather than performs. PR body now says selected count fields, which is what the guard actually validates. A suggestion about this reviewNine rounds in, every blocking finding has been in If it would speed things up, I am happy to split the script and its tests into a separate PR so the plan and the tracking issues can proceed on their own merits, and the tool can be hardened, rewritten in Python, or dropped entirely without holding up the architecture we already agree on. The document does not depend on the tool existing; it depends on its numbers being reproducible, which a follow-up can keep guaranteeing. Your call. If you would rather keep them together, I will keep going. |
…ect class Review found the same class of defect in the bash implementation across seven passes: a failed git call, a broken grep selector, an unreadable file, or a legitimate no-match would each become a plausible zero or a silent abort. Each fix was correct and each time the class reappeared somewhere else, because shell carries failure in exit statuses that are easy to discard and hard to distinguish from "found nothing". Fixing the instances was not converging, so this replaces the mechanism. The rewrite follows tools/ci/github-release, the existing Python tool in this directory, and uses only the standard library. What changes structurally: - Failure raises InventoryError. The report is built completely before anything is printed, so no path emits a partial or default-valued report. - git() checks the return code. An empty result from a failed call can no longer masquerade as a repository with no tracked files. - read() raises on an unreadable tracked file rather than contributing nothing to a count. - Absent and unparseable stay distinct by construction: zero sources reports "none", and a source that yields nothing raises. - No temporary files, so the tool cannot dirty the tree it is inspecting, and no external text tools, so their exit statuses cannot affect the result. Output is byte-identical to the bash version on this repository, verified by diffing both against the same commit, so the numbers quoted in the plan and mirrored in the tracking issue are unchanged. One behavior change: a multiline go_sdk.download is now parsed rather than rejected. The bash version failed on it, which was a capability gap rather than correct strictness. The unparseable case is now a genuinely unparseable one, a version that is not a literal. Testing: - tools/ci/test-bazel-consolidation-inventory.py: 16 unit tests over parsing and error semantics, following the test-github-release.py pattern. - tools/scripts/test/test-bazel-consolidation-inventory: the end-to-end fixture suite, updated. The grep-injection case tested a shell implementation detail that no longer exists; it is replaced by a test asserting the output is unchanged with failing grep, sed and awk on PATH. - Both wired into build-test.yml next to the existing Python tool tests. Every defect found during review was replayed against the rewrite: missing tracked file, dirty tree, broken git status, malformed .bazelversion, non-literal Go SDK version, TMPDIR inside the repository, the zero-subtree goal state, and hostile text tools on PATH. All fail closed or report correctly. Co-authored-by: Balaji Ganesan <bganesan@nvidia.com> Signed-off-by: Balaji Ganesan <bganesan@nvidia.com>
|
Rewritten in Python: Why a rewrite rather than a ninth fixYour last eight passes found the same class of defect in eight different places. Each fix was correct, and each time it reappeared somewhere I had not looked. That is not a series of unrelated mistakes; it is the mechanism. Shell carries failure in exit statuses that are easy to discard and hard to distinguish from "found nothing", so every selector, counter, and parser in a bash implementation is an independent opportunity to convert a failure into a plausible zero. Fixing instances was not converging. The rewrite follows What is now structural rather than defensive:
Output is unchangedByte-identical to the bash version on this repository, verified by diffing both against the same commit. The numbers in the plan and mirrored in #448 are untouched. One deliberate behavior change: a multiline Testing
Every defect from this review was replayed against the rewrite: I should have proposed this several passes ago. The signal was there by the third repeat and I kept treating each finding as a local bug. |
tools/AGENTS.md says to avoid new Python repo tooling and to prefer Go for "structured parsing, manifest scanning ... logic that benefits from unit tests and a compiled binary", noting that some CI environments here do not guarantee Python. That describes this tool exactly. The previous change chose Python from the tools/ci/github-release precedent without checking that guidance, so this follows it instead of carving out a second exception. Layout matches tools/docs-version-sync: its own module under tools/bazel-consolidation-inventory with _test.go coverage, and a thin wrapper at tools/ci/bazel-consolidation-inventory so the documented command is unchanged. Four parsing defects are fixed, all of which previously turned an input the tool could not interpret into an absent one: - Tracked symlinks were followed. A committed symlink can point outside the repository, where changing only the target alters the report while leaving git status clean, so the output no longer corresponds to the commit it stamps. Only regular files are measured now; symlinks and other non-regular files are rejected by name. - oci.pull was matched with a regular expression anchored on a closing parenthesis at column zero and on double-quoted attributes. Calls with an indented ")", single-quoted attributes, or a non-literal image all reported zero declarations. Calls are now found by balanced-parenthesis scanning that understands strings and comments, and every call must be parsed and classified or the run fails. - go_sdk.download extracted any version-like token in the call body, so a version bound to a variable with a version number in a trailing comment reported that number. The version is now read from the version attribute specifically, and must be a string literal. - .bazelversion accepted any line starting with a digit, so "9-not-a-version" was reported as a version. It must now be a release identifier. Output is byte-identical to the previous implementation on this repository, verified by diff, so the numbers in the plan and in the tracking issue are unchanged. Testing: - tools/bazel-consolidation-inventory/inventory_test.go covers the parser, the absent-versus-unparseable distinction, symlink and missing-file rejection, dirty-tree handling, and that a fully consolidated repository reports successfully. Each reported defect has a case. - The end-to-end fixture suite passes unchanged against the new implementation. - Both now run in a dedicated build-test job rather than inside the unrelated release-helper check, so they can be a required context. Document correction: the CI section explained that root-scoped Java rows share the root graph and then said only the root row has such a graph. It now says narrowing needs a graph spanning the changed files, that Java components already sit in one, and that the nested services have to move first. Co-authored-by: Balaji Ganesan <bganesan@nvidia.com> Signed-off-by: Balaji Ganesan <bganesan@nvidia.com>
|
Reimplemented in Go: On the language question first, because it changed the rest
Layout matches The three parsing blockersAll three were the same shape as everything before them: an input the tool could not interpret became an absent one. I dropped the regular expression for call finding.
Symlinks are rejected rather than followed, with the target named in the message. This was the sharpest of the three: a committed symlink pointing outside the repository means changing only the external target alters the report while
CorrectionsThe tests have a dedicated The CI section contradiction is fixed. It now says narrowing needs a graph spanning the changed files, that root-scoped Java components already sit in one, and that the nested services have to move first. PR body rewritten for the Go implementation. #448 re-synced. VerificationOutput is byte-identical to the previous implementation, verified by diff, so the numbers in the plan and the issue are unchanged. Every defect from this review has a test case, and the end-to-end fixture suite passed against the Go implementation without modification, which is a reasonable sign the behavior contract held across the rewrite. |
1 participant
TL;DR
Bazel arrived here service by service, so first-party code builds as 19 nested modules plus the root. That has a correctness cost: services consume in-repository libraries through published Go pseudoversions, so a library and its consumer can change together in one commit while Bazel still tests the consumer against the older published copy. Green build, broken code.
This adds the consolidation proposal and the tooling that keeps its numbers honest.
docs/dev/bazel-consolidation.md(proposal; mirrored in Consolidate first-party Bazel code onto a single root module #448)tools/bazel-consolidation-inventory/(Go tool that regenerates every quoted measurement, with unit tests)tools/ci/bazel-consolidation-inventory(wrapper, so the documented command is stable)tools/scripts/test/test-bazel-consolidation-inventory(end-to-end fixtures)Additional Details
See #448 for the full proposal, inventory, phases, and CI analysis.
The tool exists because prose counts go stale silently: the first draft was invalidated within hours by an unrelated change. The document carries a measurement stamp.
Merging this commits to the analysis, not to the migration.
For the Reviewer
Closest attention on
tools/bazel-consolidation-inventory/. The tool was previously bash, then Python, and is now Go pertools/AGENTS.md, which prefers Go for structured parsing that benefits from unit tests and notes that some CI environments here do not guarantee Python.The recurring defect was a tool that answered confidently when it could not compute the answer: a failed
gitcall, a broken selector, an unreadable file, a no-match, or a declaration the parser did not understand each became a plausible zero. Every measurement now returns an error on anything it cannot account for, and nothing is printed on failure.Specifically worth checking:
starlark.gofinds calls by balanced-parenthesis scanning rather than a regular expression, because a regexp missed indented closing parentheses and single-quoted attributes; symlinked tracked files are rejected rather than followed, since their target can change outside the repository; and absence is kept distinct from unparseability throughout.Output is byte-identical to the previous implementation, verified by diff, so the quoted numbers are unchanged.
For QA
Not applicable. No runtime or deployment behavior changes.
Both test suites run in the
bazel consolidation inventoryjob.Issues
Relates to #448
Checklist