Enable creating "multi-watch" processes#7
Conversation
📝 WalkthroughWalkthroughAdds a concurrent watcher helper Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant Task as Task Runner (flake task)
participant Orchestrator as mkWatchMany
participant ProcTree as mkProcessTree
participant WatchA as watchexec (watcher-0)
participant WatchB as watchexec (watcher-1)
participant OS as OS (signals)
User->>Task: run `watch-all` task
Task->>Orchestrator: invoke mkWatchMany with `watchers` list
Orchestrator->>WatchA: spawn watchexec --watch ... -- command A
Orchestrator->>WatchB: spawn watchexec --watch ... -- command B
Orchestrator->>ProcTree: register processes (watcher-0, watcher-1)
Note right of Orchestrator: record PIDs and monitor
OS-->>Orchestrator: INT/TERM signal
Orchestrator->>WatchA: kill PID0
Orchestrator->>WatchB: kill PID1
Orchestrator->>WatchA: wait PID0
Orchestrator->>WatchB: wait PID1
Orchestrator-->>Task: exit with aggregated status
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@example/flake.nix`:
- Around line 153-174: The watch-all attribute is using pkgs.lib.mkWatch but
supplies a watchers list (the mkWatchMany API); replace the call to mkWatch with
mkWatchMany so the watchers list is accepted, i.e. change the function name from
mkWatch to mkWatchMany in the watch-all block where the watchers, command,
paths, and extensions are defined so evaluation succeeds.
In `@lib/watch.nix`:
- Around line 91-95: taskModuleArgs is built by calling builtins.removeAttrs on
args but misses the exitMsg key, so exitMsg (which is consumed internally) will
be forwarded to the task module; update the removeAttrs call that builds
taskModuleArgs to include "exitMsg" in the list of removed attributes so exitMsg
is not passed into the task module and won't cause unexpected evaluation errors.
🪄 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: 13a0802c-1a52-47d4-8c39-f4de282606c0
📒 Files selected for processing (5)
README.mdexample/flake.nixexample/src/bin/server.rslib/default.nixlib/watch.nix
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/watch.nix`:
- Around line 103-111: The shutdown() function currently interpolates exitMsg
directly into the shell script (echo "${exitMsg}" >&2) which can break the
generated shell if exitMsg contains quotes or metacharacters; change the code to
emit a shell-safe, quoted form of exitMsg instead of raw interpolation—either
call a shell-escaping helper or use printf '%q' (or equivalent
single-quote-with-embedded-quote escaping) when emitting exitMsg so the
shutdown() echo/write uses a safely quoted variable, and ensure pids handling
remains unchanged.
🪄 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: 920a6139-a845-4665-8d20-67c9ef4f56a9
📒 Files selected for processing (4)
.github/workflows/checks.yaml.github/workflows/flakehub-publish-rolling.yamlexample/flake.nixlib/watch.nix
✅ Files skipped from review due to trivial changes (1)
- .github/workflows/flakehub-publish-rolling.yaml
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/watch.nix`:
- Around line 108-119: The code currently builds processes using processNameOf
and then passes them to builtins.listToAttrs which silently collapses duplicate
names; detect duplicates first by mapping each indexed entry through
processNameOf to a list of resolved names, check for any duplicates (same name
appearing more than once), and if found raise a clear error (e.g., via throw or
lib.error) listing the duplicated name(s) and their watcher indices; only
proceed to build processes with listToAttrs if no duplicates are detected.
Ensure you reference the existing processNameOf and processes logic when adding
the duplicate-checking step so the validation happens before listToAttrs is
invoked.
- Around line 34-36: The mkWatchexecCmd snippet that appends the "--debounce"
argument currently passes a unit-less number via (toString debounce); change it
to append the explicit milliseconds unit (e.g. "${toString debounce}ms" or
equivalent string concatenation) so the debounce value matches the pattern used
in lib/process-tree.nix (which uses "${toString w.debounce}ms") and avoids
future watchexec errors; update the array element built in lib.optionals
(debounce != null) to include the "ms" suffix.
🪄 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: a4b11036-8b95-40cf-8d6b-1f5c62a9f0f2
📒 Files selected for processing (3)
lib/default.nixlib/process-tree.nixlib/watch.nix
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/default.nix
| ++ lib.optionals (debounce != null) [ | ||
| "--debounce" | ||
| (toString debounce) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, explore the repository structure
git ls-files | head -20Repository: DeterminateSystems/up
Length of output: 480
🏁 Script executed:
# Find and read lib/watch.nix around lines 34-36
if [ -f lib/watch.nix ]; then
wc -l lib/watch.nix
fiRepository: DeterminateSystems/up
Length of output: 83
🏁 Script executed:
# Read the relevant section from lib/watch.nix
if [ -f lib/watch.nix ]; then
sed -n '30,40p' lib/watch.nix
fiRepository: DeterminateSystems/up
Length of output: 292
🏁 Script executed:
# Search for debounce usage in lib/process-tree.nix
if [ -f lib/process-tree.nix ]; then
rg -n "debounce" lib/process-tree.nix -A 2 -B 2
fiRepository: DeterminateSystems/up
Length of output: 412
🌐 Web query:
watchexec --debounce duration syntax milliseconds documentation
💡 Result:
The syntax for the --debounce option in watchexec is -d, --debounce , where specifies the time to wait for new events before taking action (default: 50ms). It accepts: - A unit-less value in milliseconds (e.g., 100 for 100ms). This is deprecated, will warn, and become an error in the future. - A time span value (e.g., 5sec, 20ms, 1min 30s, 30min). Example: watchexec -d 100 command or watchexec --debounce 500ms command. Setting to 0 is highly discouraged. This is from the official watchexec manual.
Citations:
- 1: https://github.com/watchexec/watchexec/blob/main/doc/watchexec.1.md
- 2: https://manpages.opensuse.org/Tumbleweed/watchexec/watchexec.1.en.html
- 3: https://man.archlinux.org/man/extra/watchexec/watchexec.1.en
Add explicit ms unit to --debounce argument.
Watchexec accepts unit-less debounce values as milliseconds but marks this form deprecated and will error on it in future versions. The lib/process-tree.nix path already uses "${toString w.debounce}ms", so this inconsistency makes the mkWatchexecCmd helper fragile and diverges from the established pattern.
🔧 Proposed fix
++ lib.optionals (debounce != null) [
"--debounce"
- (toString debounce)
+ "${toString debounce}ms"
]📝 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.
| ++ lib.optionals (debounce != null) [ | |
| "--debounce" | |
| (toString debounce) | |
| + lib.optionals (debounce != null) [ | |
| "--debounce" | |
| "${toString debounce}ms" | |
| ] |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/watch.nix` around lines 34 - 36, The mkWatchexecCmd snippet that appends
the "--debounce" argument currently passes a unit-less number via (toString
debounce); change it to append the explicit milliseconds unit (e.g. "${toString
debounce}ms" or equivalent string concatenation) so the debounce value matches
the pattern used in lib/process-tree.nix (which uses "${toString w.debounce}ms")
and avoids future watchexec errors; update the array element built in
lib.optionals (debounce != null) to include the "ms" suffix.
| # Process name: either user-supplied `name`, or `watcher-<index>`. | ||
| processNameOf = { i, watcher }: watcher.name or "watcher-${toString i}"; | ||
|
|
||
| processes = lib.listToAttrs ( | ||
| map (entry: { | ||
| name = processNameOf entry; | ||
| value = { | ||
| command = mkWatchexecCmd entry.watcher; | ||
| packages = [ entry.watcher.package ]; | ||
| }; | ||
| }) indexed | ||
| ); |
There was a problem hiding this comment.
Reject duplicate watcher names before building processes.
If two watchers resolve to the same name here, builtins.listToAttrs collapses the duplicate and only one process survives, so one watcher never starts. That can happen with repeated explicit names or with an explicit name colliding with the default watcher-<index> fallback. (releases.nixos.org)
🔧 Proposed fix
# Process name: either user-supplied `name`, or `watcher-<index>`.
processNameOf = { i, watcher }: watcher.name or "watcher-${toString i}";
- processes = lib.listToAttrs (
- map (entry: {
- name = processNameOf entry;
- value = {
- command = mkWatchexecCmd entry.watcher;
- packages = [ entry.watcher.package ];
- };
- }) indexed
- );
+ processNames = map processNameOf indexed;
+
+ processes =
+ assert lib.assertMsg
+ (lib.length processNames == lib.length (lib.unique processNames))
+ "mkWatchMany: watcher names must be unique";
+ lib.listToAttrs (
+ map (entry: {
+ name = processNameOf entry;
+ value = {
+ command = mkWatchexecCmd entry.watcher;
+ packages = [ entry.watcher.package ];
+ };
+ }) indexed
+ );📝 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.
| # Process name: either user-supplied `name`, or `watcher-<index>`. | |
| processNameOf = { i, watcher }: watcher.name or "watcher-${toString i}"; | |
| processes = lib.listToAttrs ( | |
| map (entry: { | |
| name = processNameOf entry; | |
| value = { | |
| command = mkWatchexecCmd entry.watcher; | |
| packages = [ entry.watcher.package ]; | |
| }; | |
| }) indexed | |
| ); | |
| # Process name: either user-supplied `name`, or `watcher-<index>`. | |
| processNameOf = { i, watcher }: watcher.name or "watcher-${toString i}"; | |
| processNames = map processNameOf indexed; | |
| processes = | |
| assert lib.assertMsg | |
| (lib.length processNames == lib.length (lib.unique processNames)) | |
| "mkWatchMany: watcher names must be unique"; | |
| lib.listToAttrs ( | |
| map (entry: { | |
| name = processNameOf entry; | |
| value = { | |
| command = mkWatchexecCmd entry.watcher; | |
| packages = [ entry.watcher.package ]; | |
| }; | |
| }) indexed | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/watch.nix` around lines 108 - 119, The code currently builds processes
using processNameOf and then passes them to builtins.listToAttrs which silently
collapses duplicate names; detect duplicates first by mapping each indexed entry
through processNameOf to a list of resolved names, check for any duplicates
(same name appearing more than once), and if found raise a clear error (e.g.,
via throw or lib.error) listing the duplicated name(s) and their watcher
indices; only proceed to build processes with listToAttrs if no duplicates are
detected. Ensure you reference the existing processNameOf and processes logic
when adding the duplicate-checking step so the validation happens before
listToAttrs is invoked.
1 participant
This PR adds a new
mkWatchManyfunction that enables you to instantiate multiple watchexec processes at the same time:Summary by CodeRabbit
New Features
Documentation
Chores
Bug Fixes / UX
Configuration