Fix run exports to restrict the minor version

[vyasr]

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.

The current run exports for this library are too loose. Minor releases within the same major family are not ABI-compatible with one another. Arrow has never released a minor version before, so we've never observed this issue, but with the recent 16.1 release we see that the libraries packaged by this recipe all contain SOVERSIONs that are based on both the major and minor versions of the library, so packages built against one minor version cannot use a different one at runtime.

[conda-forge-webservices]

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.

[vyasr]

@conda-forge-admin, please rerender

[jakirkham]

Thanks Vyas! 🙏

Have a few questions below. Admittedly these may be things other maintainers have thoughts on as well

Also whatever we decide, we will likely want a repodata patch as well. We can wait to do that until we have finished this PR though

[jakirkham]

Am noticing in pyarrow, we are doing this

- libarrow {{ version }}.*=*{{ build_ext }}

Do we want a similarly tight pin here or should pyarrow relax its pin?

Recognize more is happening in the pyarrow case. Am only focusing on how tight we pin (and whether we include the package hash)

[h-vetinari]

Am noticing in pyarrow, we are doing this

This is necessary because pyarrow is built on a different feedstock, and so cannot use pin_subpackage.

Do we want a similarly tight pin here or should pyarrow relax its pin?

Neither.

[jakirkham]

For these is a sufficiently tight libarrow pin sufficient? If so, could we drop max_pin altogether?

Or do we want all of these equivalently tight? In which case maybe a Jinja for-loop would be more DRY and easier to maintain

Also this comes up below

[h-vetinari]

In which case maybe a Jinja for-loop would be more DRY and easier to maintain

Let's not do that. It creates more comprehension overhead than it reduces, especially if the list of elements if not next to the loop anymore. For artefacts testing that's less of an issue, but for run-exports we really need to be crystal-clear, because errors have far more wide-reaching consequences.

[h-vetinari]

The current run exports for this library are too loose. Minor releases within the same major family are not ABI-compatible with one another. Arrow has never released a minor version before, so we've never observed this issue, but with the recent 16.1 release we see that the libraries packaged by this recipe all contain SOVERSIONs that are based on both the major and minor versions of the library, so packages built against one minor version cannot use a different one at runtime.

@raulcd, this is exactly what I meant when asking about arrow's first minor version bump in #1407. 😅

Given that this was explicitly loosened in #1096, perhaps the ABI should be the same, and the libraries only versioned by the major versions? CC @kou @jorisvandenbossche

[h-vetinari]

@vyasr, could you send a PR to mark the just-released libarrow* builds for 16.1.0 as broken? I'd like to not have to rush this PR.

[vyasr]

Yup will do.

[vyasr]

Opened conda-forge/admin-requests#996. Let me know if that looks right (there are a lot of arrow libraries, I went ahead and marked all of them).

[h-vetinari]

