-
Notifications
You must be signed in to change notification settings - Fork 714
Add support for disabling exceptions and improve CI #619
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
Add support for disabling exceptions and improve CI #619
Conversation
@michaelbautin Looks good! I have some concerns regarding the use of |
@dyashuni @yurymalkov : thanks for the review and for validating the approach! I will clean up the code / tests, restore the explicit types, and update the PR. |
d8ee83b
to
733751d
Compare
@yurymalkov , @dyashuni : I've updated the PR, addressed the review comments, added ctest support and ran the tests locally. Could you please trigger CI/CD again on this PR? |
194da62
to
137581c
Compare
8003319
to
619f6ca
Compare
@yurymalkov , @dyashuni : I think the PR is ready for both of you to take another look. The test matrix has been significantly enhanced and the tests are passing. |
f0047b4
to
aca90ff
Compare
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.
@michaelbautin Looks very good, added a couple of refactoring comments
d4ffad0
to
6d314a8
Compare
Adding a mode of error handling that does not rely on C++ exceptions, with Abseil-like Status and StatusOr return types (but not adding a dependency on Abseil). The old exceptions-based API is still available, and the new functions are suffixed with "NoExceptions". Refactoring the CMake build system. Organizing examples and tests and reducing repetition. Adding the HNSWLIB_ENABLE_EXCEPTIONS option (ON by default). Exceptions are also enabled when building Python bindings, and in a few examples/tests where the code currently relies on exception handling, even if HNSWLIB_ENABLE_EXCEPTIONS is set to OFF. Adding ctest support. The only tests not supported by ctest are test_updates invoked with and without the "update" argument, because that requires installing numpy and running a data generation script and doing that from a CMake test might be controversial. Those tests are still run as part of GitHub Actions CI. Enabling sanitizer support using ENABLE_ASAN / ENABLE_UBSAN / ENABLE_TSAN / ENABLE_MSAN CMake options. Adding a combined ASAN/UBSAN build type to the C++ test matrix in GitHub Actions in non-Windows builds. Also adding Clang compiler on Ubuntu to the build matrix. Making sure that assertions are enabled during tests. Using the RelWithDebInfo build type and customizing it to remove the NDEBUG flag. Also removing the default /EHsc flag when building with MSVC and instead enabling exceptions when needed. Fixing race conditions when accessing random number generators and distributions from multiple threads in multiThreadLoad_test. These race conditions were caught using ASAN/UBSAN.
6d314a8
to
7e2c1bb
Compare
@michaelbautin Good job! Thank you! |
Thanks @dyashuni @yurymalkov! Is there anything else to update, or could this be merged? |
We can merge @michaelbautin |
Adding support for disabling exceptions and improving CI
Adding a mode of error handling that does not rely on C++ exceptions,
with Abseil-like Status and StatusOr return types (but not adding a
dependency on Abseil). The old exceptions-based API is still available,
and the new functions are suffixed with "NoExceptions".
Refactoring the CMake build system. Organizing examples and tests and
reducing repetition. Adding the HNSWLIB_ENABLE_EXCEPTIONS option (ON by
default). Exceptions are also enabled when building Python bindings, and
in a few examples/tests where the code currently relies on exception
handling, even if HNSWLIB_ENABLE_EXCEPTIONS is set to OFF.
Adding ctest support. The only tests not supported by ctest are
test_updates invoked with and without the "update" argument, because
that requires installing numpy and running a data generation script and
doing that from a CMake test might be controversial. Those tests are
still run as part of GitHub Actions CI.
Enabling sanitizer support using ENABLE_ASAN / ENABLE_UBSAN /
ENABLE_TSAN / ENABLE_MSAN CMake options. Adding a combined ASAN/UBSAN
build type to the C++ test matrix in GitHub Actions in non-Windows
builds. Also adding Clang compiler on Ubuntu to the build matrix.
Making sure that assertions are enabled during tests. Using the
RelWithDebInfo build type and customizing it to remove the NDEBUG flag.
Also removing the default /EHsc flag when building with MSVC and instead
enabling exceptions when needed.
Fixing race conditions when accessing random number generators and
distributions from multiple threads in multiThreadLoad_test. These race
conditions were caught using ASAN/UBSAN.