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

[cmake] propagate dependencies #1312

Merged
merged 3 commits into from
Dec 8, 2022
Merged

[cmake] propagate dependencies #1312

merged 3 commits into from
Dec 8, 2022

Conversation

fabiencastan
Copy link
Member

No description provided.

@fabiencastan fabiencastan added this to the 2.5.0 milestone Dec 7, 2022
@simogasp
Copy link
Member

simogasp commented Dec 7, 2022

are we using the target ZLIB::ZLIB to link with zlib? This is the only case in which we should add find_dependency().
Or it could be another lib we depend on that switched to the modern cmake style but it should be its responsibility to add it in its config file. If we don't have a direct dependency it shouldn't be adding that.

@simogasp
Copy link
Member

simogasp commented Dec 7, 2022

ok i see it's here

While at it we can remove the last two lines with ${ZLIB_INCLUDE_DIR} in AliceVision/src/aliceVision/mvsData/CMakeLists.txt

alicevision_add_library(aliceVision_mvsData
  SOURCES ${mvsData_files_headers} ${mvsData_files_sources}
  PUBLIC_LINKS
    aliceVision_system
    aliceVision_numeric
    aliceVision_image
    ZLIB::ZLIB
    Boost::filesystem
    Boost::boost
    OpenImageIO::OpenImageIO
    OpenImageIO::OpenImageIO_Util
  PUBLIC_INCLUDE_DIRS
   ${ZLIB_INCLUDE_DIR}
)

so manually adding the include is a duplicate
Copy link
Member

@simogasp simogasp left a comment

Choose a reason for hiding this comment

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

LGTM

@fabiencastan fabiencastan merged commit c8d74b8 into develop Dec 8, 2022
@fabiencastan fabiencastan deleted the cmake/propagateDeps branch December 8, 2022 07:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants