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

pybind11-abi test script is silently broken for pybind11 2.12, and may be missing ABI version 5 #96

Open
1 task done
AaronOpfer opened this issue Apr 8, 2024 · 16 comments · May be fixed by #106
Open
1 task done
Labels

Comments

@AaronOpfer
Copy link

Solution to issue cannot be found in the documentation.

  • I checked the documentation.

Issue

- if [ $(grep "#define PYBIND11_INTERNALS_VERSION" include/pybind11/detail/internals.h | cut -d' ' -f3) != "{{ abi_version }}" ]; then exit 1; fi

This test looks for a define in pybind11 source that does not actually exist anymore

https://github.com/pybind/pybind11/blob/f33f6afb667b6b5c0da7dee98dc02f51b4cc0e96/include/pybind11/detail/internals.h#L40

In addition it looks like there are interesting ABI changes for py 3.12 that the recipe might not be reflecting

You can check the build log and see that the bash test now emits a scripting error about missing arguments and then the build passes anyway. So this test is broken.

Sorry that this is light on details, I am away from my computers at the moment and could not log an issue from where I spotted the problem originally due to network policy.

Installed packages

N/A

Environment info

N/A
@AaronOpfer AaronOpfer added the bug label Apr 8, 2024
@traversaro
Copy link

Thanks @AaronOpfer, this is probably what caused #96 .

@traversaro
Copy link

@traversaro
Copy link

Thinking a bit more about this, now that pybind11 does not have a unique PYBIND11_INTERNALS_VERSION for its version, I do not think it makes a lot of sense to have a test on pybind11-abi that is a noarch: generic package independent of the operating system and the python version used, I guess the tests should be on the pybind11 build to check if it has the correct run_constrained.

@traversaro
Copy link

As emerged in #105 (comment), since pybind11 2.13.6, 2.12.1 and 2.11.2 a new system is in place in pybind11 (see pybind/pybind11#5296) for which PYBIND11_INTERNALS_VERSION does not play anymore a role. At this point, probably we can convert pybind-abi to distinguish between packages built before pybind/pybind11#5296 or after it.

@conda-forge/pybind11 do you have any opinion on this?

Note that this will fix #96 and the first part of #95, but the problem of incompatibility between different major versions of gcc/clang (#77) or minor versions of msvc (part of #95).

@henryiii
Copy link
Contributor

I think that makes sense.

@traversaro
Copy link

@conda-forge/pybind11 do you have any opinion on this?

fyi also @isuruf @beckermr @bluescarni @tdegeus that partecipated in discussion in this repo.

@rwgk
Copy link

rwgk commented Sep 30, 2024

As emerged in #105 (comment), since pybind11 2.13.6, 2.12.1 and 2.11.2 a new system is in place in pybind11 (see pybind/pybind11#5296) for which PYBIND11_INTERNALS_VERSION does not play anymore a role.

FWIW: PYBIND11_INTERNALS_VERSION plays a much lesser role, but it still matters for cross-extension inheritance relationships. See "NOTE: Matching PYBIND11_INTERNALS_IDs are still critically important for this situation:" in the Description of pybind/pybind11#5296.

