Registry improvements #273
Conversation
It's not used anywhere.
WalkthroughAdds a builtin flake registry JSON and embeds it in the build, refactors Registry APIs to remove Settings dependency and add in-memory JSON parsing with builtin fallback, updates call sites, adds a Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as "nix registry resolve"
participant Store
participant Registries as "Registries\n(Flag, User, System, Global/Builtin)"
User->>CLI: nix registry resolve <indirect-flakeref>
CLI->>CLI: parse flakeref
CLI->>Store: resolveFlake(parsedRef)
Store->>Registries: lookup indirect ref
alt Found in a registry
Registries-->>Store: return direct flakeref
else Redirect chain
Registries->>Registries: follow redirects across registries
Registries-->>Store: return final direct flakeref
else Not found in registries
Registries->>Registries: consult builtin JSON fallback
Registries-->>Store: builtin mapping (if present) or error
end
Store-->>CLI: resolved direct flakeref or error
CLI->>User: print resolved flakeref (stdout)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🪛 LanguageToolsrc/nix/registry-resolve.md[uncategorized] ~22-~22: The official name of this software platform is spelled with a capital “H”. (GITHUB) ⏰ 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)
🔇 Additional comments (1)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/libfetchers/registry.cc (1)
28-66: Consider catching broader exception types.The catch block at line 61 only catches
nlohmann::json::exception, but JSON parsing at line 35 can throw other exceptions (e.g.,std::bad_allocduring large allocations). These uncaught exceptions would propagate up, bypassing the fallback mechanism.Consider broadening the catch to handle unexpected errors gracefully:
} catch (nlohmann::json::exception & e) { warn("cannot parse flake registry '%s': %s", whence, e.what()); + } catch (std::exception & e) { + warn("unexpected error parsing flake registry '%s': %s", whence, e.what()); }
🧹 Nitpick comments (3)
src/libfetchers/builtin-flake-registry.json (1)
1-425: LGTM! Consider maintenance strategy for future updates.The builtin flake registry is well-structured with comprehensive mappings for popular flakes. All entries follow a consistent format and the JSON is valid.
As an optional improvement, consider documenting the maintenance strategy for keeping this registry synchronized with upstream changes. Since this is now embedded as static data, stale entries could cause confusion if upstream repositories move or are archived.
src/nix/registry-resolve.md (1)
1-28: LGTM! Documentation is clear and comprehensive.The documentation effectively explains the command's purpose, provides practical examples, and describes the resolution process. The static analysis warning about "github" capitalization is a false positive—lowercase is correct in URL/code contexts.
As a minor enhancement, consider making the first example more explicit that it's resolving two flake-refs simultaneously:
* Resolve the `nixpkgs` and `blender-bin` flakerefs: ```console # nix registry resolve nixpkgs blender-bin github:NixOS/nixpkgs/nixpkgs-unstable github:edolstra/nix-warez?dir=blenderThis would immediately clarify that the command accepts multiple arguments. </blockquote></details> <details> <summary>src/libfetchers/registry.cc (1)</summary><blockquote> `164-169`: **Consider adding a comment to clarify the inline include pattern.** The `#include` directive at line 167 within the function argument list is valid but unusual. A brief comment explaining that the generated header contains a string literal would improve readability. ```diff // Use builtin registry as fallback + // Note: builtin-flake-registry.json.gen.hh contains a string literal with the registry JSON return Registry::read( settings, "builtin flake registry", #include "builtin-flake-registry.json.gen.hh" , Registry::Global);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
maintainers/flake-module.nix(1 hunks)src/libcmd/common-eval-args.cc(1 hunks)src/libcmd/installables.cc(0 hunks)src/libfetchers/builtin-flake-registry.json(1 hunks)src/libfetchers/include/nix/fetchers/registry.hh(2 hunks)src/libfetchers/meson.build(1 hunks)src/libfetchers/registry.cc(4 hunks)src/nix/registry-resolve.md(1 hunks)src/nix/registry.cc(2 hunks)tests/functional/flakes/flakes.sh(1 hunks)
💤 Files with no reviewable changes (1)
- src/libcmd/installables.cc
🧰 Additional context used
🧬 Code graph analysis (4)
src/libcmd/common-eval-args.cc (2)
src/libfetchers/include/nix/fetchers/registry.hh (2)
overrideRegistry(61-61)from(46-46)src/libfetchers/registry.cc (2)
overrideRegistry(136-139)overrideRegistry(136-136)
tests/functional/flakes/flakes.sh (2)
src/nix/registry.cc (2)
registry(31-41)registry(146-151)src/libfetchers/registry.cc (4)
add(90-93)add(90-90)remove(95-100)remove(95-95)
src/nix/registry.cc (2)
src/libflake/include/nix/flake/flakeref.hh (4)
parseFlakeRef(89-95)fetchSettings(67-70)fetchSettings(72-72)fetchSettings(75-75)src/libflake/flakeref.cc (2)
parseFlakeRef(45-58)parseFlakeRef(45-51)
src/libfetchers/registry.cc (1)
src/libfetchers/include/nix/fetchers/registry.hh (4)
settings(39-39)settings(42-42)path(44-44)from(46-46)
🪛 LanguageTool
src/nix/registry-resolve.md
[uncategorized] ~22-~22: The official name of this software platform is spelled with a capital “H”.
Context: ...g. nixpkgs) to direct flakerefs (e.g. github:NixOS/nixpkgs) using the flake registr...
(GITHUB)
⏰ 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). (1)
- GitHub Check: build_aarch64-darwin / build
🔇 Additional comments (12)
src/libfetchers/meson.build (1)
59-64: LGTM!The build configuration correctly integrates the builtin flake registry by generating a header from the JSON file, which aligns with the PR's objective of including a copy of the upstream flake registry as a fallback.
maintainers/flake-module.nix (1)
103-103: LGTM!Appropriately excludes the new JSON data file from clang-format processing.
src/libcmd/common-eval-args.cc (1)
130-130: LGTM!The call to
overrideRegistrycorrectly matches the updated API signature that no longer requires a Settings parameter.src/libfetchers/include/nix/fetchers/registry.hh (3)
34-36: LGTM!The constructor simplification removes the Settings dependency, aligning with the PR's objective to support builtin registry handling without requiring Settings configuration.
41-42: LGTM!The new
readoverload acceptingjsonStrenables in-memory registry parsing, which is essential for loading the builtin flake registry from the generated header.
61-61: LGTM!The simplified
overrideRegistrysignature is consistent with removing Settings dependencies from the registry API.src/nix/registry.cc (2)
205-237: LGTM!The
CmdRegistryResolveimplementation is clean and straightforward. It correctly:
- Accepts multiple flake-refs as documented
- Resolves each via the registry using the store
- Outputs resolved URLs line-by-line
249-249: LGTM!The new
resolvesubcommand is properly registered in the registry command set.tests/functional/flakes/flakes.sh (1)
268-282: LGTM!The test changes comprehensively cover the new
nix registry resolvecommand functionality, including:
- Resolution with percent-encoded paths
- Registry count verification after add/remove operations
- Builtin fallback registry behavior when the upstream registry is unreachable
The test at line 281 cleverly uses
--download-attempts 1and an invalid URL to force fallback to the builtin registry, validating that the embedded registry serves as a proper fallback mechanism.src/libfetchers/registry.cc (3)
13-26: LGTM! Graceful fallback on registry read errors.The error handling approach returns an empty registry when the path doesn't exist or when reading/parsing fails, which aligns well with the fallback mechanism introduced in this PR. This ensures the system remains functional even when registries are unavailable.
130-139: LGTM! Clean removal of Settings dependency.The signature changes successfully decouple the flag registry from Settings, and the static initialization is thread-safe due to C++11 guarantees for static local variables.
141-174: Build system correctly generates the registry header - code is verified.The verification confirms:
src/libfetchers/builtin-flake-registry.jsonexists as the sourcesrc/libfetchers/meson.build(lines 59-64) properly configures the build rule viagen_header.process()- The generator wraps JSON in a C++ raw string literal (
R"__NIX_STR(...content...)__NIX_STR"), which is a valid expression for the#includein the function argumentThe fallback mechanism is well-designed and will function correctly.
This makes Nix more resilient if the registry is down or we're offline.
edolstra
force-pushed
the
registry-improvements
branch
from
0f6572c to
535a168
Compare
| for (auto & url : urls) { | ||
| auto ref = parseFlakeRef(fetchSettings, url); | ||
| auto resolved = ref.resolve(fetchSettings, store); | ||
| logger->cout("%s", resolved.to_string()); |
There was a problem hiding this comment.
nit: since it's possible there's more than 1 flake ref supplied, could be prefix them which each, so it's immediately obvious? (i.e. if someone had typo'd their local flake registry and had an entry "nixos-unstable" pointing to 25.11, while "nixos-25.11" pointed at unstable, they wouldn't have to reason about "why are they out of order?", it'd just be "oh I guess I swapped them oops")
There was a problem hiding this comment.
I'd be fine special casing that behavior "if more than 1 flakeref was supplied", as well as "if --json was supplied" (in the future that it grows a --json flag)
There was a problem hiding this comment.
IMHO that makes it harder to script.
Co-authored-by: Cole Helbling <cole.e.helbling@outlook.com>
Motivation
nix registry resolvefor doing registry lookups. This is useful for testing.Context
Summary by CodeRabbit
New Features
nix registry resolvecommand to resolve indirect flake references to direct upstream URLs.Documentation
nix registry resolve.Tests
Refactor
Chores