@jorisvandenbossche @kou @raulcd
Can we please have some guidance on what arrow wants to do here?¹ Should we change the pinning to minor level (meaning we have to remigrate again for 16.1 after 16.0), or should the minor level be removed from the SOVERSION (with the implication that the ABI doesn't change between minor versions)?

Footnotes

1. There are several migrations waiting in the wings, which we cannot launch right now without reverting back to 16.0, as 16.1 is currently not publishable, unless we bite the bullet of this PR and then remigrate again. ↩

[raulcd]

should the minor level be removed from the SOVERSION (with the implication that the ABI doesn't change between minor versions)?

I have no idea why the original implementation did it this way. I wasn't aware the ABI would change between minor versions.

Nonetheless for 16.1.0 we already have a change of ABI so the only solution for conda is to pin the minor version. Otherwise we would require a new Arrow release and for conda to skip 16.1.0, right?

[h-vetinari]

I have no idea why the original implementation did it this way.

Please see #1096 by your colleagues.

[h-vetinari]

Nonetheless for 16.1.0 we already have a change of ABI

Do we know that the ABI actually changed? Or is it just the default way the SOVERSION is constructed (taking into account the minor version) that makes it seem like the ABI has changed?

[jorisvandenbossche]

This is all very unclear and undocumented, a situation we should definitely improve on the Arrow side.
But my understanding is that conda-forge actually should never have loosened those pins in the first place (I asked about it when the pin was loosened in apache/arrow#37624 (comment) and #1096 (comment), but never got to follow up on that, my apologies).

See also apache/arrow#41659 and apache/arrow#41679

My understanding:

• Our release logic clearly bumps the so version for minor feature releases (as we notice here), and this is done intentionally (ARROW-5848: [C++] SO versioning schema after release 1.0.0 apache/arrow#4801, https://lists.apache.org/thread/r1ztq38xz5tn4p4v0xc1xk2d8x82p84v)
• We don't check for ABI stability, and given how easily you change the ABI in a C++ project, I doubt that our minor/features releases would be ABI stable (the 16.1.0 was a small feature release with only changes of two weeks, so that might "accidentally" have kept a stable ABI just because of only having few changes, but that's not am indication how future minor feature releases would be)
• The above point is actually a problem for our bug-fix releases, where we do promise a stable ABI, but are not very good at this .. (see eg [C++] ABI break in patch release 15.0.1 apache/arrow#40604). Given this, I would personally even consider going back to the most strict pinning at the patch level as it was originally, just to be safe.

[h-vetinari]

Given this, I would personally even consider going back to the most strict pinning at the patch level as it was originally, just to be safe.

That's fair enough, though there's a cost to this safety - incompatibility of a lot of packages between each other, as well as the need to migrate all arrow-dependent packages every time. I'm OK with migrating per minor version if that's the reality, but I'd really ask if you can make it work to not break ABI on patch releases.

[jorisvandenbossche]

I'm OK with migrating per minor version if that's the reality, but I'd really ask if you can make it work to not break ABI on patch releases.

Yep, that's fair. Migrating per minor version is what conda-forge should definitely do at the moment, because as long as we do not re-discuss and decide on this on the Arrow side, the official policy and what we do in practice is bumping the so version for minor versions. And we indeed should do a better job of ensuring to not break the ABI in patch releases (I also assume it hasn't happened much, I only remember this one report)

Anyway, I think starting with moving the pin to minor versions sounds good, and I opened the following issues to follow-up on the Arrow side:

• [C++][Docs] Document our ABI stability policy apache/arrow#41707
• [Release][C++] Check the ABI stability during the release process of bug-fix releases apache/arrow#41708

[h-vetinari]

OK, sounds good to me, thanks. Before we can merge this PR, we need a repodata patch that tightens the run-deps of existing packages that built against libarrow 16 to depend on >=16.0.0,<16.1 instead of >=16.0.0,<17. @vyasr, do you feel up for trying your hand at a repodata patch here?

[vyasr]

FWIW from the RAPIDS end libcudf has not encountered ABI-breaking issues with patch releases of arrow that I can recall. We have historically used a runtime pinning that allows both the minor and patch version to float, and it hasn't caused us any issues. The minor version of course hasn't mattered since arrow hasn't had a minor release before now, but at least that indicates that empirically patch versions haven't broken the ABI.

Sure I can put together a repodata patch. I won't get to it until next week, but I should be able to get it done early next week.

[h-vetinari]

OK thanks! In this case, I want to move forward with a bunch of migrations, so ended up doing it myself. 🙃

If you're interested in how it's done (for next time 😉), see here: conda-forge/conda-forge-repodata-patches-feedstock#731

[h-vetinari]

Thanks for the PR!

[vyasr]

@h-vetinari thanks! I'm familiar with repodata patches but I definitely would not have done it so fast.

@conda-forge/arrow-cpp we'll need to backport this to get new builds of 16.0 up that have the correct run exports. The fix here ensures that 16.1 builds are find, but any packages on other channels that are building against 16.0 (which is still the conda-forge pinning) will still inherit the wrong run exports.

[vyasr]

Created an issue with this request in #1418