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

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

Open
mcepl opened this issue Feb 21, 2023 · 14 comments · Fixed by #159 · May be fixed by #161
Open

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

mcepl opened this issue Feb 21, 2023 · 14 comments · Fixed by #159 · May be fixed by #161

Comments

@mcepl
Copy link

mcepl commented Feb 21, 2023

#include <hiredis/sdsalloc.h>

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
Copy link
Contributor

chayim commented Feb 23, 2023

@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
Copy link
Author

mcepl commented Feb 23, 2023

@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
Copy link
Contributor

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
Copy link
Contributor

Apteryks commented Mar 18, 2023

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 added a commit to Apteryks/hiredis-py that referenced this issue Mar 18, 2023
Fixes redis#158.

* src/pack.c: Replace sdsalloc.h with alloc.h.
(pack_command): Replace s_malloc with hi_malloc.
bmwiedemann pushed a commit to bmwiedemann/openSUSE that referenced this issue Mar 19, 2023
https://build.opensuse.org/request/show/1072863
by user mcepl + dimstar_suse
- Update to 2.2.2:
  - Reverting gcc -BSymbolic due to symbol collisions
  - Add pack_command to support writing via hiredis-py
  - Fixing broken windows builds on python < 3.8
  - Fix url in Issue tracker
  - Restores publishing of source distribution
  - Supporting hiredis 1.1.0
  - Modernizing: Restoring CI, Moving to pytest
  - Adding LICENSE to Repository
  - Python 3.11 trove, and links back to the project
  - Integrating release drafter
  - Implement pack_command that serializes redis-py command to
    the RESP bytes object.
- Add 159-sdsalloc-to-alloc.patch (gh#redis/hiredis-py#158),
  which replaces use of sdsalloc with plain alloc.
Apteryks added a commit to Apteryks/hiredis-py that referenced this issue Mar 19, 2023
Fixes redis#158 fully, including using a system-prodived hiredis.

When the hiredis git submodule hasn't been initialized, print a
message about it, and attempt to link against the a system-provided
hiredis library instead.

* setup.py (is_hiredis_bundled): New procedure.
(get_hiredis_bundled_sources): Likewise.  Print a message when
bundled_hiredis_sources is empty.
(get_sources): Adjust to use the above procedure.
(get_linker_args): Add -lhiredis when the bundled hiredis is not used.
Apteryks added a commit to Apteryks/hiredis-py that referenced this issue Mar 19, 2023
Fixes redis#158 fully, including using a system-provided hiredis.

When the hiredis git submodule hasn't been initialized, print a
message about it, and attempt to link against the a system-provided
hiredis library instead.

* setup.py (is_hiredis_bundled): New procedure.
(get_hiredis_bundled_sources): Likewise.  Print a message when
bundled_hiredis_sources is empty.
(get_sources): Adjust to use the above procedure.
(get_linker_args): Add -lhiredis when the bundled hiredis is not used.
@Apteryks
Copy link
Contributor

@mcepl hi! you may be interested in #159 and #161 to allow using a system-provided hiredis library.

@mcepl
Copy link
Author

mcepl commented Mar 19, 2023

@chayim
Copy link
Contributor

chayim commented Jun 5, 2023

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

@bugant
Copy link

bugant commented Dec 4, 2023

@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 gerzse closed this as completed in f4dd081 Jul 11, 2024
@gerzse gerzse reopened this Jul 11, 2024
@gerzse
Copy link
Contributor

gerzse commented Jul 11, 2024

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
Copy link
Contributor

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
Copy link
Contributor

@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
Copy link

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
Copy link
Contributor

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
Copy link

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

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