conda-forge / pybind11-feedstock

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

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

Open AaronOpfer opened 7 months ago

AaronOpfer commented 7 months ago

Solution to issue cannot be found in the documentation.

Issue

https://github.com/conda-forge/pybind11-feedstock/blob/d08856e73dd6ce7aa3d5dce3dbde9e0d4616b6a0/recipe/meta.yaml#L37

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
traversaro commented 5 months ago

Thanks @AaronOpfer, this is probably what caused https://github.com/conda-forge/pybind11-feedstock/issues/96 .

traversaro commented 5 months ago

See a related change on the anaconda-recipes side: https://github.com/AnacondaRecipes/pybind11-feedstock/pull/15/files#diff-f3725a55bf339595bf865fec73bda8ac99f283b0810c205442021f29c06eea9aR38 .

traversaro commented 5 months ago

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 commented 2 months ago

As emerged in https://github.com/conda-forge/pybind11-feedstock/pull/105#issuecomment-2366822069, since pybind11 2.13.6, 2.12.1 and 2.11.2 a new system is in place in pybind11 (see https://github.com/pybind/pybind11/pull/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 https://github.com/pybind/pybind11/pull/5296 or after it.

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

Note that this will fix https://github.com/conda-forge/pybind11-feedstock/issues/96 and the first part of https://github.com/conda-forge/pybind11-feedstock/issues/95, but the problem of incompatibility between different major versions of gcc/clang (https://github.com/conda-forge/pybind11-feedstock/issues/77) or minor versions of msvc (part of https://github.com/conda-forge/pybind11-feedstock/issues/95).

henryiii commented 2 months ago

I think that makes sense.

traversaro commented 2 months ago

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

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

rwgk commented 2 months ago

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 (https://github.com/conda-forge/pybind11-feedstock/issues/77) or minor versions of msvc (part of https://github.com/conda-forge/pybind11-feedstock/issues/95).

For MSVC: See the comment I posted a few minutes ago: https://github.com/conda-forge/pybind11-feedstock/issues/95#issuecomment-2384265704

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 commented 2 months ago

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 commented 2 months ago

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 https://github.com/conda-forge/pybind11-feedstock/issues/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 https://github.com/conda-forge/pybind11-feedstock/issues/77 is about.

rwgk commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 https://github.com/conda-forge/pybind11-feedstock/issues/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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

https://github.com/pybind/pybind11/pull/4953 was my attempt at it

rwgk commented 1 month ago

Oh ... I posted questions back in July:

https://github.com/pybind/pybind11/pull/4953#issuecomment-2206806268

Did you see that comment already?