feat(check_leaks): make memory leak check mandatory to pass CI - #578
Conversation
Sorry, something went wrong.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 982bb8e4-0c47-4f64-9ea7-db1429f6c7dd 📥 CommitsReviewing files that changed from the base of the PR and between fab11f0 and acd43e0. 📒 Files selected for processing (1)
📝 Walkthrough WalkthroughThe XMake leak-check script now aborts by raising an error when an expected target executable is missing and raises an exception (failing the run) when any memory leaks are detected; behavior when no leaks are found is unchanged. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Suggested labelsci, test Poem🚥 Pre-merge checks | ✅ 4 | ❌ 1 ❌ Failed checks (1 warning)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches 📝 Generate docstrings
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. ❤️ ShareComment @coderabbitai help to get the list of available commands and usage tips. |
Sorry, something went wrong.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)tools/xmake/check_leaks.lua (1)11-14: ⚠️ Potential issue | 🔴 Critical
Mandatory leak gate is still bypassable on prerequisite failures.
If executables are missing or leaks is unavailable, the task currently prints and continues/returns, which can still end in a green CI result without performing the mandatory check.
💡 Proposed fix@@ - if not os.isfile(bin_path) then - print("Executable not found for target: " .. target_name) - goto continue - end + if not os.isfile(bin_path) then + table.insert(failing_targets, target_name .. " (executable not found)") + goto continue + end @@ local leaks_tool = find_program("leaks", { check = "--help" }) if not leaks_tool then - print("leaks tool not found!") - return + raise("leaks tool not found on macOS runner; cannot perform mandatory memory leak check.") endAlso applies to: 68-71
🤖 Prompt for AI AgentsVerify each finding against the current code and only fix it if needed. In `@tools/xmake/check_leaks.lua` around lines 11 - 14, The script currently prints and uses "goto continue" when os.isfile(bin_path) is false (and similarly around the availability of the leaks tool), which lets the mandatory leak gate be bypassed; change those paths in tools/xmake/check_leaks.lua so that when os.isfile(bin_path) or the leaks check fails you abort with a non-zero exit (e.g., call error(...) or os.exit(1)) instead of printing and continuing, and make the same change for the analogous checks around lines 68-71 so missing prerequisites cause the task to fail CI rather than silently pass.
tools/xmake/check_leaks.lua (1)🤖 Prompt for all review comments with AI agents9-9: Avoid hardcoded debug; expose task mode and resolve binary path from it.
Hardcoding "debug" makes the task fragile outside CI defaults and can silently miss binaries in other modes.
♻️ Proposed refactor@@ -local function check_leaks_macos(targets, leaks_tool, project, os, verbose) +local function check_leaks_macos(targets, leaks_tool, os, verbose, mode) @@ - local bin_path = path.join(os.projectdir(), target:targetdir(), "debug", target:filename()) + local bin_path = path.join(target:targetdir(), mode, target:filename()) @@ import("lib.detect.find_program") + local mode = option.get("mode") or "debug" @@ - failing_targets = check_leaks_macos(targets, leaks_tool, project, os, option.get("verbose")) + failing_targets = check_leaks_macos(targets, leaks_tool, os, option.get("verbose"), mode) @@ options = { + {nil, "mode", "kv", "debug", "Build mode to check (debug/release)"}, {nil, "targets", "vs", nil, "Targets to check for leaks (default: all test targets)"} } }Based on learnings: in tools/xmake/check_leaks.lua, task context should use option.get("mode") or "debug" and build the path as path.join(target:targetdir(), mode, target:filename()).
Also applies to: 34-39, 72-72, 90-92
🤖 Prompt for AI AgentsVerify each finding against the current code and only fix it if needed. In `@tools/xmake/check_leaks.lua` at line 9, Replace hardcoded "debug" mode when building binary paths by reading the task option mode = option.get("mode") or "debug" and using that variable in path.join; specifically update the bin_path construction (local bin_path = path.join(os.projectdir(), target:targetdir(), "debug", target:filename())) to use path.join(target:targetdir(), mode, target:filename()) (or include os.projectdir() only where appropriate), and apply the same change to the other path.join usages in this file that build binary paths so they all derive the directory from mode instead of the literal "debug"; ensure you declare mode once (e.g., local mode = option.get("mode") or "debug") and reference it in every affected expression.
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@tools/xmake/check_leaks.lua`:
- Around line 11-14: The script currently prints and uses "goto continue" when
os.isfile(bin_path) is false (and similarly around the availability of the leaks
tool), which lets the mandatory leak gate be bypassed; change those paths in
tools/xmake/check_leaks.lua so that when os.isfile(bin_path) or the leaks check
fails you abort with a non-zero exit (e.g., call error(...) or os.exit(1))
instead of printing and continuing, and make the same change for the analogous
checks around lines 68-71 so missing prerequisites cause the task to fail CI
rather than silently pass.
---
Nitpick comments:
In `@tools/xmake/check_leaks.lua`:
- Line 9: Replace hardcoded "debug" mode when building binary paths by reading
the task option mode = option.get("mode") or "debug" and using that variable in
path.join; specifically update the bin_path construction (local bin_path =
path.join(os.projectdir(), target:targetdir(), "debug", target:filename())) to
use path.join(target:targetdir(), mode, target:filename()) (or include
os.projectdir() only where appropriate), and apply the same change to the other
path.join usages in this file that build binary paths so they all derive the
directory from mode instead of the literal "debug"; ensure you declare mode once
(e.g., local mode = option.get("mode") or "debug") and reference it in every
affected expression.
ℹ️ Review info ⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e9f85f4a-89cc-469d-b9dd-e0dfac2022e3
📥 CommitsReviewing files that changed from the base of the PR and between 5019723 and fab11f0.
📒 Files selected for processing (1)- tools/xmake/check_leaks.lua
Sorry, something went wrong.
|
Sorry, something went wrong.
Pull Request
Description
Force memory leaks to be resolved in CI
Related Issues (Put "None" if there are no related issues)
close #566
Type of Change
Please delete options that are not relevant.
Changes Made
List the main changes in this PR:
Testing
Describe the tests you ran to verify your changes. Please delete options that are not relevant.
Test Environment
Screenshots/Videos (Put "None" if there are no related issues)
None
Documentation
Please delete options that are not relevant.
Checklist (Don't delete any options)
Breaking Changes (Put "None" if there are no related issues)
None
Additional Notes (Put "None" if there are no related issues)
None
Summary by CodeRabbit