Skip to content

Conversation

@willkill07
Copy link
Member

@willkill07 willkill07 commented Apr 17, 2025

Description

pybind11 version bump resulted in this workflow for pybind11 stubgen not working. This PR fixes the command.

Checklist

  • I am familiar with the Contributing Guidelines.
  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.

Co-authored-by: Paul Taylor <pataylor@nvidia.com>

Signed-off-by: Will Killian <wkillian@nvidia.com>
@willkill07 willkill07 requested a review from a team as a code owner April 17, 2025 19:03
@willkill07 willkill07 added bug Something isn't working non-breaking Non-breaking change labels Apr 17, 2025
add_custom_command(
OUTPUT ${module_binary_stub_file}
COMMAND ${Python3_EXECUTABLE} -m pybind11_stubgen ${TARGET_NAME} --no-setup-py --log-level WARN -o ./ --root-module-suffix \"\"
COMMAND env PYTHONPATH=${module_root_binary_dir}:$PYTHONPATH ${Python3_EXECUTABLE} -m pybind11_stubgen ${TARGET_NAME} --no-setup-py --log-level WARN -o ./ --root-module-suffix ""
Copy link
Contributor

Choose a reason for hiding this comment

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

I think using the CMake command -E is preferred here. Something like add_custom_target(newtarget ${CMAKE_COMMAND} -E env NAME=VALUE somecommand)

Copy link
Member Author

Choose a reason for hiding this comment

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

add_custom_target has the following behavior:

The target has no output file and is always considered out of date.

I don't think we want this which is why we have add_custom_command generate the stub file since we can easily specify the output file.


# Change which setup we use if we are using inplace
if(_ARGS_IS_INPLACE)
list(APPEND _pip_command "-e")
Copy link
Contributor

Choose a reason for hiding this comment

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

Why was this removed? This will cause the files to be copied and leads to some confusion.

Copy link
Member Author

Choose a reason for hiding this comment

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

I was told it was unnecessary 🤷🏻‍♂️

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

Labels

bug Something isn't working non-breaking Non-breaking change

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

2 participants