-
Notifications
You must be signed in to change notification settings - Fork 93
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
FreeType: improve library dependency handling, add example #320
Conversation
zlib/project_include.cmake
Outdated
set(ZLIB_FOUND 1 CACHE BOOL "") | ||
set(ZLIB_INCLUDE_DIR ${CMAKE_CURRENT_LIST_DIR}/zlib CACHE PATH "") | ||
get_filename_component(component_name ${CMAKE_CURRENT_LIST_DIR} NAME) | ||
set(ZLIB_LIBRARY idf::${component_name} CACHE STRING "" FORCE) |
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.
Hello @igrr ,
with my limited understanding it looks ok to me. Maybe few questions if you don't mind.
- Do we need to keep this in cache or is this intentional to prevent possible future override?
- The
idf
prefix seems hardcoded inesp-idf
, so it should be fine. But I'm wondering if this can change. Meaning is it possible that the target/alias will have different prefix thanidf
? - I guess that using e.g.
ZLIBConfig.cmake
and CMAKE_FIND_PACKAGE_REDIRECTS_DIR or other cmake's ways how to find a package(config/module mode) is not suitable for this use case and IIUC it would be probably unnecessary complicated.
Thank you
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.
@fhrbata Sorry for missing your comment.
Do we need to keep this in cache or is this intentional to prevent possible future override?
I will check this again, I guess it didn't work for me without defining the variables as CACHE due to the (otherwise limited) variable scope.
The idf prefix seems hardcoded in esp-idf, so it should be fine. But I'm wondering if this can change. Meaning is it possible that the target/alias will have different prefix than idf?
This is a good point! I guess it would be unreasonable to change this prefix now, as many components were already written under the assumption that alias targets such as idf::component_name
exist...
CMAKE_FIND_PACKAGE_REDIRECTS_DIR
I didn't try this. I will need more time to check if this works.
For now I have removed Zlib changes from this PR, leaving just the fix to FreeType dependencies configuration, and the addition of the new example.
I will open another PR for FindPackage support once I check the two points mentioned above.
4f9b34e
to
16435e6
Compare
@espressif2022 PTAL as well |
cc @espzav |
@espressif2022 @suda-morris Could you please help review? |
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.
@igrr Nice example! Thank you (I am thinking that it can be used for example for BSP NOGLIB - for testing)
LGTM
Thanks for the review @espzav! |
Updated Zlib library to be compatible with CMakeFindPackage
module. Now, third party libraries which callfind_package(ZLIB)
will findespressif/zlib
component automatically, provided that it has been added as a dependency into the project.We should do the same in other common components, as well.Updatedfreetype
component to have a dependency onzlib
Configured
freetype
to not look for Brotli, Harfbuzz, BZ2, libpng, zlib libraries in the system (espressif__freetype component doesn't build (IEC-101) #317)Added a simple example to
freetype
component to validate its operation, added pytest script