Mass fixes

.agents/skills/best-practices/SKILL.md

@@ -0,0 +1,272 @@
+---
+name: best-practices
+description: Review a Python package expression for best-practices compliance. Use when reviewing PRs, auditing existing packages, or writing new ones — checks meta completeness, pyproject conventions, dependency placement, testing strategy, source fetching, and formatting.
+---
+
+# Best Practices Review
+
+Review a Python package expression and report violations. Read the package file(s), then check every rule below. Report each finding with the line number and a short explanation.
+
+## Meta Attributes
+
+### Required Fields
+
+Every package must have all three:
+
+| Field | What to check |
+|-------|---------------|
+| `meta.description` | Present, non-empty, does not repeat the package name verbatim |
+| `meta.homepage` | Present, upstream project URL |
+| `meta.license` | Present, uses `lib.licenses.*` (not a raw string) |
+
+### Recommended Fields
+
+Flag as "recommended" (not error) if missing:
+
+- `meta.mainProgram` — for packages that install a binary (check `top-level.nix` for `toPythonApplication` usage)
+
+### Forbidden Fields
+
+Flag as error if present:
+
+- `meta.maintainers` — not a recognised key in this repo (except `maintainers = [ ]` which is accepted but unnecessary)
+- `meta.teams` — not a recognised key
+- `meta.platforms` — omit; inherited automatically from the Python interpreter
+
+## Build System
+
+### pyproject
+
+- `pyproject = true` **must** be set for PEP 517/518 builds. Flag legacy `format = "setuptools"` or `format = "pyproject"` as "prefer `pyproject = true`".
+- `format = "wheel"` is acceptable for pre-built wheel packages.
+- `format = "other"` is acceptable when the package has a non-standard build process.
+- `build-system` **must** be present when `pyproject = true` is set.
+- Flag any package lacking both `pyproject` and `format` — it falls back to legacy setuptools implicitly.
+
+### Common Build Backends
+
+Flag if `build-system` uses an unusual or outdated backend:
+
+| Backend | Notes |
+|---------|-------|
+| `setuptools` | Most common; acceptable |
+| `hatchling` | Modern; acceptable |
+| `flit-core` | Lightweight; acceptable |
+| `poetry-core` | Acceptable; do **not** use `poetry` (the full tool) as a build backend |
+| `pdm-backend` | Acceptable |
+| `maturin` | For Rust extensions; acceptable |
+| `meson-python` | For C/C++ extensions; acceptable |
+| `setuptools-scm` | Version management plugin; usually paired with `setuptools` |
+| `scikit-build-core` | CMake-based; acceptable |
+
+### CMake (in Python Packages)
+
+- If `cmakeFlags` or `cmakeBuildType` are used alongside Python, `cmake.configurePhaseHook` **must** be in `nativeBuildInputs`.
+- Native build tools (cmake, ninja, pkg-config, meson) belong in `nativeBuildInputs`, not `buildInputs`.
+
+## Dependencies
+
+### Attribute Naming
+
+| Attribute | Purpose | Flag if misused |
+|-----------|---------|-----------------|
+| `build-system` | PEP 517 build backends (setuptools, hatchling, etc.) | — |
+| `dependencies` | Runtime Python dependencies | Flag `propagatedBuildInputs` as "prefer `dependencies`" |
+| `optional-dependencies` | Extras groups (attrset of lists) | — |
+| `nativeBuildInputs` | Build-time tools (compilers, code generators, pkg-config, cmake) | — |
+| `buildInputs` | C/C++ libraries linked at build time | — |
+| `nativeCheckInputs` | Test-only dependencies | Flag bare `checkInputs` as "prefer `nativeCheckInputs`" |
+
+### Dependency Placement
+
+- **Build tools** (compilers, code generators, pkg-config, cmake, ninja, meson) belong in `nativeBuildInputs`, not `buildInputs`.
+- **C/C++ libraries** belong in `buildInputs`, not `nativeBuildInputs`.
+- **Test frameworks** (`pytestCheckHook`, `unittestCheckHook`, `pytest-mock`, etc.) belong in `nativeCheckInputs`.
+- **Python runtime dependencies** belong in `dependencies`, not `propagatedBuildInputs`.
+- `strictDeps = true` should be set when `depsBuildBuild` is used.
+
+### Dependency Relaxation
+
+- `pythonRelaxDeps` is acceptable when upstream pins tighter than necessary — list the specific packages being relaxed.
+- `pythonRemoveDeps` is acceptable for removing dependencies unavailable in the package set — prefer this over patching `pyproject.toml` when a simple removal suffices.
+- Flag blanket `pythonRelaxDeps = true` (without a package list) as "prefer explicit package list".
+
+## Testing
+
+### Preferred: `testPaths` + `passthru.tests`
+
+The preferred testing strategy uses `testPaths` which auto-generates `passthru.tests.python` and a `test_src` output, decoupling tests from the main build:
+
+```nix
+nativeCheckInputs = [ pytestCheckHook ];
+testPaths = [ "tests" ];
+```
+
+This is **strongly preferred** over in-build testing because:
+- Test failures don't invalidate the package or its downstream consumers
+- Tests run against the installed package, not the build tree
+- Avoids rebuilding the package just to re-run tests
+
+### Acceptable: `pytestCheckHook` without `testPaths`
+
+When `pytestCheckHook` is in `nativeCheckInputs` but `testPaths` is not set, the tests still run but without the `passthru.tests` decoupling. Flag as "recommend adding `testPaths`" when the source tree contains an obvious test directory.
+
+### Acceptable: Manual `passthru.tests`
+
+For complex test setups (e.g., circular dependencies with pytest), a manual `passthru.tests` is acceptable:
+
+```nix
+passthru.tests = {
+  pytest = callPackage ./tests.nix { };
+};
+```
+
+### Smoke Checks
+
+- `pythonImportsCheck` is **recommended** for all packages. It provides basic import validation during the build.
+- Flag missing `pythonImportsCheck` as a recommendation.
+
+### What to Flag
+
+| Pattern | Severity | Recommendation |
+|---------|----------|----------------|
+| `pytestCheckHook` without `testPaths` | Recommendation | Add `testPaths = [ "tests" ];` (or appropriate path) |
+| `doCheck = true` explicitly | Warning | Remove — enable testing via `testPaths` or `passthru.tests` instead |
+| `doCheck = false` explicitly | Warning | Remove — `false` is already the default |
+| `doInstallCheck = false` explicitly | Warning | Remove — already the default |
+| Custom `checkPhase` with pytest | Recommendation | Prefer `pytestCheckHook` + `testPaths` |
+| `disabledTests` without `pytestCheckHook` | Warning | Tests won't actually be disabled without the hook |
+
+### Test Attributes
+
+These attributes are forwarded to the `testPaths` test derivation automatically:
+- `disabledTests`, `disabledTestPaths`, `enabledTestPaths`
+- `pytestFlags`, `pytestFlagsArray`, `unittestFlagsArray`
+- `preCheck`, `postCheck`, `preInstallCheck`, `postInstallCheck`
+
+## Packaging Conventions
+
+### Structural
+
+- Use `finalAttrs` pattern — flag `rec {` as "prefer finalAttrs". The `finalAttrs` pattern enables better overriding and self-reference.
+- Do **not** set `__structuredAttrs = true` — it is the default.
+- Do **not** set `doCheck = false` or `doInstallCheck = false` — they are defaults.
+- Avoid `placeholder` when possible — prefer shell output references for better content-addressed derivation support. E.g., `"PREFIX=${placeholder "out"}"` → `"PREFIX=$(out)"`.
+- Directory name **must** match `pname` and use hyphens, not underscores (e.g., `zope-interface`, not `zope_interface`).
+
+### Formatting
+
+- Attributes inside a `buildPythonPackage` block should be separated by an empty line between each attribute.
+- Exception: `pname`, `version`, `pyproject`, `format`, and `disabled` may be grouped together without blank lines.
+
+### Function Arguments
+
+- Comment-group function arguments by purpose:
+
+```nix
+{
+  lib,
+  buildPythonPackage,
+  fetchFromGitHub,
+  # build-system
+  setuptools,
+  # dependencies
+  requests,
+  urllib3,
+  # tests
+  pytestCheckHook,
+}:
+```
+
+## Sources
+
+### Fetchers
+
+- **`fetchPypi`** is preferred for PyPI sdist tarballs. This repo uses `fetchPypi` (lowercase 'i').
+- **`fetchFromGitHub`** is preferred when patches or test fixtures are needed from the repository.
+- Flag `fetchurl` or `fetchzip` pointing at a PyPI or GitHub tarball as "prefer `fetchPypi` or `fetchFromGitHub`".
+- `fetchFromGitHub`/`fetchFromGitLab` should use `tag`, not `rev` with a tag value.
+- Hash should be SRI format (`sha256-...`), not bare hex.
+
+### Version References
+
+When using `finalAttrs`:
+```nix
+src = fetchFromGitHub {
+  tag = "v${finalAttrs.version}";
+  ...
+};
+```
+
+When using `rec`:
+```nix
+src = fetchPypi {
+  inherit pname version;
+  ...
+};
+```
+
+## Python-Specific Patterns
+
+### Version Gating
+
+- Use `disabled = pythonOlder "3.x";` to skip on older interpreters, not conditional `meta.platforms`.
+- Import `pythonOlder` or `pythonAtLeast` in the function arguments when needed.
+
+### Namespace Packages
+
+- Use `pythonNamespacesHook` for PEP 420 namespace packages (e.g., `zope.*`, `azure.*`).
+
+### Wheels
+
+- `format = "wheel"` is acceptable for pre-built binary packages.
+- Wheel packages should still have `pythonImportsCheck`.
+- Avoid wheels when a source build is feasible — source builds are more reproducible.
+
+### Alternate Versions
+
+- Place alternate versions in separate files alongside `default.nix` (e.g., `5.nix`, `3.nix`).
+- The `default.nix` should contain the latest/primary version.
+
+### Patching pyproject.toml
+
+When patching `pyproject.toml`, prefer targeted fixes:
+
+```nix
+postPatch = ''
+  substituteInPlace pyproject.toml \
+    --replace-fail "setuptools>=68" "setuptools"
+'';
+```
+
+Flag broad `sed` replacements that could break unrelated content.
+
+## Commits
+
+- Each commit should be one logical change.
+- Commit message format: `python3Packages.<pname>: init at <version>` or `python3Packages.<pname>: <old> -> <new>`.
+- Fixup commits should be squashed before merge.
+- Do **not** add AI attribution (`Co-Authored-By`) to commits.
+
+## Output Format
+
+Group findings by severity:
+
+```
+## Errors
+- line 12: `build-system` missing when `pyproject = true` is set
+- line 30: `meta.maintainers = [ lib.maintainers.foo ]` is forbidden — remove or use `[ ]`
+
+## Warnings
+- line 8: `propagatedBuildInputs` — prefer `dependencies`
+- line 25: `doCheck = false` is the default — remove redundant line
+- line 40: `__structuredAttrs = true` is the default — remove redundant line
+- line 5: `rec {` — prefer `finalAttrs` pattern
+
+## Recommendations
+- missing `pythonImportsCheck`
+- line 15: `pytestCheckHook` present but no `testPaths` — consider adding `testPaths = [ "tests" ];`
+- missing `meta.mainProgram` for CLI package
+```
+
+If there are no findings in a severity group, omit that group. If the package is clean, say so.

.github/workflows/nix.yml

@@ -0,0 +1,36 @@
+name: Nix
+
+on:
+  push:
+    branches: [ master ]
+  pull_request:
+    branches: [ master ]
+
+jobs:
+  lint:
+
+    runs-on: ubuntu-latest
+
+    steps:
+    - uses: actions/checkout@v4
+    - uses: cachix/install-nix-action@v30
+      with:
+        extra_nix_config: |
+          extra-substituters = https://ekala-corepkgs.cachix.org
+          extra-trusted-public-keys = ekala-corepkgs.cachix.org-1:DcZV+vegWoEzacbSdXFXU4S7728C0eS9RfGpKeyHd6w=
+    - name: Nix Lint
+      run: nix fmt . -- --ci
+
+  eval:
+
+    runs-on: ubuntu-latest
+
+    steps:
+    - uses: actions/checkout@v4
+    - uses: cachix/install-nix-action@v30
+      with:
+        extra_nix_config: |
+          extra-substituters = https://ekala-corepkgs.cachix.org
+          extra-trusted-public-keys = ekala-corepkgs.cachix.org-1:DcZV+vegWoEzacbSdXFXU4S7728C0eS9RfGpKeyHd6w=
+    - name: Evaluate all packages
+      run: ./ci/eval.sh

AGENTS.md

@@ -76,6 +76,7 @@ buildPythonPackage (finalAttrs: {
 - **Source fetchers:** prefer `fetchPyPi`; use `fetchFromGitHub` when patches or test fixtures are needed.
 - **Alternate versions:** place in a separate file (e.g., `chardet/5.nix`) alongside `default.nix`.
 - **Patches:** place `.patch` files alongside `default.nix` in the package directory.
+- **Formatting:** run `nix fmt -- <file>` after every edit to ensure consistent formatting.
 
 ## Commit Message Format
 

ci/eval.sh

@@ -13,7 +13,7 @@ trap 'rm -f "$stderr"' EXIT
 # Real failures are `abort`s, missing attributes and type errors, none of which
 # `tryEval` can catch -- so they surface here as a non-zero exit with Nix's own
 # message, which names the offending expression and its source location.
-if ! result="$(nix-instantiate --eval --strict --json ci/eval.nix --show-trace 2>"$stderr")"; then
+if ! result="$(nix-instantiate --eval --strict --json ci/eval.nix --show-trace --option allow-import-from-derivation false 2>"$stderr")"; then
   cat "$stderr" >&2
   exit 1
 fi

pkgs/arrow-cpp/default.nix

@@ -103,6 +103,7 @@ stdenv.mkDerivation (finalAttrs: {
     ninja
     autoconf # for vendored jemalloc
     flatbuffers
+    protobuf
   ]
   ++ lib.optional stdenv.hostPlatform.isDarwin fixDarwinDylibNames;
 
@@ -140,8 +141,18 @@ stdenv.mkDerivation (finalAttrs: {
 
   preConfigure = ''
     patchShebangs build-support/
+
     substituteInPlace "src/arrow/vendored/datetime/tz.cpp" \
       --replace-fail 'discover_tz_dir();' '"${tzdata}/share/zoneinfo";'
+
+    # Fix unsubstituted @build_protobuf@ from corepkgs protobuf setup hook
+    ProtobufCMakeFlags() {
+      cmakeFlagsArray+=(
+        "-DPROTOC_EXE=${protobuf}/bin/protoc"
+        "-DProtobuf_PROTOC_EXE=${protobuf}/bin/protoc"
+        "-DProtobuf_PROTOC_EXECUTABLE=${protobuf}/bin/protoc"
+      )
+    }
   '';
 
   cmakeFlags = [
@@ -182,6 +193,7 @@ stdenv.mkDerivation (finalAttrs: {
     (lib.cmakeBool "ARROW_S3" enableS3)
     (lib.cmakeBool "ARROW_GCS" enableGcs)
     (lib.cmakeBool "ARROW_AZURE" enableAzure)
+    (lib.cmakeFeature "Protobuf_PROTOC_EXECUTABLE" "${protobuf}/bin/protoc")
     (lib.cmakeBool "ARROW_ORC" true)
     # Parquet options:
     (lib.cmakeBool "ARROW_PARQUET" true)