-
Notifications
You must be signed in to change notification settings - Fork 149
fix broken build flag, move build to one directory #2442
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
Changes from all commits
c62b4ee
4431ea4
5d61402
dd9a595
c57ea42
f4e5c43
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -329,11 +329,52 @@ task windowsPatches(type:Exec) { | |
commandLine 'cmd', '/c', "Powershell -File $rootDir\\scripts\\windowsScript.ps1" | ||
} | ||
|
||
task cmakeJniLib(type:Exec) { | ||
workingDir 'jni' | ||
def findExecutable(String executableName, List<String> additionalPaths = []) { | ||
// Print the task's environment before setting it | ||
// Print PATH specifically | ||
logger.lifecycle("\nSystem PATH:") | ||
logger.lifecycle(System.getenv("PATH")) | ||
|
||
def commonBasePaths = [ | ||
"/opt/homebrew/bin", | ||
"/usr/local/bin", | ||
"/usr/bin" | ||
] | ||
|
||
// Start with just the executable name (will use system PATH) | ||
def execPath = executableName | ||
|
||
if (Os.isFamily(Os.FAMILY_MAC)) { | ||
def searchPaths = [] | ||
// Add common paths | ||
commonBasePaths.each { basePath -> | ||
searchPaths.add("${basePath}/${executableName}") | ||
} | ||
// Add any additional specific paths | ||
searchPaths.addAll(additionalPaths) | ||
|
||
for (path in searchPaths) { | ||
if (new File(path).exists()) { | ||
logger.lifecycle("Found ${executableName} at: ${path}") | ||
execPath = path | ||
break | ||
} | ||
} | ||
} | ||
|
||
return execPath | ||
} | ||
|
||
tasks.register('cmakeJniLib', Exec) { | ||
def cmakePath = findExecutable("cmake") | ||
logger.lifecycle("Using cmake at: ${cmakePath}") | ||
// Inherit the current environment | ||
environment System.getenv() | ||
|
||
def args = [] | ||
args.add("cmake") | ||
args.add(".") | ||
args.add(cmakePath) | ||
args.add("-S jni") | ||
args.add("-B jni/build") | ||
args.add("-DKNN_PLUGIN_VERSION=${opensearch_version}") | ||
args.add("-DAVX2_ENABLED=${avx2_enabled}") | ||
args.add("-DAVX512_ENABLED=${avx512_enabled}") | ||
|
@@ -343,7 +384,7 @@ task cmakeJniLib(type:Exec) { | |
def javaHome = Jvm.current().getJavaHome() | ||
logger.lifecycle("Java home directory used by gradle: $javaHome") | ||
if (Os.isFamily(Os.FAMILY_MAC)) { | ||
environment('JAVA_HOME',javaHome) | ||
environment('JAVA_HOME', javaHome) | ||
} | ||
if (Os.isFamily(Os.FAMILY_WINDOWS)) { | ||
dependsOn windowsPatches | ||
|
@@ -353,13 +394,30 @@ task cmakeJniLib(type:Exec) { | |
args.add("-DLAPACK_LIBRARIES=$rootDir\\src\\main\\resources\\windowsDependencies\\libopenblas.dll") | ||
} | ||
|
||
// Print the task's environment after setting it | ||
logger.lifecycle("\nTask Environment PATH:") | ||
logger.lifecycle(environment.get('PATH')) | ||
|
||
// Print the command that will be executed | ||
logger.lifecycle("CMake command: ${args.join(' ')}") | ||
def outputStream = new ByteArrayOutputStream() | ||
commandLine args | ||
standardOutput = outputStream | ||
} | ||
|
||
task buildJniLib(type:Exec) { | ||
dependsOn cmakeJniLib | ||
workingDir 'jni' | ||
commandLine 'make', 'opensearchknn_nmslib', 'opensearchknn_faiss', 'opensearchknn_common', '-j', "${nproc_count}" | ||
def makePath = findExecutable("make") | ||
logger.lifecycle("Using make at: ${makePath}") | ||
println "Make path: ${makePath}" | ||
// Ensure makePath is treated as a String. If parsing to an int is required, | ||
// handle it explicitly, though a path typically should not be parsed as an int. | ||
if (makePath.isEmpty()) { | ||
throw new GradleException("Make not found in PATH. Please install Make.") | ||
} | ||
def outputStream = new ByteArrayOutputStream() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shall we move this block of code from line 376 to 393 into a separate gradle task like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. let me refactor that part, I think I can completely avoid the outputStream and just pull from System.getEnv |
||
commandLine makePath, '-Cjni/build','opensearchknn_nmslib', 'opensearchknn_faiss', 'opensearchknn_common', '-j', "${nproc_count}" | ||
standardOutput = outputStream | ||
} | ||
|
||
test { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -111,10 +111,10 @@ endif() | |
if(NOT DEFINED AVX512_SPR_ENABLED) | ||
# Check if the system is Intel(R) Sapphire Rapids or a newer-generation processor | ||
execute_process(COMMAND bash -c "lscpu | grep -q 'GenuineIntel' && lscpu | grep -i 'avx512_fp16' | grep -i 'avx512_bf16' | grep -i 'avx512_vpopcntdq'" OUTPUT_VARIABLE SPR_FLAGS OUTPUT_STRIP_TRAILING_WHITESPACE) | ||
if (AND NOT "${SPR_FLAGS}" STREQUAL "") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch @sam-herman . @mulugetam Can you check this and make sure this change makes sense? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is a valid change. I fixed it in 2.x and 2.19 but somehow missed backporting it to main |
||
set(AVX512_SPR_ENABLED true) | ||
if (NOT "${SPR_FLAGS}" STREQUAL "") | ||
set(AVX512_SPR_ENABLED true) | ||
else() | ||
set(AVX512_SPR_ENABLED false) | ||
set(AVX512_SPR_ENABLED false) | ||
endif() | ||
endif() | ||
|
||
|
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.
Good update. This makes cleaning some much easier. That being said, in the buildJniLib, instead of changing workingDir, can we just do:
I worry if we add "/" we might end up having issues with path resolution on different platforms (unless gradle handles this for us)
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.
The windows build seems to work fine https://github.com/opensearch-project/k-NN/actions/runs/13020810027/job/36325044749?pr=2442
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.
Sure - think thats fine