Skip to content

Library split, README rewrite, BUILD correctness and supply-chain gates - #5

Merged
codeitlikemiley merged 6 commits into
mainfrom
claude/bazel-lib-split-and-cleanup
Aug 2, 2026
Merged

Library split, README rewrite, BUILD correctness and supply-chain gates#5
codeitlikemiley merged 6 commits into
mainfrom
claude/bazel-lib-split-and-cleanup

Conversation

@codeitlikemiley

Copy link
Copy Markdown
Owner

Stacked on #3 — base is claude/bazel-single-workspace, so the diff here is only this work. Merge #3 first, then this retargets to main.

Three commits, reviewable one at a time.

commit what
dad1279 give server a library, so both build systems run the same tests
ad18b6a rewrite the README so it describes the repo that exists
3ecf5bf BUILD-file correctness and supply-chain gates

dad1279 — the library split

server was binary-only, with two consequences.

tests/ could not test anything. An integration test can only import a library, and there was none — which is why tests/just_test.rs held nothing but assert!(true). Replaced with six tests that drive the real Router through tower::ServiceExt::oneshot: real requests, real status codes, real response bodies. One of them GETs /shared/ada and asserts the serialised corex::User — an end-to-end proof of the workspace collapse.

The repo's best assertions were never run by cargo. Both fibonacci correctness tests lived in the bench file, and harness = false means cargo compiles #[test] functions and then hands main() to criterion — so only bazel test //server:bench_test ever executed them. The algorithms and their tests moved to src/lib.rs; the bench now use server::.... bench_test is deleted, since it existed only to reach those assertions.

before after
cargo 3 unit tests 7 unit + 6 integration + 2 doctests
bazel 4 targets, 2 meaningful 4 targets, all meaningful

Also adds a /fib/{n} endpoint that returns 400 above MAX_FIB_INPUT = 93 instead of overflowing, with a test proving the boundary via checked_add.

ad18b6a — the README

701 lines, and a large fraction fiction. Patching individual falsehoods would leave a document still mostly wrong, so it is rewritten against bazel query. What was actually wrong:

  • Four BUILD snippets were not parseable Starlark. Commit 2c55b45 — titled "fix: remove invalid square brackets from Bazel list comprehensions" — broke four correct comprehensions and added a note asserting Starlark doesn't use square brackets. It does.
  • rust_benchmark does not exist, loaded and called in four places.
  • Eleven edition = "2021" in a repo whose every manifest says 2024.
  • Six of nine "Quick Commands" named nonexistent targets, and the 138-line cargo-runner section referenced //corex:unit_tests, //corex:example_client, //combos/shared:unit_tests and others — none real, one in a directory that doesn't exist.
  • The Axum example used axum::Server::bind and /users/:id (both removed in axum 0.8) and a corex::Calculator that has never existed.
  • Line 44's repin command named a repo that didn't exist and a step that isn't needed.

Now 254 lines. Verified: every //label across README, guide and CONTRIBUTING resolves against bazel query //..., and all four Starlark blocks parse.

3ecf5bf — BUILD correctness + supply chain

  • Package identity. Every Bazel artifact reported CARGO_PKG_NAME = "server_bin" at version 0.0.0 while cargo said server 0.1.0. Now set explicitly on every source-compiling target.
  • Dropped deps from rust_test(crate = …). rules_rust unions them, so those lists were additive — attaching criterion and ~30 crates to targets that never mention it.
  • srcs are globs. Not one target used one, so the first mod helpers; would compile under cargo and fail under Bazel with E0583. The likeliest remaining one-line divergence, now closed.
  • Deleted build.rs and its orphaned cargo_build_script: the script was one println! that emitted no cargo: directive, had zero reverse deps in Bazel, and re-ran for all five cargo targets on every touch because it declared no rerun-if-changed.
  • deny.toml, dependabot, and a cargo-deny CI job.

A nice moment: the --incompatible_disallow_empty_glob guard added in the toolchain PR caught my own bug here — my first proxy srcs glob excluded every file in server/src, and Bazel refused to load the package instead of silently building an empty list.

cargo-deny was run, not assumed. It rejected three things in my first draft: BSD-2-Clause and Zlib are never actually encountered (they appear only inside OR expressions that MIT satisfies), the syn skip was unnecessary (cargo's graph has one version even though Bazel's has two), and wildcards = "deny" flagged corex = { path = "../corex" }, which needs allow-wildcard-paths. Final run: advisories ok, bans ok, licenses ok, sources ok.

Deliberately not included: the hermetic C toolchain

The gold linker is deprecated warning comes from Bazel autodetecting the runner's host cc. The real fix is a pinned toolchain (toolchains_llvm), and I left it out for three reasons worth your call rather than my assumption:

  1. I cannot verify it here. The warning does not even reproduce in this sandbox — Bazel's autodetected toolchain picks a different linker — so I would be shipping unverified config into every contributor's build.
  2. It costs every CI run. toolchains_llvm downloads a ~1 GB LLVM tarball. That is a real price to pay for a warning.
  3. It changes the compiler for everyone, not just CI. That is an architectural decision, not cleanup.

The config is four lines if you want it:

bazel_dep(name = "toolchains_llvm", version = "1.4.0")
llvm = use_extension("@toolchains_llvm//toolchain/extensions:llvm.bzl", "llvm")
llvm.toolchain(llvm_version = "19.1.0")
use_repo(llvm, "llvm_toolchain")
register_toolchains("@llvm_toolchain//:all")

Say the word and I'll open it as its own PR and drive it green.

Verification

cargo fmt --all --check                        ok
cargo clippy --workspace --all-targets -D warnings   ok
cargo test --workspace --all-targets           8 suites ok
cargo deny check                               advisories ok, bans ok, licenses ok, sources ok
bazel test  --config=ci //...                  4/4 pass
bazel build --config=ci --config=lint //...    ok

Generated by Claude Code

claude added 3 commits August 2, 2026 02:48
server was binary-only. Two consequences, both now fixed.

tests/ could not test anything. An integration test file can only import a
library, and there was none, which is why tests/just_test.rs contained nothing
but assert!(true) and was deleted rather than repaired. server/tests/integration.rs
replaces it with six tests that drive the real Router through
tower::ServiceExt::oneshot -- actual requests, actual status codes, actual
response bodies -- including one that GETs /shared/ada and asserts the
serialised corex::User, an end-to-end proof of the workspace collapse.

The repo's most substantive assertions were never run by cargo. Both fibonacci
correctness tests lived in benches/fibonacci_benchmark.rs, and `harness = false`
means cargo compiles the #[test] functions and then hands main() to criterion,
so only `bazel test //server:bench_test` ever executed them. The algorithms and
their tests now live in src/lib.rs; the bench `use server::...` instead of
carrying its own copies. bench_test is deleted -- it existed only to reach
those assertions.

Layout
  server/src/lib.rs   the router, handlers, and the four fibonacci
                      implementations, all pub, with 7 unit tests
  server/src/main.rs  seven lines: bind a socket, serve server::app()
  server/tests/       six integration tests over the real router

Also
  - A /fib/{n} endpoint that rejects n > 93 with 400 rather than overflowing.
    MAX_FIB_INPUT = 93 is now a named constant with a test proving the boundary
    (fib(92) + fib(93) has no checked_add), instead of a magic number in three
    benchmark sweeps.
  - rust_library uses glob(["src/**/*.rs"]) rather than a hardcoded single file,
    so the first `mod foo;` does not break Bazel while cargo stays happy.
  - integration_tests takes all_crate_deps(normal = True, normal_dev = True).
    normal_dev alone is exclusive, not additive: it would hand the suite
    criterion and tower and nothing else, and the first `use axum::...` would
    fail under Bazel while passing under cargo.
  - tower is a new dev-dependency, used only to drive the Router in tests. axum
    already pulls it into the graph.

Test inventory, before -> after
  cargo   : 3 unit                 -> 7 unit + 6 integration + 2 doctests
  bazel   : 4 targets, 2 real ones -> 4 targets, all real

Verified: cargo fmt/clippy -D warnings/test/doc green; bazel test --config=ci
green with //server:server_lib_test and
//server:integration_tests_tests/integration_test both passing.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01D3MwkT9fzAzZweK4birHYz
The README was 701 lines and a large fraction of it was fiction. Patching the
individual falsehoods would have left a document that was still mostly wrong, so
it is rewritten against the actual target list from `bazel query`.