but the problem of incompatibility between different major versions of gcc/clang (#77) or minor versions of msvc (part of #95).

For MSVC: See the comment I posted a few minutes ago: #95 (comment)

For gcc/clang: Are we missing something in pybind11? — I assumed what we have is as good as one can make it, without crossing into Undefined Behavior territory. No?

@traversaro
Copy link

As emerged in #105 (comment), since pybind11 2.13.6, 2.12.1 and 2.11.2 a new system is in place in pybind11 (see pybind/pybind11#5296) for which PYBIND11_INTERNALS_VERSION does not play anymore a role.

FWIW: PYBIND11_INTERNALS_VERSION plays a much lesser role, but it still matters for cross-extension inheritance relationships. See "NOTE: Matching PYBIND11_INTERNALS_IDs are still critically important for this situation:" in the Description of pybind/pybind11#5296.

Thanks for clarifying that. To be honest, I am not sure how to handle that at the conda-forge level. Trying to constraint all the pybind11 dependency to share the same PYBIND11_INTERNALS_VERSION just for that case seems a bit of an overkill. Just to clarify the specific context, do you have any real world example of two open source libraries that are affected by this? Thanks!

@traversaro
Copy link

For gcc/clang: Are we missing something in pybind11? — I assumed what we have is as good as one can make it, without crossing into Undefined Behavior territory. No?

From my understanding (at least reading from #77) conda-forge is considering gcc and clang compiled-code effectively ABI compatible (even if that is not true in some corner case/bugs, correct me if I am wrong), while pybind11 considers them ABI-incompatible, and that is what #77 is about.

@rwgk
Copy link

rwgk commented Sep 30, 2024

Trying to constraint all the pybind11 dependency to share the same PYBIND11_INTERNALS_VERSION just for that case seems a bit of an overkill.

Yeah, sorry I didn't mean to suggest that.

Just to clarify the specific context, do you have any real world example of two open source libraries that are affected by this?

Nope. — That's really difficult to pin-point. My gut feeling: it's rare, maybe even really rare. But it only takes one major package to depend on it and I'm wrong already ...

@traversaro
Copy link

Trying to constraint all the pybind11 dependency to share the same PYBIND11_INTERNALS_VERSION just for that case seems a bit of an overkill.

Yeah, sorry I didn't mean to suggest that.

Sure, and I was not implying you were suggesting it, my bad if I give you that impression. I was just sharing my preliminary tought on a this while typing the issue.

Just to clarify the specific context, do you have any real world example of two open source libraries that are affected by this?

Nope. — That's really difficult to pin-point. My gut feeling: it's rare, maybe even really rare. But it only takes one major package to depend on it and I'm wrong already ...

Perhaps we can be pragmatic and leave to who is packaging affected packages to handle that? I mean, if therea are libA and libB in such condition, nothing prevents libA to create a liba-pybind11-abi-internals package (to reflect the PYBIND11_INTERNALS_VERSION value use to compile libA) and nothing prevents libB to depend on a specific value of liba-pybind11-abi-internals to ensure consistency between the two if that is necessary.

@rwgk
Copy link

rwgk commented Sep 30, 2024

Perhaps we can be pragmatic

I'm a big fan of being pragmatic. (The opposing force: I don't want to create traps.)

From my understanding (at least reading from #77) conda-forge is considering gcc and clang compiled-code effectively ABI compatible (even if that is not true in some corner case/bugs, correct me if I am wrong),

Sounds pragmatic. — Re corner case/bugs, I'd guess that's again something very difficult to make a judgement on.

while pybind11 considers them ABI-incompatible,

Yes. I didn't author that part of pybind11, but I always understood it as inspired by a "better safe than sorry" mindset. (Which made sense to me.)

and leave to who is packaging affected packages to handle that?

Have you seen this code already?

https://github.com/pybind/pybind11/blob/7e418f49243bb7d13fa92cf2634af1eeac386465/include/pybind11/detail/internals.h#L272-L331

All 5 sub-macros that are combined into PYBIND11_PLATFORM_ABI_ID can be defined externally. Could that work for the purpose you have in mind?

@isuruf
Copy link
Member

isuruf commented Oct 1, 2024

All 5 sub-macros that are combined into PYBIND11_PLATFORM_ABI_ID can be defined externally. Could that work for the purpose you have in mind?

Yes, but Wenzel was against doing that in conda-forge and instead wanted us to figure out a solution upstream.

@rwgk
Copy link

rwgk commented Oct 1, 2024

All 5 sub-macros that are combined into PYBIND11_PLATFORM_ABI_ID can be defined externally. Could that work for the purpose you have in mind?

Yes, but Wenzel was against doing that in conda-forge and instead wanted us to figure out a solution upstream.

Is someone working on it?

@isuruf
Copy link
Member

isuruf commented Oct 1, 2024

pybind/pybind11#4953 was my attempt at it

@rwgk
Copy link

rwgk commented Oct 1, 2024

Oh ... I posted questions back in July:

pybind/pybind11#4953 (comment)

Did you see that comment already?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment