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

ENH looser ABI compat for pybind11 #79

Open
wants to merge 17 commits into
base: main
Choose a base branch
from
Open

Conversation

beckermr
Copy link
Member

@beckermr beckermr commented Mar 18, 2022

Checklist

  • Used a personal fork of the feedstock to propose changes
  • Bumped the build number (if the version is unchanged)
  • Reset the build number to 0 (if the version changed)
  • Re-rendered with the latest conda-smithy (Use the phrase @conda-forge-admin, please rerender in a comment in this PR for automated rerendering)
  • Ensured the license file is being packaged.

closes #77

@conda-forge-linter
Copy link
Contributor

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
Copy link
Member Author

@conda-forge-admin rerender

@beckermr beckermr changed the title WIP looser ABI compat for pybind11 ENH looser ABI compat for pybind11 Mar 18, 2022
@beckermr
Copy link
Member Author

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

recipe/meta.yaml Outdated Show resolved Hide resolved
@beckermr
Copy link
Member Author

@conda-forge-admin rerender

@github-actions
Copy link
Contributor

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.

recipe/meta.yaml Outdated Show resolved Hide resolved
@beckermr
Copy link
Member Author

@conda-forge-admin rerender

@beckermr
Copy link
Member Author

@conda-forge-admin rerender

@github-actions
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Member Author

@conda-forge-admin rerender

@beckermr
Copy link
Member Author

@isuruf This is ready for another look!

@beckermr beckermr requested a review from isuruf March 23, 2022 11:37
@beckermr
Copy link
Member Author

Any other comments here @isuruf?

@tdegeus
Copy link
Member

tdegeus commented Mar 25, 2022

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

@tdegeus
Copy link
Member

tdegeus commented Apr 22, 2022

Any news on this?

@beckermr
Copy link
Member Author

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

@tdegeus
Copy link
Member

tdegeus commented Apr 27, 2022

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
Copy link
Member

tdegeus commented Jul 6, 2022

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

@tdegeus
Copy link
Member

tdegeus commented Dec 8, 2022

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
Copy link
Member Author

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

varlackc

This comment was marked as off-topic.

@jakirkham
Copy link
Member

@varlackc please raise that as a new issue

@wjakob
Copy link
Contributor

wjakob commented Nov 27, 2023

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
Copy link
Member

isuruf commented Nov 27, 2023

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
Copy link
Member

tdegeus commented Nov 27, 2023

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
Copy link
Contributor

wjakob commented Nov 27, 2023

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: pybind/pybind11#3793 (comment).

@beckermr
Copy link
Member Author

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.

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.

Does pybind11-abi shield from using different versions of GCC
7 participants