Add a toolchains concept - #1
Closed
lucperkins wants to merge 8 commits into
Closed
lucperkins wants to merge 8 commits into
lucperkins wants to merge 8 commits into
Conversation
📝 WalkthroughWalkthroughAdds README "Toolchains" documentation; refactors flake outputs and devShell wiring (removing taskRunners/tasks/processTrees), changes lib import paths, adds a new Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 2
🧹 Nitpick comments (3)
flake.nix (3)
220-236:symlinkJoinwith empty paths may produce unexpected results.If all options are
false(the default),pathswill be an empty list. This creates a derivation with no contents, which may confuse users expecting at least one tool.Consider requiring at least one option or providing a default:
Proposed fix - default to nodejs
js = { - nodejs ? false, + nodejs ? true, bun ? false, npm ? false, pnpm ? false, }:Or add a validation:
assert (nodejs || bun || npm || pnpm) || throw "js toolchain requires at least one of: nodejs, bun, npm, pnpm";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@flake.nix` around lines 220 - 236, The js attrset can produce an empty pkgs.symlinkJoin when nodejs, bun, npm, and pnpm are all false; add a guard so at least one tool is enabled or provide a sensible default: update the js block (referencing nodejs, bun, npm, pnpm, packages, pkgs.symlinkJoin, and paths) to either default nodejs to true when none are set or add an assertion such as requiring (nodejs || bun || npm || pnpm) and throwing a clear error message if none are enabled; ensure the check runs before constructing packages so paths is never an empty list.
263-266: Unused localshellHookvariable.This
shellHookbinding (lines 263-266) is defined but never used. The returned attribute set defines its ownshellHookinline at lines 276-279 with identical content.Proposed fix - use the local variable
shellHook = lib.optionalString (fpm.pools != { }) '' php-fpm -y ${fpmConf} -D trap "php-fpm -y ${fpmConf} -F -R 2>/dev/null" EXIT ''; in { packages = if ini == "" then phpPkg else phpPkg.buildEnv { extraConfig = ini; }; - shellHook = lib.optionalString (fpm.pools != { }) '' - php-fpm -y ${fpmConf} -D - trap "php-fpm -y ${fpmConf} -F -R 2>/dev/null" EXIT - ''; + inherit shellHook; env = { }; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@flake.nix` around lines 263 - 266, The local variable shellHook is defined but not used; either remove the unused local binding or reuse it in the returned attribute set to avoid duplication. Edit the flake.nix local scope where shellHook is defined (the binding that uses lib.optionalString with fpm.pools and ${fpmConf}) and then replace the inline shellHook in the final attribute set with a reference to that local shellHook, or simply delete the local binding and keep the inline shellHook—choose one consistent approach so there is only a single shellHook definition.
193-203: Unclear version format expected for Go toolchain.The version substitution replaces
.with_, producing attribute names likego_1_22. If a user passesversion = "1.22", this generatesgo_1_1_22(incorrect). Users must pass just"22"to getgo_1_22.Consider documenting the expected format or adjusting the logic to handle both formats (e.g., strip leading
"1."prefix).Proposed fix to handle both formats
go = { version ? null, }: - { - packages = - if version == null then - pkgs.go - else - pkgs.${"go_1_${builtins.replaceStrings [ "." ] [ "_" ] (toString version)}"}; - }; + let + # Strip leading "1." if present: "1.22" -> "22", "22" -> "22" + normalizedVersion = + if version == null then null + else if lib.hasPrefix "1." (toString version) + then lib.removePrefix "1." (toString version) + else toString version; + in + { + packages = + if normalizedVersion == null then + pkgs.go + else + pkgs.${"go_1_${builtins.replaceStrings [ "." ] [ "_" ] normalizedVersion}"}; + };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@flake.nix` around lines 193 - 203, The go input attrlet currently builds pkgs.${"go_1_${builtins.replaceStrings [ \".\" ] [ \"_\" ] (toString version)}"} which treats "1.22" as "go_1_1_22"; normalize the version before constructing the attribute: in the go function normalize the version variable (e.g., detect and strip a leading "1." or take the last numeric segment after dots) so both "22" and "1.22" map to "22", then use that normalized value when creating the pkgs.go_1_<normalized> attribute; update the packages branch that references builtins.replaceStrings to use this normalized value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@flake.nix`:
- Around line 401-404: The documentation string for the `toolchains` output is
incorrect — it claims "Each toolchain returns `{ packages, shellHook }`" but
`rust` returns `{ env, packages }`, and `go` and `js` return only `{ packages
}`. Update the doc string in `flake.nix` to accurately list the possible return
keys (e.g. `{ packages, shellHook?, env? }` or explain per-toolchain
differences) and mention which toolchains provide `env` (rust) and which omit
`shellHook` (go, js) so the schema matches the actual `toolchains`
implementations.
- Around line 59-82: The shell's env in pkgs.mkShellNoCC currently uses
rustToolchain.env // self.envVars.postgres but omits terraformToolchain.env, so
TF_CLI_ARGS from terraformToolchain isn't exported; update the env merge to
include terraformToolchain.env (e.g. combine rustToolchain.env,
terraformToolchain.env and self.envVars.postgres) so TF_CLI_ARGS from
terraformToolchain is present in the dev shell; locate the terraformToolchain
and the pkgs.mkShellNoCC block to apply the merge.
---
Nitpick comments:
In `@flake.nix`:
- Around line 220-236: The js attrset can produce an empty pkgs.symlinkJoin when
nodejs, bun, npm, and pnpm are all false; add a guard so at least one tool is
enabled or provide a sensible default: update the js block (referencing nodejs,
bun, npm, pnpm, packages, pkgs.symlinkJoin, and paths) to either default nodejs
to true when none are set or add an assertion such as requiring (nodejs || bun
|| npm || pnpm) and throwing a clear error message if none are enabled; ensure
the check runs before constructing packages so paths is never an empty list.
- Around line 263-266: The local variable shellHook is defined but not used;
either remove the unused local binding or reuse it in the returned attribute set
to avoid duplication. Edit the flake.nix local scope where shellHook is defined
(the binding that uses lib.optionalString with fpm.pools and ${fpmConf}) and
then replace the inline shellHook in the final attribute set with a reference to
that local shellHook, or simply delete the local binding and keep the inline
shellHook—choose one consistent approach so there is only a single shellHook
definition.
- Around line 193-203: The go input attrlet currently builds
pkgs.${"go_1_${builtins.replaceStrings [ \".\" ] [ \"_\" ] (toString version)}"}
which treats "1.22" as "go_1_1_22"; normalize the version before constructing
the attribute: in the go function normalize the version variable (e.g., detect
and strip a leading "1." or take the last numeric segment after dots) so both
"22" and "1.22" map to "22", then use that normalized value when creating the
pkgs.go_1_<normalized> attribute; update the packages branch that references
builtins.replaceStrings to use this normalized value.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8032ceae-4cf8-4998-b414-0ad17f2bf568
⛔ Files ignored due to path filters (1)
flake.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
README.mdflake.nixlib/default.nixlib/process-tree.nixlib/task-runner.nixlib/task.nixlib/toolchain.nix
Comment on lines
+59
to
+82
| terraformToolchain = toolchains.terraform { | ||
| plugins = [ | ||
| "hashicorp_aws" | ||
| "hashicorp_google" | ||
| "hashicorp_kubernetes" | ||
| ]; | ||
| }; | ||
| in | ||
| pkgs.mkShellNoCC { | ||
| packages = with pkgs; [ | ||
| self.taskRunners.${system}.default | ||
| self.formatter.${system} | ||
|
|
||
| phpToolchain.packages | ||
| pythonToolchain.packages | ||
| rustToolchain.packages | ||
| terraformToolchain.packages | ||
| ]; | ||
| shellHook = '' | ||
| ${self.taskRunners.${system}.default.shellHook} | ||
| ${pythonToolchain.shellHook} | ||
| ${phpToolchain.shellHook} | ||
| ''; | ||
| env = rustToolchain.env // self.envVars.postgres; |
There was a problem hiding this comment.
terraformToolchain.env is not merged into the shell environment.
The terraform toolchain returns an env attribute containing TF_CLI_ARGS = "-no-color" (line 215-217), but this is not included in the shell's env attribute on line 82. The TF_CLI_ARGS environment variable won't be set in the dev shell.
Proposed fix
- env = rustToolchain.env // self.envVars.postgres;
+ env = rustToolchain.env // terraformToolchain.env // self.envVars.postgres;📝 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.
Suggested change
| terraformToolchain = toolchains.terraform { | |
| plugins = [ | |
| "hashicorp_aws" | |
| "hashicorp_google" | |
| "hashicorp_kubernetes" | |
| ]; | |
| }; | |
| in | |
| pkgs.mkShellNoCC { | |
| packages = with pkgs; [ | |
| self.taskRunners.${system}.default | |
| self.formatter.${system} | |
| phpToolchain.packages | |
| pythonToolchain.packages | |
| rustToolchain.packages | |
| terraformToolchain.packages | |
| ]; | |
| shellHook = '' | |
| ${self.taskRunners.${system}.default.shellHook} | |
| ${pythonToolchain.shellHook} | |
| ${phpToolchain.shellHook} | |
| ''; | |
| env = rustToolchain.env // self.envVars.postgres; | |
| terraformToolchain = toolchains.terraform { | |
| plugins = [ | |
| "hashicorp_aws" | |
| "hashicorp_google" | |
| "hashicorp_kubernetes" | |
| ]; | |
| }; | |
| in | |
| pkgs.mkShellNoCC { | |
| packages = with pkgs; [ | |
| self.taskRunners.${system}.default | |
| self.formatter.${system} | |
| phpToolchain.packages | |
| pythonToolchain.packages | |
| rustToolchain.packages | |
| terraformToolchain.packages | |
| ]; | |
| shellHook = '' | |
| ${self.taskRunners.${system}.default.shellHook} | |
| ${pythonToolchain.shellHook} | |
| ${phpToolchain.shellHook} | |
| ''; | |
| env = rustToolchain.env // terraformToolchain.env // self.envVars.postgres; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@flake.nix` around lines 59 - 82, The shell's env in pkgs.mkShellNoCC
currently uses rustToolchain.env // self.envVars.postgres but omits
terraformToolchain.env, so TF_CLI_ARGS from terraformToolchain isn't exported;
update the env merge to include terraformToolchain.env (e.g. combine
rustToolchain.env, terraformToolchain.env and self.envVars.postgres) so
TF_CLI_ARGS from terraformToolchain is present in the dev shell; locate the
terraformToolchain and the pkgs.mkShellNoCC block to apply the merge.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
flake.nix (1)
94-97:⚠️ Potential issue | 🟡 MinorSchema docs still advertise the wrong return shape.
Lines 94-97 say every toolchain returns
{ packages, shellHook }, but the toolchain API also needs to cover builders that exposeenv, and not every toolchain providesshellHook. This doc string is stricter than the actual contract.Suggested doc fix
doc = '' The `toolchains` output provides language toolchain builder functions. - Each toolchain returns `{ packages, shellHook }`. + Each toolchain returns an attribute set that may include `packages`, `env`, and `shellHook`. '';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@flake.nix` around lines 94 - 97, Update the docstring that describes the toolchains output: it currently claims each toolchain returns `{ packages, shellHook }` but the contract also permits builders to expose `env` and `shellHook` may be absent; revise the description of `toolchains` to state that each toolchain returns an attribute set which includes `packages` and may include `shellHook` and/or `env` (or other optional keys), and clarify which keys are required vs optional so the `toolchains` API documentation matches the actual return shape.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@flake.nix`:
- Around line 92-111: The flake defines schemas.toolchains but never exposes a
corresponding top-level output, so add a top-level output named toolchains that
exports the same structure described in the diff (include version, doc,
appendSystem and inventory) so consumers can access
inputs.up.toolchains.${system}; ensure the new outputs.toolchains uses the same
inventory mapping logic (the builtins.mapAttrs over systems producing children
with evalChecks.isFunction checks and what = "language toolchain builder") so it
matches schemas.toolchains and avoids attribute-missing errors.
- Line 51: The import of ./lib using only `{ inherit lib; }` will fail because
the module expects both `lib` and `pkgs`; update the import used to populate
`self.lib` so it passes `pkgs` as well (match the pattern used in
`overlays.default`), e.g. import ./lib with both `lib` and `pkgs` provided so
`self.lib` can evaluate correctly.
---
Duplicate comments:
In `@flake.nix`:
- Around line 94-97: Update the docstring that describes the toolchains output:
it currently claims each toolchain returns `{ packages, shellHook }` but the
contract also permits builders to expose `env` and `shellHook` may be absent;
revise the description of `toolchains` to state that each toolchain returns an
attribute set which includes `packages` and may include `shellHook` and/or `env`
(or other optional keys), and clarify which keys are required vs optional so the
`toolchains` API documentation matches the actual return shape.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| }; | ||
| } | ||
| ); | ||
| lib = import ./lib { inherit lib; }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n--- flake.nix (around Line 51) ---\n'
sed -n '48,58p' flake.nix
printf '\n--- lib/default.nix (module signature) ---\n'
sed -n '1,20p' lib/default.nixRepository: DeterminateSystems/up
Length of output: 713
self.lib will fail to evaluate—missing pkgs argument in the import.
Line 51 imports ./lib with only lib, but the module signature requires both { lib, pkgs }:. Any consumer trying to access inputs.up.lib will hit an evaluation error. The corrected pattern is shown in overlays.default (lines 54–59), which properly passes both lib and pkgs.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@flake.nix` at line 51, The import of ./lib using only `{ inherit lib; }` will
fail because the module expects both `lib` and `pkgs`; update the import used to
populate `self.lib` so it passes `pkgs` as well (match the pattern used in
`overlays.default`), e.g. import ./lib with both `lib` and `pkgs` provided so
`self.lib` can evaluate correctly.
Comment on lines
+92
to
+111
| toolchains = { | ||
| version = 1; | ||
| doc = '' | ||
| The `toolchains` output provides language toolchain builder functions. | ||
| Each toolchain returns `{ packages, shellHook }`. | ||
| ''; | ||
| appendSystem = true; | ||
| inventory = | ||
| output: | ||
| inputs.flake-schemas.lib.mkChildren ( | ||
| builtins.mapAttrs (system: toolchains: { | ||
| forSystems = [ system ]; | ||
| children = builtins.mapAttrs (_name: toolchain: { | ||
| forSystems = [ system ]; | ||
| evalChecks.isFunction = builtins.isFunction toolchain; | ||
| what = "language toolchain builder"; | ||
| }) toolchains; | ||
| }) output | ||
| ); | ||
| }; |
There was a problem hiding this comment.
The PR adds a schema for toolchains, but never exports toolchains.
The new schemas.toolchains entry describes inputs.up.toolchains.${system}, but there is no sibling top-level toolchains = ... output anywhere in this flake. As written, consumers will get an attribute-missing error instead of the new toolchain API.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@flake.nix` around lines 92 - 111, The flake defines schemas.toolchains but
never exposes a corresponding top-level output, so add a top-level output named
toolchains that exports the same structure described in the diff (include
version, doc, appendSystem and inventory) so consumers can access
inputs.up.toolchains.${system}; ensure the new outputs.toolchains uses the same
inventory mapping logic (the builtins.mapAttrs over systems producing children
with evalChecks.isFunction checks and what = "language toolchain builder") so it
matches schemas.toolchains and avoids attribute-missing errors.
Member
Author
|
Not going to do this just yet |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Imagine being able to do this in dev shells:
Summary by CodeRabbit
New Features
Documentation
Refactor
Chores