Skip to content

Set checks_level=0 on linux-meson-clang-tests build #5144

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
May 11, 2025

Conversation

kazarmy
Copy link
Member

@kazarmy kazarmy commented May 11, 2025

Your checklist for this pull request

  • I've read the guidelines for contributing to this repository.
  • I made sure to follow the project's coding style.
  • I've documented every RZ_API function and struct this PR changes.
  • I've added tests that prove my changes are effective (required for changes to RZ_API).
  • I've updated the Rizin book with the relevant information (if needed).

Detailed description

This pr sets checks_level to 0 on the linux-meson-clang-tests build. This is just to guard against side-effects in code being asserted by rz_assert.h macros. This kind of problem should be easy to fix, so it should be ok that the linux-meson-clang-tests build is not run on every pr.

Test plan

The change makes sense. The linux-meson-clang-tests build is green.

Closing issues

...

@kazarmy kazarmy marked this pull request as ready for review May 11, 2025 07:48
Copy link

codecov bot commented May 11, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 44.88%. Comparing base (d53c920) to head (f76978d).
Report is 1 commits behind head on dev.

Additional details and impacted files

see 7 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d53c920...f76978d. Read the comment docs.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@Rot127
Copy link
Member

Rot127 commented May 11, 2025

What were the side effects you encountered?
Because we would miss ASAN problems in the assert statements itself this way, right?

@kazarmy
Copy link
Member Author

kazarmy commented May 11, 2025

What were the side effects you encountered?

An example of problematic side-effect code is the original code in this diff ... checks_level=0 will simply remove the first argument of rz_return_val_if_fail() as if it didn't exist, and thus the conversion of RL78 operand to string does not happen, resulting in test failures like this:

RL78-test-failure
(https://github.com/rizinorg/rizin/actions/runs/14774758066/job/41480902215#step:18:15463)

This is not an assertion failure and thus checks_level=3 will not catch this.

Because we would miss ASAN problems in the assert statements itself this way, right?

The ASAN builds are still built with checks_level=2 so no removal of code will be done (and they probably will never be built with checks_level=0 since they are debugoptimized builds). This pr doesn't touch the ASAN builds.

Copy link
Member

@Rot127 Rot127 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This pr doesn't touch the ASAN builds.

Was on the go and misread it.
Thanks. All good.

@kazarmy kazarmy merged commit 9721f4f into rizinorg:dev May 11, 2025
43 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants