microsoft
[vcpkg] Controlling top-level find_package
Based on recent changes to vcpkg.cmake, this PR adds two behaviours which I was looking for in the past, both related to being able to control (automatic) dependencies with regard to installation order.
Tracing find_package
Merged as https://github.com/microsoft/vcpkg/pull/27982.
Controlling top-level find_package
Some projects enable behaviours automatically based on results of top-level find_package calls. To implement port feature control without patching, maintainers use CMAKE_REQUIRE_FIND_PACKAGE_<Pkg> and/or CMAKE_DISABLE_FIND_PACKAGE_<Pkg>. However, these variable maybe used as a pair, and they affect nested requirements.
The second commit adds variable (template) CMAKE_FIND_PACKAGE_<Pkg> which
- does nothing if undefined,
- makes top-level
find_package(<Pkg>)required if true, - makes top-level
find_package(<Pkg>)disabled if false, - avoid CMake warnings when
CMAKE_FIND_PACKAGE_<Pkg>=ONmeetsfind_package(<Pkg> REQUIRED).
This variable can be used easily with vcpkg_check_features.
Hints can be printed when tracing is enabled.
Example output
From gdal with -DVCPKG_TRACE_FIND_PACKAGE=ON -DVCPKG_FIND_PACKAGE_CryptoPP=0 -DVCPKG_FIND_PACKAGE_PROJ=1 -DVCPKG_FIND_PACKAGE_TIFF=0:
-- find_package(CryptoPP )
-- (disabled by VCPKG_FIND_PACKAGE_CryptoPP=0)
-- find_package(PROJ 9 CONFIG QUIET)
-- (required by VCPKG_FIND_PACKAGE_PROJ=1)
-- find_package(unofficial-sqlite3 CONFIG QUIET REQUIRED)
-- find_package(Threads QUIET REQUIRED)
-- find_package(TIFF QUIET REQUIRED)
-- using share/tiff/vcpkg-cmake-wrapper.cmake
-- find_package(LibLZMA REQUIRED QUIET)
-- using share/liblzma/vcpkg-cmake-wrapper.cmake
-- find_package(Threads )
-- find_package(JPEG REQUIRED QUIET)
-- using share/jpeg/vcpkg-cmake-wrapper.cmake
-- find_package(ZLIB REQUIRED QUIET)
-- using share/zlib/vcpkg-cmake-wrapper.cmake
-- find_package(CURL QUIET REQUIRED)
-- using share/curl/vcpkg-cmake-wrapper.cmake
-- find_package(OpenSSL 3 QUIET REQUIRED)
-- using share/openssl/vcpkg-cmake-wrapper.cmake
-- find_package(PkgConfig QUIET)
-- find_package(Threads )
-- find_package(Threads REQUIRED)
-- find_package(ZLIB 1 QUIET REQUIRED)
-- using share/zlib/vcpkg-cmake-wrapper.cmake
-- find_package(ZSTD NAMES zstd)
-- (could be controlled by VCPKG_FIND_PACKAGE_ZSTD)
NB: The nested find_package(TIFF) is not affected by VCPKG_FIND_PACKAGE_TIFF=0.
(FTR port gdal shouldn't use VCPKG_FIND_PACKAGE_<Pkg> because it has proper feature control.)
Use cases
- https://github.com/microsoft/vcpkg/pull/14333#discussion_r1171859003
FTR: The tracing part can be removed anytime without consequence. However, the control part creates dependencies between ports and the proposed behaviour. Suggesting team review.
This seems to do something similar to #27606 and I like the idea behind this change.
I don't understand how "bad" (i.e. without REQUIRED) and "good" find_package calls can be differentiated?
Can you give an example on how the output of bad find_package calls would look like?
Still has the problem of consecutive calls being wrongly required.
find_package(<Pkg> CONFIG) <- allowed to fail.
find_package(<Pkg> REQUIRED)
I like the trace however.
Still has the problem of consecutive calls being wrongly required.
True, but I haven't seen this pattern very often. (And your example even ends with REQUIRED).
It is just an offer for a common pattern.
I like the trace however.
I admit it isn't always as nice. The find_modules will insert extra output in many cases. But tracing remains possible. And in particular, you can grep for -- find_package to find top-level dependencies.
I don't understand how "bad" (i.e. without
REQUIRED) and "good"find_packagecalls can be differentiated? Can you give an example on how the output of badfind_packagecalls would look like?
I can't give such an example because this is out of scope (if I understand at all what you mean).
The point is to see all find_package calls (for a given execution path) and to identify top-level calls.
Continuing my comment from https://github.com/microsoft/vcpkg/pull/27606, the top-level packages should refer to direct dependencies. And apart from stdout, the tracing might go to a separate file which can be post-processed.
pattern very often.
All qt6 ports do that. Component based lookup is also a possibilty
Does the indentation reflect transitive dependencies?
I can't give such an example because this is out of scope (if I understand at all what you mean). The point is to see all find_package calls (for a given execution path) and to identify top-level calls.
In the output above I would consider
-- find_package(ZSTD NAMES zstd)
-- (could be controlled by VCPKG_FIND_PACKAGE_ZSTD)
bad because the configure step wouldn't fail if ZSTD could not be found for some reason.
In the output above I would consider
-- find_package(ZSTD NAMES zstd) -- (could be controlled by VCPKG_FIND_PACKAGE_ZSTD)bad because the configure step wouldn't fail if
ZSTDcould not be found for some reason.
This is exactly the kind of pattern the VCPKG_FIND_PACKAGE_ZSTD is meant to control, either directly or bound to a feature via vcpkg_check_features.
FTR there is another broken pattern: using include(Find<Pkg>) instead of include(<Pkg>).
- It circumvents the wrappers.
- It circumvents the tracking of nesting depth.
@BillyONeal Can the team look at this proposal? It would simplify to resolve unguarded function calls as noted in https://github.com/microsoft/vcpkg/pull/29070#issuecomment-1406008823. The next step might be to make vcpkg_cmake_configure even warn about uncontrolled top-level find_package calls, similar to the current warning about unused variables.
We do want something like this but haven't had time to seriously look yet :(. Thanks!
We do want something like this but haven't had time to seriously look yet :(. Thanks!
@BillyONeal Do you maybe now have time to do it? :)
@BillyONeal Do you maybe now have time to do it? :)
I don't expect it to happen in this planning cycle because getting a good answer to the tools situation, possibly solving the alternatives problem, and some artifacts stuff blowing up in important partners' faces are all higher priority things. I think Neumann-A had a starting point on what it would look like. (Different toolchain file that uses the new cmake dependency provider thingy they added a version or two ago)
Oh I got confused as to what this was actually doing. Still not sure on priority of it vs. those other bits I mentioned.
The second commit adds variable (template)
CMAKE_FIND_PACKAGE_<Pkg>which
I guess you mean VCPKG_FIND_PACKAGE_<Pkg>?
Is there some use case I'm missing where it should be REQUIRED only at top level?
I think the case is in your post: We have no control over find_package in transitive dependencies. They may intentionally try repeated calls with different parameters (CONFIG, NO_CONFIG, VERSION, COMPONENTS X, HINTS ...). REQUIRED would make the first attempt required.
And the transitive code comes from other ports (= dependencies) or CMake. It changes independently. What builds now might fail when a dependency is updated. Maintainers of independent ports generally do not like to fix dependent ports. It is better to not increase coupling.
They may intentionally try repeated calls with different parameters (CONFIG, NO_CONFIG, VERSION, COMPONENTS X, HINTS ...).
REQUIREDwould make the first attempt required.
Makes sense.
If this PR gets a chance now, I can try to add a unit test.
The PR is getting a chance now :). The holdup has always been that we only had one reviewer who was confident enough in this CMake surgery to do a real review of something like this but he is rarely available to do reviews like that since he has manager-y things to do (that being @ras0219-msft ). In the last couple of years I think I finally understand this area well enough to review it.
@ras0219-msft is concerned that this exposes a 'new feature' to people who aren't using vcpkg at all and wants these to only do something when they are inside a port somehow. Copying out the command line from vcpkg_cmake_configure should work though, so perhaps we need a Z_BUILDING_IN_A_PORT or similar to trigger this.
Also I withdraw requesting "TOP_LEVEL" in the name but I would still like "LOCK". I can make that change if you want...
Copying out the command line from
vcpkg_cmake_configureshould work though, so perhaps we need aZ_BUILDING_IN_A_PORTor similar to trigger this.
It doesn't depend on building ports, it depends on using the vcpkg toolchain.
I can make that change if you want...
If you can do it now, it might speed it up. I will avoid pushes until July 14.