Skip to content

Change internals ID and versioning scheme to avoid module conflicts - #1012

Merged
dean0x7d merged 4 commits into
pybind:masterfrom
dean0x7d:internals
Aug 23, 2017
Merged

dean0x7d merged 4 commits into
pybind:masterfrom
dean0x7d:internals

Conversation

@dean0x7d

@dean0x7d dean0x7d commented Aug 20, 2017 •

Copy link
Copy Markdown
Member

The current PYBIND11_INTERNALS_ID depends on the version of the library in order to isolate binary incompatible internals capsules. However, this does not preclude conflicts between modules built from different (binary incompatible) commits with the same version number.

For example, if one module was built with an early v2.2.dev and submitted to PyPI, it could not be loaded alongside a v2.2.x release module -- it would segfault because of incompatible internals with the same ID. (Well, whichever module was loaded first would work, but the second one would segfault on initialization.) I'm sure that a lot of projects include pybind11 as a submodule and check out a particular commit from the master branch.

This PR changes the ID to depend on PYBIND11_INTERNALS_VERSION which is independent of the main library version. It's an integer which should be incremented whenever a binary incompatible change is made to internals.

PYBIND11_INTERNALS_KIND is also introduced for a similar reason.

Note: This would require some manual effort to make sure PYBIND11_INTERNALS_VERSION was correctly incremented for incompatible changes to internals or the related functions.

@jagerman

Copy link
Copy Markdown
Member

Could we move the definition down a bit, right before struct internals, so that it's a very visible reminder whenever some touches internals? I'm worried that it might be easy to forget and having it right there is a nice reminder.

@dean0x7d
dean0x7d force-pushed the internals branch 2 times, most recently from 0db6b51 to a5f3cd9 Compare August 20, 2017 15:27
@dean0x7d

dean0x7d commented Aug 20, 2017 •

Copy link
Copy Markdown
Member Author

I consolidated everything related to internals into a new header: detail/internals.h (~200 LOC). This code was previously split between common.h and cast.h.

I also made a couple of tweaks as stated in the commit messages. I don't think there was any reason for using void* instead of type_info*, or noinline on those functions.

@dean0x7d

dean0x7d commented Aug 22, 2017 •

Copy link
Copy Markdown
Member Author

type_info and module_local are also affected by incompatible changes. I modified the module_local attribute lookup to use the same versioning scheme as internals. (And also moved type_info into detail/internals.h.)

The current PYBIND11_INTERNALS_ID depends on the version of the library
in order to isolate binary incompatible internals capsules. However,
this does not preclude conflicts between modules built from different
(binary incompatible) commits with the same version number.

For example, if one module was built with an early v2.2.dev and
submitted to PyPI, it could not be loaded alongside a v2.2.x release
module -- it would segfault because of incompatible internals with
the same ID.

This PR changes the ID to depend on PYBIND11_INTERNALS_VERSION which is
independent of the main library version. It's an integer which should be
incremented whenever a binary incompatible change is made to internals.

PYBIND11_INTERNALS_KIND is also introduced for a similar reason.

The same versioning scheme is also applied to `type_info` and the
`module_local` type attribute.
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