-
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 all 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) | ||||||||||||||||||||||||||
|
Comment on lines
+133
to
+135
|
||||||||||||||||||||||||||
| 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_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) |
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.
[nitpick] These commented-out CMake options for TBB configuration should either be removed if they're not needed, or uncommented and properly configured if they are. Leaving them commented suggests uncertainty about the TBB build configuration. If these options are needed for specific platforms or use cases, they should be documented; otherwise, they add clutter.
| # 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 comment "Ensure the Python module name has no 'lib' prefix on Unix systems" is now misleading. With pybind11_add_module (line 198), the PREFIX is automatically set to empty string on all platforms, so this comment should be updated to reflect that we're simply setting the output name, not specifically handling the 'lib' prefix issue.
| # Ensure the Python module name has no 'lib' prefix on Unix systems | |
| # Set the output name of the Python module |
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 |
|---|---|---|
|
|
@@ -78,6 +78,9 @@ | |
| "-DBUILD_PYTHON_BINDINGS=ON", | ||
| ] | ||
|
|
||
| if platform.system() == "Windows": | ||
| cmake_args += [f"-DCMAKE_LIBRARY_OUTPUT_DIRECTORY_{cfg.upper()}={extdir}"] | ||
|
|
||
| # Add macOS-specific CMake prefix paths for Homebrew dependencies | ||
| if platform.system() == "Darwin": # macOS | ||
| try: | ||
|
|
@@ -123,6 +126,65 @@ | |
| 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}") | ||
| else: | ||
| print("Warning: No TBB shared libraries found to copy.") | ||
|
|
||
| elif platform.system() == "Windows": | ||
| print(f"Searching for TBB DLLs in {build_temp}...") | ||
| # Look for tbb*.dll recursively | ||
| tbb_dlls = list(build_temp.glob("**/tbb*.dll")) | ||
|
|
||
| if tbb_dlls: | ||
| print(f"Found TBB DLLs: {tbb_dlls}") | ||
| # We want to copy them to the 'dsf' package directory so we can load them in __init__.py | ||
Check warningCode scanning / Pylintpython3 (reported by Codacy) Line too long (104/100) Warning
Line too long (104/100)
Check warningCode scanning / Pylint (reported by Codacy) Line too long (104/100) Warning
Line too long (104/100)
|
||
| # extdir is where dsf_cpp.pyd is (site-packages root usually) | ||
| # We want site-packages/dsf/ | ||
|
|
||
| # self.build_lib is usually the root of the build (e.g. build/lib.win...) | ||
| # So we can construct the path to dsf package | ||
| dsf_pkg_dir = Path(self.build_lib) / "dsf" | ||
| dsf_pkg_dir.mkdir(parents=True, exist_ok=True) | ||
Check warningCode scanning / Pylint (reported by Codacy) Instance of 'PurePath' has no 'mkdir' member Warning
Instance of 'PurePath' has no 'mkdir' member
|
||
|
|
||
| # Also copy to source directory for editable installs | ||
| source_dsf_dir = Path(__file__).parent / "src" / "dsf" | ||
|
|
||
| for dll in tbb_dlls: | ||
| print(f"Copying {dll} to {dsf_pkg_dir}") | ||
| shutil.copy2(dll, dsf_pkg_dir) | ||
| print(f"Copying {dll} to {source_dsf_dir}") | ||
| shutil.copy2(dll, source_dsf_dir) | ||
| else: | ||
| print("Warning: No TBB DLLs found. This might cause import errors.") | ||
|
|
||
| def pre_build(self): | ||
| """Extracts doxygen documentation from XML files and creates a C++ unordered_map""" | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,19 @@ | ||
| import sys | ||
Check warningCode scanning / Pylint (reported by Codacy) Missing module docstring Warning
Missing module docstring
Check warningCode scanning / Pylintpython3 (reported by Codacy) Missing module docstring Warning
Missing module docstring
|
||
| import os | ||
|
|
||
| # On Windows, we need to explicitly load the bundled TBB DLLs | ||
| if sys.platform == "win32": | ||
| import glob | ||
| import ctypes | ||
|
|
||
| # Look for tbb dlls in the same directory as this __init__.py | ||
| _dll_dir = os.path.dirname(__file__) | ||
Check warningCode scanning / Pylint (reported by Codacy) Constant name "_dll_dir" doesn't conform to UPPER_CASE naming style Warning
Constant name "_dll_dir" doesn't conform to UPPER_CASE naming style
|
||
| for _dll in glob.glob(os.path.join(_dll_dir, "tbb*.dll")): | ||
| try: | ||
| ctypes.CDLL(_dll) | ||
| except Exception as e: | ||
Check warningCode scanning / Pylintpython3 (reported by Codacy) Variable name "e" doesn't conform to snake_case naming style Warning
Variable name "e" doesn't conform to snake_case naming style
Check warningCode scanning / Pylint (reported by Codacy) Variable name "e" doesn't conform to snake_case naming style Warning
Variable name "e" doesn't conform to snake_case naming style
Check noticeCode scanning / Pylint (reported by Codacy) Catching too general exception Exception Note
Catching too general exception Exception
Check noticeCode scanning / Pylintpython3 (reported by Codacy) Catching too general exception Exception Note
Catching too general exception Exception
|
||
| print(f"Warning: Failed to load {_dll}: {e}") | ||
|
Comment on lines
+12
to
+15
|
||
|
|
||
| from dsf_cpp import __version__, LogLevel, get_log_level, set_log_level, mobility, mdt | ||
|
|
||
| from .python.cartography import ( | ||
|
|
||
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 wheel build doesn't use a wheel repair tool like
delocate(used for macOS on line 148 of this file) orauditwheel(typically used for Linux). This means TBB DLLs won't be automatically bundled into the wheel by a repair tool. While the setup.py does copy TBB DLLs to the package directory (setup.py lines 161-184), those files need to be included inpackage_datain setup.py to actually be packaged. Consider either:delvewheel(Windows equivalent of delocate/auditwheel) to bundle DLLs into the wheelpackage_dataincludes*.dllpatterns (which it currently doesn't)