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

Enable building with system hiredis #378

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

orgads
Copy link
Contributor

@orgads orgads commented Sep 23, 2024

No description provided.

davehorton added a commit that referenced this pull request Sep 23, 2024
Copy link
Collaborator

@davehorton davehorton left a comment

Choose a reason for hiding this comment

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

This was clearly not tested with a local hiredis install. In my testing if fails to build.

/usr/bin/ld: /home/runner/work/drachtio-server/drachtio-server/build/../src/blacklist.cpp:216: undefined reference to `redisFree'
/usr/bin/ld: src/drachtio-blacklist.o: in function `drachtio::QuerySentinel(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, boost::asio::ip::basic_endpoint<boost::asio::ip::tcp> const&, std::unordered_set<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::hash<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, std::equal_to<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, std::allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > >&)':
/home/runner/work/drachtio-server/drachtio-server/build/../src/blacklist.cpp:76: undefined reference to `redisConnect'

@orgads
Copy link
Contributor Author

orgads commented Sep 23, 2024

It was tested on docker. I'll test it further.

@orgads
Copy link
Contributor Author

orgads commented Sep 23, 2024

Sorry, I cherry-picked it from my branch with the wrong base. Should be ok now.

@davehorton
Copy link
Collaborator

@orgads
just so I understand, is your intent for this PR that drachtio server only builds with system hiredis, or that it builds with either (system provided or deps provided)?

@orgads
Copy link
Contributor Author

orgads commented Sep 26, 2024

Both work. The previous revision had -d instead of -f in the condition, so it failed when hiredis was not installed on the system. I corrected that.

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