Add nix nario command
#215
Conversation
This replaces `nix-store --export` and `nix-store --import`.
The old format put the NAR before the metadata, which made it hard to start adding the path to the store in a streaming way. The new format stores the metadata first, so we can use the regular streaming `addToStore()` API.
WalkthroughAdds a versioned nario export/import format (v1, v2), new nario CLI commands (export/import/list) and docs/tests, extends exportPaths API with a version parameter, enforces required CLI flags via Args::checkArgs, and minor error output formatting change. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant CLI as nix CLI
participant Store
participant EI as Export/Import
rect rgb(245,248,255)
note over User,CLI: nario export (versioned)
User->>CLI: nix nario export --format V PATHS
CLI->>EI: exportPaths(store, paths, sink, V)
alt V == 1
EI->>EI: write exportMagicV1 header (legacy format)
loop per path
EI->>Store: queryPathInfo
EI->>EI: serialize meta + NAR
end
EI->>EI: write terminator (0)
else V == 2
EI->>EI: write exportMagicV2 header
loop per path
EI->>Store: queryPathInfo
EI->>EI: serialize WorkerProto path info
EI->>EI: dump NAR and verify hash
end
else unsupported
EI-->>CLI: error
end
CLI-->>User: stream to stdout
end
sequenceDiagram
autonumber
actor User
participant CLI as nix CLI
participant EI as Export/Import
participant Store
rect rgb(245,255,245)
note over User,CLI: nario import / list
User->>CLI: nix nario import < dump or nix nario list < dump
CLI->>EI: importPaths(targetStore, source)
EI->>EI: read version header
alt version == 0 (empty)
EI-->>CLI: no-op
else version == 1
loop until terminator
EI->>EI: parse path info + NAR
EI->>Store: addToStore
end
else version == exportMagicV2
loop per record
EI->>EI: read WorkerProto path info
EI->>Store: addToStore
EI->>EI: read/verify NAR dump
end
end
CLI-->>User: done / listing output
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Comment |
edolstra
force-pushed
the
export-import
branch
from
eedc992 to
da64b9e
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/libutil/args.cc (1)
391-421: Don’t increment timesUsed during completion parsingCurrently timesUsed++ runs even when we’re in completion mode for this flag (anyCompleted == true). That can skew state and future validations if the parser instance is reused.
Apply this diff to gate the increment together with handler invocation:
- if (!anyCompleted) - flag.handler.fun(std::move(args)); - flag.timesUsed++; + if (!anyCompleted) { + flag.handler.fun(std::move(args)); + flag.timesUsed++; + }
🧹 Nitpick comments (18)
src/nix/sigs.cc (2)
155-155: Required flag + non-optional field: add a minimal runtime guard for empty values.Switching to std::string with a required flag is good. However, users can still pass an empty value (e.g.
--key-name=). Add a lightweight check to reject empty names to avoid generating keys with an empty identifier.Apply this diff within run():
void run() override { logger->stop(); - writeFull(getStandardOutput(), SecretKey::generate(keyName).to_string()); + if (keyName.empty()) + throw UsageError("key name must not be empty"); + writeFull(getStandardOutput(), SecretKey::generate(keyName).to_string()); }Please also confirm whether additional validation (e.g., disallowing whitespace) is desired given the documented example format
cache.example.org-1.
180-184: Output behavior: consider trailing newline and secret material handling (optional).Current behavior writes the secret key without a trailing newline, consistent with convert-secret-to-public. Consider appending a newline for shell ergonomics, or explicitly document that no newline is printed. Also, if SecretKey supports secure zeroization, consider ensuring temporary buffers (e.g., the to_string result) are cleared post-write.
src/libutil/include/nix/util/args.hh (2)
205-206: Good addition: required flag supportThe field is clear and defaults safe. Consider surfacing this in JSON/help output so UIs and docs can reflect required options.
209-211: Avoid exposing mutable usage counters; provide an API insteadtimesUsed should be private to prevent accidental mutation. Provide markUsed() and useCount() accessors, keeping designated-initializer friendliness for other fields.
src/libutil/args.cc (1)
512-519: Deduplicate alias checks and use canonical flag name in errorIterating longFlags checks each alias entry, which can produce inconsistent error messages referencing an alias instead of the canonical longName. Skip alias keys and always report the canonical name.
Apply this diff:
void Args::checkArgs() { - for (auto & [name, flag] : longFlags) { - if (flag->required && flag->timesUsed == 0) - throw UsageError("required argument '--%s' is missing", name); - } + for (auto & [name, flag] : longFlags) { + // Skip alias entries to avoid duplicate checks and inconsistent names. + if (flag->aliases.count(name)) continue; + if (flag->required && flag->timesUsed == 0) + throw UsageError("required argument '--%s' is missing", flag->longName); + } }Additionally, consider adding “required” to toJSON() so tooling can surface this state:
// In Args::toJSON() when building j for each flag: j["required"] = flag->required;src/libstore/export-import.cc (5)
11-12: Clarify magic constant comment.The hex literal for exportMagicV2 reads little-endian vs the ASCII comment. Consider adding a note about numeric encoding to avoid confusion later.
51-66: De-duplicate the WorkerProto version literal.Hardcoding 16 in both write/read risks drift. Define a single constant and reuse in both places.
Apply:
@@ -static const uint32_t exportMagicV1 = 0x4558494e; -static const uint64_t exportMagicV2 = 0x324f4952414e; // = 'NARIO2' +static const uint32_t exportMagicV1 = 0x4558494e; +static const uint64_t exportMagicV2 = 0x324f4952414e; // = 'NARIO2' +static constexpr uint32_t kWorkerProtoVersion = 16; @@ - WorkerProto::Serialise<ValidPathInfo>::write( - store, WorkerProto::WriteConn{.to = sink, .version = 16}, *info); + WorkerProto::Serialise<ValidPathInfo>::write( + store, WorkerProto::WriteConn{.to = sink, .version = kWorkerProtoVersion}, *info); @@ - auto info = WorkerProto::Serialise<ValidPathInfo>::read( - store, WorkerProto::ReadConn{.from = source, .version = 16}); + auto info = WorkerProto::Serialise<ValidPathInfo>::read( + store, WorkerProto::ReadConn{.from = source, .version = kWorkerProtoVersion});Also applies to: 138-140
67-69: Error text: minor type mismatch.
versionis unsigned; consider%ufor consistency across platforms.
87-129: V1 import buffers whole NAR in memory.Acceptable for legacy; a brief comment noting intentional non-constant memory could help future readers.
151-153: Generic error hides header mismatches.If feasible, add hint when
versionis close toexportMagicV2or {0,1} to aid debugging.src/nix/nario-list.md (1)
1-18: Doc is clear; add version note (optional).Consider noting it lists both v1 and v2 streams from stdin, for clarity.
tests/functional/export.sh (4)
12-13: Quote redirection targets to handle spaces safelySafer to quote $TEST_ROOT paths in redirections.
-nix nario export --format 1 "$outPath" > $TEST_ROOT/exp2 +nix nario export --format 1 "$outPath" > "$TEST_ROOT/exp2" -cmp "$TEST_ROOT/exp" "$TEST_ROOT/exp2" +cmp "$TEST_ROOT/exp" "$TEST_ROOT/exp2"
17-19: Quote redirection targetsSame here for consistency and safety.
-nix nario export --format 1 -r "$outPath" > $TEST_ROOT/exp_all2 +nix nario export --format 1 -r "$outPath" > "$TEST_ROOT/exp_all2" -cmp "$TEST_ROOT/exp_all" "$TEST_ROOT/exp_all2" +cmp "$TEST_ROOT/exp_all" "$TEST_ROOT/exp_all2"
49-52: Quote file arguments in redirections and commandsAvoid word splitting if TEST_ROOT contains spaces.
-clearStore -nix nario import < $TEST_ROOT/exp_all -nix path-info "$outPath" +clearStore +nix nario import < "$TEST_ROOT/exp_all" +nix path-info "$outPath"
54-55: Quote file argument in pipeline; consider adding v2 coverage
- Quote the file in input redirection.
- Recommend adding a v2 export/import/list test to exercise the new format.
-nix nario list < $TEST_ROOT/exp_all | grepQuiet "dependencies-input-0: .* bytes" +nix nario list < "$TEST_ROOT/exp_all" | grepQuiet "dependencies-input-0: .* bytes"Example additional v2 coverage (add near this block):
# Format 2 roundtrip and list nix nario export --format 2 -r "$outPath" > "$TEST_ROOT/exp_all_v2" clearStore nix nario import < "$TEST_ROOT/exp_all_v2" nix path-info "$outPath" nix nario list < "$TEST_ROOT/exp_all_v2" | grepQuiet "dependencies-input-0: .* bytes"src/libstore/include/nix/store/export-import.hh (1)
7-12: Update API comment to document the version parameterClarify supported values and compatibility (v1 compatible with nix-store import; v2 is preferred and memory-efficient).
-/** - * Export multiple paths in the format expected by `nix-store - * --import`. The paths will be sorted topologically. - */ -void exportPaths(Store & store, const StorePathSet & paths, Sink & sink, unsigned int version); +/** + * Export multiple paths in the `nario` format. The paths are sorted topologically. + * - version == 1: legacy format compatible with `nix-store --import`. + * - version == 2: current format (recommended). + */ +void exportPaths(Store & store, const StorePathSet & paths, Sink & sink, unsigned int version);src/nix/nario.cc (2)
37-44: Enforce allowed values for --format at parse timeAvoid deferring invalid values to deeper layers; fail fast with a clear message.
CmdNarioExport() { addFlag({ .longName = "format", .description = "Version of the nario format to use. Must be `1` or `2`.", .labels = {"nario-format"}, .handler = {&version}, .required = true, }); + addFlagAction([this]() { + if (version != 1 && version != 2) + throw UsageError("invalid value for --format: %u (expected 1 or 2)", version); + }); }
58-62: Constructing StorePathSet can be simplifiedNo need for a temporary; use iterator-range constructor directly or std::move into a set only once.
- exportPaths(*store, StorePathSet(storePaths.begin(), storePaths.end()), sink, version); + StorePathSet paths{storePaths.begin(), storePaths.end()}; + exportPaths(*store, paths, sink, version);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
src/libmain/shared.cc(1 hunks)src/libstore/export-import.cc(1 hunks)src/libstore/include/nix/store/export-import.hh(1 hunks)src/libutil/args.cc(6 hunks)src/libutil/include/nix/util/args.hh(3 hunks)src/nix/meson.build(1 hunks)src/nix/nario-export.md(1 hunks)src/nix/nario-import.md(1 hunks)src/nix/nario-list.md(1 hunks)src/nix/nario.cc(1 hunks)src/nix/nix-store/nix-store.cc(1 hunks)src/nix/sigs.cc(3 hunks)src/perl/lib/Nix/Store.xs(1 hunks)tests/functional/export.sh(2 hunks)tests/functional/nix-profile.sh(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
src/libstore/export-import.cc (1)
src/nix/nario.cc (13)
store(58-62)store(58-58)store(81-85)store(81-81)sink(60-60)info(142-149)info(143-143)path(126-130)path(126-127)path(163-166)path(163-163)source(83-83)source(180-180)
src/nix/nario.cc (2)
src/libstore/export-import.cc (4)
exportPaths(14-70)exportPaths(14-14)importPaths(72-156)importPaths(72-72)src/libstore/include/nix/store/export-import.hh (2)
exportPaths(11-11)importPaths(17-17)
src/nix/nix-store/nix-store.cc (3)
src/libstore/export-import.cc (2)
exportPaths(14-70)exportPaths(14-14)src/libstore/include/nix/store/export-import.hh (1)
exportPaths(11-11)src/nix/nario.cc (5)
store(58-62)store(58-58)store(81-85)store(81-81)sink(60-60)
src/libstore/include/nix/store/export-import.hh (1)
src/nix/nario.cc (5)
store(58-62)store(58-58)store(81-85)store(81-81)sink(60-60)
src/nix/sigs.cc (1)
src/libutil/include/nix/util/file-descriptor.hh (1)
getStandardOutput(127-127)
src/libutil/args.cc (1)
src/libutil/include/nix/util/args.hh (1)
checkArgs(290-290)
🪛 Shellcheck (0.11.0)
tests/functional/export.sh
[warning] 15-15: Quote this to prevent word splitting.
(SC2046)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build_x86_64-linux / test
- GitHub Check: build_x86_64-linux / vm_tests_smoke
🔇 Additional comments (16)
src/nix/sigs.cc (1)
164-165: Marking --key-name as required is correct; confirm CLI error path and docs.Good move to enforce this at the args layer for consistent UX. Please verify:
- The command emits the standardized required-flag error when omitted.
- User-facing docs/usage reflect the requirement, and any release notes call out the behavior change.
src/libutil/include/nix/util/args.hh (2)
290-291: Hook looks goodAdding a protected virtual checkArgs() is appropriate for post-parse validation.
438-439: MultiCommand override is correctOverriding checkArgs() here aligns subcommand validation with the parent.
src/libutil/args.cc (2)
358-360: Run validation only outside completion mode: goodThe guard avoids side effects during completion generation.
659-665: Subcommand cascading is correctCalling Args::checkArgs() then delegating to the selected subcommand ensures both levels are validated.
src/libstore/export-import.cc (6)
7-7: Include looks correct and necessary.Brings in WorkerProto for v2 serialisation; good placement.
19-35: LGTM: hash check while streaming nar.The TeeSink approach verifies nar integrity without extra pass; solid.
39-49: V1 export path layout retained.Matches the v1 importer flow; backward compatibility preserved.
76-86: Version dispatch is sensible.V1 detection via leading 0/1 preserves existing streams; empty-stream fast path is nice.
130-147: V2 import preserves streaming/constant-memory semantics.Reads metadata first, then streams NAR into addToStore; matches stated goals.
14-14: No stale 3-arg exportPaths calls remain. All call sites now pass the newversionparameter.src/nix/meson.build (1)
91-91: Wire-up looks correct.Adds nario.cc to build; no linkage concerns apparent.
src/nix/nix-store/nix-store.cc (1)
778-778: Explicitly exporting v1 preserves nix-store compatibility.Good choice to keep nix-store’s stream stable while the new nario CLI handles v2.
Please ensure docs/manpage for nix-store --export still describe the v1 wire format or clarify that nix-store emits v1 while nix nario supports v2.
src/libmain/shared.cc (1)
337-338: Improved UX hint on usage errors.Newline + help suggestion aligns with test updates; no side effects.
tests/functional/nix-profile.sh (1)
98-99: Test updated to reflect new help hint.Matches the stderr change in handleExceptions.
src/perl/lib/Nix/Store.xs (1)
237-238: Perl XS updated to pass v1 explicitly.Consistent with nix-store behavior; ensure any Perl callers expecting 3‑arg exportPaths are recompiled.
To catch stale XS consumers, grep for perl-side wrappers that may have fixed arity assumptions.
| References of a path are not exported by default; use `-r` to export a complete closure. | ||
| Paths are exported in topographically sorted order (i.e. if path `X` refers to `Y`, then `Y` appears before `X`). | ||
|
|
There was a problem hiding this comment.
Typo: “topographically” → “topologically”
Use the correct term for DAG ordering.
-Paths are exported in topographically sorted order (i.e. if path `X` refers to `Y`, then `Y` appears before `X`).
+Paths are exported in topologically sorted order (i.e. if path `X` refers to `Y`, then `Y` appears before `X`).📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| References of a path are not exported by default; use `-r` to export a complete closure. | |
| Paths are exported in topographically sorted order (i.e. if path `X` refers to `Y`, then `Y` appears before `X`). | |
| References of a path are not exported by default; use `-r` to export a complete closure. | |
| Paths are exported in topologically sorted order (i.e. if path `X` refers to `Y`, then `Y` appears before `X`). |
🤖 Prompt for AI Agents
In src/nix/nario-export.md around lines 21 to 23, the term "topographically" is
incorrect; replace it with "topologically" so the sentence reads that paths are
exported in topologically sorted order (i.e. if path `X` refers to `Y`, then `Y`
appears before `X`). Ensure only the single word is corrected and keep the rest
of the sentence intact.
| R""( | ||
|
|
||
| # Examples | ||
|
|
||
| * Import store paths from the file named `dump`: | ||
|
|
||
| ```console | ||
| # nix nario import < dump | ||
| ``` | ||
|
|
||
| # Description | ||
|
|
||
| This command reads from standard input a serialization of store paths produced by `nix nario export` and adds them to the Nix store. | ||
|
|
||
| )"" |
There was a problem hiding this comment.
Document signature checking semantics.
nario import currently calls importPaths with NoCheckSigs (see src/nix/nario.cc Lines 80-84). Either document that signatures are not verified yet, or change the default to CheckSigs with a flag to disable.
🤖 Prompt for AI Agents
In src/nix/nario-import.md around lines 1 to 15, the docs don't mention that
nario import currently disables signature verification (importPaths is called
with NoCheckSigs); update the documentation to explicitly state that signatures
are not verified by default and why, and add a note about the intended behavior
change (or the safe alternative) — or instead change the implementation to use
CheckSigs by default and add a new command-line flag (e.g., --no-check-sigs) to
opt out; ensure the doc reflects whichever approach you take (if you change
code, mention the new flag and its default; if you keep NoCheckSigs, clearly
warn users and recommend verifying signatures externally).
| void run(ref<Store> store) override | ||
| { | ||
| FdSource source(getStandardInput()); | ||
| importPaths(*store, source, NoCheckSigs); // FIXME | ||
| } |
There was a problem hiding this comment.
Import defaults to NoCheckSigs (security posture)
Importing with signature checks disabled can admit untrusted content. Provide a flag to control this, defaulting to CheckSigs for safety, and allow an override like --no-check-sigs for current behavior.
- importPaths(*store, source, NoCheckSigs); // FIXME
+ // Consider a --no-check-sigs flag; default to CheckSigs.
+ importPaths(*store, source, CheckSigs);If you need backwards-compatibility, add a boolean flag and pass NoCheckSigs only when explicitly requested.
🤖 Prompt for AI Agents
In src/nix/nario.cc around lines 81 to 85, the call to importPaths currently
uses NoCheckSigs by default which weakens security; add a boolean option (e.g.
check_sigs = true) controlled by a command-line flag such as --no-check-sigs to
invert it, wire the flag into the code that constructs the Nario run context,
and pass CheckSigs when the flag is not set and NoCheckSigs only when the user
explicitly requested --no-check-sigs (maintaining backward-compat by defaulting
to CheckSigs); update parsing/usage help accordingly and ensure the new boolean
is used in place of the hardcoded NoCheckSigs in the importPaths call.
| void | ||
| addToStore(const ValidPathInfo & info, Source & source, RepairFlag repair, CheckSigsFlag checkSigs) override | ||
| { | ||
| logger->cout(fmt("%s: %d bytes", printStorePath(info.path), info.narSize)); | ||
| // Discard the NAR. | ||
| NullFileSystemObjectSink parseSink; | ||
| parseDump(parseSink, source); | ||
| } |
There was a problem hiding this comment.
Potential printf-format mismatch for narSize
info.narSize is typically a 64-bit/size_t value; using %d risks truncation/UB. Use PRIu64 (if uint64_t) or %zu (if size_t), or cast appropriately.
- logger->cout(fmt("%s: %d bytes", printStorePath(info.path), info.narSize));
+ logger->cout(fmt("%s: %s bytes",
+ printStorePath(info.path),
+ fmt("%" PRIu64, (uint64_t) info.narSize)));Alternatively, if narSize is size_t:
- logger->cout(fmt("%s: %d bytes", printStorePath(info.path), info.narSize));
+ logger->cout(fmt("%s: %zu bytes", printStorePath(info.path), (size_t) info.narSize));Please align with the actual type of ValidPathInfo::narSize.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| void | |
| addToStore(const ValidPathInfo & info, Source & source, RepairFlag repair, CheckSigsFlag checkSigs) override | |
| { | |
| logger->cout(fmt("%s: %d bytes", printStorePath(info.path), info.narSize)); | |
| // Discard the NAR. | |
| NullFileSystemObjectSink parseSink; | |
| parseDump(parseSink, source); | |
| } | |
| void | |
| addToStore(const ValidPathInfo & info, Source & source, RepairFlag repair, CheckSigsFlag checkSigs) override | |
| { | |
| logger->cout(fmt("%s: %s bytes", | |
| printStorePath(info.path), | |
| fmt("%" PRIu64, (uint64_t) info.narSize))); | |
| // Discard the NAR. | |
| NullFileSystemObjectSink parseSink; | |
| parseDump(parseSink, source); | |
| } |
| void | |
| addToStore(const ValidPathInfo & info, Source & source, RepairFlag repair, CheckSigsFlag checkSigs) override | |
| { | |
| logger->cout(fmt("%s: %d bytes", printStorePath(info.path), info.narSize)); | |
| // Discard the NAR. | |
| NullFileSystemObjectSink parseSink; | |
| parseDump(parseSink, source); | |
| } | |
| void | |
| addToStore(const ValidPathInfo & info, Source & source, RepairFlag repair, CheckSigsFlag checkSigs) override | |
| { | |
| logger->cout(fmt("%s: %zu bytes", printStorePath(info.path), (size_t) info.narSize)); | |
| // Discard the NAR. | |
| NullFileSystemObjectSink parseSink; | |
| parseDump(parseSink, source); | |
| } |
🤖 Prompt for AI Agents
In src/nix/nario.cc around lines 142 to 149, the fmt call uses "%d" for
info.narSize which can be a size_t or 64-bit value and causes printf-format
mismatch/truncation; determine the exact type of ValidPathInfo::narSize and
replace "%d" with the correct specifier (use "%zu" if size_t, or PRIu64 with
<inttypes.h> if uint64_t), or explicitly cast narSize to an int64_t/unsigned
long and use the matching format string so the format and argument types align.
1 participant
Motivation
Includes upstream NixOS#13969 and NixOS#14087, as well as version 2 of the
nariofile format, which can be unpacked using constant memory and includes more info like signatures.Context
Add 👍 to pull requests you find important.
The Nix maintainer team uses a GitHub project board to schedule and track reviews.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation