-
Notifications
You must be signed in to change notification settings - Fork 127
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
Fixed jni test and compile warnings #940
Conversation
fdc561c
to
efa00a4
Compare
@luyuncheng Thanks for fixing the tests. This: GFORTRAN_LIBRARY, I am not sure what it is doing. Can you add some details why this is needed and what it is doing? |
@luyuncheng please look at this: https://github.com/opensearch-project/k-NN/blob/main/CONTRIBUTING.md, I see few things missing like Developer Certificate, and add your change in CHANGELOG.md file too. |
Codecov Report
@@ Coverage Diff @@
## main #940 +/- ##
=========================================
Coverage 85.08% 85.08%
Complexity 1112 1112
=========================================
Files 152 152
Lines 4519 4519
Branches 405 405
=========================================
Hits 3845 3845
Misses 489 489
Partials 185 185 |
@navneet1v i think this because
|
c93c3df
to
f3d0aa2
Compare
@luyuncheng is there any performance benefit of adding this? or this is just to resolve the compilation warnings? |
Signed-off-by: luyuncheng <[email protected]>
Signed-off-by: luyuncheng <[email protected]>
Signed-off-by: luyuncheng <[email protected]>
Signed-off-by: luyuncheng <[email protected]>
@navneet1v Resolve compilation errors it is because we install OpenBLAS with all package with
so we need link to |
CHANGELOG.md
Outdated
@@ -7,6 +7,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), | |||
### Features | |||
### Enhancements | |||
### Bug Fixes | |||
* Fixed jni test and compile error ([#940](https://github.com/opensearch-project/k-NN/pull/940)) |
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.
can we put this like Fixed jni test and compile warnings
, as if it would have been error the build should have faied.
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.
++ LGTM
Signed-off-by: luyuncheng <[email protected]>
@jmazanec15 can you take a look? |
@@ -132,6 +132,8 @@ if (${CONFIG_FAISS} STREQUAL ON OR ${CONFIG_ALL} STREQUAL ON OR ${CONFIG_TEST} S | |||
find_package(BLAS REQUIRED) | |||
enable_language(Fortran) | |||
find_package(LAPACK REQUIRED) | |||
find_library(GFORTRAN_LIBRARY NAMES libgfortran.so.3) |
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.
do we need the version number?
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.
libgfortran
has different version number for different GCC version.
like gcc 4.5.2
maps /usr/lib64/libgfortran.so.1
like gcc 6.2.0
gcc 5.x
maps /usr/lib64/libgfortran.so.3
AND there is no soft link from libgfortran.so
to libgfortran.so.3.0.0
So how about enum some version of libgfortran in find_library?
Description
when i try to compile and test jni with main branch. i see the following error:
i think this because
faiss
update to new branch,so i pr this for fix the compile error:
faiss::Index::idx_t ==> faiss::idx_t
cmake add GFORTRAN_LIBRARY
ISSUE: #941