Commit 08762febf396

Vincent Demeester <vincent@sbr.pm>
2026-01-13 23:12:31
docs(nixpkgs): add comprehensive PR review checklist
Add detailed checklist for reviewing nixpkgs PRs including: - finalAttrs pattern requirements (RFC 0035) - Package structure and meta attributes - Source fetching best practices - Init PR specific requirements - Common issues and review comment templates
1 parent 61065af
Changed files (1)
dots
.config
claude
skills
dots/.config/claude/skills/Nixpkgs/review-checklist.md
@@ -0,0 +1,248 @@
+# Nixpkgs PR Review Checklist
+
+## General Review Process
+
+1. **Check CI Status**
+   - All ofborg checks passing
+   - GitHub Actions passing
+   - No eval errors
+
+2. **Build and Test**
+   ```bash
+   nixpkgs-review pr <number>
+   ```
+
+3. **Verify the package works**
+   - Test `--version` flag
+   - Test `--help` output
+   - Run basic functionality tests
+
+---
+
+## Code Quality Checks
+
+### 1. Use `finalAttrs` Pattern (CRITICAL)
+
+**Why**: Ensures proper behavior with `overrideAttrs` and self-referential packages.
+
+**Bad**:
+```nix
+buildGoModule rec {
+  pname = "example";
+  version = "1.0.0";
+  
+  src = fetchFromGitHub {
+    rev = "v${version}";  # ❌ Uses 'rec' binding
+  };
+  
+  ldflags = [ "-X main.version=v${version}" ];  # ❌ Won't update on override
+}
+```
+
+**Good**:
+```nix
+buildGoModule (finalAttrs: {
+  pname = "example";
+  version = "1.0.0";
+  
+  src = fetchFromGitHub {
+    rev = "v${finalAttrs.version}";  # ✅ Uses finalAttrs
+  };
+  
+  ldflags = [ "-X main.version=v${finalAttrs.version}" ];  # ✅ Properly updates
+})
+```
+
+**What to check**:
+- ✅ Function signature: `buildGoModule (finalAttrs: { ... })`
+- ✅ All self-references use `finalAttrs.attribute` not `rec`
+- ✅ Version in URLs: `"v${finalAttrs.version}"`
+- ✅ Version in ldflags: `finalAttrs.version`
+- ✅ Changelog URLs: `finalAttrs.version`
+
+### 2. Package Structure
+
+- ✅ Located in `pkgs/by-name/XX/package-name/package.nix`
+  - XX = first 2 letters of package name (lowercase)
+- ✅ File named `package.nix` (not `default.nix`)
+- ✅ Appropriate builder used:
+  - `buildGoModule` for Go
+  - `buildPythonPackage` for Python
+  - `buildRustPackage` for Rust
+  - etc.
+
+### 3. Meta Attributes (REQUIRED)
+
+```nix
+meta = {
+  description = "Clear, concise description";  # ✅ Required
+  homepage = "https://example.com";             # ✅ Required
+  changelog = "https://github.com/.../v${finalAttrs.version}";  # ✅ Recommended
+  license = lib.licenses.mit;                   # ✅ Required
+  maintainers = with lib.maintainers; [ username ];  # ✅ Required
+  mainProgram = "binary-name";                  # ✅ Required for CLIs
+  platforms = lib.platforms.unix;               # ✅ Specify supported platforms
+};
+```
+
+**What to check**:
+- ✅ Description is accurate (verify against upstream)
+- ✅ License matches upstream (check LICENSE file)
+- ✅ Homepage URL is correct
+- ✅ Changelog link uses `finalAttrs.version`
+- ✅ Maintainer is added to maintainer list
+- ✅ `mainProgram` specified for CLI tools
+
+### 4. Source Fetching
+
+**For GitHub releases**:
+```nix
+src = fetchFromGitHub {
+  owner = "user";
+  repo = "repo";
+  rev = "v${finalAttrs.version}";  # ✅ Or tag/commit
+  hash = "sha256-...";              # ✅ Use SRI hash
+};
+```
+
+**What to check**:
+- ✅ Uses appropriate fetcher (`fetchFromGitHub`, `fetchurl`, etc.)
+- ✅ Hash is correct and uses SRI format (`sha256-...`)
+- ✅ `rev` or `tag` references `finalAttrs.version`
+- ❌ No hardcoded versions in URLs
+
+### 5. Dependencies
+
+```nix
+nativeBuildInputs = [ ];  # Build-time tools (pkg-config, cmake, etc.)
+buildInputs = [ ];        # Runtime libraries
+nativeCheckInputs = [ ];  # Test-time tools
+```
+
+**What to check**:
+- ✅ Dependencies properly categorized
+- ✅ No unnecessary dependencies
+- ✅ Runtime dependencies are correct (`ldd` check for binaries)
+
+### 6. Version Handling
+
+**For Go modules**:
+```nix
+buildGoModule (finalAttrs: {
+  ldflags = [
+    "-s"
+    "-w"
+    "-X github.com/user/repo/version.Version=v${finalAttrs.version}"
+  ];
+})
+```
+
+**What to check**:
+- ✅ Version embedded correctly
+- ✅ `vendorHash` is specified and correct
+- ✅ Tests are enabled (`nativeCheckInputs` if needed)
+
+### 7. Tests
+
+**What to check**:
+- ✅ Upstream tests run during build
+- ✅ `doCheck` is not disabled without reason
+- ✅ If tests require additional tools, they're in `nativeCheckInputs`
+- ✅ Package includes `passthru.tests` if applicable
+
+---
+
+## Init PR Specific Checks
+
+For new packages (`init at X.Y.Z`), additionally verify:
+
+### 1. Package Readiness
+- ✅ Package is stable (not alpha/beta)
+- ✅ Clear license statement
+- ✅ Actively maintained upstream
+- ✅ Will be used by others (check GitHub stars/issues)
+
+### 2. Version Currency
+- ✅ Latest stable version is packaged
+- 🔍 Check: `curl -s https://api.github.com/repos/USER/REPO/releases/latest`
+
+### 3. Comprehensive Testing
+- ✅ Actually run the binary
+- ✅ Test basic functionality
+- ✅ Check `--version` output matches
+- ✅ Verify help text appears
+
+### 4. Security Considerations
+- ✅ No vendored outdated dependencies
+- ✅ Built from source (not binary-only)
+- ✅ No known vulnerabilities
+
+---
+
+## Common Issues
+
+### 1. Missing `finalAttrs`
+**Issue**: Using `rec` instead of `finalAttrs`
+**Fix**: Convert to `buildGoModule (finalAttrs: { ... })` pattern
+
+### 2. Hardcoded Versions
+**Issue**: `rev = "v1.0.0"` instead of `rev = "v${finalAttrs.version}"`
+**Fix**: Use `finalAttrs.version` for all version references
+
+### 3. Wrong License
+**Issue**: License doesn't match upstream
+**Fix**: Check upstream LICENSE file, update `meta.license`
+
+### 4. Missing mainProgram
+**Issue**: CLI tool without `mainProgram`
+**Fix**: Add `mainProgram = "binary-name";` to meta
+
+### 5. Incorrect Dependencies
+**Issue**: Build-time dependencies in `buildInputs`
+**Fix**: Move to `nativeBuildInputs`
+
+---
+
+## Review Comment Templates
+
+### Request finalAttrs Pattern
+```
+Please use the `finalAttrs` pattern for proper override behavior:
+
+\`\`\`suggestion
+buildGoModule (finalAttrs: {
+\`\`\`
+
+And update all version references:
+
+\`\`\`suggestion
+  rev = "v${finalAttrs.version}";
+\`\`\`
+```
+
+### Request Version Update
+```
+Upstream has released v X.Y.Z. Consider updating before merging.
+
+https://github.com/USER/REPO/releases/latest
+```
+
+### Approve with Minor Suggestion
+```
+Looks good! Built and tested successfully with `nixpkgs-review pr XXXXX`.
+
+One small thing: [suggestion]
+
+Either way, this is ready to go. Nice work! 👍
+```
+
+---
+
+## Reference Documentation
+
+- [Contributing to Nixpkgs](https://github.com/NixOS/nixpkgs/blob/master/CONTRIBUTING.md)
+- [Package Guidelines](https://github.com/NixOS/nixpkgs/blob/master/pkgs/README.md)
+- [Fixed-point Arguments](https://nixos.org/manual/nixpkgs/stable/#chap-build-helpers-finalAttrs)
+- [stdenv Manual](https://nixos.org/manual/nixpkgs/stable/#chap-stdenv)
+- [RFC 0035 - pname/version](https://github.com/NixOS/rfcs/pull/35)
+