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:
📝 WalkthroughWalkthroughAdds optional Sentry crash reporting (build-time and runtime), a debug output artifact and conditional CI upload step, a script to fetch and upload ELF debug symbols to Sentry, a test crash command and functional test, SIGSEGV handler preservation, and filtering to hide undocumented commands from completion and manpage generation. Changes
Sequence Diagram(s)sequenceDiagram
participant CI as CI Job
participant Build as build.yml workflow
participant Script as upload-debug-info-to-sentry.py
participant Cache as cache.nixos.org
participant NAR as nix nar
participant Sentry as sentry-cli
CI->>Build: trigger build with optional Sentry secrets
Build->>Build: build `debug` output
Build->>Script: run upload step (executable + debug dirs) [conditional: linux && secret present]
Script->>Script: run ldd / readelf -> collect build IDs
Script->>Cache: GET /debuginfo/<build_id>
alt 200 OK
Cache-->>Script: JSON with archive member
Script->>NAR: download NAR & extract member
NAR-->>Script: writes /tmp/debug-info/<build_id>.debug
else 404
Cache-->>Script: 404 (skip)
end
Script->>Sentry: sentry-cli debug-files upload --project? <.debug files>
Sentry-->>Script: upload result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
e306227 to
088b068
Compare
338ef39 to
1d0df75
Compare
1d0df75 to
963a0d5
Compare
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/nix/meson.build (1)
84-85:⚠️ Potential issue | 🟡 MinorDuplicate source file entry.
flake-prefetch-inputs.ccis listed twice on lines 84 and 85. This should be deduplicated.🧹 Proposed fix
'flake-prefetch-inputs.cc', - 'flake-prefetch-inputs.cc', 'flake.cc',🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/nix/meson.build` around lines 84 - 85, The source list in meson.build contains a duplicate entry 'flake-prefetch-inputs.cc'; remove the redundant occurrence so the file appears only once in the sources array (update the sources array where 'flake-prefetch-inputs.cc' is listed twice to a single entry), and run a quick build or lint to confirm no other duplicates remain.
🧹 Nitpick comments (2)
src/nix/meson.build (1)
197-197: Consider using Meson'sdependency()for sentry.Using
-lsentrydirectly inlink_argsworks but is less idiomatic than using Meson'sdependency()function which provides better cross-platform handling and pkg-config integration. However, ifsentry-nativedoesn't provide a reliable pkg-config file, this approach is acceptable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/nix/meson.build` at line 197, Replace the hard-coded '-lsentry' in the link_args conditional with Meson's dependency() so Meson can resolve sentry cross-platform; specifically, create a dependency object (e.g., sentry_dep = dependency('sentry' or 'sentry-native', required: sentry_required.enabled())) and when enabled append sentry_dep's link arguments or library to the targets (use sentry_dep.get_link_args() or add the dependency object to link_with/link_args) instead of the literal '-lsentry'; keep the current literal fallback only if sentry-native does not provide a pkg-config file.maintainers/upload-debug-info-to-sentry.py (1)
123-130: Consider validating JSON response keys for clearer error messages.If the debuginfo JSON response is missing
memberorarchivekeys, a rawKeyErrorwill be raised with no context about which library or build ID caused the failure.💡 Suggested improvement for error clarity
debuginfo = fetch_debuginfo(build_id) if debuginfo is None: print(f" {lib} ({build_id}, no debug info in cache)", file=sys.stderr) continue + if "member" not in debuginfo or "archive" not in debuginfo: + print(f" {lib} ({build_id}): malformed debuginfo response", file=sys.stderr) + continue print(f" {lib} ({build_id}): member={debuginfo['member']}", file=sys.stderr) nar_path = download_nar(build_id, debuginfo["archive"])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@maintainers/upload-debug-info-to-sentry.py` around lines 123 - 130, The code assumes debuginfo dict contains "member" and "archive" but will raise an unhelpful KeyError; update the block after fetch_debuginfo(build_id) to explicitly check for required keys ("member" and "archive") on debuginfo and, if missing, log or print a descriptive error including lib and build_id and then continue (do not call download_nar or extract_debug_symbols). Use the debuginfo variable and functions download_nar and extract_debug_symbols in the message so it's clear which library/build failed and why before appending to debug_files.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/build.yml:
- Around line 62-67: The Sentry upload step (the run invoking
./maintainers/upload-debug-info-to-sentry.py with env vars SENTRY_AUTH_TOKEN,
SENTRY_ORG, SENTRY_PROJECT) should be guarded by a GitHub Actions conditional so
it only runs when those secrets are populated; add an if: expression to that
job/step such as checking each secret is non-empty (e.g. if: ${{
secrets.sentry_auth_token != '' && secrets.sentry_org != '' &&
secrets.sentry_project != '' }} ) so the upload is skipped for forks/PRs without
secrets.
In `@doc/manual/generate-manpage.nix`:
- Around line 79-83: The code currently filters out category id 103 only inside
the showCommand() mapping, but processCommand(), tableOfContents, and other
consumers still iterate over details.commands causing undocumented commands
(e.g., nix __crash) to be rendered; fix this by creating a single filtered
attrset (e.g., filteredCommands = builtins.filter (cmd: cmd.category.id != 103)
(attrValues details.commands) or an attrset variant) and use filteredCommands
everywhere—replace uses of details.commands in processCommand(),
tableOfContents, and showCommand() with this new filteredCommands so the same
set is reused for both subcommand rendering and page generation.
In `@maintainers/upload-debug-info-to-sentry.py`:
- Around line 63-73: The code opens nar_path and passes it to subprocess.Popen
for xz without closing the file and doesn't check xz's exit status; update the
block around xz = subprocess.Popen(...) and xz.wait() to use a context manager:
open nar_path with "with open(nar_path, 'rb') as nar_f:" and pass nar_f as stdin
to subprocess.Popen (xz), then run subprocess.run for ["nix", "nar", "cat",
"/dev/stdin", member] with stdin=xz.stdout and check=True, ensure you close
xz.stdout (e.g., call xz.stdout.close()) and then call xz.wait() and verify
xz.returncode (or use check to raise on non-zero) before writing nar_cat.stdout
to out_path using a with open(out_path, "wb") as f: block; reference symbols:
xz, subprocess.Popen, open(nar_path, "rb"), nar_cat, xz.wait, out_path.
In `@nix-meson-build-support/common/assert-fail/meson.build`:
- Around line 27-32: The conditional block currently uses Meson comment syntax
"#if"/"#endif" so it never runs; change it to a real Meson conditional using if
can_wrap_assert_fail ... endif and restore the declare_dependency call so that
deps_other is appended with sources: 'wrap-assert-fail.cc' and link_args:
wrap_assert_fail_args when can_wrap_assert_fail is true (refer to the
can_wrap_assert_fail variable, deps_other list, and
wrap_assert_fail_args/wrap-assert-fail.cc symbols).
In `@src/libmain/unix/stack.cc`:
- Around line 13-14: The code currently overwrites savedSigsegvAction each time
detectStackOverflow() installs sigsegvHandler; modify the install logic in
detectStackOverflow() so it preserves the first external SIGSEGV action instead
of overwriting it on subsequent calls: only populate savedSigsegvAction when it
has not been initialized (or when its handler/sa_sigaction is not already
sigsegvHandler), or add a boolean flag like savedSigsegvActionInitialized to
guard the save; leave the existing restore logic that uses savedSigsegvAction
unchanged so restore still chains to the original handler.
In `@src/nix/crash.cc`:
- Line 1: The file uses std::bitset<4> (referenced as std::bitset<4>) but does
not explicitly include <bitset>; add an explicit `#include` <bitset> near the
other includes (e.g., alongside the existing `#include` "nix/cmd/command.hh") so
the use of std::bitset<4> is correct and not dependent on transitive includes.
In `@src/nix/main.cc`:
- Around line 384-403: The Finally cleanup currently always calls
sentry_shutdown() even if sentry_init() was skipped; modify the logic so the
cleanup callback is only registered when sentryEnabled is true (or make the
callback check sentryEnabled before calling sentry_shutdown()). Specifically,
update the block around sentry_init/sentryEnabled and the Finally
cleanupSentry([]() { sentry_shutdown(); }); usage so sentry_shutdown() is not
invoked unless sentry_init() actually ran (refer to sentryEnabled, sentry_init,
sentry_shutdown, and cleanupSentry).
In `@tests/functional/meson.build`:
- Line 179: The test enabling production Sentry is being added to the always-on
suite via inclusion of sentry.sh; either remove sentry.sh from the default suite
or change the test to use a test-only DSN/offline transport so it cannot
initialize the production Sentry from main. Fix options: (1) in the meson.build
test list, remove sentry.sh from the always-on suite so it runs only in an
explicit Sentry test suite; or (2) modify sentry.sh to preserve the
NIX_DISABLE_SENTRY guard or set a test SENTRY_DSN/offline transport before
unsetting NIX_DISABLE_SENTRY (e.g., export SENTRY_DSN=a_dummy_value or enable
offline transport) so src/nix/main.cc will not send real events; reference
sentry.sh, NIX_DISABLE_SENTRY, and src/nix/main.cc to locate and apply the
change.
---
Outside diff comments:
In `@src/nix/meson.build`:
- Around line 84-85: The source list in meson.build contains a duplicate entry
'flake-prefetch-inputs.cc'; remove the redundant occurrence so the file appears
only once in the sources array (update the sources array where
'flake-prefetch-inputs.cc' is listed twice to a single entry), and run a quick
build or lint to confirm no other duplicates remain.
---
Nitpick comments:
In `@maintainers/upload-debug-info-to-sentry.py`:
- Around line 123-130: The code assumes debuginfo dict contains "member" and
"archive" but will raise an unhelpful KeyError; update the block after
fetch_debuginfo(build_id) to explicitly check for required keys ("member" and
"archive") on debuginfo and, if missing, log or print a descriptive error
including lib and build_id and then continue (do not call download_nar or
extract_debug_symbols). Use the debuginfo variable and functions download_nar
and extract_debug_symbols in the message so it's clear which library/build
failed and why before appending to debug_files.
In `@src/nix/meson.build`:
- Line 197: Replace the hard-coded '-lsentry' in the link_args conditional with
Meson's dependency() so Meson can resolve sentry cross-platform; specifically,
create a dependency object (e.g., sentry_dep = dependency('sentry' or
'sentry-native', required: sentry_required.enabled())) and when enabled append
sentry_dep's link arguments or library to the targets (use
sentry_dep.get_link_args() or add the dependency object to link_with/link_args)
instead of the literal '-lsentry'; keep the current literal fallback only if
sentry-native does not provide a pkg-config file.
🪄 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: 0a84855e-60b7-4ec6-8345-b834ac644486
📒 Files selected for processing (18)
.github/workflows/build.yml.github/workflows/ci.ymldoc/manual/generate-manpage.nixmaintainers/upload-debug-info-to-sentry.pynix-meson-build-support/common/assert-fail/meson.buildpackaging/dependencies.nixpackaging/everything.nixsrc/libcmd/include/nix/cmd/command.hhsrc/libmain/unix/stack.ccsrc/libutil/args.ccsrc/nix/crash.ccsrc/nix/main.ccsrc/nix/meson.buildsrc/nix/meson.optionssrc/nix/package.nixtests/functional/common/init.shtests/functional/meson.buildtests/functional/sentry.sh
| unique ( | ||
| map (cmd: { inherit (cmd.category) id description; }) ( | ||
| builtins.filter (cmd: cmd.category.id != 103) (attrValues details.commands) | ||
| ) | ||
| ) |
There was a problem hiding this comment.
Filter undocumented commands out of the command set, not just the category list.
Line 81 only removes category 103 from the grouping logic inside showCommand(). processCommand() later still recurses over every entry in details.commands, and tableOfContents still renders every generated page, so nix __crash will still get its own manpage and SUMMARY.md entry. Please compute a filtered command attrset once and reuse it for both subcommand rendering and page generation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@doc/manual/generate-manpage.nix` around lines 79 - 83, The code currently
filters out category id 103 only inside the showCommand() mapping, but
processCommand(), tableOfContents, and other consumers still iterate over
details.commands causing undocumented commands (e.g., nix __crash) to be
rendered; fix this by creating a single filtered attrset (e.g., filteredCommands
= builtins.filter (cmd: cmd.category.id != 103) (attrValues details.commands) or
an attrset variant) and use filteredCommands everywhere—replace uses of
details.commands in processCommand(), tableOfContents, and showCommand() with
this new filteredCommands so the same set is reused for both subcommand
rendering and page generation.
| #if can_wrap_assert_fail | ||
| # deps_other += declare_dependency( | ||
| # sources : 'wrap-assert-fail.cc', | ||
| # link_args : wrap_assert_fail_args, | ||
| # ) | ||
| #endif |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== assert-fail block ==\n'
sed -n '21,32p' nix-meson-build-support/common/assert-fail/meson.build
printf '\n== active dependency patterns in parallel Meson files ==\n'
sed -n '76,86p' nix-meson-build-support/common/cxa-throw/meson.build
sed -n '1,20p' nix-meson-build-support/common/asan-options/meson.build
printf '\n== active declare_dependency lines in assert-fail ==\n'
rg -n --fixed-strings "deps_other += declare_dependency" nix-meson-build-support/common/assert-fail/meson.build || true
printf '\n== wrap implementation source ==\n'
fd -a wrap-assert-fail.ccRepository: DeterminateSystems/nix-src
Length of output: 1106
Restore a real Meson if block here.
Lines 27–32 use #if and #endif, which are comments in Meson—not conditionals. This block never executes, so wrap-assert-fail.cc is never added to deps_other even when can_wrap_assert_fail is true. This silently disables the assert-fail interposer.
Meson uses if/endif syntax (without the #). Parallel files like nix-meson-build-support/common/asan-options/meson.build correctly use active conditionals: if 'address' in get_option('b_sanitize') ... deps_other += declare_dependency(...) ... endif.
Suggested fix
-#if can_wrap_assert_fail
-# deps_other += declare_dependency(
-# sources : 'wrap-assert-fail.cc',
-# link_args : wrap_assert_fail_args,
-# )
-#endif
+if can_wrap_assert_fail
+ deps_other += declare_dependency(
+ sources : 'wrap-assert-fail.cc',
+ link_args : wrap_assert_fail_args,
+ )
+endif📝 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.
| #if can_wrap_assert_fail | |
| # deps_other += declare_dependency( | |
| # sources : 'wrap-assert-fail.cc', | |
| # link_args : wrap_assert_fail_args, | |
| # ) | |
| #endif | |
| if can_wrap_assert_fail | |
| deps_other += declare_dependency( | |
| sources : 'wrap-assert-fail.cc', | |
| link_args : wrap_assert_fail_args, | |
| ) | |
| endif |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@nix-meson-build-support/common/assert-fail/meson.build` around lines 27 - 32,
The conditional block currently uses Meson comment syntax "#if"/"#endif" so it
never runs; change it to a real Meson conditional using if can_wrap_assert_fail
... endif and restore the declare_dependency call so that deps_other is appended
with sources: 'wrap-assert-fail.cc' and link_args: wrap_assert_fail_args when
can_wrap_assert_fail is true (refer to the can_wrap_assert_fail variable,
deps_other list, and wrap_assert_fail_args/wrap-assert-fail.cc symbols).
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 @.github/workflows/build.yml:
- Around line 62-69: The step-level if currently uses the secrets context (if:
contains(inputs.system, 'linux') && secrets.sentry_auth_token != ''), which is
invalid; update the "Upload debug info to Sentry" step so the step-level
condition only uses allowed contexts (e.g., contains(inputs.system, 'linux')),
and move the secrets check into an environment or shell check: either set
SENTRY_AUTH_TOKEN in env (or at job-level) and use if: contains(inputs.system,
'linux') && env.SENTRY_AUTH_TOKEN != '' or keep the step conditional only on the
platform and inside the run command for
./maintainers/upload-debug-info-to-sentry.py add a shell guard that exits early
if $SENTRY_AUTH_TOKEN is empty; ensure SENTRY_AUTH_TOKEN, SENTRY_ORG and
SENTRY_PROJECT are supplied via env rather than referenced in the step-level if.
🪄 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: 8603603a-d2b0-46c9-88e0-3b1a8299df84
📒 Files selected for processing (1)
.github/workflows/build.yml
This ensures the sentry signal handler is called.
This ensures that meson flags defined in src/nix/package.nix are respected in the dev shell.
f6a2e2f to
9e2138b
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/nix/main.cc (1)
391-403:⚠️ Potential issue | 🟠 MajorGate Sentry cleanup on successful initialization.
Line 400 sets
sentryEnabled = trueunconditionally, butsentry_init()returns0on success. If init fails, Line 406 suppressesregisterCrashHandler(), and Line 403 still invokes deprecatedsentry_shutdown()during teardown. Only mark Sentry active after a successful init, and usesentry_close()for cleanup instead. (raw.githubusercontent.com)Suggested fix
- sentry_init(options); - sentryEnabled = true; + sentryEnabled = sentry_init(options) == 0; } - Finally cleanupSentry([]() { sentry_shutdown(); }); + Finally cleanupSentry([&]() { + if (sentryEnabled) + sentry_close(); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/nix/main.cc` around lines 391 - 403, sentryEnabled is being set unconditionally even if sentry_init fails and cleanupSentry always calls sentry_shutdown; change the logic so you call sentry_init(options) and check its return value (sentry_init returns 0 on success) before setting sentryEnabled and registering crash handling; also modify the cleanup passed to cleanupSentry to call sentry_close() (the current sentry_shutdown() is deprecated) only when sentryEnabled was set after a successful sentry_init. Ensure the checks reference sentry_init, sentryEnabled, cleanupSentry, sentry_shutdown, and sentry_close in the updated flow.
🧹 Nitpick comments (1)
src/nix/meson.build (1)
53-61: Model Sentry as a Meson dependency instead of raw-lsentry.Right now the
sentryfeature is reduced toenabled()plus a linker flag, soautonever attempts discovery and the target never gets a dependency object to carry the include/link metadata needed for<sentry.h>. A properdependency()/find_library()flow would preserve the feature semantics and propagate the compiler/link flags automatically. (mesonbuild.com)Also applies to: 205-205
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/nix/meson.build` around lines 53 - 61, The current code treats `sentry` as a plain option and injects `-lsentry` instead of using Meson's dependency resolution; change the flow to call dependency('sentry', required: sentry_required == 'enabled' or use find_library('sentry', required: ...)/dependency() so you get a dependency object (e.g., `sentry_dep`) and use `sentry_dep.found()` to set `HAVE_SENTRY` via `configdata.set('HAVE_SENTRY', sentry_dep.found().to_int())` and propagate include/link metadata from that object instead of raw flags; keep the crashpad-handler assertion and `configdata.set_quoted('CRASHPAD_HANDLER_PATH', crashpad_handler_path)` as before but ensure any target uses `sentry_dep` in its dependencies to carry compiler/link flags.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/nix/main.cc`:
- Around line 391-403: sentryEnabled is being set unconditionally even if
sentry_init fails and cleanupSentry always calls sentry_shutdown; change the
logic so you call sentry_init(options) and check its return value (sentry_init
returns 0 on success) before setting sentryEnabled and registering crash
handling; also modify the cleanup passed to cleanupSentry to call sentry_close()
(the current sentry_shutdown() is deprecated) only when sentryEnabled was set
after a successful sentry_init. Ensure the checks reference sentry_init,
sentryEnabled, cleanupSentry, sentry_shutdown, and sentry_close in the updated
flow.
---
Nitpick comments:
In `@src/nix/meson.build`:
- Around line 53-61: The current code treats `sentry` as a plain option and
injects `-lsentry` instead of using Meson's dependency resolution; change the
flow to call dependency('sentry', required: sentry_required == 'enabled' or use
find_library('sentry', required: ...)/dependency() so you get a dependency
object (e.g., `sentry_dep`) and use `sentry_dep.found()` to set `HAVE_SENTRY`
via `configdata.set('HAVE_SENTRY', sentry_dep.found().to_int())` and propagate
include/link metadata from that object instead of raw flags; keep the
crashpad-handler assertion and `configdata.set_quoted('CRASHPAD_HANDLER_PATH',
crashpad_handler_path)` as before but ensure any target uses `sentry_dep` in its
dependencies to carry compiler/link flags.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4551d378-2750-4ee1-af58-40d9e28eefbc
📒 Files selected for processing (10)
packaging/dependencies.nixpackaging/dev-shell.nixpackaging/sentry-native.nixsrc/nix/crash.ccsrc/nix/main.ccsrc/nix/meson.buildsrc/nix/meson.optionssrc/nix/package.nixtests/functional/common/init.shtests/functional/sentry.sh
✅ Files skipped from review due to trivial changes (2)
- src/nix/meson.options
- packaging/sentry-native.nix
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/functional/common/init.sh
- tests/functional/sentry.sh
- src/nix/package.nix
- packaging/dependencies.nix
Also respect the DETSYS_IDS_TELEMETRY environment variable.
| mandir = fs.is_absolute(mandir) ? mandir : prefix / mandir | ||
| configdata.set_quoted('NIX_MAN_DIR', mandir) | ||
|
|
||
| sentry_required = get_option('sentry') |
There was a problem hiding this comment.
nit: why sentry_required vs sentry?
There was a problem hiding this comment.
In Meson, sentry would be the result of calling dependency('sentry').
| link_args : wrap_assert_fail_args, | ||
| ) | ||
| endif | ||
| #if can_wrap_assert_fail |
There was a problem hiding this comment.
Does this not play well with sentry, or are we not wrapping it so sentry can handle it?
Motivation
Context
Summary by CodeRabbit
New Features
Tests
Documentation
Packaging / CI