Fix build for macos #2397

sam-herman · 2025-01-16T04:42:21Z

Description

This change fixes the build issues for the C++ portion of the codebase on MacOSX for both nmslib by upgrading to the latest version and for faiss by setting the correct flags and using the right dependencies and directory paths
Ideally with this fix we will not need to ask developers to manually run text replacement commands like there are currently in the DEVELOPER.md file

Related Issues

Resolves #2272

Check List

[x ] New functionality includes testing.
[x ] New functionality has been documented.
[ x] API changes companion pull request created.
[ x] Commits are signed per the DCO using --signoff.
[ x] Public documentation issue/PR created.

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.

auto load java home path on macos update to latest nmslib revision Signed-off-by: Samuel Herman <[email protected]>

Signed-off-by: Samuel Herman <[email protected]>

0ctopus13prime · 2025-01-16T17:32:39Z

Thank you for the issue. Will take a look within this week

jmazanec15 · 2025-01-16T21:21:12Z

build issues for the C++ portion of the codebase on MacOSX for both nmslib by upgrading to the latest version

Is there a particular commit we are trying to pull in for nmslib?

jmazanec15 · 2025-01-16T21:17:50Z

build.gradle

@@ -327,13 +327,17 @@ task cmakeJniLib(type:Exec) {
    args.add("-DAVX512_ENABLED=${avx512_enabled}")
    args.add("-DCOMMIT_LIB_PATCHES=${commit_lib_patches}")
    args.add("-DAPPLY_LIB_PATCHES=${apply_lib_patches}")
+    if (Os.isFamily(Os.FAMILY_MAC)) {
+        args.add("-DJAVA_HOME=\$(/usr/libexec/java_home)")


Is this going to work for MacOS? I guess concern is if some mac wont have this set.

yeah, MacOs should have this set when Java is installed.

We should avoid this and make it rely on user's environment variable when building.
Including myself, we should be able to run OS with several JDKs. (ex: JDK21, JDK17 etc)

If you setup your Java correctly on mac, /usr/libexec/java_home this should always point to your local selected java home directory. Also if the home directory is not correct, cmake will ignore this parameter.
Generally speaking, you want the build to be smooth with as little manual overhead as possible to avoid confused developers.
For reference:
https://stackoverflow.com/a/31667558/29214278

I got your point, my bad. I thought you're overriding JAVA_HOME in CMake, but having it in build.gradle.
Yeah, this makes sense.

On the second thought, this should be the actual Java home path that gradle is using.
Could you replace it with org.gradle.internal.jvm.Jvm.current().getJavaHome() instead?
User may configure different JDK to run gradle, and we want to make sure that the same JDK to be used for building JNI and runtime.

makes sense, will add the change.

jmazanec15 · 2025-01-16T21:20:11Z

jni/cmake/init-faiss.cmake

+        if(CMAKE_C_COMPILER_ID MATCHES "Clang\$")
+            set(OpenMP_C_FLAGS "-Xpreprocessor -fopenmp")
+            set(OpenMP_C_LIB_NAMES "omp")
+            set(OpenMP_omp_LIBRARY /opt/homebrew/opt/libomp/lib/libomp.dylib)


Is this also going to be system dependent?

yes this is system independent. Newer version of brew will use that path by default while the old ones use the old path that was hard coded.

My concern is more that users may not use brew to install omp

The guideline in the instructions right now for Mac is to use brew, if this guideline is followed the build will fail unless the above is set.

sam-herman · 2025-01-17T00:15:20Z

build issues for the C++ portion of the codebase on MacOSX for both nmslib by upgrading to the latest version

Is there a particular commit we are trying to pull in for nmslib?

Yes, note that this issue is occurring on CLang 14+
nmslib/nmslib#175

0ctopus13prime · 2025-01-17T21:04:07Z

Hi @sam-herman
Let's just have CMake and build.gradle changes in this commit. Lot of BWC testings are failing due to the new commit of Nnmslib. Will add a TODO list in the ticket #2272.

Could you also update DEVELOPER.md as well to say that user can build with a particular JDK they want by providing custom Java path in JAVA_HOME environment, and by default, will use Java that gradle is using.

sam-herman · 2025-01-17T23:37:52Z

Hi @sam-herman Let's just have CMake and build.gradle changes in this commit. Lot of BWC testings are failing due to the new commit of Nnmslib. Will add a TODO list in the ticket #2272.

sounds good, I actually have an idea how we can avoid those issues going forward, but for now I can remove that and we can break into a different PR.

Could you also update DEVELOPER.md as well to say that user can build with a particular JDK they want by providing custom Java path in JAVA_HOME environment, and by default, will use Java that gradle is using.

will do.

Signed-off-by: Samuel Herman <[email protected]>

Signed-off-by: sam-herman <[email protected]>

sam-herman · 2025-01-21T16:14:18Z

Hi @0ctopus13prime @naveentatikonda thank you very much for reviewing and approving! looks like all the checks have passed, can one of you merge the PR? Unfortunately I don't have permissions to do so.

opensearch-trigger-bot · 2025-01-21T16:37:08Z

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-2.x 2.x
# Navigate to the new working tree
cd .worktrees/backport-2.x
# Create a new branch
git switch --create backport/backport-2397-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 8374b8f165124d96bc4be398d84d35b18d4bf290
# Push it to GitHub
git push --set-upstream origin backport/backport-2397-to-2.x
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-2397-to-2.x.

navneet1v · 2025-01-21T16:37:52Z

@sam-herman looks like automated backport failed. Please do the manual backport.

* fix paths for libomp on newer homebrew versions auto load java home path on macos update to latest nmslib revision Signed-off-by: Samuel Herman <[email protected]> * make it work for x86_64 mac as well Signed-off-by: Samuel Herman <[email protected]> * remove commented code Signed-off-by: Samuel Herman <[email protected]> * review comments Signed-off-by: Samuel Herman <[email protected]> --------- Signed-off-by: Samuel Herman <[email protected]> Signed-off-by: sam-herman <[email protected]> Co-authored-by: Samuel Herman <[email protected]> (cherry picked from commit 8374b8f)

sam-herman · 2025-01-21T18:38:18Z

@navneet1v created a backport PR
#2409

* fix paths for libomp on newer homebrew versions auto load java home path on macos update to latest nmslib revision Signed-off-by: Samuel Herman <[email protected]>

* fix paths for libomp on newer homebrew versions auto load java home path on macos update to latest nmslib revision Signed-off-by: Samuel Herman <[email protected]> * make it work for x86_64 mac as well Signed-off-by: Samuel Herman <[email protected]> * remove commented code Signed-off-by: Samuel Herman <[email protected]> * review comments Signed-off-by: Samuel Herman <[email protected]> --------- Signed-off-by: Samuel Herman <[email protected]> Signed-off-by: sam-herman <[email protected]> Co-authored-by: Samuel Herman <[email protected]>

samh83063 and others added 2 commits January 15, 2025 20:27

fix paths for libomp on newer homebrew versions

56e7e7d

auto load java home path on macos update to latest nmslib revision Signed-off-by: Samuel Herman <[email protected]>

make it work for x86_64 mac as well

e99b78a

Signed-off-by: Samuel Herman <[email protected]>

sam-herman requested review from heemin32, navneet1v, VijayanB, vamshin, jmazanec15, naveentatikonda, junqiu-lei, martin-gaievski, ryanbogan, luyuncheng, shatejas and 0ctopus13prime as code owners January 16, 2025 04:42

remove commented code

48db1dc

Signed-off-by: Samuel Herman <[email protected]>

0ctopus13prime self-assigned this Jan 16, 2025

jmazanec15 reviewed Jan 16, 2025

View reviewed changes

sam-herman and others added 2 commits January 17, 2025 15:51

review comments

e4f48b6

Signed-off-by: Samuel Herman <[email protected]>

Merge branch 'main' into fix-build-for-macos

ccdcd0d

Signed-off-by: sam-herman <[email protected]>

0ctopus13prime approved these changes Jan 20, 2025

View reviewed changes

naveentatikonda approved these changes Jan 21, 2025

View reviewed changes

naveentatikonda added Infrastructure Changes to infrastructure, testing, CI/CD, pipelines, etc. backport 2.x labels Jan 21, 2025

navneet1v merged commit 8374b8f into opensearch-project:main Jan 21, 2025
37 checks passed

sam-herman deleted the fix-build-for-macos branch January 21, 2025 18:20

sam-herman mentioned this pull request Jan 21, 2025

Backport/backport 2397 to 2.x #2409

Merged

sam-herman mentioned this pull request Jan 22, 2025

[FEATURE] Improve build to pull transient source dependencies #2421

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix build for macos #2397

Fix build for macos #2397

sam-herman commented Jan 16, 2025

0ctopus13prime commented Jan 16, 2025

jmazanec15 commented Jan 16, 2025

jmazanec15 Jan 16, 2025

sam-herman Jan 17, 2025

0ctopus13prime Jan 17, 2025 •

edited

Loading

sam-herman Jan 17, 2025 •

edited

Loading

0ctopus13prime Jan 17, 2025

0ctopus13prime Jan 17, 2025 •

edited

Loading

sam-herman Jan 17, 2025

jmazanec15 Jan 16, 2025

sam-herman Jan 17, 2025

jmazanec15 Jan 17, 2025

sam-herman Jan 17, 2025

sam-herman commented Jan 17, 2025

0ctopus13prime commented Jan 17, 2025 •

edited

Loading

sam-herman commented Jan 17, 2025

sam-herman commented Jan 21, 2025

opensearch-trigger-bot bot commented Jan 21, 2025

navneet1v commented Jan 21, 2025

sam-herman commented Jan 21, 2025

Fix build for macos #2397

Fix build for macos #2397

Conversation

sam-herman commented Jan 16, 2025

Description

Related Issues

Check List

0ctopus13prime commented Jan 16, 2025

jmazanec15 commented Jan 16, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0ctopus13prime Jan 17, 2025 • edited Loading

Choose a reason for hiding this comment

sam-herman Jan 17, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0ctopus13prime Jan 17, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sam-herman commented Jan 17, 2025

0ctopus13prime commented Jan 17, 2025 • edited Loading

sam-herman commented Jan 17, 2025

sam-herman commented Jan 21, 2025

opensearch-trigger-bot bot commented Jan 21, 2025

navneet1v commented Jan 21, 2025

sam-herman commented Jan 21, 2025

0ctopus13prime Jan 17, 2025 •

edited

Loading

sam-herman Jan 17, 2025 •

edited

Loading

0ctopus13prime Jan 17, 2025 •

edited

Loading

0ctopus13prime commented Jan 17, 2025 •

edited

Loading