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

ABI pinning via SONAME in build string #48

Draft
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

carterbox
Copy link
Member

@carterbox carterbox commented Jul 26, 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.

NOTES:

https://www.debian.org/doc/debian-policy/ch-sharedlibs.html
conda-forge/conda-forge.github.io#157

cfep-22.md Outdated Show resolved Hide resolved
cfep-22.md Outdated Show resolved Hide resolved
Co-authored-by: Dominik Kutra <[email protected]>
@hmaarrfk
Copy link
Contributor

I'm not sure where I should report this bug but it seems related conda-forge/hdf5-feedstock#177 (comment)

@carterbox
Copy link
Member Author

@hmaarrfk, that lint error has a patch in PR already (but it has not been merged). After the patch the lint will be unaffected by wider use of pinnings with build string matches.

@isuruf
Copy link
Member

isuruf commented Jul 27, 2022

Why not go with the prior art, which is to have a libgfortran5 output? This is what every other binary distro does.

cfep-22.md Outdated
Comment on lines 126 to 130
### Adding ABI name to package name

This alternatives renames outputs to `{{ name|lower }}{{ so_major_version }}`.
This is not backward compatible and would require migrating all downstream
packages to the new output names.
Copy link
Member Author

@carterbox carterbox Jul 27, 2022

Choose a reason for hiding this comment

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

Why not go with the prior art, which is to have a libgfortran5 output? This is what every other binary distro does.

I have addressed this in the Alternatives section. It is also discussed in one of the linked issues: conda-forge/conda-forge.github.io#157

An upside to this approach which I have added is that you can install multiple versions simultaneously.

cfep-22.md Outdated
Comment on lines 129 to 130
This is not backward compatible and would require migrating all downstream
packages to the new output names. However, it does enable installing multiple
Copy link
Member

@isuruf isuruf Jul 27, 2022

Choose a reason for hiding this comment

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

This is not true. There's a {{ name|lower }} output too, so it is backwards compatible. Please see https://github.com/conda-forge/ctng-compilers-feedstock/blob/main/recipe/meta.yaml

Copy link
Member Author

Choose a reason for hiding this comment

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

That's a good point! I'll update this section with a better description and example using your suggestion.

Copy link
Member Author

Choose a reason for hiding this comment

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

I have added a more detailed example of this alternative. I think it is the best alternative. I assume that its major utility is that it allows for multiple ABIs of a library to be installed into the same environment. Judging whether this is an important feature for conda-forge is beyond my depth.

I prefer the build string approach because it is simpler and because I haven't run into a case when I wanted to install multiple versions of the same library. However, this is just my personal experience.

Copy link
Member Author

Choose a reason for hiding this comment

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

Allowing multiple ABI versions of a package simultaneously causes much more work for refactoring a recipe in order to prevent clobbering. I have updated the proposal to better show how splitting a recipe this way would work.

Copy link
Member Author

Choose a reason for hiding this comment

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

Related thought: The purpose of the global conda-forge-pinnings/migrations is to keep all packages in sync with respect to the ABI/API of popular packages. Since publishing ABI-in-the-name packages allows installing multiple ABIs of the same package, does that mean that global conda-forge-pinnings would no longer be needed?

If so, which is easier? Maintaining global pinnings or maintaining individual feedstocks to ensure that ABI-in-the-name packages are implemented (correctly).

Copy link
Member

Choose a reason for hiding this comment

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

I'm not suggesting we do multiple ABIs at the same time. We have both libgfortran and libgfortran5 as run_exports and if a person really wants multiple ABIs, they can do that, but that's not the default. Because both libgfortran and libgfortran5 are dependencies, you can only have one package.

@carterbox
Copy link
Member Author

carterbox commented Sep 15, 2022

https://github.com/conda-forge/libidn11-feedstock/blob/main/recipe/meta.yaml

(Another package that uses SONAME in an new output)

@hmaarrfk
Copy link
Contributor

I think i would like to see conda-forge/abseil-cpp-feedstock#46 resolved before moving more pinnings to conda build strings. It seems that this would exacerbate the issue

@carterbox
Copy link
Member Author

I think i would like to see conda-forge/abseil-cpp-feedstock#46 resolved before moving more pinnings to conda build strings. It seems that this would exacerbate the issue

I didn't anticipate that conda was unprepared to do complex matching in the build string. We could end up with some very long build strings:

v{{ so_name_major }}so_{{ mpi_prefix }}_{{ cuda_prefix }}_h{{ PKG_HASH }}_{{ build }}

which various packages in the same environment could try to match in different ways:

foo 1.*     v{{ so_name_major }}so*
foo >=1.1   v{{ so_name_major }}so*{{ mpi_prefix }}*
foo =1.1.*  v{{ so_name_major }}so*{{ cuda_prefix }}*

And apparently the conda solver is not equipped to resolve these patterns right now.

@h-vetinari
Copy link
Member

And apparently the conda solver is not equipped to resolve these patterns right now.

It will be, as soon conda/conda#11612 is merged (and then percolates into the ecosystem).

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.

5 participants