What was wrong

  Four BUILD snippets were not parseable Starlark. Commit 2c55b45, titled
  "fix: remove invalid square brackets from Bazel list comprehensions", took
  four correct list comprehensions and broke them, then added a note at line 180
  asserting "In Bazel, list comprehensions at the top level don't use square
  brackets". That is false; Starlark comprehensions are written exactly like
  Python's. All four blocks in the new README parse under ast.parse.

  rust_benchmark does not exist. Loaded and called at four places. There is no
  benchmark rule in rules_rust -- a criterion bench is a rust_binary, which is
  what this repo has always actually done.

  Eleven `edition = "2021"` in a repo whose every manifest says 2024.

  Six of nine "Quick Commands" named targets that do not exist, and the
  138-line cargo-runner section referenced //corex:unit_tests,
  //corex:test_integration_test, //corex:example_client, //corex:bench_performance,
  //server:example_demo and //combos/shared:unit_tests -- not one of which is a
  real target, in a package (combos/shared) that is not a real directory.

  The "Working Axum Server" example used axum::Server::bind and /users/:id, both
  axum 0.6 API removed in 0.8, and a corex::Calculator type that has never
  existed. The 60-line library example documented that same imaginary type.

  Line 44 told readers to run
  `CARGO_BAZEL_REPIN=1 bazel sync --only=crates --enable_workspace`,
  naming a repo (`crates`) that did not exist at the time and a step that is not
  needed at all: crate.from_cargo sets no `lockfile` attribute, so crate_universe
  re-resolves on its own.

  The directory tree omitted benches/, examples/, tests/ and src/bin/, and
  predated the workspace collapse.

What replaced it

  A layout that matches `git ls-files`, the complete target table taken from
  `bazel query`, real commands, and the two invariants that actually keep this
  repo honest: the toolchain pinned at the same version in MODULE.bazel and
  rust-toolchain.toml, and first-party deps declared in both Cargo.toml and
  BUILD.bazel. The BUILD patterns section documents the traps this repo has
  already hit -- glob("tests/*.rs") not glob("tests/**"), normal_dev being
  exclusive rather than additive, `deps` on a `crate = ...` test being additive,
  and hardcoded single-file srcs breaking on the first `mod`.

Verified: every //label mentioned across README.md, BAZEL_RUST_GUIDE.md and
CONTRIBUTING.md resolves against `bazel query //...`, except four regex
false-positives (a //localhost URL, two repo-qualified @rules_rust labels, and
one clearly-marked placeholder). All four python blocks parse.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01D3MwkT9fzAzZweK4birHYz
BUILD files

  Package identity was wrong on every Bazel-built artifact. rules_rust bakes
  CARGO_PKG_NAME = the TARGET name and defaults CARGO_PKG_VERSION to "0.0.0"
  (rust.bzl: version = attr.string(default = "0.0.0")), so binaries reported
  themselves as "server_bin" at version 0.0.0 while cargo said "server" 0.1.0 --
  a difference that only shows up in telemetry, a /version handler or a
  User-Agent. Every source-compiling target now sets `version` and
  `rustc_env = {"CARGO_PKG_NAME": ...}`, including the tests, which compute
  their env from their own attrs.

  Dropped `deps` from every rust_test that sets `crate`. rules_rust unions the
  two (rust.bzl: depset(deps, transitive = [crate.deps])), so those lists were
  additive, not overriding -- they attached criterion and ~30 transitive crates
  to targets whose sources never mention it. corex's was pure noise: corex has
  no [dev-dependencies] at all.

  srcs are globs now, not hardcoded single files. Not one target in the repo
  used a glob, so the first `mod helpers;` anyone wrote would compile under
  cargo and fail under Bazel with E0583. This is the likeliest one-line
  divergence left in the repo, and it is now closed.

  Added proc_macro_deps = all_crate_deps(proc_macro = True) everywhere. It
  resolves to nothing today -- serde_derive arrives through serde's feature --
  but it is the attribute that stops a future proc-macro dependency from
  landing in `deps`, where it silently does not work.

  axum_example took dev-deps only; cargo gives examples normal + dev. Fixed.

  Deleted server/build.rs and its cargo_build_script. The whole script was
  `println!("this is rust build.rs")` -- not a cargo: directive, so it emitted
  nothing -- and the Bazel target had zero reverse dependencies, so it never
  ran there anyway. Meanwhile cargo re-ran it for all five targets on every
  touch because it declared no rerun-if-changed. Deleting it removes a
  build+run action per consumer and one divergence.

  The empty-glob guard added with the toolchain pin earned its place here: my
  first version of proxy's srcs excluded every file in server/src and Bazel
  refused to load the package rather than silently building an empty srcs list.

Supply chain

  deny.toml, with an allow-list taken from `cargo metadata` over the real graph
  rather than guessed. Verified by actually running cargo-deny, which rejected
  three things in my first draft: BSD-2-Clause and Zlib are never encountered
  (they only appear inside OR expressions that MIT satisfies), the `syn` skip
  was unnecessary because cargo's graph has one version even though Bazel's has
  two, and `wildcards = "deny"` flagged `corex = { path = "../corex" }` -- an
  intra-workspace path dep, which needs allow-wildcard-paths. Final run:
  advisories ok, bans ok, licenses ok, sources ok.

  publish = false on [workspace.package]. None of these crates goes to
  crates.io, and none declares a license, which cargo-deny would otherwise
  report as unlicensed.

  dependabot.yml for cargo, github-actions and bazel-modules, plus a
  cargo-deny CI job.

Verified: cargo fmt/clippy -D warnings/test all green; cargo deny check exits 0;
bazel test --config=ci 4/4; bazel build --config=ci --config=lint green.

Not included: the hermetic C toolchain. See the PR description -- it cannot be
verified in this environment and imposes a large per-CI cost, so it belongs in
its own change rather than bundled here.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01D3MwkT9fzAzZweK4birHYz
@codeitlikemiley
codeitlikemiley changed the base branch from claude/bazel-single-workspace to main August 2, 2026 03:16
@codeitlikemiley
codeitlikemiley marked this pull request as ready for review August 2, 2026 03:16
claude and others added 3 commits August 2, 2026 03:17
…t-and-cleanup

# Conflicts:
#	.github/workflows/ci.yml
Regenerated by CI from MODULE.bazel. Pins the resolved module graph and the registry file hashes that bcr.bazel.build served; --config=locked verifies it.
The bazel job on the previous run failed with

  MODULE.bazel.lock is no longer up-to-date because: One or more files the
  extension '...crate_universe:extensions.bzl%crate' is using have changed

which is correct: this PR adds a tower dev-dependency and publish = false, both
of which change crate_universe's inputs, so the lock inherited from main really
was stale. The lockfile job in that same run regenerated and committed it.

Documented rather than engineered away. Folding the regeneration into the bazel
job would make --lockfile_mode=error a no-op, since it would always be checking
a lock it had just written.

This commit also serves as the push that validates the regenerated lock.
@codeitlikemiley
codeitlikemiley merged commit 0d998ba into main Aug 2, 2026
4 checks passed
codeitlikemiley added a commit that referenced this pull request Aug 2, 2026
…ig bugs

Archives docs/bazel-review-2026-08.md, the round-2 review that drove #1-#7,
behind a prominent ARCHIVED header -- the body is written in the present tense
about a repo that no longer exists in that state, so unmarked it would be the
same class of confidently-wrong document the review itself catalogued. The
header maps each finding area to the commit that resolved it.

Two silent bugs surfaced while doing it, both the same shape: config quietly
deciding what exists.

.gitignore:1 `bazel-*` was unanchored, matching any path segment starting with
"bazel-" at any depth rather than the root bazel-bin/bazel-out symlinks it was
written for. The new doc was unaddable; so would docs/bazel-guide.md be.
Anchored to /bazel-*, verified in both directions.

.github/dependabot.yml declared package-ecosystem "bazel-modules", which does
not exist. Dependabot rejects the entire file on one invalid value, so cargo and
github-actions updates were dead too -- the repo has had no dependency
automation since #5 while appearing to have three ecosystems configured. Fixed
to "bazel"; the .github/dependabot.yml check passes for the first time.

Also documents an ecosystem deliberately left off: rust-toolchain is valid and
would bump rust-toolchain.toml alone, resplitting cargo and Bazel onto different
compilers -- the original defect. Both halves of that pin move together, by hand.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants