-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
libwallet_api_tests: add missing dependency on boost locale #4847
Conversation
The message says on boost locale but the patch adds icu libs ? |
I'm not too familiar with this locale business, but the Boost Locale library seems to depend on some ICU libraries. Replacing
|
AFAIK. boost_locale depends on libicu. And monero depend on boost_locale. |
I chose to use if(DEPENDS)
set(ICU_LIBRARIES ${Boost_LOCALE_LIBRARY} sicuio sicuin sicuuc sicudt sicutu iconv)
else()
set(ICU_LIBRARIES ${Boost_LOCALE_LIBRARY} icuio icuin icuuc icudt icutu iconv) which is used (only) by simplewallet. Would you like the following instead? ${Boost_LOCALE_LIBRARY} iconv |
That's... peculiar for ICU libs to include boost. That'd be like defining libcrypto.so to include libwallet.so just because wallet depend son crypto. I've just double checked that libicu does not depend on boost, just in case :) It feels wrong to build upon that since it's wrong. My opinion would be to add: |
480051b
to
6d3311a
Compare
Agreed, that style (made in #3313) seems quite odd. How about this fix? |
This seems much better, thanks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed
No description provided.