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

Switch to libabseil #153

Merged
merged 5 commits into from
Aug 23, 2022
Merged

Switch to libabseil #153

merged 5 commits into from
Aug 23, 2022

Conversation

h-vetinari
Copy link
Member

Follow-up to #138, with some light clean-ups along the way

@conda-forge-linter
Copy link

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.

@h-vetinari h-vetinari added the automerge Merge the PR when CI passes label Aug 23, 2022
@github-actions github-actions bot merged commit 94b4641 into conda-forge:main Aug 23, 2022
@github-actions
Copy link
Contributor

Hi! This is the friendly conda-forge automerge bot!

I considered the following status checks when analyzing this PR:

  • linter: passed
  • azure: passed

Thus the PR was passing and merged! Have a great day!

@h-vetinari h-vetinari deleted the libabseil branch August 23, 2022 17:15
@ngam
Copy link

ngam commented Aug 24, 2022

@h-vetinari I am not sure if this is needed. Why don't we just pin harder/exactly in abseil-cpp like we discussed in the following issue? conda-forge/abseil-cpp-feedstock#41

@h-vetinari
Copy link
Member Author

h-vetinari commented Aug 24, 2022

Yes it's needed. Going forward, all feedstocks should replace abseil-cpp -> libabseil, and eventually, we can remove the compatibility wrapper.

@ngam
Copy link

ngam commented Aug 24, 2022

Yes it's needed.

Why is it needed?

Going forward, all feedstocks should replace abseil-cpp -> libabseil, and eventually, we can remove the compatibility wrapper.

This is going to be too much of an intervention with a long time already using abseil-cpp and migrations, etc. --- I am not sure if it is worth making this change unilaterally and disrupting maintainers' rhythm.

Maybe I am wrong, but I don't particularly see any benefits, only downsides, from this abseil-cpp -> libabseil change.

@h-vetinari
Copy link
Member Author

The main discussion is in conda-forge/conda-forge.github.io#1073. I've taken the infrastructural changes around the C++ ABI as a good enough reason to finally follow through on this for some of the few outstanding "-cpp" feedstocks. The abseil rename was also discussed in two core dev meetings (I explicitly called it out) and there was no opposition.

This is going to be too much of an intervention with a long time already using abseil-cpp and migrations, etc. --- I am not sure if it is worth making this change unilaterally and disrupting maintainers' rhythm.

There are not that many feedstocks using abseil, and we're not in a rush - the abseil-cpp package keeps working (well, aside from some small papercuts you identified that can be fixed). I'm also happy to contribute this change to the respective feedstocks myself.

So it's really not such a big deal IMO.

@ngam
Copy link

ngam commented Aug 24, 2022

I'm also happy to contribute this change to the respective feedstocks myself.

So it's really not such a big deal IMO.

Yes if you are willing to smooth the corners/transition, then yes it is definitely not a big deal. I got the (wrong) impression that the onus will be on maintainers alone, but if you're willing to help and guide through this, then all is good.

Thank YOU! I obviously agree your approach is a step forward and desired change; my main concern was how the change was going to take place and as long as you're in the frame as you say, I feel good about this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Merge the PR when CI passes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants