redis / hiredis-py

Python wrapper for hiredis
MIT License
500 stars 103 forks source link

sdsalloc.h doesn't seem to be installed as part of regular hiredis #158

Open mcepl opened 1 year ago

mcepl commented 1 year ago

https://github.com/redis/hiredis-py/blob/a5574a9e4544aa0e2864b8e4e4bdeab89c971831/src/pack.c#L19

When I look at the Makefile of hiredis:

https://github.com/redis/hiredis/blob/6f5bae8c6900e051da6e677756508707565ce56e/Makefile#L302-L310

I don’t see sdsalloc.h to be installed at all, therefore it is not installed on my openSUSE system. Any ideas how to get this file? Is it a bug in hiredis-py or in the installation Makefile of hiredis?

chayim commented 1 year ago

@mcepl Have you seen vendor/sdsalloc.h in the codebase? You need to ensure you update submodules as well.

git submodule update --init --recursive is your friend

mcepl commented 1 year ago

@mcepl Have you seen vendor/sdsalloc.h in the codebase? You need to ensure you update submodules as well.

git submodule update --init --recursive is your friend

Given I am trying to use tarball from https://files.pythonhosted.org/packages/source/h/hiredis/hiredis-2.2.2.tar.gz I would hope it is included. It actually is, but I would prefer if I could use include files from the hiredis package itself, not the vendored one.

Apteryks commented 1 year ago

I'm in the same situation, trying to update this package to 2.2.2 on GNU Guix; there's no sdalloc.h file in hiredis 1.1.1:

$ find /gnu/store/0clwbra3j29m97rajyi5y83lm29s2636-hiredis-1.1.0/include/
/gnu/store/0clwbra3j29m97rajyi5y83lm29s2636-hiredis-1.1.0/include/
/gnu/store/0clwbra3j29m97rajyi5y83lm29s2636-hiredis-1.1.0/include/hiredis
/gnu/store/0clwbra3j29m97rajyi5y83lm29s2636-hiredis-1.1.0/include/hiredis/read.h
/gnu/store/0clwbra3j29m97rajyi5y83lm29s2636-hiredis-1.1.0/include/hiredis/async.h
/gnu/store/0clwbra3j29m97rajyi5y83lm29s2636-hiredis-1.1.0/include/hiredis/alloc.h
/gnu/store/0clwbra3j29m97rajyi5y83lm29s2636-hiredis-1.1.0/include/hiredis/sockcompat.h
/gnu/store/0clwbra3j29m97rajyi5y83lm29s2636-hiredis-1.1.0/include/hiredis/adapters
/gnu/store/0clwbra3j29m97rajyi5y83lm29s2636-hiredis-1.1.0/include/hiredis/adapters/ae.h
/gnu/store/0clwbra3j29m97rajyi5y83lm29s2636-hiredis-1.1.0/include/hiredis/adapters/glib.h
/gnu/store/0clwbra3j29m97rajyi5y83lm29s2636-hiredis-1.1.0/include/hiredis/adapters/ivykis.h
/gnu/store/0clwbra3j29m97rajyi5y83lm29s2636-hiredis-1.1.0/include/hiredis/adapters/libev.h
/gnu/store/0clwbra3j29m97rajyi5y83lm29s2636-hiredis-1.1.0/include/hiredis/adapters/libevent.h
/gnu/store/0clwbra3j29m97rajyi5y83lm29s2636-hiredis-1.1.0/include/hiredis/adapters/libhv.h
/gnu/store/0clwbra3j29m97rajyi5y83lm29s2636-hiredis-1.1.0/include/hiredis/adapters/libuv.h
/gnu/store/0clwbra3j29m97rajyi5y83lm29s2636-hiredis-1.1.0/include/hiredis/adapters/macosx.h
/gnu/store/0clwbra3j29m97rajyi5y83lm29s2636-hiredis-1.1.0/include/hiredis/adapters/poll.h
/gnu/store/0clwbra3j29m97rajyi5y83lm29s2636-hiredis-1.1.0/include/hiredis/adapters/qt.h
/gnu/store/0clwbra3j29m97rajyi5y83lm29s2636-hiredis-1.1.0/include/hiredis/hiredis.h
/gnu/store/0clwbra3j29m97rajyi5y83lm29s2636-hiredis-1.1.0/include/hiredis/sds.h

I find it curious that a 'vendored' file doesn't exist upstream; is it something added by hiredis-py?

I would also like to be able to simply link against a system-provided hiredis.

Apteryks commented 1 year ago

The https://github.com/redis/hiredis/blob/v1.1.0/sdsalloc.h file seems to contain 3 re-definitions, perhaps for backward compatibility purpose? hiredis-py could probably be adjusted to use alloc.h and the originally named definitions it contains instead.

Apteryks commented 1 year ago

@mcepl hi! you may be interested in https://github.com/redis/hiredis-py/pull/159 and https://github.com/redis/hiredis-py/pull/161 to allow using a system-provided hiredis library.

mcepl commented 1 year ago

Thank you, https://build.opensuse.org/request/show/1072918

chayim commented 1 year ago

@Apteryks where did you get a hiredis 1.1.1 - the last release I see is 1.1.0

bugant commented 12 months ago

@chayim is there any interest in having #159 and #161 merged? I'm asking because I'm in the process of creating a Fedora package for this project and having the same issue described in here.

gerzse commented 4 months ago

I merged #159 for now.

I tried #161 locally, on Ubuntu 22.04.4 LTS, and the hiredis system header files are so old that the build breaks, it does not see some of the REDIS_REPLY_* definitions. Not sure how to handle this? It can be left in the responsibility of the one who's invoking the build, I guess.

Apteryks commented 2 months ago

Typically build systems would probe for the version available, set some flag, then condition the code based on that flag. Not sure something can be done like this in the Python world though, at least when working with the basic setup.py or newer toml project build systems.

Apteryks commented 2 months ago

@Apteryks where did you get a hiredis 1.1.1 - the last release I see is 1.1.0

It was a typo; I had tested with 1.1.0 indeed.

paulwouters commented 2 months ago

I can fix this upstream but the only thing sdsalloc.h contains is:

#include "alloc.h"

#define s_malloc hi_malloc
#define s_realloc hi_realloc
#define s_free hi_free

Maybe just use that instead of trying to include sdsalloc.h ?

Apteryks commented 2 months ago

That seems easy in isolation but if you force all downstream users to do this, the effort/pain adds up :-).

For this reason I'd favor having that properly resolved upstream.

Thanks for getting back!

paulwouters commented 2 months ago

My comment was on a wrong PR, I thought this was our in-house PR, sorry :)

I do think redis should fix that, but I would also be happy to have fedora just install the file if upstream won't