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 versioned nario export/import formats (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, centralizes --no-check-sigs into MixNoCheckSigs, and adjusts a UsageError message newline. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant CLI as nix CLI
participant EI as Export/Import
participant Store
rect rgb(250,250,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\nper-path header + NAR, terminator
else V == 2
EI->>EI: write exportMagicV2 header
loop per path
EI->>Store: queryPathInfo
EI->>EI: serialize path info (WorkerProto, shortStorePaths)
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 (legacy/empty)
EI-->>CLI: legacy no-op/old handling
else version == 1
loop until terminator
EI->>EI: parse legacy per-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 (beta)
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.
| 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.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
edolstra
force-pushed
the
export-import
branch
from
a843a7d to
097b3ac
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/nix/nario-export.md (2)
8-8: Use v2 in the primary example to match recommendationSince v2 is recommended below, update the example accordingly.
- # nix nario export --format 1 -r nixpkgs#hello > dump + # nix nario export --format 2 -r nixpkgs#hello > dump
27-27: Document key v2 features (constant‑memory unpack, signatures)Aligns docs with PR objectives.
-* `2`: The latest version. Recommended. +* `2`: The latest version. Recommended. Supports constant‑memory unpacking and includes additional metadata such as signatures.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/nix/nario-export.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
src/nix/nario-export.md
[grammar] ~22-~22: There might be a mistake here.
Context: ...rs to Y, then Y appears before X). You must specify the desired nario ver...
(QB_NEW_EN)
⏰ 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_aarch64-darwin / build
- GitHub Check: build_x86_64-linux / build
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/functional/export.sh (1)
18-19: Quote command substitutions to prevent word splitting.Both lines 16 and 37 (referenced in static analysis) use unquoted command substitutions
$(nix-store -qR $outPath)which could cause issues if paths contain spaces.Apply this pattern to fix the quoting issues:
-nix-store --export $(nix-store -qR $outPath) > $TEST_ROOT/exp_all +nix-store --export "$(nix-store -qR $outPath)" > $TEST_ROOT/exp_allAnd similarly at line 37:
-nix-store --export $(nix-store -qR $outPath) > $TEST_ROOT/exp_all2 +nix-store --export "$(nix-store -qR $outPath)" > $TEST_ROOT/exp_all2src/libstore/export-import.cc (1)
37-69: Extract the magic number 16 into a named constant. This literal marks the WorkerProto minor version whenultimateandsigswere added; define aconstexpr(e.g.kPathInfoProtoVersion = 16) and reuse it in bothWriteConn/ReadConnand other occurrences.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/libstore/common-protocol.cc(2 hunks)src/libstore/export-import.cc(1 hunks)src/libstore/include/nix/store/common-protocol.hh(2 hunks)src/libstore/include/nix/store/worker-protocol-impl.hh(1 hunks)src/libstore/include/nix/store/worker-protocol.hh(2 hunks)src/libstore/worker-protocol.cc(2 hunks)src/nix/sigs.cc(4 hunks)tests/functional/export.sh(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
tests/functional/export.sh (1)
src/nix/nario.cc (4)
store(58-62)store(58-58)store(81-85)store(81-81)
src/libstore/include/nix/store/worker-protocol-impl.hh (3)
src/libstore/common-protocol.cc (26)
read(16-19)read(16-16)read(27-30)read(27-27)read(38-41)read(38-38)read(49-53)read(49-49)read(61-64)read(61-61)read(72-77)read(73-73)read(87-91)read(88-88)write(21-25)write(21-22)write(32-36)write(32-33)write(43-47)write(43-44)write(55-59)write(55-56)write(66-70)write(66-67)write(79-85)write(79-80)src/libstore/include/nix/store/worker-protocol.hh (4)
store(123-123)store(124-124)write(133-136)write(133-133)src/libstore/include/nix/store/common-protocol.hh (2)
write(54-57)write(54-54)
src/nix/sigs.cc (1)
src/libutil/include/nix/util/file-descriptor.hh (1)
getStandardOutput(127-127)
src/libstore/export-import.cc (3)
src/libstore/include/nix/store/worker-protocol.hh (4)
store(123-123)store(124-124)sink(223-226)sink(223-223)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/libstore/common-protocol.cc (28)
write(21-25)write(21-22)write(32-36)write(32-33)write(43-47)write(43-44)write(55-59)write(55-56)write(66-70)write(66-67)write(79-85)write(79-80)write(93-97)write(93-94)read(16-19)read(16-16)read(27-30)read(27-27)read(38-41)read(38-38)read(49-53)read(49-49)read(61-64)read(61-61)read(72-77)read(73-73)read(87-91)read(88-88)
src/libstore/common-protocol.cc (2)
src/libstore/include/nix/store/worker-protocol.hh (6)
store(123-123)store(124-124)write(133-136)write(133-133)s(233-236)s(233-233)src/libstore/include/nix/store/common-protocol.hh (2)
write(54-57)write(54-54)
🪛 Shellcheck (0.11.0)
tests/functional/export.sh
[warning] 16-16: Quote this to prevent word splitting.
(SC2046)
[warning] 37-37: 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_aarch64-darwin / build
- GitHub Check: build_x86_64-linux / build
🔇 Additional comments (20)
src/nix/sigs.cc (3)
108-116: LGTM! Improved validation by making flag required.Moving the validation from runtime to parse time is a good refactor. The
.required = trueflag ensuressecretKeyFileis set beforerun()executes, making the code cleaner and catching user errors earlier.
153-153: LGTM! Type simplification aligns with required flag.Changing from
std::optional<std::string>tostd::stringis appropriate since the flag is now required. This simplifies the type and eliminates the need for optional unwrapping.
157-163: LGTM! Consistent refactor matching CmdSign pattern.The changes consistently apply the same pattern as
CmdSign:
- Making the flag required at parse time (line 162)
- Using
keyNamedirectly without optional checks (line 181)This improves code clarity and ensures validation occurs before command execution.
Also applies to: 178-182
src/libstore/include/nix/store/worker-protocol-impl.hh (2)
48-49: LGTM!The shortStorePaths flag is correctly propagated from WorkerProto::ReadConn to CommonProto::ReadConn, ensuring consistent path serialization behavior across protocol layers.
54-55: LGTM!The shortStorePaths flag is correctly propagated from WorkerProto::WriteConn to CommonProto::WriteConn, maintaining symmetry with the read path.
src/libstore/include/nix/store/common-protocol.hh (2)
33-33: LGTM!The shortStorePaths flag with a safe default of false enables optional short-form path serialization while maintaining backward compatibility.
43-43: LGTM!The shortStorePaths flag is consistently added to WriteConn with the same default value, ensuring symmetric behavior for read and write operations.
src/libstore/include/nix/store/worker-protocol.hh (2)
69-69: LGTM!The shortStorePaths flag is correctly added to WorkerProto::ReadConn with a backward-compatible default.
80-80: LGTM!The shortStorePaths flag is consistently added to WorkerProto::WriteConn, maintaining symmetry with the read connection.
src/libstore/worker-protocol.cc (2)
222-225: LGTM!The refactoring from manual string handling to the standardized
WorkerProto::Serialise<std::optional<StorePath>>improves type safety and eliminates duplicate parsing logic. The use ofstd::moveis appropriate for the optional value.
239-240: LGTM!The write path now uses the generic
WorkerProto::writefor the deriver field, making the code more consistent with other serialization patterns. The narHash serialization remains separate as appropriate.src/libstore/common-protocol.cc (4)
29-29: LGTM!The conditional logic correctly branches on
conn.shortStorePathsto handle short-form path deserialization viaStorePath(readString(conn.from))versus long-form viastore.parseStorePath(readString(conn.from)).
35-35: LGTM!The write path correctly uses
storePath.to_string()for short form versusstore.printStorePath(storePath)for long form, maintaining symmetry with the read logic.
76-76: LGTM!The optional path handling correctly preserves the empty-string-means-null pattern while adding the short vs long form branching for non-empty values.
82-84: LGTM!The write logic for optional paths correctly handles both the presence/absence check and the short/long form serialization in a clear ternary expression.
tests/functional/export.sh (2)
12-14: Good test coverage for the new error handling.Line 12 tests the error case when no format version is provided, addressing the previous reviewer's request. Lines 13-14 verify format 1 compatibility with the legacy export.
45-62: Excellent test coverage for the new nario commands and format 2.The tests comprehensively verify:
- Import with/without signature checking
- The
nario listcommand- Format 2 export with signatures
- Key generation and signature verification
This ensures the new versioned export/import flow works correctly end-to-end.
src/libstore/export-import.cc (3)
11-12: LGTM!The magic constants clearly distinguish between version 1 (0x4558494e = "EXIN" reversed) and version 2 (0x324f4952414e = "NARIO2" as bytes). The different sizes (uint32_t vs uint64_t) prevent format confusion during version detection.
19-35: Excellent corruption prevention.The
dumpNarlambda validates the NAR hash against the stored hash before export, preventing filesystem corruption from propagating to other machines. The hash comparison correctly allows zero (unknown) hashes while catching actual mismatches.
76-153: Clean version detection and handling.The import logic correctly:
- Detects the version by reading the first uint64_t
- Handles version 0/1 as the legacy format (where 0/1 are path presence indicators)
- Handles exportMagicV2 as the new format
- Provides clear error messages for unrecognized formats
The use of
shortStorePaths = trueat line 139 is consistent with the export side (line 60).
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/nix/nario-export.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
src/nix/nario-export.md
[grammar] ~22-~22: There might be a mistake here.
Context: ...rs to Y, then Y appears before X). You must specify the desired nario ver...
(QB_NEW_EN)
⏰ 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). (3)
- GitHub Check: build_x86_64-linux / manual
- GitHub Check: build_x86_64-linux / vm_tests_smoke
- GitHub Check: build_aarch64-darwin / build
|
Some observations:
Otherwise, this looks pretty good to me! (I'd like the first 2 bullets to be addressed before we merge, and we can just keep the last bullet in the back of our mind) |
cole-h
left a comment
cole-h
left a comment
There was a problem hiding this comment.
LGTM excepting my above comment
edolstra
force-pushed
the
export-import
branch
from
87b738a to
db4e80b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/nix/nario.cc (1)
153-160: Fix the printf format mismatch for narSize.This issue was previously flagged and remains unresolved. The
%dformat specifier is incorrect forinfo.narSize, which is a 64-bit value (uint64_t). Using%dcan cause truncation and undefined behavior for files larger than 2GB.Apply this diff to fix the format specifier:
- logger->cout(fmt("%s: %d bytes", printStorePath(info.path), info.narSize)); + logger->cout(fmt("%s: %lu bytes", printStorePath(info.path), static_cast<unsigned long>(info.narSize)));Alternatively, if you prefer to keep the exact type semantics:
- logger->cout(fmt("%s: %d bytes", printStorePath(info.path), info.narSize)); + logger->cout(fmt("%s: %zu bytes", printStorePath(info.path), static_cast<size_t>(info.narSize)));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/libutil/args.cc(6 hunks)src/nix/nario-export.md(1 hunks)src/nix/nario-import.md(1 hunks)src/nix/nario.cc(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/nix/nario-import.md
🧰 Additional context used
🧬 Code graph analysis (2)
src/nix/nario.cc (2)
src/libutil/include/nix/util/file-descriptor.hh (2)
getStandardOutput(127-127)getStandardInput(114-114)src/libstore/export-import.cc (4)
exportPaths(14-70)exportPaths(14-14)importPaths(72-156)importPaths(72-72)
src/libutil/args.cc (1)
src/libutil/include/nix/util/args.hh (1)
checkArgs(290-290)
🪛 Clang (14.0.6)
src/nix/nario.cc
[error] 1-1: 'nix/cmd/command.hh' file not found
(clang-diagnostic-error)
🪛 LanguageTool
src/nix/nario-export.md
[grammar] ~22-~22: There might be a mistake here.
Context: ...rs to Y, then Y appears before X). You must specify the desired nario ver...
(QB_NEW_EN)
⏰ 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). (4)
- GitHub Check: build_aarch64-darwin / manual
- GitHub Check: build_aarch64-darwin / test
- GitHub Check: build_x86_64-linux / vm_tests_smoke
- GitHub Check: build_x86_64-linux / test
🔇 Additional comments (9)
src/nix/nario-export.md (1)
1-29: LGTM! Documentation is clear and complete.All previously flagged issues have been addressed. The examples now correctly demonstrate the recommended format 2, and the grammar/wording improvements have been applied throughout. The documentation clearly explains the command behavior, format versions, and best practices.
src/nix/nario.cc (5)
11-29: LGTM! Clean command structure.The top-level
CmdNariomulti-command is properly structured and registered.
31-66: LGTM! Terminal safety check addresses PR feedback.The implementation correctly refuses to write nario data to a terminal (lines 61-62), directly addressing the reviewer's concern from the PR objectives about preventing junk output on the terminal. The required
--formatflag ensures users explicitly choose the nario version.
70-76: LGTM! Symmetric safety check for stdin.The helper function correctly refuses to read nario data from a terminal, providing symmetric protection to the export command.
78-97: LGTM! Secure-by-default signature checking.The use of
MixNoCheckSigsprovides proper control over signature verification, defaulting toCheckSigsfor security while allowing--no-check-sigswhen needed. This correctly addresses the previous security concern.
101-152: LGTM! ListingStore implementation is appropriate.The
ListingStoremock implementation correctly provides minimal functionality for listing nario contents without persisting data. The use ofNoCheckSigsat line 194 is appropriate since the store is only used for listing, not actual import.Also applies to: 161-196
src/libutil/args.cc (3)
358-360: LGTM! Validation is correctly gated and positioned.The checkArgs() call is appropriately skipped during completion mode and placed after all arguments are processed but before moving to the next initialization phase.
391-391: LGTM! Usage tracking implementation is correct.The lambda signature change to non-const
Flag &enables incrementingtimesUsed. Since flags are stored inshared_ptr<Flag>and aliases point to the same object (lines 24-26), usage is correctly tracked regardless of which name or alias is used.Also applies to: 420-420
659-664: LGTM! Validation correctly cascades through command hierarchy.The implementation validates parent command flags first, then delegates to the active subcommand if present, matching the delegation pattern used in
processFlagandprocessArgs.
| void Args::checkArgs() | ||
| { | ||
| for (auto & [name, flag] : longFlags) { | ||
| if (flag->required && flag->timesUsed == 0) | ||
| throw UsageError("required argument '%s' is missing", "--" + name); | ||
| } | ||
| } |
There was a problem hiding this comment.
Error message may reference alias instead of primary flag name.
The longFlags map contains both primary flag names and aliases pointing to the same Flag object (lines 24-26). When iterating in checkArgs(), if a required flag is missing, the error message will use whichever name the iterator encounters first—potentially an alias. This could confuse users who expect the primary flag name in error messages.
Apply this diff to filter out aliases, matching the pattern in toJSON() (line 527):
void Args::checkArgs()
{
for (auto & [name, flag] : longFlags) {
+ if (flag->aliases.count(name))
+ continue;
if (flag->required && flag->timesUsed == 0)
throw UsageError("required argument '%s' is missing", "--" + name);
}
}📝 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 Args::checkArgs() | |
| { | |
| for (auto & [name, flag] : longFlags) { | |
| if (flag->required && flag->timesUsed == 0) | |
| throw UsageError("required argument '%s' is missing", "--" + name); | |
| } | |
| } | |
| void Args::checkArgs() | |
| { | |
| for (auto & [name, flag] : longFlags) { | |
| if (flag->aliases.count(name)) | |
| continue; | |
| if (flag->required && flag->timesUsed == 0) | |
| throw UsageError("required argument '%s' is missing", "--" + name); | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/libutil/args.cc around lines 512 to 518, the missing-required-flag error
may report an alias name because longFlags contains aliases; modify the check to
skip alias entries (same pattern as toJSON) by continuing when name !=
flag->name, and when throwing the UsageError use the flag's primary name (e.g.
"--" + flag->name) so the error always references the canonical flag name.
2 participants
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
Tests