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

Close out the pybind11 fork #5842

Open
david-german-tri opened this issue Apr 14, 2017 · 7 comments · May be fixed by #19250
Open

Close out the pybind11 fork #5842

david-german-tri opened this issue Apr 14, 2017 · 7 comments · May be fixed by #19250
Assignees
Labels
component: build system Bazel, CMake, dependencies, memory checkers, linters priority: medium type: cleanup

Comments

@david-german-tri
Copy link
Contributor

It remains a goal to get off https://github.com/RobotLocomotion/pybind11 and back onto upstream. Not top priority, but we shouldn't forget to do it - and it may be easier to do the fresher our fork is.

cc @stonier @jamiesnape

@jamiesnape
Copy link
Contributor

Relates pybind/pybind11#1152.

@EricCousineau-TRI EricCousineau-TRI self-assigned this Jan 28, 2018
@EricCousineau-TRI
Copy link
Contributor

Porting item from #7660 to here:

Use upstream pybind11. Would need to break up pybind/pybind11#1146 and get buy-in from pybind11 authors.

  • Could relate to REPL-friendly referencing (e.g. removing use of unique_ptr in C++ arguments; use only shared_ptr).
  • Could use keep_alive workaround to prevent slicing, but would run into same caveats as for REPL-friendly referencing.
  • As a hack, could also introduce a keep-forever-alive setup for polymorphic types, and adopt a PyPy-style memory model (clean up on GC, not on reference counting).

@jwnimmer-tri
Copy link
Collaborator

Per Eric, we don't think we'll ever get to zero diffs from upstream, but we should still try to keep our diffs minimal by PRing upstream.

@EricCousineau-TRI
Copy link
Contributor

Will close this for now. Will encode that we should keep diffs minimal in our fork's README.

@EricCousineau-TRI
Copy link
Contributor

Given the recent uptick in interest in pybind11's holder setup (specifically for unique_ptr and some of the edge cases I've kludged through), this may become feasible over the next year or so:
pybind/pybind11#2646

@jwnimmer-tri
Copy link
Collaborator

Given that pybind11 upsteam just repaved over the entire universe with clang-format (pybind/pybind11#3713), continuing to maintain our fork will be even more work.

@EricCousineau-TRI
Copy link
Contributor

Agreed it will be more work, but I think reformatting our tree (hopefully idempotent with very small span of fixed points) will help PR merging. Planning to do another update in April.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: build system Bazel, CMake, dependencies, memory checkers, linters priority: medium type: cleanup
Projects
Status: No status
Development

Successfully merging a pull request may close this issue.

6 participants