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 pybind11 abi output #63

Merged
merged 8 commits into from
Dec 10, 2020
Merged

ENH pybind11 abi output #63

merged 8 commits into from
Dec 10, 2020

Conversation

beckermr
Copy link
Member

@beckermr beckermr commented Dec 9, 2020

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.

This PR adds the pybind11 ABI metapackage per the discussion here: conda-forge/conda-forge-pinning-feedstock#849 (comment)

Closes #53

@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.

I do have some suggestions for making it better though...

For recipe:

  • It looks like the 'pybind11-abi' output doesn't have any tests.

@beckermr beckermr marked this pull request as draft December 9, 2020 02:49
@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.

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

beckermr commented Dec 9, 2020

@conda-forge/core @conda-forge/pybind11

I have a few questions here:

  1. Do we need a weak or strong run export for the abi metapackage? Or stated differently, do people sometimes use pybind11 in the build section in ways that indicate an ABI constraint at runtime?

  2. What should the version of the metapackage be? Right now I used the PYBIND11_INTERNALS_VERSION. However, pybind11 puts a lot more into the final ID they use to determine the ABI. (See the linked issue above.) It appears however that most if not all of this extra info is determined by the conda-forge build stack itself in terms of compiler runtime constraints etc. So it is not clear to me if we need all of the rest.

@henryiii
Copy link
Contributor

henryiii commented Dec 9, 2020

Let's say I use pybind11 to wrap a library, say a numerics library. I have two users.

User 1 writes a library that consumes my numerics objects. He does not interact with the C++ directly, but only my Python interface. He doesn't care what version of pybind11 I used, he can use anything he wants. His library and mine can have mismatched ABI strings, it doesn't matter.

User 2 writes a library that produces my numerics objects. She does interact with the C++ directly; her C++ code returns my C++ objects; pybind11 correctly converts them to Python, even though the conversion itself is defined in a different package than hers. She does not have to have the exact same version of pybind11, but is has to have a matching ABI.

The ABI string probably goes overboard, and I"m pretty sure everything else in it will not vary in conda-forge. So only the number should matter. If we change the other parts from now on, say if we realize we are overdoing it, we should also bump the ABI number at the same time.

@beckermr beckermr changed the title WIP pybind11 abi output ENH pybind11 abi output Dec 9, 2020
@beckermr beckermr marked this pull request as ready for review December 9, 2020 12:27
@beckermr
Copy link
Member Author

beckermr commented Dec 9, 2020

@HenryII Any ideas why CMake is being built from source on ppc64le?

@beckermr
Copy link
Member Author

beckermr commented Dec 9, 2020

@conda-forge/core @isuruf Can you all check the run exports here? I went with strong but I suspect that might be overkill.

@henryiii
Copy link
Contributor

henryiii commented Dec 9, 2020

I was pretty sure pyproject.toml was disabled - that would cause cmake to be built, if it was present (there's no wheel for it). Isn't PIP_NO_BUILD_ISOLATION enabled by default?

@beckermr
Copy link
Member Author

beckermr commented Dec 9, 2020

Isn't PIP_NO_BUILD_ISOLATION enabled by default?

IDK.

@beckermr
Copy link
Member Author

beckermr commented Dec 9, 2020

@henryiii why isn't the build using cmake from the conda env? We have it in the build section.

@henryiii
Copy link
Contributor

henryiii commented Dec 9, 2020

It should be here: https://github.com/conda/conda-build/blob/cb78dd913b65dad3d82f3ba99f667a8c0c939681/conda_build/build.py#L2545

This should cause the pyproject.toml to be ignored, and then it should only come from the conda spec.

@beckermr
Copy link
Member Author

beckermr commented Dec 9, 2020

OK. Not a big deal if this does not work precisely as long as it builds.

@henryiii
Copy link
Contributor

henryiii commented Dec 9, 2020

You could try adding --no-build-isolation to the pip line to see if it makes a difference?

@beckermr
Copy link
Member Author

beckermr commented Dec 9, 2020

This worked @henryiii! Thanks!

@henryiii
Copy link
Contributor

henryiii commented Dec 9, 2020

Okay, that bothers me. Why didn't this happen automatically? It is possible that those "write build scripts" only affect the environment if you don't use the build.sh/bat files?

@henryiii
Copy link
Contributor

henryiii commented Dec 9, 2020

@chrisburr Would you happen to know why adding --no-build-isolation changed things? I thought this was always set in the environment.

@beckermr
Copy link
Member Author

beckermr commented Dec 9, 2020

Okay, that bothers me. Why didn't this happen automatically? It is possible that those "write build scripts" only affect the environment if you don't use the build.sh/bat files?

I have no idea. I really doubt this matters in the long run though. There are many bugs in conda build that go unfixed due to the limited bandwidth of the developers.

@beckermr beckermr requested a review from henryiii December 10, 2020 10:58
@beckermr
Copy link
Member Author

OK I have switched the run exports to weak for now. This PR is ready to merge!

cc @henryiii @conda-forge/pybind11

Copy link
Contributor

@henryiii henryiii left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@beckermr beckermr merged commit b837602 into conda-forge:master Dec 10, 2020
@beckermr beckermr deleted the abi branch December 10, 2020 16:38
@bluescarni
Copy link

@beckermr thanks a lot for working on this!

As consumers of the pybind11 package, is there something additional we need to do in our recipes (i.e., depending explicitly on the pybind11-abi package) in order to take advantage of the pinning? Or is this already handled automatically?

@beckermr
Copy link
Member Author

I put docs here: https://conda-forge.org/docs/maintainer/knowledge_base.html#pybind11-abi-constraints

@bluescarni
Copy link

@beckermr Thanks a lot, that's great!

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.

globally pin the pybind11 versions
4 participants