Skip to content

Build complete library#24

Merged
michael-grunder merged 3 commits intovalkey-io:mainfrom
michael-grunder:build-complete-library
Jun 24, 2024
Merged

Build complete library#24
michael-grunder merged 3 commits intovalkey-io:mainfrom
michael-grunder:build-complete-library

Conversation

@michael-grunder
Copy link
Copy Markdown
Collaborator

This is an initial restructuring of the project to use an idiomatic linux shared object structure.

── examples
│   ├── CMakeLists.txt
│   └── Makefile
│   ├── examples1.c
│   ├── examples2.c
├── include
│   └── valkey
│       ├── adapters
│       │   ├── adapter1.h
│       │   ├── adapter2.h
│       ├── valkey.h
│       ├── other-public-headers.h
├── Makefile
├── src
│   └── internal.h
│   └── sources.c
└── tests
    ├── test.c
    ├── test1.c
    └── test.sh

@michael-grunder michael-grunder force-pushed the build-complete-library branch from 7496b5b to 193bfc8 Compare June 23, 2024 02:15
Copy link
Copy Markdown
Collaborator

@bjosv bjosv left a comment

Choose a reason for hiding this comment

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

Some early comment/questions, but I think it's very good in general.
I'll look at it a bit more.

Comment thread include/valkey/valkeycluster.h
Comment thread valkey-config.cmake.in
Comment thread examples/Makefile
Comment thread Makefile
Comment thread Makefile
Comment thread Makefile
Move all the libvalkey source files and headers into their final
location.

* Source files and internal headers in in `src/`
* Public headers in `include/valkey/` and `include/valkey/adapters`.
@michael-grunder michael-grunder force-pushed the build-complete-library branch 2 times, most recently from cd44982 to b470de8 Compare June 24, 2024 01:58
* Update Makefile  and CMakeLists.txt to build both standalone and
  cluster code.
* Update dict API to make the API non-static for cluster usage.
* Minor changes to a few examples and tests/test.c to get everything
  compiling.
* Use CMake pkg-config .in files in the Makefile

Fixes valkey-io#22
@michael-grunder michael-grunder force-pushed the build-complete-library branch from b470de8 to a0fcb70 Compare June 24, 2024 02:45
Copy link
Copy Markdown
Collaborator

@bjosv bjosv left a comment

Choose a reason for hiding this comment

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

If we fix the following issue in this PR:

/home/runner/work/libvalkey/libvalkey/src/valkeycluster.c:40:10: fatal error: 'valkey/alloc.h' file not found
#include <valkey/alloc.h>
         ^~~~~~~~~~~~~~~~

by changing to #include "alloc.h" we might already get some green CI jobs.

Comment thread Makefile Outdated
Comment thread Makefile
* Use __attribute__((unused)) for the cluster asyn attach static
  functions.  This lets us continue to build with -Werror
* Fix the tests target to include ssl objects if needed.
* Tweak CI invocation of test.sh
* Install valkey-server for tests.
* Darwin doesn't have clock_nanosleep
* Add a sanity check to test.sh
@michael-grunder michael-grunder force-pushed the build-complete-library branch from 781507f to ce9f77b Compare June 24, 2024 21:19
@michael-grunder michael-grunder marked this pull request as ready for review June 24, 2024 21:32
@michael-grunder
Copy link
Copy Markdown
Collaborator Author

Non-cluster tests are now passing on Ubuntu, CentOS and RockyLinux.

The macOS failures are actually also happening in hiredis so we'll have to investigate what's going on there.

I can probably also get a windows VM set up to try and get that building again, but I know nothing about windows.

I think we can merge this and then start in on combining the unit tests?

Copy link
Copy Markdown
Collaborator

@bjosv bjosv left a comment

Choose a reason for hiding this comment

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

LGTM! A good start.

@michael-grunder michael-grunder merged commit e28f811 into valkey-io:main Jun 24, 2024
@michael-grunder michael-grunder deleted the build-complete-library branch June 24, 2024 22:32
@bjosv bjosv mentioned this pull request Jun 25, 2024
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