-
Notifications
You must be signed in to change notification settings - Fork 483
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
ORC-1826: [C++][CI] Add ASAN support to CMake #2097
Conversation
.github/workflows/asan_test.yml
Outdated
jobs: | ||
asan-test: | ||
name: "ASAN with ${{ matrix.compiler }} on Ubuntu" | ||
runs-on: ubuntu-24.04 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a question; Is it a requirement?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean the ubuntu version? No, I don't think so.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the answer. Yes, I was wondering if ASAN doesn't work on ubuntu-22.04.
If this is working on ubuntu-22.04, we may want to use ubuntu-latest
instead of specific versions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW, I'm fine for the AS-IS PR too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1, LGTM with a minor question.
Happy New Year, @wgtmac . Shall we convert this to a normal PR in order to deliver this as a part of Apache ORC 2.1.0? |
Thanks for your suggestion! Now it is ready for review. Happy new year :) @dongjoon-hyun |
Thank you, @wgtmac . Merged to main for Apache ORC 2.1. cc @williamhyun |
What changes were proposed in this pull request?
Add a CMake option to enable ASan in the C++ build and enable it in the CI.
Why are the changes needed?
We are able to detect potential memory issues with the help of address sanitizer.
How was this patch tested?
Pass all CIs (including the new ASan CIs)
Was this patch authored or co-authored using generative AI tooling?
No.