[vcpkg] Controlling top-level find_package

[dg0yt]

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 #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>=ON meets find_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

• [Qt6|6.1.0] Add new ports.  #14333 (comment)

[dg0yt]

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.

[Thomas1664]

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?

[Neumann-A]

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.

[dg0yt]

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_package calls can be differentiated?
Can you give an example on how the output of bad find_package calls 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 #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.

[Neumann-A]

pattern very often.

All qt6 ports do that. Component based lookup is also a possibilty

[Thomas1664]

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.

[Cheney-W]

@dg0yt Could you please solve the conflict? Thank you!

[dg0yt]

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.

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.

[dg0yt]

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.

[dg0yt]

Ping.

[dg0yt]

@BillyONeal Can the team look at this proposal? It would simplify to resolve unguarded function calls as noted in #29070 (comment). 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.

[BillyONeal]

We do want something like this but haven't had time to seriously look yet :(. Thanks!

[autoantwort]

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]

@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)

[BillyONeal]

Oh I got confused as to what this was actually doing. Still not sure on priority of it vs. those other bits I mentioned.

[autoantwort]

The second commit adds variable (template) CMAKE_FIND_PACKAGE_<Pkg> which

I guess you mean VCPKG_FIND_PACKAGE_<Pkg>?

[BillyONeal]

Stupid question about this feature:

I understand the use case for disabling find_package only at the top level, as that helps to workaround ports doing 'optional dependencies' without transitively blocking things.

I'm not sure I understand why requiring find_package only at the top level makes sense though: if portfile.cmake is going to ask that it be REQUIRED, it had better be making sure that the dependency is present, meaning REQUIRED should be OK everywhere.

Is there some use case I'm missing where it should be REQUIRED only at top level?

Would you be happy with naming this VCPKG_LOCK_TOP_LEVEL_FIND_PACKAGE_Xxx or similar?

• LOCK to clearly say that this is going to enforce what the result is. Alternately: ENFORCE?
• TOP_LEVEL to clearly say that the critical difference is that this only affects the top level answer. But I could also see the argument that it should just always work like this so it shouldn't have the TOP_LEVEL part?

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.

I think it's fair that this feature is an improvement vs. the status quo and that projects which break from this pattern can use patches? Or is there something about this pattern uniquely broken by @dg0yt 's feature not also broken with CMAKE_Xxx_FIND_PACKAGE_Yyy?

[BillyONeal]

Stupid question: Why is "REQUIRED" in quotes but z_vcpkg_find_package_${z_vcpkg_find_package_backup_id}_ARGN not?

[BillyONeal]

Stupid question: How does this do the right thing if CMAKE_REQUIRE_FIND_PACKAGE_Blah is undefined? It looks like the backup/restore thingy below would make it be empty string instead?

[BillyONeal]

Yeah I think this is wrong and it should be:

Suggested change

	set(z_vcpkg_find_package_${z_vcpkg_find_package_backup_id}_backup_CMAKE_REQUIRE_FIND_PACKAGE_${z_vcpkg_find_package_package_name} "${CMAKE_REQUIRE_FIND_PACKAGE_${z_vcpkg_find_package_package_name}}")
	if(DEFINED "CMAKE_REQUIRE_FIND_PACKAGE_${z_vcpkg_find_package_package_name}")
	set(z_vcpkg_find_package_${z_vcpkg_find_package_backup_id}_backup_CMAKE_REQUIRE_FIND_PACKAGE_${z_vcpkg_find_package_package_name} "${CMAKE_REQUIRE_FIND_PACKAGE_${z_vcpkg_find_package_package_name}}")
	endif()

Ditto below for the 'disable' arm

[BillyONeal]

Also, should we always backup and restore both the REQUIRED and DISABLE one? So that
-DCMAKE_DISABLE_FIND_PACKAGE_Blah=1 -DVCPKG_FIND_PACKAGE_Blah=1 chooses in favor of the VCPKG_ one. Or maybe we should just make such a case an error?

[BillyONeal]

Suggested change

	if(z_vcpkg_find_package_backup_id EQUAL "1")
	if(VCPKG_FIND_PACKAGE_${z_vcpkg_find_package_package_name})
	if(z_vcpkg_find_package_backup_id EQUAL "1")
	# This is the top-level find_package call
	if(VCPKG_FIND_PACKAGE_${z_vcpkg_find_package_package_name})

[BillyONeal]

It seems like this would result in the value being still disabled in transitive find_package calls? What clears this when we're down a level?

---

Oh, Bill is an idiot:

Suggested change

	set(CMAKE_DISABLE_FIND_PACKAGE_${z_vcpkg_find_package_package_name} 1)
	# We don't need to worry about clearing this for transitive users because
	# once this top level find_package is disabled, we immediately will return
	# not found and not try to visit transitive dependencies in the first place.
	set(CMAKE_DISABLE_FIND_PACKAGE_${z_vcpkg_find_package_package_name} 1)

[dg0yt]

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.

[dg0yt]

If this PR gets a chance now, I can try to add a unit test.

[BillyONeal]

They may intentionally try repeated calls with different parameters (CONFIG, NO_CONFIG, VERSION, COMPONENTS X, HINTS ...). REQUIRED would 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.

[BillyONeal]

@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.

[BillyONeal]

Also I withdraw requesting "TOP_LEVEL" in the name but I would still like "LOCK". I can make that change if you want...

[dg0yt]

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.

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.