Skip to content

improve nix cache action & add binaries to cache#3020

Merged
insipx merged 1 commit intomainfrom
push-pznknkuympxl
Jan 29, 2026
Merged

improve nix cache action & add binaries to cache#3020
insipx merged 1 commit intomainfrom
push-pznknkuympxl

Conversation

@insipx
Copy link
Copy Markdown
Contributor

@insipx insipx commented Jan 15, 2026

this adds the xdbg and xmtp_cli binaries to https://app.cachix.org/cache/xmtp#pins. building the binaries off main (with nix build .#xdbg) will download the already-built artifact rather than rebuild

adds a description to every crate

still need to verify they work (and fixes the broken fh cache on main that's been there for a while) but my nix flake update is broken b/c of github outage

@insipx insipx requested a review from a team as a code owner January 15, 2026 17:32
@claude
Copy link
Copy Markdown

claude Bot commented Jan 15, 2026

Claude finished @insipx's task —— View job


PR Review - January 28, 2026 at 23:47 UTC

Review Checklist:

  • Read and analyze workflow changes
  • Review Nix flake configuration
  • Check Cargo.toml changes
  • Provide comprehensive feedback

Summary

This PR improves the Nix caching infrastructure and adds CLI binaries (xdbg, xmtp_cli) to Cachix. The changes are generally well-implemented, with previous review comments addressed. I've identified a few areas requiring attention:

Critical Issues

1. Package name mismatch in rust.nix (nix/rust.nix:69)

The flake exposes packages.default = self'.packages.xdbg; but the crate is named xmtp_debug (not xdbg). The binary name is xdbg but the package name needs to match the crate name.

Impact: nix build will fail to find the package.

Fix: Either use packages.xmtp_debug or ensure the package is aliased correctly.

2. Docker image reference mismatch (flake.nix:72)

The entrypoint references self'.packages.musl-mls_validation_service but the crate is named mls_validation_service (with underscore). Need to verify this package exists.

Impact: Docker image build may fail or reference wrong package.

Quality & Best Practices

3. Inconsistent workflow patterns (.github/workflows/check-ios-android-bindings.yml)

  • iOS check job uses nix-community/cache-nix-action@v6 without Cachix
  • Android check job uses both cachix/cachix-action@v16 AND nix-community/cache-nix-action@v6

Recommendation: Standardize caching strategy across jobs. Either use Cachix for both (recommended for CI consistency) or document why they differ.

4. Cargo caching may be redundant (all workflows)

You're caching ~/.cargo/ and target/ directories with cache-nix-action, but these are ephemeral in CI and Nix manages its own build cache. This may not provide value and could increase cache storage costs.

Recommendation: Measure cache hit rates and consider removing if not beneficial.

Minor Issues

5. Missing description consistency (Cargo.toml files)

While most crates now have descriptions, the root Cargo.toml workspace doesn't have a description field. This is fine but worth noting for completeness.

6. Line 24 in workflows: nix build --version

This is actually correct (runs nix with the build --version subcommand to verify Nix installation). Previous review comment was addressed correctly - this is not an issue.

Security & Performance

  • ✅ No security concerns identified
  • ✅ Proper use of secrets (CACHIX_AUTH_TOKEN)
  • ✅ Appropriate caching strategy for Nix builds
  • ✅ Docker image properly references musl-built binary (good for size/portability)

Test Coverage

  • ✅ No new test coverage needed (infrastructure changes)
  • ✅ Existing CI tests will validate the changes
  • ⚠️ Recommend manually testing nix build .#xdbg and nix build .#xmtp_cli before merge

Recommendations

  1. Fix package name mismatch in rust.nix:69 (critical)
  2. Verify Docker entrypoint package reference (critical)
  3. Standardize caching strategy across workflows (quality)
  4. Test binary builds manually: nix build .#xdbg, nix build .#xmtp_cli, nix build .#musl-mls_validation_service (validation)

@macroscopeapp
Copy link
Copy Markdown
Contributor

macroscopeapp Bot commented Jan 15, 2026

Replace CI Nix installer with nixbuild/nix-quick-install-action@v34 and cache Cargo builds and Nix binaries across iOS, Android, WebAssembly, and Omnix workflows

CI switches to nixbuild/nix-quick-install-action@v34, adds Cargo/Nix caching, introduces a Nix-built Docker image for mls_validation_service, and refactors flake/rust modules to expose a musl binary and broaden filesets.

📍Where to Start

Start with the workflow changes in .github/workflows/fh-cache.yml, then review flake refactors in flake.nix and Rust wiring in nix/rust.nix.


Macroscope summarized ced673b.

@codecov
Copy link
Copy Markdown

codecov Bot commented Jan 15, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 73.91%. Comparing base (fb6adfb) to head (ced673b).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3020      +/-   ##
==========================================
- Coverage   74.01%   73.91%   -0.10%     
==========================================
  Files         447      447              
  Lines       54990    54990              
==========================================
- Hits        40701    40648      -53     
- Misses      14289    14342      +53     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@insipx insipx force-pushed the push-pznknkuympxl branch from 2d6e0d8 to 797a63f Compare January 20, 2026 17:57
@insipx insipx requested a review from a team as a code owner January 20, 2026 17:57
@insipx insipx force-pushed the push-pznknkuympxl branch from 797a63f to aff7cbe Compare January 20, 2026 17:58
Comment thread .github/workflows/fh-cache.yml Outdated
Comment thread .github/workflows/fh-cache.yml Outdated
@insipx insipx force-pushed the push-pznknkuympxl branch 5 times, most recently from a34166d to 832fa55 Compare January 20, 2026 19:06
Comment thread .github/workflows/fh-cache.yml Outdated
@insipx insipx force-pushed the push-pznknkuympxl branch 7 times, most recently from b4f655c to 875b45e Compare January 23, 2026 21:54
@insipx insipx force-pushed the push-pznknkuympxl branch 3 times, most recently from c9978e5 to 92ead07 Compare January 28, 2026 19:14
Comment thread .github/workflows/check-ios-android-bindings.yml
Comment thread .github/workflows/check-ios-android-bindings.yml
@insipx insipx force-pushed the push-pznknkuympxl branch 2 times, most recently from b79007f to ae7ade3 Compare January 28, 2026 21:34
Comment thread .github/workflows/fh-cache.yml
Comment thread .github/workflows/test-webassembly.yml Outdated
@insipx insipx force-pushed the push-pznknkuympxl branch from ae7ade3 to e4c3340 Compare January 28, 2026 21:44
Comment thread .github/workflows/test-webassembly.yml
@insipx insipx force-pushed the push-pznknkuympxl branch from e4c3340 to 1027701 Compare January 28, 2026 23:45
@insipx insipx force-pushed the push-pznknkuympxl branch from 1027701 to ced673b Compare January 28, 2026 23:45
Comment thread Cargo.toml
# Bindings (excluding wasm - only for wasm32 target)
"bindings/mobile",
"bindings/node",
"bindings/[!wasm]*",
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Medium

The pattern [!wasm]* is a character class negation, meaning "not any single character in {w,a,s,m}". Since mobile starts with m, it won't match. Consider using a different approach to exclude only wasm, such as explicitly listing bindings/mobile and bindings/node.

Suggested change
"bindings/[!wasm]*",
"bindings/mobile",
"bindings/node",

🚀 Want me to fix this? Reply ex: "fix it for me".

@insipx insipx enabled auto-merge (squash) January 29, 2026 00:00
@insipx insipx merged commit 299b443 into main Jan 29, 2026
25 checks passed
@insipx insipx deleted the push-pznknkuympxl branch January 29, 2026 00:04
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