Improve Nix flake app build flag support#1221
Improve Nix flake app build flag support#1221Arctis-Fireblight merged 2 commits intoRedot-Engine:masterfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR refactors the Nix build and execution workflow by consolidating Nix usage instructions from README.md into a dedicated doc/nix.md guide. Simultaneously, flake.nix is enhanced with a dynamic wrapper script that handles SCons build flag parsing, runtime argument forwarding, binary discovery, and conditional building. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 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)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Tip You can generate walkthrough in a markdown collapsible section to save space.Enable the |
MichaelFisher1997
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
flake.nix (1)
173-173: Consider potential duplicateplatform=argument.If a user explicitly passes
platform=...in their SCons args, the invocation will have twoplatform=values sinceplatform=${platform}is hardcoded first. SCons uses the last value, so this works correctly, but it may cause minor confusion in the build output.This is a very low-priority observation—no action needed unless you want cleaner output.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@flake.nix` at line 173, The scons invocation hardcodes platform=${platform} then appends "''${scons_args[@]}", which can duplicate a user-supplied platform= arg; update the call site so it either (a) conditionally includes platform=${platform} only when no element of scons_args starts with "platform=" or (b) filter scons_args to remove any "platform=" entries before appending, referencing the scons invocation line and the scons_args array to implement the conditional check/filter and then invoke scons without producing duplicate platform= arguments.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@flake.nix`:
- Line 173: The scons invocation hardcodes platform=${platform} then appends
"''${scons_args[@]}", which can duplicate a user-supplied platform= arg; update
the call site so it either (a) conditionally includes platform=${platform} only
when no element of scons_args starts with "platform=" or (b) filter scons_args
to remove any "platform=" entries before appending, referencing the scons
invocation line and the scons_args array to implement the conditional
check/filter and then invoke scons without producing duplicate platform=
arguments.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 26a8e633-4fbf-40e6-b590-164036c23a88
📒 Files selected for processing (2)
README.mdflake.nix
|
I think it would be best to move the extra documentation to it's own .md file in /docs similar to the ai-integration.md file. This cleans up the README to only include necessary things. |
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 `@docs/nix.md`:
- Line 38: Update the sentence to clarify that only a subset of forwarded SCons
flags affect the binary-matching/reuse logic: explicitly list the keys used by
the flake (target, arch, dev_build, precision, threads, extra_suffix) and state
that other flags (e.g., production=yes, module_mono_enabled=yes, custom ccflags)
are passed to scons but do not influence the wrapper’s reuse check, so an
existing binary may be reused unless removed and rebuilt; place this note
immediately after the supported-flags list and alongside the rebuild caveat
mentioned on lines ~61-62.
- Around line 54-56: The example binary name in docs/nix.md only shows the base
form redot.<platform>.editor.<arch> but must reflect the wrapper's optional
suffixes; update the text and examples to show the full naming scheme using the
base name plus optional components .dev, .double, .nothreads and any
extra_suffix (e.g., include at least one example for a dev build, a
double-precision build, and a no-threads build), and mention that these suffixes
are inserted by the wrapper so users with dev_build=yes, precision=double, or
threads=no can find the correct filename.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 304996af-5097-433e-bae2-1d65cd9110e4
📒 Files selected for processing (2)
README.mddocs/nix.md
🚧 Files skipped from review as they are similar to previous changes (1)
- README.md
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 `@README.md`:
- Line 64: Update the README link text and target: replace the non-descriptive
"[here](doc/nix.md)" with a descriptive anchor like "Nix usage guide" and point
it to the correct path "docs/nix.md" so the README links to the actual file;
look for the link in README.md (the sentence referencing Nix usage and SCons
flags) and update both the display text and the href accordingly.
README.md
Outdated
| ``` | ||
|
|
||
| Nix works on Linux and macOS, and is available at [nixos.org/download.html](https://nixos.org/download.html). The `nix run .` command automatically detects your platform and architecture. | ||
| Detailed Nix usage, including passing SCons build flags through `nix run`, forwarding runtime arguments, and manual `nix develop` workflows, is documented [here](doc/nix.md). |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd '^nix\.md$' .
rg -n 'doc[s]?/nix\.md|Detailed Nix usage' README.mdRepository: Redot-Engine/redot-engine
Length of output: 260
Fix broken link and use descriptive link text.
Line 64 links to doc/nix.md, but the file is actually at docs/nix.md. Additionally, [here](...) is non-descriptive anchor text.
Suggested fix
-Detailed Nix usage, including passing SCons build flags through `nix run`, forwarding runtime arguments, and manual `nix develop` workflows, is documented [here](doc/nix.md).
+Detailed Nix usage, including passing SCons build flags through `nix run`, forwarding runtime arguments, and manual `nix develop` workflows, is documented in the [Nix workflow guide](docs/nix.md).📝 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.
| Detailed Nix usage, including passing SCons build flags through `nix run`, forwarding runtime arguments, and manual `nix develop` workflows, is documented [here](doc/nix.md). | |
| Detailed Nix usage, including passing SCons build flags through `nix run`, forwarding runtime arguments, and manual `nix develop` workflows, is documented in the [Nix workflow guide](docs/nix.md). |
🧰 Tools
🪛 markdownlint-cli2 (0.21.0)
[warning] 64-64: Link text should be descriptive
(MD059, descriptive-link-text)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` at line 64, Update the README link text and target: replace the
non-descriptive "[here](doc/nix.md)" with a descriptive anchor like "Nix usage
guide" and point it to the correct path "docs/nix.md" so the README links to the
actual file; look for the link in README.md (the sentence referencing Nix usage
and SCons flags) and update both the display text and the href accordingly.
There was a problem hiding this comment.
@coderabbitai doc is the actual name, docs was a typo on @MichaelFisher1997 's part, the actual directory in the engine is doc
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
@coderabbitai doc is the actual name, docs was a typo on MichaelFisher1997's part, the actual directory in the engine is doc
There was a problem hiding this comment.
@JoltedJon, thank you for the clarification! You're right — the link path doc/nix.md in the PR is correct, and my suggestion to change it to docs/nix.md was mistaken. I apologize for the confusion.
The only remaining valid improvement is updating the non-descriptive [here] anchor text:
-Detailed Nix usage, including passing SCons build flags through `nix run`, forwarding runtime arguments, and manual `nix develop` workflows, is documented [here](doc/nix.md).
+Detailed Nix usage, including passing SCons build flags through `nix run`, forwarding runtime arguments, and manual `nix develop` workflows, is documented in the [Nix workflow guide](doc/nix.md).✏️ Learnings added
Learnt from: JoltedJon
Repo: Redot-Engine/redot-engine PR: 1221
File: README.md:64-64
Timestamp: 2026-03-09T03:31:48.920Z
Learning: In the Redot Engine repository (Redot-Engine/redot-engine), the documentation directory is named `doc` (not `docs`). For example, the Nix workflow documentation lives at `doc/nix.md`.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
|
@MichaelFisher1997 By the way we're no longer using dev branch. Apparently it was causing too many issues with developers targeting master. Could you please change this PR to target master? Just realized you were targeting dev |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/nix.md`:
- Around line 1-64: The new documentation page nix.md was added under the wrong
docs tree; move the file named nix.md into the repository's doc documentation
directory (replace the docs/ placement) and update any internal links,
TOC/website index entries, and CI/doc build references to point to the new
doc/nix.md location so doc discovery and links remain consistent; ensure the
top-level header "Nix workflow" and examples remain unchanged.
In `@flake.nix`:
- Around line 108-110: The case branch that sets target_arch from arg copies
"arch=auto" literally into target_arch, causing binary_pattern to search for
"...auto" instead of the resolved host architecture; update the parsing logic in
the case handling of "arch=*" to detect if the parsed value equals "auto" and,
if so, replace it with the resolved host architecture (use the existing
host-arch detection or a uname-based resolution), otherwise use the provided
arch; apply the same normalization in the identical parsing block around the
other occurrence (the one referenced at lines ~158-159) so binary_pattern is
built with the actual architecture rather than the literal "auto".
- Around line 168-189: The wrapper currently uses a glob
matches=(./bin/"$binary_pattern"*) which overmatches (e.g. threads=yes matches
nothreads) and can pick the wrong build; change the lookup in the
matches/binary_path logic to require an exact expected filename instead of a
trailing * glob: construct the full expected binary filename from binary_pattern
(including any configured suffix/extra_suffix) and test that exact path exists
(instead of populating matches with a wildcard), then use that exact path as
binary_path and exec it with runtime_args; update the scons fallback to rebuild
when that exact path is missing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b510453a-f97d-435b-b80b-aa89158c4f9f
📒 Files selected for processing (3)
README.mddocs/nix.mdflake.nix
🚧 Files skipped from review as they are similar to previous changes (1)
- README.md
|
@MichaelFisher1997 Sorry I hate to make you do more changes especially since it was kind of my fault. You created a directory called docs when there was already a directory doc for this purpose |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
flake.nix (1)
180-186:⚠️ Potential issue | 🟡 MinorAdd error handling to scons invocation to preserve build failure diagnostics.
At Line 182, scons can fail without stopping execution, causing the generic error message at Line 186 to mask the actual build failure. If scons exits non-zero, the script should propagate that error instead of reaching the "Could not find" message.
Additionally, using
-x(executable check) instead of-f(file exists check) at Lines 180 and 185 is more precise since the binary must be executable, and quoting$scons_platformat Line 182 is defensive against edge cases.Suggested patch
- if [ ! -f "$binary_path" ]; then + if [ ! -x "$binary_path" ]; then echo "Building Redot..." - scons "''${scons_args[@]}" platform=$scons_platform + scons "''${scons_args[@]}" platform="$scons_platform" || exit $? fi - if [ ! -f "$binary_path" ]; then + if [ ! -x "$binary_path" ]; then echo "Could not find a built Redot binary at $binary_path" >&2 exit 1 fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@flake.nix` around lines 180 - 186, The script currently uses -f to test "$binary_path" and calls scons without propagating failures; change the file-existence checks to executable checks (-x) for "$binary_path" and invoke scons with a quoted platform argument ("$scons_platform") and the same scons_args, then immediately test scons' exit status and exit the script with that status on failure (e.g., capture $? or use || exit $?): this ensures build failures from the scons invocation are preserved and the final "Could not find" message only runs when the binary is truly missing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@flake.nix`:
- Around line 180-186: The script currently uses -f to test "$binary_path" and
calls scons without propagating failures; change the file-existence checks to
executable checks (-x) for "$binary_path" and invoke scons with a quoted
platform argument ("$scons_platform") and the same scons_args, then immediately
test scons' exit status and exit the script with that status on failure (e.g.,
capture $? or use || exit $?): this ensures build failures from the scons
invocation are preserved and the final "Could not find" message only runs when
the binary is truly missing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8271f8da-5217-4cce-9979-0763141bddb0
📒 Files selected for processing (3)
README.mddoc/nix.mdflake.nix
🚧 Files skipped from review as they are similar to previous changes (1)
- README.md
|
@JoltedJon its ok, i recall seeing that folder before... its my fault for trying to do stuff at 4am without thinking or checking what i was doing. moved the guide to doc/nix.md to match the repo layout, updated the README link, and folded in the wrapper follow-ups for arch=auto and exact binary matching as well. |
3 participants
Problem
The current Nix entrypoint is convenient for
nix run ., but it only builds with the defaultscons platform=...invocation.That means users cannot pass normal SCons build flags through the flake app, so workflows like
dev_build=yes,production=yes,target=template_release, or module toggles require dropping down tonix developand runningsconsmanually.There were also two follow-on issues in the wrapper:
dev_build=yesproduced a valid binary that the wrapper then failed to launchgcc-ar, which was not available in the wrapper environmentWhat This PR Changes
nix run . -- ...to pass SCons build flags into the flake wrappersconsand arguments after an internal--still reach the built Redot binarytarget,dev_build,precision,threads, andextra_suffix--helpoutput so the Nix syntax is discoverable from the CLIREADME.md, including build-flag examples, runtime-arg forwarding, and rebuild caveatsResult
Nix users can keep using the simple flake entrypoint while still accessing the build customization that Redot already exposes through SCons.
Examples supported by this change:
nix run . -- target=editor dev_build=yes num_jobs=12nix run . -- target=template_release production=yesnix run . -- target=editor dev_build=yes -- --path /tmp/projectTesting
nix flake show .nix run . -- --helpnix run . -- target=editor dev_build=yes num_jobs=12 -- --helpnix run . -- target=editor num_jobs=12 -- --helpnix run . -- target=template_release production=yes num_jobs=12 -- --helpSummary by CodeRabbit
Documentation
New Features