Download a pre-built tbb #378

Grufoony · 2025-12-05T15:26:06Z

Close Add full Python support for Windows os #337
Close Add Windows support in binding.yml #338
Close Add Windows support in pypi.yml #339

codecov · 2025-12-05T16:29:54Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 83.41%. Comparing base (e46ba54) to head (75bde4d).

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #378   +/-   ##
=======================================
  Coverage   83.41%   83.41%           
=======================================
  Files          53       53           
  Lines        5392     5392           
  Branches      614      614           
=======================================
  Hits         4498     4498           
  Misses        883      883           
  Partials       11       11

Flag	Coverage Δ
unittests	`83.41% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

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

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

setup.py

+
+            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


setup.py

+                # 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)


src/dsf/__init__.py

@@ -1,3 +1,18 @@
+import sys


src/dsf/__init__.py

+    import glob
+    import ctypes
+    # Look for tbb dlls in the same directory as this __init__.py
+    _dll_dir = os.path.dirname(__file__)


src/dsf/__init__.py

+    for _dll in glob.glob(os.path.join(_dll_dir, "tbb*.dll")):
+        try:
+            ctypes.CDLL(_dll)
+        except Exception as e:


src/dsf/__init__.py

+    for _dll in glob.glob(os.path.join(_dll_dir, "tbb*.dll")):
+        try:
+            ctypes.CDLL(_dll)
+        except Exception as e:


setup.py

+
+            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


setup.py

src/dsf/__init__.py

@@ -1,3 +1,18 @@
+import sys


src/dsf/__init__.py

+    for _dll in glob.glob(os.path.join(_dll_dir, "tbb*.dll")):
+        try:
+            ctypes.CDLL(_dll)
+        except Exception as e:


src/dsf/__init__.py

+    for _dll in glob.glob(os.path.join(_dll_dir, "tbb*.dll")):
+        try:
+            ctypes.CDLL(_dll)
+        except Exception as e:


setup.py

Copilot

Pull request overview

This pull request transitions the project from using system-installed TBB (Threading Building Blocks) to downloading and bundling a pre-built TBB via CMake's FetchContent. This change enables Windows support and simplifies cross-platform builds by eliminating system dependency requirements.

Key changes:

TBB is now fetched via CMake FetchContent instead of requiring system installation
Platform-specific library bundling: DLLs copied to package directory on Windows with ctypes loading, shared objects copied with RPATH configuration on Linux/macOS
GitHub Actions workflows updated to remove TBB/spdlog/simdjson system dependencies and add Windows wheel builds
Version bumped from 4.5.0 to 4.5.1

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 9 comments.

Show a summary per file

File	Description
src/dsf/dsf.hpp	Patch version bumped from 0 to 1 (4.5.0 → 4.5.1)
src/dsf/init.py	Added Windows-specific TBB DLL loading via ctypes.CDLL on import
setup.py	Added TBB library copying logic for all platforms: to extension dir for Linux/macOS, to package dir for Windows
CMakeLists.txt	Simplified TBB FetchContent configuration, switched to pybind11_add_module, added Doxygen requirement check, added Linux RPATH configuration
.github/workflows/pytest.yml	Removed system TBB/spdlog/simdjson dependencies from apt install
.github/workflows/pypi.yml	Removed system dependencies from all platforms, added new Windows wheel build job
.github/workflows/codeql.yml	Removed system dependency installation and Homebrew prefix configuration
.github/workflows/cmake_tests.yml	Removed macOS TBB/simdjson installation and CMake prefix path configuration
.github/workflows/cmake_examples.yml	Removed TBB/simdjson from apt and brew install commands
.github/workflows/binding.yml	Removed system dependencies, removed vcpkg setup for Windows, enabled Windows in test matrix

Comments suppressed due to low confidence (2)

setup.py:106

This code still references tbb, fmt, and spdlog Homebrew packages, but the PR is transitioning to use FetchContent for these dependencies. Since TBB is now being fetched via CMake's FetchContent (see CMakeLists.txt lines 139-144), this code should be removed or updated. The CMakeLists.txt already handles bundling the TBB library via RPATH settings (lines 227, 233), so these Homebrew prefix paths are no longer needed for TBB. However, if spdlog and fmt are still expected to come from Homebrew, this should be clarified, but based on the workflow changes removing these system dependencies, this code appears to be dead code.

        if platform.system() == "Darwin":  # macOS
            try:
                tbb_prefix = subprocess.check_output(
                    ["brew", "--prefix", "tbb"], text=True
                ).strip()
                fmt_prefix = subprocess.check_output(
                    ["brew", "--prefix", "fmt"], text=True
                ).strip()
                spdlog_prefix = subprocess.check_output(
                    ["brew", "--prefix", "spdlog"], text=True
                ).strip()

                cmake_prefix_path = f"{tbb_prefix};{fmt_prefix};{spdlog_prefix}"
                cmake_args.append(f"-DCMAKE_PREFIX_PATH={cmake_prefix_path}")
                print(f"Added macOS Homebrew prefix paths: {cmake_prefix_path}")

            except (subprocess.CalledProcessError, FileNotFoundError):
                print(
                    "Warning: Could not determine Homebrew prefix paths. Make sure Homebrew is installed and dependencies are available."
                )
                # Fallback to common Homebrew paths
                cmake_args.append("-DCMAKE_PREFIX_PATH=/opt/homebrew;/usr/local")

src/dsf/init.py:15

Print statement may execute during import.

            print(f"Warning: Failed to load {_dll}: {e}")

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-12-06T12:18:07Z

src/dsf/__init__.py

+        try:
+            ctypes.CDLL(_dll)
+        except Exception as e:
+            print(f"Warning: Failed to load {_dll}: {e}")


Using a bare except Exception with only a print statement silently swallows errors during DLL loading. This could make debugging difficult if there are issues with TBB DLLs. Consider either being more specific about which exceptions to catch (e.g., OSError, WindowsError) or logging the error in a way that's more visible, rather than just printing. Alternatively, if the DLL loading failure is critical, consider re-raising the exception after logging.

Copilot · 2025-12-06T12:18:07Z

CMakeLists.txt

+# set(TBB_BUILD_SHARED ON CACHE BOOL "Build TBB as shared library" FORCE)
+# set(TBB_BUILD_TBBMALLOC OFF CACHE BOOL "Disable TBB malloc" FORCE)


[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.

Suggested change

# 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 · 2025-12-06T12:18:08Z

CMakeLists.txt

-  add_library(dsf_python_module MODULE src/dsf/bindings.cpp)
+  pybind11_add_module(dsf_python_module src/dsf/bindings.cpp)

  # Ensure the Python module name has no 'lib' prefix on Unix systems


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.

Suggested change

# Ensure the Python module name has no 'lib' prefix on Unix systems

# Set the output name of the Python module

Copilot · 2025-12-06T12:18:08Z

.github/workflows/pypi.yml

+        python -m pip install build wheel setuptools pybind11-stubgen
+
+    - name: Build wheel
+      run: python -m build --wheel
+


The Windows wheel build doesn't use a wheel repair tool like delocate (used for macOS on line 148 of this file) or auditwheel (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 in package_data in setup.py to actually be packaged. Consider either:

Using delvewheel (Windows equivalent of delocate/auditwheel) to bundle DLLs into the wheel

Ensuring the setup.py package_data includes *.dll patterns (which it currently doesn't)

Suggested change

python -m pip install build wheel setuptools pybind11-stubgen

- name: Build wheel

run: python -m build --wheel

python -m pip install build wheel setuptools pybind11-stubgen delvewheel

- name: Build wheel

run: python -m build --wheel

- name: Repair wheel with delvewheel

run: |

for whl in dist/*.whl; do

python -m delvewheel repair "$whl" -w dist

done

Copilot · 2025-12-06T12:18:08Z

setup.py

+                # 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*")))


The glob pattern **/libtbb.so* will match versioned files like libtbb.so.12, but the destination uses lib.name which is just the filename (e.g., libtbb.so.12). On Linux, the extension might be linked against a symlink like libtbb.so, but this code copies the actual versioned files. If the versioned file is copied but the .so symlink is not, runtime linking might fail. Consider either:

Also copying the non-versioned symlink if it exists

Creating a symlink in the destination to match what the linker expects

Filtering to only copy the actual library file that matches what the NEEDED entry in the ELF header expects

setup.py

Copilot · 2025-12-06T12:18:09Z

CMakeLists.txt

+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)


[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:

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)

Suggested change

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 · 2025-12-06T12:18:10Z

CMakeLists.txt

      dsf_python_module
      PROPERTIES BUILD_WITH_INSTALL_RPATH TRUE
-                 INSTALL_RPATH "${HOMEBREW_LIB}"
+                 INSTALL_RPATH "${HOMEBREW_LIB};@loader_path"


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.

Suggested change

INSTALL_RPATH "${HOMEBREW_LIB};@loader_path"

INSTALL_RPATH "@loader_path"

Copilot · 2025-12-06T12:18:10Z

CMakeLists.txt

+  if(NOT DOXYGEN_FOUND)
+    message(FATAL_ERROR "Doxygen is required to build the docstrings.")
+  endif()


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.

Suggested change

if(NOT DOXYGEN_FOUND)

message(FATAL_ERROR "Doxygen is required to build the docstrings.")

endif()

Grufoony force-pushed the tbb branch 3 times, most recently from 85bcce5 to 4c55a8c Compare December 5, 2025 16:02

Download a pre-built tbb

b2e142b

Grufoony force-pushed the tbb branch from 4c55a8c to b2e142b Compare December 5, 2025 16:23

Grufoony force-pushed the tbb branch from 0225380 to 558d51f Compare December 5, 2025 16:32

These should not be necessary anymore

aa49e6d

Grufoony force-pushed the tbb branch from 558d51f to aa49e6d Compare December 5, 2025 16:37

github-advanced-security bot found potential problems Dec 5, 2025

View reviewed changes

setup.py Fixed Show fixed Hide fixed

setup.py Fixed Show fixed Hide fixed

setup.py Fixed Show fixed Hide fixed

setup.py Fixed Show fixed Hide fixed

setup.py Fixed Show fixed Hide fixed

github-advanced-security bot found potential problems Dec 5, 2025

View reviewed changes

setup.py Fixed Show fixed Hide fixed

setup.py Fixed Show fixed Hide fixed

setup.py Fixed Show fixed Hide fixed

setup.py Fixed Show fixed Hide fixed

setup.py Fixed Show fixed Hide fixed

Grufoony added 9 commits December 6, 2025 10:30

Is this necessary?

ddc619b

Yes it is

a6d96c9

Not necessary

eeca1e3

What about this?

6470f04

Yes it is

4c5aeb9

Remove apt installations

920d40e

Also this should be removed

5c2e0f2

Proviaml

a30d237

Try to fix macos bindings

6a45c56