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 redis #16170

Merged
merged 21 commits into from
Sep 28, 2021
Merged

Add redis #16170

merged 21 commits into from
Sep 28, 2021

Conversation

SimonBoothroyd
Copy link
Contributor

@SimonBoothroyd SimonBoothroyd commented Sep 14, 2021

Checklist

  • Title of this PR is meaningful: e.g. "Adding my_nifty_package", not "updated meta.yaml".
  • License file is packaged (see here for an example).
  • Source is from official source.
  • Package does not vendor other packages. (If a package uses the source of another package, they should be separate packages or the licenses of all packages need to be packaged).
  • If static libraries are linked in, the license of the static library is packaged.
  • Build number is 0.
  • A tarball (url) rather than a repo (e.g. git_url) is used in your recipe (see here for more details).
  • GitHub users listed in the maintainer section have posted a comment confirming they are willing to be listed there.
  • When in trouble, please check our knowledge base documentation before pinging a team.

@conda-forge-linter
Copy link

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 (recipes/redis) and found some lint.

Here's what I've got...

For recipes/redis:

  • Feedstock with the same name exists in conda-forge.

@SimonBoothroyd
Copy link
Contributor Author

@conda-forge/help-c-cpp it would be great to get help with a couple of questions:

  • the redis-py package was originally named redis and so had a redis-feedstock which I think is causing the linting error - is there any way to work around this?
  • it seems that redis builds a number of dependencies (some of which contain non-standard changes) and doesn't give the option to link against existing libraries. Would the Makefile need to be patched to target C-F deps or should redis continue to build its own deps?

@SimonBoothroyd
Copy link
Contributor Author

it seems that redis builds a number of dependencies (some of which contain non-standard changes) and doesn't give the option to link against existing libraries. Would the Makefile need to be patched to target C-F deps or should redis continue to build its own deps?

Per @wolfv's comments on gitter it seems we should be safe not to do any patching here.

@wolfv
Copy link
Member

wolfv commented Sep 16, 2021

Just curious -- why go back to an older version?
Do you know what's going on with these link errors on macOS?

@wolfv
Copy link
Member

wolfv commented Sep 16, 2021

PS: this is the homebrew recipe: https://github.com/Homebrew/homebrew-core/blob/HEAD/Formula/redis.rb

@SimonBoothroyd
Copy link
Contributor Author

SimonBoothroyd commented Sep 16, 2021

Just curious -- why go back to an older version?

I was seeing some build errors:

networking.c:3438:5: error: builtin functions must be directly called
    atomicSetWithSync(io_threads_pending[i], count);
    ^
./atomicvar.h:150:56: note: expanded from macro 'atomicSetWithSync'
    while(!__sync_bool_compare_and_swap(&var,var,value,__sync_synchronize)); \
                                                       ^

and was curious if dropping back to the old version would be an easy fix as the atomic operations are new in 6.x.x as far as I can tell!

It seems like the older versions have a different linking issues however...

It's strange as homebrew doesn't seem to have build issues.

Could both errors be caused by clang vs gcc differences?

@wolfv
Copy link
Member

wolfv commented Sep 16, 2021

a difference could be the required macOS SDK version... although I googled a bit and couldn't find anything. conda-forge uses a pretty old macOS SDK to be compatible with old macOS versions -- but that can be configured.

@SimonBoothroyd
Copy link
Contributor Author

Ah I think you're absolutely right - it seems stdatomic support wasn't included until 10.12

@SimonBoothroyd
Copy link
Contributor Author

Thanks @wolfv - that as well as forcing C11 on OSX seems to resolve the build issues.

Running the make tests on linux currently raise an unexpected exception, however, looking over the redis GH issues it seems that the test suite can be a bit flaky so I think this is probably fine for now?

@conda-forge/help-c-cpp this should be ready for review

@wolfv
Copy link
Member

wolfv commented Sep 17, 2021

@SimonBoothroyd when I looked at the Fedora recipe it seemed that they move some redis configuration files to certain places. Also in homebrew. Did you have a look at that?

Otherwise looks good to me (except for the lint!)

@SimonBoothroyd
Copy link
Contributor Author

Ah great catch - those should be added in c640b99!

Otherwise looks good to me (except for the lint!)

@wolfv do you have any suggestions for how to move forward with the lint issues? Presumably the archived existing feedstock needs to be removed / renamed by one of the C-F team?

@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 (recipes/redis) and found it was in an excellent condition.

@SimonBoothroyd
Copy link
Contributor Author

@conda-forge/help-c-cpp the linter seems to be happy now - this should be ready for review!

@wolfv wolfv merged commit dfb6892 into conda-forge:main Sep 28, 2021
@SimonBoothroyd
Copy link
Contributor Author

Thanks so much @wolfv!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants