Skip to content

Vendor narinfo-related logic#182

Merged
cole-h merged 1 commit intomainfrom
vendor-narinfo-logic
Apr 17, 2026
Merged

Vendor narinfo-related logic#182
cole-h merged 1 commit intomainfrom
vendor-narinfo-logic

Conversation

@cole-h
Copy link
Copy Markdown
Member

@cole-h cole-h commented Mar 26, 2026

So we don't have to depend on attic-server for "just" this.

Summary by CodeRabbit

  • Refactor

    • Restructured internal module organization for improved code maintainability and Nix cache manifest handling.
  • Chores

    • Updated dependencies to support enhanced serialization performance.

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Mar 26, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: db0fe62e-784f-49a5-9371-ecc898f48f28

📥 Commits

Reviewing files that changed from the base of the PR and between 3ab0168 and bd6e350.

⛔ Files ignored due to path filters (1)
  • Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (6)
  • magic-nix-cache/Cargo.toml
  • magic-nix-cache/src/gha.rs
  • magic-nix-cache/src/main.rs
  • magic-nix-cache/src/narinfo/mod.rs
  • magic-nix-cache/src/narinfo/nix_manifest/mod.rs
  • magic-nix-cache/src/narinfo/nix_manifest/serializer.rs
✅ Files skipped from review due to trivial changes (2)
  • magic-nix-cache/src/main.rs
  • magic-nix-cache/Cargo.toml
🚧 Files skipped from review as they are similar to previous changes (3)
  • magic-nix-cache/src/narinfo/nix_manifest/serializer.rs
  • magic-nix-cache/src/narinfo/nix_manifest/mod.rs
  • magic-nix-cache/src/narinfo/mod.rs

📝 Walkthrough

Walkthrough

This pull request extracts Nix binary cache manifest (.narinfo) handling from the attic-server dependency into a local module with custom Serde-based serialization support, removing the external dependency while adding internal serialization utilities.

Changes

Cohort / File(s) Summary
Dependency Updates
Cargo.toml
Removed attic-server git dependency; added serde_with, itoa, and ryu crates.
Module Declaration
src/main.rs
Added new narinfo module declaration.
NarInfo Structures
src/narinfo/mod.rs
Introduced NarInfo struct and Compression enum with Serde support, mapping Nix manifest key names to Rust struct fields with space-delimited references list handling.
Custom Serializer Foundation
src/narinfo/nix_manifest/mod.rs
Added public to_string() utility function, SpaceDelimitedList type alias, and custom Error enum for serialization support.
Serializer Implementation
src/narinfo/nix_manifest/serializer.rs
Implemented custom Serializer struct with Serde trait implementations to render values as compact key-value text format, using itoa for integers and ryu for floats.
Import Updates
src/gha.rs
Switched Compression and NarInfo imports from attic_server::narinfo to crate::narinfo; simplified error handling in upload_path.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~22 minutes

Poem

🐰 A cache of manifests, once borrowed, now grown—
From attic's far branches to gardens we've sown,
With serializers spinning and formats so tight,
The narinfo dances in Serde's delight! ✨
One module, self-sovereign, no imports to beg,
We hop forward proudly on our own sturdy leg!

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'Vendor narinfo-related logic' clearly and concisely summarizes the main change: moving narinfo parsing/serialization code into the project instead of depending on attic-server.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch vendor-narinfo-logic

Comment @coderabbitai help to get the list of available commands and usage tips.

@cole-h cole-h marked this pull request as ready for review April 17, 2026 14:25
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 6

🧹 Nitpick comments (1)
magic-nix-cache/src/narinfo/mod.rs (1)

182-186: Prefer implementing Display instead of ToString.

Implementing Display is more idiomatic in Rust and provides ToString automatically via blanket impl. This also enables use with format!, println!, etc.

♻️ Proposed refactor
-impl ToString for Compression {
-    fn to_string(&self) -> String {
-        String::from(self.as_str())
+impl std::fmt::Display for Compression {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        f.write_str(self.as_str())
     }
 }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@magic-nix-cache/src/narinfo/mod.rs` around lines 182 - 186, The impl ToString
for Compression should be replaced with an impl std::fmt::Display for
Compression so the type implements fmt::Display (enabling format!, println!,
etc.) and ToString is provided automatically; update the impl block for
Compression to implement fmt::Display by adding fn fmt(&self, f: &mut
std::fmt::Formatter) -> std::fmt::Result that writes self.as_str() to the
formatter using write!(f, "{}", self.as_str()) (remove the existing impl
ToString block).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@magic-nix-cache/src/narinfo/mod.rs`:
- Line 107: Fix the typo in the doc comment that currently reads "Other store
paths this object directly refereces." — change "refereces" to "references" in
the comment string in magic-nix-cache::narinfo::mod.rs (the doc comment line
"Other store paths this object directly refereces.").

In `@magic-nix-cache/src/narinfo/nix_manifest/deserializer.rs`:
- Around line 143-146: Remove the debug eprintln! in the closure that handles
missing colon: in deserializer.rs where you compute let colon =
line.find(':').ok_or_else(|| { eprintln!("Current input: {:?}", self.input);
Error::ExpectedColon })?; drop the eprintln!("Current input: {:?}", self.input)
and simplify the closure to just return Error::ExpectedColon (i.e.,
ok_or_else(|| Error::ExpectedColon)), leaving the rest of the logic unchanged;
references: colon, line.find, self.input, Error::ExpectedColon.
- Line 10: The deserializer (deserializer.rs) refers to Error enum variants that
don't exist, causing compilation failures; update the Error enum (Error) to
include the missing variants referenced by the deserializer: UnexpectedEof,
ExpectedInteger, ExpectedBoolean, ExpectedColon, FloatUnsupported, and
Unexpected, making sure each variant name matches exactly what deserializer.rs
uses (and keep existing variants Unsupported, NoneUnsupported,
NestedMapUnsupported, Custom); add appropriate variants (plain or with payloads
if deserializer expects data) so the code compiles and error handling remains
consistent with the deserializer's usage.

In `@magic-nix-cache/src/narinfo/nix_manifest/mod.rs`:
- Line 91: Fix the typo in the Display/format implementation for the Error
variant NestedMapUnsupported: change the message string used in the match arm
for Error::NestedMapUnsupported (currently "Nexted maps are unsupported.") to
the correct "Nested maps are unsupported." so the error text is spelled
correctly when formatting errors.
- Around line 37-53: The doc example references the old path
attic_server::nix_manifest which is stale; update the hidden use to the current
module path by replacing that import with crate::narinfo::nix_manifest::{self,
SpaceDelimitedList} (so the example imports SpaceDelimitedList and
nix_manifest::from_str from the correct location), or alternatively mark the
example as ignored if you don't want it run as a doctest; update the reference
near the example that mentions SpaceDelimitedList and from_str accordingly.
- Line 21: The crate exposes serialization via mod.rs but the deserialization
implementation (deserializer.rs) and its public function from_str are not
exposed, causing the documented example nix_manifest::from_str(s) to be
unreachable; either add a module declaration such as pub mod deserializer; in
mod.rs to publicly expose deserializer::from_str (and update pub re-exports if
you want it at the crate root), or remove/adjust the documentation example and
any public API surface that references from_str if deserialization should remain
internal; ensure references to deserializer, from_str, and mod.rs are updated
consistently.

---

Nitpick comments:
In `@magic-nix-cache/src/narinfo/mod.rs`:
- Around line 182-186: The impl ToString for Compression should be replaced with
an impl std::fmt::Display for Compression so the type implements fmt::Display
(enabling format!, println!, etc.) and ToString is provided automatically;
update the impl block for Compression to implement fmt::Display by adding fn
fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result that writes
self.as_str() to the formatter using write!(f, "{}", self.as_str()) (remove the
existing impl ToString block).
🪄 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: defaults

Review profile: CHILL

Plan: Pro

Run ID: 50ea3c5b-121e-451c-89ee-d9916c9ef47c

📥 Commits

Reviewing files that changed from the base of the PR and between 628eb14 and 3ab0168.

⛔ Files ignored due to path filters (1)
  • Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (7)
  • magic-nix-cache/Cargo.toml
  • magic-nix-cache/src/gha.rs
  • magic-nix-cache/src/main.rs
  • magic-nix-cache/src/narinfo/mod.rs
  • magic-nix-cache/src/narinfo/nix_manifest/deserializer.rs
  • magic-nix-cache/src/narinfo/nix_manifest/mod.rs
  • magic-nix-cache/src/narinfo/nix_manifest/serializer.rs

Comment thread magic-nix-cache/src/narinfo/mod.rs
Comment thread magic-nix-cache/src/narinfo/nix_manifest/deserializer.rs Outdated
Comment thread magic-nix-cache/src/narinfo/nix_manifest/deserializer.rs Outdated
Comment thread magic-nix-cache/src/narinfo/nix_manifest/mod.rs
Comment thread magic-nix-cache/src/narinfo/nix_manifest/mod.rs Outdated
Comment thread magic-nix-cache/src/narinfo/nix_manifest/mod.rs
So we don't have to depend on attic-server for "just" this.
@cole-h cole-h force-pushed the vendor-narinfo-logic branch from 3ab0168 to bd6e350 Compare April 17, 2026 14:51
@cole-h cole-h enabled auto-merge April 17, 2026 15:47
@cole-h cole-h merged commit 58ea4ed into main Apr 17, 2026
12 checks passed
@cole-h cole-h deleted the vendor-narinfo-logic branch April 17, 2026 15:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants