refactor(base): use final for dep lookups in envfs overlay override#167
refactor(base): use final for dep lookups in envfs overlay override#167
Conversation
PR #160 squash-merged a state where `Merge branch 'main' into feat/csec-tools` (22973ff) left literal conflict markers in `modules/tpnix/custom-packages-overlay.nix:28,37,57`. The result is not valid Nix syntax: `nix-instantiate --parse` rejects the file, the `tpnix` host build is broken, and both the `wfuzz` (feat/csec-tools) and `librepods` (main) overrides are mutually exclusive in that state. Resolution mirrors the system76 sibling at `modules/system76/custom-packages-overlay.nix`, which was resolved correctly during the same merge: keep both blocks, since the wfuzz and librepods overrides are independent. Verified with `nix-instantiate --parse` and `nix flake check --accept-flake-config --no-build`. Reported by claude-code review on PR #160 (run 25340703466).
`modules/base/envfs.nix:13-34` pinned `envfs` to 1.2.0 with dependency lookups (`prev.fetchFromGitHub`, `prev.rustPlatform.fetchCargoVendor`) sourced from `prev`. Switch those to `final.*` so any later overlay -- e.g. a future `rustPlatform`/`fetchurl` patch overlay -- propagates into the override instead of silently linking against the un-modified upstream copy. The `prev.envfs.overrideAttrs` entry point itself stays on `prev` (it must extend the unmodified upstream attrs, not recurse through the fixpoint). This is the same shape applied to `librepods` and `marktext` in #166. Comment updated above the overlay to record the rationale. `envfs.drvPath` is byte-identical pre/post: `/nix/store/f6npid91mdh5dpyxxbrn5wyzcn9dxf11-envfs-1.2.0.drv` -- no overlay in either host closure currently rebinds `fetchFromGitHub` or `rustPlatform.fetchCargoVendor`, so the change is purely defensive against future overlay composition. Validated with `nix flake check --accept-flake-config --no-build` (all checks passed). Suggested as an out-of-scope follow-up by claude-review on PR #166 (run 25344427322).
|
Claude finished @Bad3r's task in 1m 53s —— View job Code review
VerdictThe substantive 1. PR description references a file that does not exist
Please correct the path in the description so reviewers can audit the precedent. 2. Stated precedent (librepods / marktext) does not match current codePR description says #166 already applied the
librepods = prev.librepods.overrideAttrs (_old: rec {
version = "0.2.5";
src = prev.fetchFromGitHub { ... }; # still prev
buildInputs = [
prev.libpulseaudio # still prev
prev.openssl
prev.qt6.qtbase
prev.qt6.qtconnectivity
prev.qt6.qtdeclarative
prev.qt6.qttools
];
});
marktext = prev.marktext.overrideAttrs (old: {
nativeBuildInputs = (old.nativeBuildInputs or [ ]) ++ [
prev."node-gyp" # still prev
];
npm_config_node_gyp = "${prev."node-gyp"}/bin/node-gyp";
NODE_GYP = "${prev."node-gyp"}/bin/node-gyp";
});Either the description's reference to #166 is wrong, or #166's idiom was reverted/never landed. Worth resolving before claiming consistency. If the intent is uniformity across all overlay overrides, the 3. envfs.nix change itself — looks correct
4. Comment wording — minor nit (non-blocking)
Strictly speaking, fixed-point evaluation in overlays does not depend on registration order — any overlay (earlier or later in the list) that rebinds 5. Branch hygiene observation
The first commit is presumably what #165 contains. The PR description states this branch is "stacked on #165's hotfix" and rebases cleanly once #165 lands — that matches what I see. Worth ensuring #165 lands first to keep the merged history clean (otherwise this PR will produce a redundant merge-resolution commit on Summary
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the envfs overlay to use final for dependency lookups, ensuring that later overlays are correctly applied, and removes git merge conflict markers from the custom-packages-overlay.nix file. Feedback suggests deduplicating the source definition in the envfs overlay using a let binding to improve maintainability and updating other package overrides to use final for consistency across the configuration.
The comment above the `envfs` overlay said "any later overlay that rebinds them feeds into this build". That implied registration order mattered. Nixpkgs overlays are evaluated as a single fixed point, so any overlay (regardless of registration order) that rebinds the referenced deps is observed via `final`. Reword to "any other overlay" and add a parenthetical noting that ordering is irrelevant here. Suggested by claude-review on PR #167 (run 25344637086).
|
Claude finished @Bad3r's task in 2m 17s —— View job Code review (follow-up after latest push)
VerdictApproving in spirit. The latest push (commit I cannot run 1. Resolution of prior review items
2. Latest push (
|
| Area | Status |
|---|---|
envfs.nix code change |
OK — correct idiom |
envfs.nix comment wording (post-a61da18) |
OK — no longer implies overlay order |
Conflict-marker cleanup in tpnix/custom-packages-overlay.nix |
OK — clean mechanical resolution |
| PR description accuracy | OK — corrected since prior review |
| Scoping (defer librepods/marktext to #166) | OK — well-justified |
cargoDeps.src duplication |
Pre-existing; optional follow-up |
Local nix flake check / drvPath verification |
Not possible from this runner |
No blockers from me. Once #165 lands and this rebases cleanly onto main, this is good to merge.
• Branch: refactor/envfs-final-deps
•
Reply to first-round review (run 25344637086)Thanks for the careful audit. Three findings; two adopted, one explained. Adopted
Acknowledged but deliberately not addressed in this PR
Verified, no action
Verification
|
The `cargoDeps.src` argument inside the `envfs` override was an exact duplicate of the outer `src` (same `owner` / `repo` / `rev` / `hash`). Bind once via a `let` block inside the `overrideAttrs` body and reuse via `inherit src;` at both call sites. The four-line drift risk -- a future bump touching only one of the two `src` literals -- is now structurally impossible. `envfs.drvPath` is byte-identical pre/post: `/nix/store/f6npid91mdh5dpyxxbrn5wyzcn9dxf11-envfs-1.2.0.drv`. Suggested by gemini-code-assist on PR #167 (line 30 of `envfs.nix`) and seconded by claude-review (run 25344831509).
Reply to second-round review (run 25344831509)Both gemini-code-assist threads resolved. Adopted
Acknowledged but covered elsewhere
Verification
Both gemini review threads resolved via the API. |
|
Claude finished @Bad3r's task in 1m 29s —— View job Code review (third round, post-
|
| Area | Status |
|---|---|
envfs.nix final-for-deps refactor |
OK — correct idiom |
cargoDeps.src dedup via let + inherit |
OK — clean, structurally eliminates drift risk |
| Comment wording | OK — accurate, no ordering implication |
Conflict-marker cleanup in tpnix overlay |
OK — clean mechanical resolution |
| drvPath equivalence claim | Plausible; no overlay rebinds the touched builders (verified by grep) |
| Scoping (defer librepods/marktext to #166) | OK — well-justified |
Local nix flake check / drvPath verification |
Not possible from runner; inferred from static analysis |
No blockers. Once #165 lands and this rebases cleanly onto main, ready to merge.
• Branch: refactor/envfs-final-deps
1 participant
Summary
This was suggested as an out-of-scope follow-up by claude-review on PR #166 (run 25344427322).
Branch / merge order
This branch is based on `fix/tpnix-merge-conflict-markers` (PR #165) so that `nix flake check` evaluates -- without #165, the tree carries unresolved conflict markers in `modules/tpnix/custom-packages-overlay.nix` from the squash merge of #160. Land order: #165 -> #166 -> #167. Each PR rebases cleanly onto `main` once its predecessor lands.
The librepods / marktext `final`-for-deps adoption is intentionally not duplicated in this PR -- it is owned by #166, where the dedup refactor lifts those overrides into the shared `flake.lib.overlays.customPackages` definition with `final.*` already in place. Folding it in here would create a redundant churn that #166's dedup undoes.
Test plan