Skip to content
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

Fix build_and_run_tests.sh command error. #1319

Closed
wants to merge 1 commit into from

Conversation

tsogoo
Copy link

@tsogoo tsogoo commented Mar 7, 2025

First, kudos to the OpenSpiel team for maintaining such an impressive and valuable framework for reinforcement learning and game theory research!

This PR addresses an issue with the pybind11 dependency in OpenSpiel, stemming from the smart_holder branch being archived and renamed to archive/smart_holder in the pybind11 repository (see: https://github.com/pybind/pybind11/tree/archive/smart_holder). The original build process attempted to clone the now-deprecated smart_holder branch, causing errors.

Changes Made

  1. Updated pybind11 Branch: Modified the cached_clone command in ./open_spiel/scripts/build_and_run_tests.sh to use the master branch of pybind11 instead of smart_holder or archive/smart_holder. The smart_holder features are now fully integrated into master, making this the appropriate target.
  2. Removed Deprecated Code: As per the pybind11 migration guidelines, removed the obsolete #include <pybind11/smart_holder.h> and the following deprecated macros from the codebase:
    • PYBIND11_SMART_HOLDER_TYPE_CASTERS
      These are no longer needed with the latest pybind11.

Result

These changes resolve the build errors encountered when running ./open_spiel/scripts/build_and_run_tests.sh, ensuring compatibility with the current pybind11 master branch.

Please review and let me know if additional adjustments are needed!

Copy link

google-cla bot commented Mar 7, 2025

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@tsogoo tsogoo closed this Mar 7, 2025
@tsogoo
Copy link
Author

tsogoo commented Mar 7, 2025

I realized another PR #1318 that fixes the error. so i closed.

@lanctot
Copy link
Collaborator

lanctot commented Mar 7, 2025

Oh, wow, wonderful. Thanks for this!

@lanctot
Copy link
Collaborator

lanctot commented Mar 7, 2025

(Yes I've been chatting with @rwgk from the pybind11 team who has been looking at fixing this in a similar way -- but thank you for fixing this nonetheless.)

@lanctot
Copy link
Collaborator

lanctot commented Mar 7, 2025

Opened a new version now (#1320) based on a unification of this with #1318

@lanctot lanctot closed this Mar 7, 2025
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