Skip to content
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

Update helio #2538

Merged
merged 1 commit into from
Feb 6, 2024
Merged

Update helio #2538

merged 1 commit into from
Feb 6, 2024

Conversation

romange
Copy link
Collaborator

@romange romange commented Feb 4, 2024

Add support for asan/ubsan checkers in our dev environment.
Once we fix all the problems we will enable them in our CI as well.
Fix most of the clang warnings

@romange romange force-pushed the UpdateHelio branch 12 times, most recently from 71bf732 to 5bd2e70 Compare February 6, 2024 07:16
@romange romange marked this pull request as ready for review February 6, 2024 07:59
@romange romange force-pushed the UpdateHelio branch 3 times, most recently from 715971f to 46fca5e Compare February 6, 2024 08:32
Add support for asan/ubsan checkers in our dev environment.
Remove more clang warnings.

Once we fix all the problems we will enable them in our CI as well.

Signed-off-by: Roman Gershman <roman@dragonflydb.io>
@@ -1,11 +1,9 @@
option(DF_ENABLE_MEMORY_TRACKING "Adds memory tracking debugging via MEMORY TRACK command" ON)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

just reshuffling code here

Copy link
Contributor

@kostasrim kostasrim left a comment

Choose a reason for hiding this comment

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

I am especially happy for the unqualified -> qualified calls to move (We had a lot of warnings from these)

@@ -3268,7 +3268,7 @@ void ZSetFamily::Register(CommandRegistry* registry) {
<< CI{"ZREM", CO::FAST | CO::WRITE, -3, 1, 1, acl::kZRem}.HFUNC(ZRem)
<< CI{"ZRANGE", CO::READONLY, -4, 1, 1, acl::kZRange}.HFUNC(ZRange)
<< CI{"ZRANDMEMBER", CO::READONLY, -2, 1, 1, acl::kZRandMember}.HFUNC(ZRandMember)
<< CI{"ZRANK", CO::READONLY | CO::FAST, 3, 1, 1, acl::kZRange}.HFUNC(ZRank)
<< CI{"ZRANK", CO::READONLY | CO::FAST, 3, 1, 1, acl::kZRank}.HFUNC(ZRank)
Copy link
Contributor

Choose a reason for hiding this comment

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

nice catch!

@@ -94,9 +106,15 @@ jobs:
- name: Build
run: |
cd ${GITHUB_WORKSPACE}/build
ninja search_family_test
Copy link
Contributor

Choose a reason for hiding this comment

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

Why we need this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

it's an arbitrary target that we would build anyway provides a consistent target being built. helps if there is a failure like disk space which gets lost when we build multiple targets in parallel.

run: |
uname -a
cmake --version
mkdir -p ${GITHUB_WORKSPACE}/build

echo "===================Before freeing up space ============================================"
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this needed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

because -fsanitize eats up all the space on the runner and we fail building the repo

@romange romange merged commit 336d6ff into main Feb 6, 2024
10 checks passed
@romange romange deleted the UpdateHelio branch February 6, 2024 09:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants