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

Fix build with Poco Redis #7835

Merged
merged 2 commits into from
Dec 3, 2019

Conversation

filimonov
Copy link
Contributor

I hereby agree to the terms of the CLA available at: https://yandex.ru/legal/cla/?lang=en

Changelog category (leave one):

  • Non-significant (changelog entry is not needed)

Was getting that (Debug build, clang-8)

[build] FAILED: dbms/src/Dictionaries/CMakeFiles/clickhouse_dictionaries.dir/RedisDictionarySource.cpp.o 
...
[build] ../../dbms/src/Dictionaries/RedisDictionarySource.cpp:38:14: fatal error: 'Poco/Redis/Array.h' file not found
[build] #    include <Poco/Redis/Array.h>
[build]              ^~~~~~~~~~~~~~~~~~~~
[build] 1 error generated.

@filimonov filimonov requested a review from a team November 19, 2019 08:53
@ghost ghost requested review from stavrolia and removed request for a team November 19, 2019 08:53
@filimonov
Copy link
Contributor Author

filimonov commented Nov 19, 2019

Not sure if needed. Maybe cmake cache cleanup was enough.

See also:

if (NOT DEFINED ENABLE_POCO_REDIS OR ENABLE_POCO_REDIS)
set (Poco_Redis_LIBRARY PocoRedis)
set (Poco_Redis_INCLUDE_DIR "${ClickHouse_SOURCE_DIR}/contrib/poco/Redis/include/")
endif ()

if(USE_POCO_REDIS)
# for code highlighting in CLion
# target_include_directories(clickhouse_dictionaries SYSTEM PRIVATE ${Poco_Redis_INCLUDE_DIR})
# for build
target_link_libraries(clickhouse_dictionaries PRIVATE ${Poco_Redis_LIBRARY})
endif()

@alexey-milovidov alexey-milovidov requested review from abyss7 and removed request for stavrolia November 19, 2019 23:05
@abyss7
Copy link
Contributor

abyss7 commented Nov 20, 2019

@filimonov did the cache clean-up actually help?

@alexey-milovidov alexey-milovidov added st-need-info We need extra data to continue (waiting for response) can be tested labels Nov 25, 2019
@alexey-milovidov
Copy link
Member

Yes, the cache cleanup actually helps.

Copy link
Member

@alexey-milovidov alexey-milovidov left a comment

Choose a reason for hiding this comment

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

The changes are unnecessarily but look good for consistency.

@alexey-milovidov alexey-milovidov merged commit 4159e77 into ClickHouse:master Dec 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
st-need-info We need extra data to continue (waiting for response)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants