Skip to content

Updates needed for replacing hiredis in the valkey repo#182

Merged
bjosv merged 5 commits intovalkey-io:mainfrom
bjosv:adaptable
Apr 2, 2025
Merged

Updates needed for replacing hiredis in the valkey repo#182
bjosv merged 5 commits intovalkey-io:mainfrom
bjosv:adaptable

Conversation

@bjosv
Copy link
Copy Markdown
Collaborator

@bjosv bjosv commented Mar 21, 2025

  • Add SDS_INCLUDE_DIR and DICT_INCLUDE_DIR to be able to replace sds and dict.
  • Handle sdsrange() API difference
    The sdsrange in Valkey return void and performs no length check.
  • Use same return type as Valkey from the hashFunction in dictType
    Fixes issues on 32bit and macOS
  • Update the nodeIterator memory blob to fit Valkeys larger dict iterator.

Changes needed in valkey repo:
valkey-io/valkey@unstable...bjosv:valkey:refs/heads/use-libvalkey

@zuiderkwast
Copy link
Copy Markdown
Collaborator

  • Moved sds and dict to own directory.

  • Add Makefile option USE_INCLUDE to be able to replace sds and dict with types provided in the Valkey repo.
    The build in valkey will use USE_INCLUDE=../../src/ (relative to deps) to provide its sds.h and dict.h.

Do they need to be in a separate sub directory? Can't they stay where they are and when it's built and linked by valkey, we just give precedence to valkey's own src dir?

I could also accept specific variables like SDS_INCLUDE_DIR, DICT_INCLUDE_DIR, etc. to explicitly indicate these two libs.

In your valkey branch, I don't see how you link libvalkey with valkey's sds and dict. When linking e.g. valkey-cli, we link to libvalkey.a, which is built without dict and sds?

@bjosv
Copy link
Copy Markdown
Collaborator Author

bjosv commented Mar 24, 2025

Do they need to be in a separate sub directory? Can't they stay where they are and when it's built and linked by valkey, we just give precedence to valkey's own src dir?

It should be possible, this was to follow the initial talks how it could be done. I'll try it so we can see how it looks.

In your valkey branch, I don't see how you link libvalkey with valkey's sds and dict. When linking e.g. valkey-cli, we link to libvalkey.a, which is built without dict and sds?

When building valkey-cli I added sds.o and sds's dependencies to ENGINE_CLI_OBJ.
When linking valkey-cli with libvalkey the linker already knows sds and since the cli is not using, nor will incorporate, the cluster.c or async.c code the dict type is not required.

ENGINE_CLI_OBJ=anet.o ... sds.o util.o sha256.o

Copy link
Copy Markdown
Collaborator

@michael-grunder michael-grunder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Smaller surface area change than I would've guessed.

bjosv added 5 commits April 1, 2025 13:06
- Add SDS_INCLUDE_DIR and DICT_INCLUDE_DIR to be able to
  replace the types sds and dict.
- Search for sds.h and dict.h in the include path first.

Signed-off-by: Björn Svensson <bjorn.a.svensson@est.tech>
Add SDS_INCLUDE_DIR and DICT_INCLUDE_DIR to be able to
replace the types sds and dict.

Signed-off-by: Björn Svensson <bjorn.a.svensson@est.tech>
Signed-off-by: Björn Svensson <bjorn.a.svensson@est.tech>
Fixes issues on 32bit and macOS

Signed-off-by: Björn Svensson <bjorn.a.svensson@est.tech>
Signed-off-by: Björn Svensson <bjorn.a.svensson@est.tech>
@bjosv
Copy link
Copy Markdown
Collaborator Author

bjosv commented Apr 1, 2025

Added support to replace dict/sds in cmake builds which also is used in valkey.

@zuiderkwast
Copy link
Copy Markdown
Collaborator

Still draft?

@bjosv bjosv marked this pull request as ready for review April 1, 2025 13:23
@bjosv
Copy link
Copy Markdown
Collaborator Author

bjosv commented Apr 1, 2025

Still draft?

Seems ready now. The test-build to use the latest changes passed in valkey.

@zuiderkwast
Copy link
Copy Markdown
Collaborator

OK, feel free to merge then.

@bjosv bjosv merged commit 0b10912 into valkey-io:main Apr 2, 2025
45 checks passed
@bjosv bjosv deleted the adaptable branch April 2, 2025 06:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants