-
-
Notifications
You must be signed in to change notification settings - Fork 26
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
use cmake to obtain correct soname #70
Conversation
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 ( |
@conda-forge-admin, please rerender |
…nda-forge-pinning 2022.06.10.06.38.26
recipe/build.sh
Outdated
if [[ $MAJOR_VERSION == 3 ]]; then | ||
SONAME_BASE=13 | ||
else | ||
echo "MAJOR_VERSION $MAJOR_VERSION not recognized. Update build.sh to specify its SONAME_BASE" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm assuming we won't need any repo patching or rebuilds, right? This ensure the old packages built with this will work.
I'm just concern of the sustainability of this else
but I guess this is better than causing more churn in the ecosystem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Existing packages should work on Linux, but there's no workaround on macOS :(
See this comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also I'd say we should only add the symlinks in this transition PR. Next release (e.g. 3.6) should not add the buggy sonames and just stick to upstream conventions (should be so.19 by then).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm OK with that. Let's wait for some else to comment before merging just so we have third opinion.
@jakirkham you are the only other maintainer here BTW 😬
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If possible I'd like to wait til next core call to discuss it with the team. We might need to rebuild libmamba on osx among others.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah not sure about this else
either
Also feel a little weird about mucking with the SONAME like this generally. Have we tried raising an issue with upstream?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my opinion the autoconf workflow has a bug. The expected so names are single integers according to their own docs:
It's unclear to me why we ended up with these weird 13.5.2
so names. Looks like 13
+ the minor.patch version, string-concatenated, instead of arithmetically summed. CMake does produce 18
.
There have been some similar issues in other distros, in which they couldn't agree on the soname version, but all of them use single integers:
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 ( |
@ocefpaf, this is probably going to become a bigger issue when
As a result I'd like to move forward with this PR. I've added symlinks to ensure backwards compatibility with the previous builds and a "release bomb" for 3.8 so we remove them. We'll also need to backport this to the 3.6 and 3.5 branches. If it doesn't work, well, I'll mark them as broken and go back to the drawing board 😬 |
Hi @jaimergp I did a quick check with defaults and I see that we build with |
@JeanChristopheMorinPerso - True, good catch! I missed that line in |
Np. But you know what, it's actually the default. I didn't realize it is the default. Anyway, it doesn't hurt to be explicit! Thanks for making the modification! |
Hm, @isuruf pointed me to this list of items to check when making this precise change (autotools -> CMake), and it might be trickier than expected (or not possible at all). I'll report later. |
Closing here as we won't be able to fix it this way. Check the issue for more details on the strategy going forward. |
Checklist
0
(if the version changed)conda-smithy
(Use the phrase@conda-forge-admin, please rerender
in a comment in this PR for automated rerendering)See #69 for more details