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 dev guide to fix clang linking issue on arm #1746

Merged

Conversation

finnroblin
Copy link
Contributor

Description

The current DEVELOPER_GUIDE.md M-series instructions did not work for me or @MrFlap. These updated instructions use gcc/g++ instead of clang to build and link the external kNN libraries. The project builds for me (m3 chip) and Andrew (m2 chip).

Issues Resolved

Resolves linking issues with m-series macbook chips when building kNN locally.

Check List

  • New functionality includes testing.
    • All tests pass
  • New functionality has been documented.
    • New functionality has javadoc added
  • Commits are signed as per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Comment on lines 115 to 116
export CC=/opt/homebrew/opt/llvm/bin/clang
export CXX=/opt/homebrew/opt/llvm/bin/clang++
Copy link
Collaborator

@shatejas shatejas Jun 11, 2024

Choose a reason for hiding this comment

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

/opt/homebrew/opt/llvm/bin/clang works for me. Can we keep both? you can add a note saying change to

in case of error: [Add the error] change to
export CC=gcc
export CXX=g++

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure and let me ask Andrew what his error was -- I think they were different.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pasting entire error trace here so that people can find it in search. Both of us had linking errors with clang:

<after running make>
77%] Building CXX object CMakeFiles/opensearchknn_faiss.dir/src/faiss_wrapper.cpp.o
[ 79%] Building CXX object CMakeFiles/opensearchknn_faiss.dir/src/faiss_util.cpp.o
<FAILS HERE> [ 79%] Linking CXX shared library release/libopensearchknn_faiss.jnilib

Here's the trace:

Homebrew clang version 18.1.7
Target: arm64-apple-darwin23.5.0
Thread model: posix
InstalledDir: /opt/homebrew/opt/llvm/bin
 "/usr/bin/ld" -demangle -lto_library /opt/homebrew/Cellar/llvm/18.1.7/lib/libLTO.dylib -dynamic -dylib -arch arm64 -dylib_install_name @rpath/libopensearchknn_faiss.jnilib -platform_version macos 14.0.0 14.4 -syslibroot /Library/Developer/CommandLineTools/SDKs/MacOSX14.sdk -mllvm -enable-linkonceodr-outlining -o release/libopensearchknn_faiss.jnilib -headerpad_max_install_names CMakeFiles/opensearchknn_faiss.dir/src/org_opensearch_knn_jni_FaissService.cpp.o CMakeFiles/opensearchknn_faiss.dir/src/faiss_wrapper.cpp.o CMakeFiles/opensearchknn_faiss.dir/src/faiss_util.cpp.o -rpath /Users/finnrobl/Code/k-NN/jni/release external/faiss/faiss/libfaiss.a release/libopensearchknn_util.jnilib -framework Accelerate -lm -ldl /opt/homebrew/opt/llvm/lib/libomp.dylib -lc++ -lSystem /opt/homebrew/Cellar/llvm/18.1.7/lib/clang/18/lib/darwin/libclang_rt.osx.a
Undefined symbols for architecture arm64:
  "std::exception_ptr::__from_native_exception_pointer(void*)", referenced from:
      std::__1::promise<bool>::~promise() in libfaiss.a[77](WorkerThread.cpp.o)
  "___cxa_init_primary_exception", referenced from:
      std::__1::promise<bool>::~promise() in libfaiss.a[77](WorkerThread.cpp.o)
ld: symbol(s) not found for architecture arm64
clang++: error: linker command failed with exit code 1 (use -v to see invocation)
make[2]: *** [release/libopensearchknn_faiss.jnilib] Error 1
make[1]: *** [CMakeFiles/opensearchknn_faiss.dir/all] Error 2
make: *** [all] Error 2

@@ -452,4 +457,4 @@ The Github workflow in [`backport.yml`](.github/workflows/backport.yml) creates
original PR with an appropriate label `backport <backport-branch-name>` is merged to main with the backport workflow
run successfully on the PR. For example, if a PR on main needs to be backported to `1.x` branch, add a label
`backport 1.x` to the PR and make sure the backport workflow runs on the PR along with other checks. Once this PR is
merged to main, the workflow will create a backport PR to the `1.x` branch.
merged to main, the workflow will create a backport PR to the `1.x` branch.
Copy link
Member

Choose a reason for hiding this comment

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

What is the difference between these lines?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nothing intentional. I may have deleted a trailing whitespace but I'm not sure what triggered the diff.

@ryanbogan
Copy link
Member

Can you add a changelog entry? Otherwise looks good to me!

Copy link

codecov bot commented Jun 12, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.06%. Comparing base (9023604) to head (eb14858).
Report is 7 commits behind head on main.

Current head eb14858 differs from pull request most recent head 535092e

Please upload reports for the commit 535092e to get more accurate results.

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #1746      +/-   ##
============================================
- Coverage     85.09%   85.06%   -0.04%     
- Complexity     1473     1486      +13     
============================================
  Files           178      178              
  Lines          5937     6051     +114     
  Branches        607      632      +25     
============================================
+ Hits           5052     5147      +95     
- Misses          633      644      +11     
- Partials        252      260       +8     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: Finn Roblin <[email protected]>
@naveentatikonda naveentatikonda merged commit 9075fb5 into opensearch-project:main Jun 12, 2024
52 checks passed
shatejas pushed a commit to shatejas/k-NN that referenced this pull request Jun 26, 2024
…t#1746)

* updated dev guide to fix clang linking issue on arm

Signed-off-by: Finn Roblin <[email protected]>

* restore clang instructions and add additional msg to use gcc if clang linker error

Signed-off-by: Finn Roblin <[email protected]>

* update changelog

Signed-off-by: Finn Roblin <[email protected]>

---------

Signed-off-by: Finn Roblin <[email protected]>
shatejas pushed a commit to shatejas/k-NN that referenced this pull request Jun 27, 2024
…t#1746)

* updated dev guide to fix clang linking issue on arm

Signed-off-by: Finn Roblin <[email protected]>

* restore clang instructions and add additional msg to use gcc if clang linker error

Signed-off-by: Finn Roblin <[email protected]>

* update changelog

Signed-off-by: Finn Roblin <[email protected]>

---------

Signed-off-by: Finn Roblin <[email protected]>
luyuncheng pushed a commit to luyuncheng/k-NN-1 that referenced this pull request Jul 7, 2024
…t#1746)

* updated dev guide to fix clang linking issue on arm

Signed-off-by: Finn Roblin <[email protected]>

* restore clang instructions and add additional msg to use gcc if clang linker error

Signed-off-by: Finn Roblin <[email protected]>

* update changelog

Signed-off-by: Finn Roblin <[email protected]>

---------

Signed-off-by: Finn Roblin <[email protected]>
luyuncheng pushed a commit to luyuncheng/k-NN-1 that referenced this pull request Jul 7, 2024
…t#1746)

* updated dev guide to fix clang linking issue on arm

Signed-off-by: Finn Roblin <[email protected]>

* restore clang instructions and add additional msg to use gcc if clang linker error

Signed-off-by: Finn Roblin <[email protected]>

* update changelog

Signed-off-by: Finn Roblin <[email protected]>

---------

Signed-off-by: Finn Roblin <[email protected]>
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.

4 participants