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

Add eigen-abi package for use in downstrem compiled libraries that use eigen in their public headers #41

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

Conversation

traversaro
Copy link
Contributor

@traversaro traversaro commented Aug 22, 2023

Note: this is meant to be a proposal for discussion, I just propose it as PR as it is easier to discuss it in this form.

eigen itself is a header-only library, so there is no need to account for its ABI. Anyhow, several feedstock (including some that I mantain) are C++ compiled libraries that use Eigen in their public headers, including:

So, any change in Eigen public ABI effectively change the ABI of the downstream library, see for example the failure in conda-forge/dartsim-feedstock#16 (comment) .

To cleanly handle this without any downside for existing users of eigen that use them either not in libraries or in libraries without using it in public headers, similarly to what is done in https://github.com/conda-forge/pybind11-feedstock I propose to add a new package, called eigen-abi, that is used to track compiled libraries that use eigen in their public headers.

If this PR is accepted, the idea is to add eigen-abi as a dependency (on the top of the existing eigen dependency) for compiled libraries that include eigen in their public headers. Furthermore, the idea is to add eigen-abi to https://github.com/conda-forge/conda-forge-pinning-feedstock, so that we can do migrations for new versions of eigen-abi.

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.

@conda-forge-webservices
Copy link
Contributor

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

I wanted to let you know that I linted all conda-recipes in your PR (recipe) and found some lint.

Here's what I've got...

For recipe:

  • Failed to even lint the recipe, probably because of a conda-smithy bug 😢. This likely indicates a problem in your meta.yaml, though. To get a traceback to help figure out what's going on, install conda-smithy and run conda smithy recipe-lint . from the recipe directory.

@traversaro
Copy link
Contributor Author

@conda-forge/eigen any suggestion or comment is welcome!

@traversaro
Copy link
Contributor Author

@conda-forge-admin, please rerender

@conda-forge-webservices
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.

@traversaro
Copy link
Contributor Author

@conda-forge-admin, please rerender

@traversaro
Copy link
Contributor Author

@conda-forge/eigen the PR is ready for comments/review

@traversaro
Copy link
Contributor Author

If it is helpful, I can also add myself to the mantainers of the feedstock.

@traversaro
Copy link
Contributor Author

I guess this PR become even more important as now microarchitecture packages are coming to conda-forge, see conda-forge/conda-forge.github.io#2091 (review) . Eigen public ABI depends on the optimization enabled (at least for what regards the used alignment, see https://gitlab.com/libeigen/eigen/-/blob/fa201f1bb3e2c05271cfc00ceda7b6a0228c4af5/Eigen/src/Core/util/ConfigureVectorization.h#L42-60), so we may want to include the microarchitecture used in the build string of eigen-abi and also somehow in the run_exports of the eigen-abi packages. Packages built against Eigen in public headers with a given microarchitecture can only be used in packages that compile for a given microarchitecture.

@jakirkham
Copy link
Member

So we would want some kind of run_exports then? Or would users of this package need to figure this out on their own?

@traversaro
Copy link
Contributor Author

Ideally, nothing should change for recipe that already use eigen and do not include eigen in public headers. Recipe of compiled libraries that include eigen in their public headers should start depending also from eigen-abi . For what regard microarch builds, I need to thing a bit about them.

@traversaro
Copy link
Contributor Author

For what regard microarch builds, I need to thing a bit about them.

I think I need to get to use microarch builds to get an idea on how to use them. Fortunately I maintain a few packages without eigen used in the public headers that would benefit from them, so I can get a bit of experience there before proceeding here.

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.

2 participants