-
Notifications
You must be signed in to change notification settings - Fork 4
Download a pre-built tbb #378
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 11 commits
b2e142b
aa49e6d
ddc619b
a6d96c9
eeca1e3
6470f04
4c5aeb9
920d40e
5c2e0f2
a30d237
6a45c56
5bcdeb9
eb31c4a
ed11354
f3bf332
6f83c8f
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 | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -130,17 +130,18 @@ if(NOT simdjson_POPULATED) | |||||||
| FetchContent_MakeAvailable(simdjson) | ||||||||
| endif() | ||||||||
| # Get TBB | ||||||||
| set(TBB_TEST | ||||||||
| OFF | ||||||||
| CACHE BOOL "Disable TBB tests" FORCE) | ||||||||
| set(TBB_TEST OFF CACHE BOOL "Disable TBB tests" FORCE) | ||||||||
| set(TBB_EXAMPLES OFF CACHE BOOL "Disable TBB examples" FORCE) | ||||||||
| set(TBB_STRICT OFF CACHE BOOL "Disable TBB strict mode" FORCE) | ||||||||
| # set(TBB_BUILD_SHARED ON CACHE BOOL "Build TBB as shared library" FORCE) | ||||||||
| # set(TBB_BUILD_TBBMALLOC OFF CACHE BOOL "Disable TBB malloc" FORCE) | ||||||||
|
Comment on lines
+136
to
+137
|
||||||||
| # set(TBB_BUILD_SHARED ON CACHE BOOL "Build TBB as shared library" FORCE) | |
| # set(TBB_BUILD_TBBMALLOC OFF CACHE BOOL "Disable TBB malloc" FORCE) |
Copilot
AI
Dec 6, 2025
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 find_package(Doxygen REQUIRED) call already sets DOXYGEN_FOUND to TRUE if found, or fails the configuration if not found (due to REQUIRED). This subsequent check is redundant and will never trigger the FATAL_ERROR message since CMake would have already failed at the find_package step if Doxygen wasn't found.
| if(NOT DOXYGEN_FOUND) | |
| message(FATAL_ERROR "Doxygen is required to build the docstrings.") | |
| endif() |
Copilot
AI
Dec 6, 2025
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 CMakeLists.txt includes @loader_path in the RPATH (line 227), which is correct for loading bundled libraries. However, it still includes ${HOMEBREW_LIB} in the RPATH. Since TBB is now being fetched via FetchContent and bundled with the extension (as evidenced by setup.py copying TBB libraries), the Homebrew library paths may no longer be necessary for TBB. If spdlog and other dependencies are also fetched via FetchContent, this entire Homebrew RPATH configuration might be obsolete. Consider reviewing whether Homebrew paths are still needed or if @loader_path alone is sufficient.
| INSTALL_RPATH "${HOMEBREW_LIB};@loader_path" | |
| INSTALL_RPATH "@loader_path" |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -123,6 +123,38 @@ | |
| cwd=build_temp, | ||
| ) | ||
|
|
||
| # Copy TBB shared library if it exists (Linux and macOS) | ||
| if platform.system() == "Linux" or platform.system() == "Darwin": | ||
| print(f"Searching for TBB shared libraries in {build_temp}...") | ||
|
|
||
| tbb_libs = [] | ||
| if platform.system() == "Linux": | ||
| # Look for libtbb.so* recursively | ||
| tbb_libs = list(build_temp.glob("**/libtbb.so*")) | ||
| # Also look for libtbb_debug.so* if we are in debug mode or if that's what was built | ||
| tbb_libs.extend(list(build_temp.glob("**/libtbb_debug.so*"))) | ||
|
Comment on lines
+135
to
+138
|
||
| else: # macOS | ||
| # Look for libtbb.dylib* recursively | ||
| tbb_libs = list(build_temp.glob("**/libtbb*.dylib")) | ||
|
|
||
| if tbb_libs: | ||
| print(f"Found TBB libraries: {tbb_libs}") | ||
| for lib in tbb_libs: | ||
| # We only want the real shared object, not symlinks if possible, | ||
| # but copying everything matching the pattern is safer to ensure we get the versioned one. | ||
|
||
| # However, we need to be careful not to overwrite if multiple matches found. | ||
| # Usually we want the one that the linker linked against. | ||
| # Since we set RPATH to $ORIGIN (Linux) or @loader_path (macOS), we need the library in the same dir as the extension. | ||
|
||
|
|
||
| # Avoid copying if it's a symlink pointing to something we already copied? | ||
| # simpler: just copy all of them. | ||
| dest = extdir / lib.name | ||
| if not dest.exists(): | ||
| shutil.copy2(lib, dest) | ||
| print(f"Copied {lib} to {dest}") | ||
Grufoony marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| else: | ||
| print("Warning: No TBB shared libraries found to copy.") | ||
|
|
||
| def pre_build(self): | ||
| """Extracts doxygen documentation from XML files and creates a C++ unordered_map""" | ||
|
|
||
|
|
||
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.
[nitpick] The formatting style for these TBB CMake options is inconsistent with the rest of the file. Other similar configurations in the file (e.g., lines 101-103, 104-106, 107-109) use multi-line formatting with proper indentation. Consider reformatting for consistency: