-
Notifications
You must be signed in to change notification settings - Fork 6.3k
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
[Qt] Update to 6.6.3 #37731
[Qt] Update to 6.6.3 #37731
Conversation
# Conflicts: # ports/qtwebengine/vcpkg.json
@dg0yt Have you looked at static ffmpeg linkage yet? Seems kind of coursed from a cmake perspective |
|
list boils down to:
|
bbc3168
to
e4a0e43
Compare
I had to deal with a symbol relocation problem when linking ffmpeg libs. That's a different story. |
@Neumann-A , nice to see CIs all turned green :) |
set(PKG_CONFIG_USE_CMAKE_PREFIX_PATH ON) # Required for CMAKE_MINIMUM_REQUIRED_VERSION VERSION_LESS 3.1 which otherwise ignores CMAKE_PREFIX_PATH | ||
|
||
if(@WITH_MP3LAME@) | ||
find_package(mp3lame CONFIG ) |
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.
Don't these need the 'transitive REQUIRED' treatment find_dependency does?
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.
Theoretical yes but this is in vcpkg-cmake-wrapper.cmake.
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.
@BillyONeal: If you explicitly want I can add what libarchive is doing.
set(z_vcpkg_libarchive_args "")
if("REQUIRED" IN_LIST ARGS)
list(APPEND z_vcpkg_libarchive_args "REQUIRED")
endif()
if("QUIET" IN_LIST ARGS)
list(APPEND z_vcpkg_libarchive_args "QUIET")
endif()
find_package(BZip2 ${z_vcpkg_libarchive_args})
but find_dependency
does not work in the wrapper. It only works within a proper find_package
call which the wrapper is not.
Practically, I convinced myself that it does not make that much of a difference. You just get a better way to locate possible errors. So instead of XY::XY target not being found you'll get that package XY was not found. However you would also need to add code like:
set(package_FOUND true)
find_package(XY ${z_vcpkg_package_args})
if(NOT XY_FOUND)
# cannot early return here since the wrapper is inside a macro
# could however FATAL_ERROR if package is REQUIRED
set(package_FOUND false)
else()
# adjust what needs adjustment
endif()
I think vcpkg.cmake needs to have helper functions for these cases instead of requiring to duplicate the same logic over and over again.
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.
vcpkg_find_dependency
? #22743
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.
What about moving much of this logic into a proper config; something like ffmpeg-vcpkg-config.cmake
? Then doing something approximately like
# ffmpeg/vcpkg-cmake-wrapper.cmake
set(ffmpeg_original_args "${ARGS}")
# Use required because this should be guaranteed to resolve
_find_package(ffmpeg-vcpkg CONFIG REQUIRED)
# ffmpeg-vcpkg/ffmpeg-vcpkg-config.cmake
# prep work, checking targets / components
_find_package(${ffmpeg_original_args})
if (@WITH_SSH@)
find_dependency(libssh CONFIG)
list(APPEND FFMPEG_LIBRARIES ssh)
endif()
# ...
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.
What about moving much of this logic into a proper config; something like ffmpeg-vcpkg-config.cmake?
a) _find_package(ffmpeg-vcpkg CONFIG REQUIRED)
still needs the correct REQUIRED/QUIET
logic. You cannot simply add REQUIRED
here
b) there is little to no value rerouting through a CONFIG. vcpkg already provides the MODULE
c) rewriting the existing CONFIG in a MODULE requires time which I don't want to spend. I didn't even want to touch FFMPEG; these fixes are just required to get qtinterfaceframework
in static builds to work. I also see no added value of having a CONFIG over a MODULE
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.
What about moving much of this logic into a proper config; something like
ffmpeg-vcpkg-config.cmake
?
Another wrapper to wrap a non-official, non-unofficial name ffmpeg-vcpkg
? This solves a reuse problem, but why not just an unofficial-ffmpeg
config in that case?
But another wrapper doesn't resolve the problem of not being able to use find_dependency
in wrappers or find modules. We can use guarded find_package(... REQUIRED)
, assuming that the dependency including a cmake package is provided by dependencies. i.e. it is always an error from a vcpkg point of view if the cmake deps don't match the vcpkg deps.
# Conflicts: # versions/o-/opencv4.json
@BillyONeal ping? |
# Conflicts: # ports/vtk/vcpkg.json # versions/baseline.json # versions/v-/vtk.json
This reverts commit 8b0a8d3.
@JavierMatosD CI is green |
Fixes #37766