Skip to content

Conversation

remicollet
Copy link

For distribution package (ex: RPM) the build is expected to be run offline.

  • add RAPIDJSON_SOURCE_DIR to avoid RapidJSON download

  • add VALKEY_INCLUDE_DIR to avoid valkey download

ex: -DVALKEY_INCLUDE_DIR=/usr/include as the valkey-devel package provides this header.

- add VALKEY_INCLUDE_DIR to avoid valkey download
- add RAPIDJSON_SOURCE_DIR to avoid RapidJSON download
@remicollet
Copy link
Author

For your information, I'm working on RPM packaging of valkey, and its modules

My work is available in my private repository, and may be integrated in Fedora official repository later

See https://blog.remirepo.net/post/2025/08/01/Valkey-version-8.1
And https://git.remirepo.net/cgit/rpms/valkey/valkey-json.git/tree/


if(ENABLE_UNIT_TESTS OR ENABLE_INTEGRATION_TESTS)
add_dependencies(${OBJECT_TARGET} valkey)
endif()
Copy link
Author

Choose a reason for hiding this comment

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

Is this dependency really needed ?

It blocks offline build (without valley tree)
build with BUILD_RELEASE, without ENABLE_UNIT_TESTS and without ENABLE_INTEGRATION_TESTS works

So I make it conditional

Copy link
Member

Choose a reason for hiding this comment

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

the BUILD_RELEASE would require ./include/valkeymodule.h and thats why we would need the valkey
But I think would be resolved if we provide the path-to-valkey

We would also need to make changes to the CI to include VALKEY_INCLUDE_DIR

@roshkhatri
Copy link
Member

roshkhatri commented Aug 25, 2025

I think this would also resolve: #10

Can you also please sign-off the commits?

@roshkhatri
Copy link
Member

We would also need to checkout valkey and add the path to the tests in order to run them as an offline build.

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.

2 participants