search

conda-forge / pybind11-feedstock

A conda-smithy repository for pybind11.
BSD 3-Clause "New" or "Revised" License
5 stars 23 forks source link

ENH looser ABI compat for pybind11 #79

Closed beckermr closed 1 month ago

beckermr commented 2 years ago

Checklist

closes #77

conda-forge-linter commented 2 years ago

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

beckermr commented 2 years ago

@conda-forge-admin rerender

beckermr commented 2 years ago

@conda-forge/core @conda-forge/pybind11 @isuruf this is ready for review!

beckermr commented 2 years ago

@conda-forge-admin rerender

github-actions[bot] commented 2 years ago

Hi! This is the friendly automated conda-forge-webservice.

I tried to rerender for you, but it looks like there was nothing to do.

This message was generated by GitHub actions workflow run https://github.com/conda-forge/pybind11-feedstock/actions/runs/2005823833.

beckermr commented 2 years ago

@conda-forge-admin rerender

beckermr commented 2 years ago

@conda-forge-admin rerender

github-actions[bot] commented 2 years ago

Hi! This is the friendly automated conda-forge-webservice.

I tried to rerender for you, but it looks like there was nothing to do.

This message was generated by GitHub actions workflow run https://github.com/conda-forge/pybind11-feedstock/actions/runs/2005924048.

github-actions[bot] commented 2 years ago

Hi! This is the friendly automated conda-forge-webservice.

I tried to rerender for you, but it looks like there was nothing to do.

This message was generated by GitHub actions workflow run https://github.com/conda-forge/pybind11-feedstock/actions/runs/2005929942.

beckermr commented 2 years ago

@conda-forge-admin rerender

beckermr commented 2 years ago

@isuruf This is ready for another look!

beckermr commented 2 years ago

Any other comments here @isuruf?

tdegeus commented 2 years ago

Great! Will a guide be added to the knowledge base?

tdegeus commented 2 years ago

Any news on this?

beckermr commented 2 years ago

Needs to be merged locally and rerender. Was waiting on review from @isuruf plus migration work.

tdegeus commented 2 years ago

I'm experience quite some extra work from the ABI check (and the fact that I'm not on the same GCC version at our computing center). It would be great if this could be merged soon!

tdegeus commented 2 years ago

At the risk of being pedantic: it would be great if this could be merged soon

tdegeus commented 1 year ago

I guess that we are not very numerous of feeling this problem, but still, it is quite annoying for me. I am using several pybind11 modules, taking a class of one as the argument of the other. Now, every time GCC is updated I will have to manually rebuild all involved modules. And things are worse if e.g. the CI is not yet switch to the new compiler version. It would be really great if this could be merged!

beckermr commented 1 year ago

I don't think we're going to get traction on this issue/PR. It might be better to focus on the upstream one.

jakirkham commented 1 year ago

@varlackc please raise that as a new issue

wjakob commented 11 months ago

I don't see what makes this a CondaForge-specific issue. This seems to me like an attempt to sneak in a change to pybind11 without going through the main repository.

What I would like to better understand: when is the ABI compatibility criterion in pybind11 too strict, and what can be done to make it more relaxed without breaking things? (The same also applies to nanobind, which uses a similar ABI tag)

Let me add one correction: simply using different versions of GCC alone is not enough to cause issues -- pybind11 queries the ABI compatibility tag of libstdc++ (__GXX_ABI_VERSION). And that's where this whole thing gets kind of tricky: when pybind11 isolates different extensions, it's does so intentionally because a change in the ABI tag. So it is your word against those of the libstdc++ authors, who are saying that there was an ABI break (which you then want to override/disable manually.)

isuruf commented 11 months ago

Let me add one correction: simply using different versions of GCC alone is not enough to cause issues -- pybind11 queries the ABI compatibility tag of libstdc++ (__GXX_ABI_VERSION). And that's where this whole thing gets kind of tricky: when pybind11 isolates different extensions, it's does so intentionally because a change in the ABI tag. So it is your word against those of the libstdc++ authors, who are saying that there was an ABI break (which you then want to override/disable manually.)

GXX_ABI_VERSION is too broad. Yes, there might have been an ABI break because of name mangling issues, but there is a compatibility scheme that adds a aliased symbol. See -fabi-compat-version in https://gcc.gnu.org/onlinedocs/gcc-13.2.0/gcc/C_002b_002b-Dialect-Options.html

tdegeus commented 11 months ago

I can comment on what causes problems in my case. Suppose that I have a library foo that provides class Foo with a pybind11 API. When I then have a library bar with say a method void bar(Foo& arg) then things would just work from the Python side using from foo import Foo.

In practice, this breaks when the Linux compiler on conda-forge is updated. Either of two solutions would be satisfactory for me:

  1. Relax pybind11's strictness on the compatibility check.
  2. Trigger rebuilds for the dependencies such that all packages with the same pybind1_abi are compatible.
wjakob commented 11 months ago

Unfortunately, __GXX_ABI_VERSION what we've got, and it is the official way of libstdc++ to signal ABI breaks. Changing or selectively ignoring this flag does not seem like a good idea to me.

Adding some CondaForge-specific meta-tag related to the libstdc++ ABI versions could be one feasible solution.

The documentation page you linked (https://gcc.gnu.org/onlinedocs/gcc-13.2.0/gcc/C_002b_002b-Dialect-Options.html) also mentions -fabi-version, which could be a solution. If packages that exchange instances consistently use such a flag, then the issue with different compiler versions would presumably go away. (I have not tried this though)

I would suggest to continue the discussion on the pybind11 repository since it is now spread out over two places: https://github.com/pybind/pybind11/issues/3793#issuecomment-1828049629.

beckermr commented 11 months ago

I want to be very clear on one point. Conda-forge reserves the right to, and regularly does in fact, patch upstream codes in order to improve how they work within the conda ecosystem.

Changing how pybind11 handles ABI compat in the conda-forge feedstock is not sneaking in changes by going around upstream.

beckermr commented 1 month ago

pybind11's ABI story has changed a lot. closing this per #96