Skip to content

Conversation

@cliffburdick
Copy link
Collaborator

No description provided.

Export pybind11 dependency for downstream users during installation and build.
Enable installation of pybind11 headers and config.
Removed visibility option for pybind in matx target.
@cliffburdick
Copy link
Collaborator Author

/build

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Greptile Overview

Greptile Summary

This PR exports pybind11 as a dependency for downstream users and removes the -fvisibility=hidden compile option that was previously required by pybind11.

Key changes:

  • Added rapids_export_package() calls for both BUILD and INSTALL export sets to make pybind11 targets available to downstream projects
  • Configured pybind11 to install headers and CMake config files via PYBIND11_INSTALL and PYBIND11_FINDPYTHON options
  • Removed the -fvisibility=hidden compile flag from the matx INTERFACE target

Issues found:

  • Minor trailing whitespace on lines 386 of CMakeLists.txt and line 37 of cmake/GetPyBind11.cmake

Confidence Score: 4/5

  • This PR is safe to merge with minor style fixes needed
  • The changes are well-structured and address a real need (exporting pybind11 for downstream users). The removal of -fvisibility=hidden follows modern pybind11 practices. Only minor whitespace issues were found that don't affect functionality.
  • Both files have minor trailing whitespace that should be cleaned up before merging

Important Files Changed

File Analysis

Filename Score Overview
CMakeLists.txt 4/5 Added rapids_export_package calls for pybind11 and removed -fvisibility=hidden compile option; minor trailing whitespace issue on line 386
cmake/GetPyBind11.cmake 5/5 Added PYBIND11_INSTALL and PYBIND11_FINDPYTHON options to enable pybind11 installation; minor trailing whitespace issue on line 37

Sequence Diagram

sequenceDiagram
    participant User as Downstream Project
    participant CMake as CMake Build System
    participant CPM as CPM Package Manager
    participant PyBind11 as pybind11
    participant MatX as MatX Target

    User->>CMake: find_package(matx)
    CMake->>MatX: Load matx-exports
    
    alt pybind11 enabled
        CMake->>CPM: CPMFindPackage(pybind11)
        CPM->>PyBind11: Fetch/Find pybind11
        Note over PyBind11: PYBIND11_INSTALL=ON<br/>PYBIND11_FINDPYTHON=ON
        PyBind11->>CMake: pybind11::embed target
        PyBind11->>CMake: pybind11::pybind11 target
        
        CMake->>MatX: rapids_export_package(BUILD)
        CMake->>MatX: rapids_export_package(INSTALL)
        Note over MatX: Exports pybind11::embed<br/>and pybind11::pybind11<br/>as global targets
        
        MatX->>MatX: target_link_libraries(pybind11::embed)
        Note over MatX: No -fvisibility=hidden flag
    end
    
    MatX-->>User: matx::matx target with pybind11
Loading


# Export pybind11 dependency for downstream users after make install
rapids_export_package(INSTALL pybind11 matx-exports GLOBAL_TARGETS pybind11::embed pybind11::pybind11)
rapids_export_package(BUILD pybind11 matx-exports GLOBAL_TARGETS pybind11::embed pybind11::pybind11)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

trailing whitespace at end of line

Suggested change
rapids_export_package(BUILD pybind11 matx-exports GLOBAL_TARGETS pybind11::embed pybind11::pybind11)
rapids_export_package(BUILD pybind11 matx-exports GLOBAL_TARGETS pybind11::embed pybind11::pybind11)

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

# This ensures pybind11 headers and cmake config are installed with matx
set(PYBIND11_INSTALL ON CACHE BOOL "Install pybind11 headers and cmake config")
set(PYBIND11_FINDPYTHON ON CACHE BOOL "Use FindPython for pybind11")

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

trailing whitespace on empty line

Suggested change

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants