[vcpkg] Document vcpkg cmake wrappers

[dg0yt]

• What does your PR fix?

  This PR fixes the lack of documentation for vcpg cmake wrappers. It is a start - some topics remain to be added.

• Which triplets are supported/not supported? Have you updated the CI baseline?

  not relevant

• Does your PR follow the maintainer guide?

  yes

• If you have added/updated a port: Have you run ./vcpkg x-add-version --all and committed the result?

  not relevant

[Hoikas]

Shouldn't this be added instead of removed?

[dg0yt]

In fact, it is added.
One cannot add it blindly, because it may already be in the list. The given code removes it blindly (which is always allowed), and then explicitly adds CONFIG to the actual _find_package call.

[Hoikas]

I think it would be good to explain why this module is inappropriate. My understanding was the opposite--that it was the preferred approach.

[Neumann-A]

It only correctly works in the original implemented find_package function and the wrapper is outside of it.

[dg0yt]

It is intended for config files.
Cf. CMake's documentation:

Note The call to return() makes this macro unsuitable to call from Find Modules.

https://cmake.org/cmake/help/v3.21/module/CMakeFindDependencyMacro.html

(Reminder to self: CMake spelling is "Find Modules".)

[Hoikas]

Right, I know what the intention is. But because the wrappers are include()ed, I don't understand why:

Note The call to return() makes this macro unsuitable to call from Find Modules.

Is still relevant. That line implies that calling return() in a find module is a bad thing(TM), but the wrappers are included, so I must be missing something.

[dg0yt]

@Neumann-A got it right, but it probably needs an extra section in this guide because I already fell over it on another occassion.

It only correctly works in the original implemented find_package function
and the wrapper is outside of it.

The point is that find_dependency relies on Package File Interface Variables which are defined by CMake's original implementation of find_package: CMAKE_FIND_PACKAGE_NAME, <PackageName>_FIND_QUIETLY etc.

But the wrapper is included from a macro in vcpkg.cmake, and the Package File Interface Variables are not available. find_dependency simply fails to achieve the side effects which are its key feature.

[Hoikas]

Thanks for the explanation. I think a synopsis of that would be great in the documentation.

[Neumann-A]

Should be dependent on the CMake version the port internally uses itself. If the ports CMakeLists.txt is requiring 3.17 I think we should also require that in the wrapper since the -Config.cmake will probably not work with an earlier version

[dg0yt]

Indeed. Now this needs some good wording.
"In general, wrappers shall not use language features of CMake newer than version 3.4. When loading config files, wrappers may use language features of the version required by the config files or port. In this case, an explicit version check and error message shall be added in order to inform vcpkg users."

[Neumann-A]

Suggested change

	- If a wrapper creates CMake targets, these targets shall be namespaced by
	- If a wrapper creates additional/new CMake targets, these targets shall be namespaced by

[dg0yt]

I think I understand the intention: "not provided by CMake or upstream". Is a single extra word enough to make it clear for new contributors?

[Neumann-A]

Yeah that was the intention. we are free to add the same targets before/as a Find does to fix the target before the module messes it ups. This only works because all modules do a if(NOT TARGET <sometarget>) check

[Neumann-A]

why declare an extra variable? theoretically it is often used as if(<somefeature>).

[dg0yt]

This is written with the tiff wraper in mind. There aren't variables like <somefeature> which reflect the individual features - it would be necessary to create them first.
But the preceding paragraph might be extended reflect the recommendation to prefer <somefeature> variables (replacements) if they exist:

"If the portfile sets variables reflecting individual dependencies, it is recomended to use these variables directly:
if(@FEATURE_VARIABLE@)
Alternatively, the wrapper may capture the FEATURES variable:
..."

[Neumann-A]

There aren't variables like which reflect the individual

vcpkg_check_features does a create variables. So we could either use those or just directly pass -DZ_VCPKG_FEATURES=${FEATURES} via vcpkg_configure_make. No need to create Z_VCPKG_FEATURES in the wrapper

[dg0yt]

vcpkg_check_features does a create variables.

Just one variable, and not in a way which has any added value here: Basically, it transforms variable FEATURES to some <OUT_VAR> which will e.g. contain -DENABLE_ZLIB=ON where FEATURES has an item zlib.

When <OUT_VAR> is passed to cmake, cmake will create a (cache) variables - in project mode while building the port. But these variables are not available in the wrapper which runs in consuming projects, and also not in the portfile which calls configure_file.

[dg0yt]

So we could either use those or just directly pass -DZ_VCPKG_FEATURES=${FEATURES} via vcpkg_configure_make.

This goes to the ported software, but this software knows nothing about wrappers (without patching).

And the software consuming port tiff knows nothing about the features built into port tiff - it must be baked into the tiff wrapper.

[Neumann-A]

ah yeah forgot that the wrapper needs to be hardcoded. I still prefer if(NOT "@<somefeature>@" STREQUAL "")

[dg0yt]

Okay. Well, I don't see a <somefeature> most of the time.
To use FEATURES without another variable, I can offer once more:

if(";@FEATURES@;" MATCHES ";my-feature;")

Last not least we may add a vcpkg function if this leads to a clean approach.

[dg0yt]

Okay. I now realized that "vcpkg_check_features does create variables", and how. It is documented.

However, I'm really surprised:

• There are three examples in the documentation - and none uses anything but FEATURE_OPTIONS.
• There is no need to make OUT_FEATURE_OPTIONS a mandatory parameter when the function has useful effects even without it.

But I don't want to start a vcpkg_check_features here. I will update this PR to avoid @FEATURES@.

[dg0yt]

I tested this for port tiff. There is one aspect which I dislike about using the individual variables from vcpkg_check_features directly: The if statements in the generated wrapper don't give any hint about the actual condition:

-if("zip" IN_LIST _tiff_features)
+if(ON)

The upside is that typos in variable names become quickly visible, because empty replacements lead to illegal syntax (if()).

[dg0yt]

One more lesson from the tiff changes:
if(ON) needs policy CMP0012. This means uo to three more lines (push, set, pop), to cater for ports which have a very low cmake_minimum_required(VERSION ...) (here: port libgeotiff).

[Neumann-A]

Suggested change

	## Enforce loading of exported config files
	If a port exports config files which provide accurate information as expected
	by users of the find module, wrappers may enforce the loading of the config
	files by adjusting the argument list:
	```cmake
	list(REMOVE_ITEM ARGS "NO_MODULE")
	list(REMOVE_ITEM ARGS "CONFIG")
	list(REMOVE_ITEM ARGS "MODULE")
	_find_package(${ARGS} CONFIG)
	```

This is not allowed in general. Make normal Find<Modules>.cmake work with the wrapper instead!

[dg0yt]

Not allowed, or simply not possible? I tried to express the limitations in the first part of the sentence.
I agree with the recommendation to make regular Find Modules just work. Loading config files is a road into the CMake language features jungle, limiting usability.

[Neumann-A]

Not allowed, or simply not possible?

not possible. Since you do not know if a custom Find with a same name is used which sets completely different variables/targets. by rerouting to config it is not possible to patch just the Find but the whole build-system needs to be patched.

[ras0219-msft]

This is awesome, thank you so much for writing all this tribal knowledge down!

I think a lot of the suggestions above are really great, so I'm marking this PR as a draft while you have a chance to look through them and apply the ones you'd like. Once you're ready for this to be merged, just mark it ready for review :)

[dg0yt]

On <PKG>_LIBRARY:

Some find modules and wrappers use select_library_configuration to merge <PKG>_LIBRARY_RELEASE and <PKG>_LIBRARY_DEBUG into <PKG>_LIBRARY. However, some find modules (FindJPEG.cmake before CMake 3.12), and possibly user projects, rely on <PKG>_LIBRARY being a singe path, e.g. using get_filename_component on this variable.

Apart from matching the particular find module behaviour, I wonder if it would be preferred to generally avoid select_library_configuration in wrappers, and rely on a single find_library(<PKG>_LIBRARY NAMES ... NAMES_PER_DIR) instead, with expected debug and release names, leveraging the toolchain's debug/release order and the known binary names?
This could still be limited to vcpkg provided binaries by a PATHS argument containing vcpkg's lib paths.

However, I have no idea if this works with the multi-config generators.

• The binary would be selected at cmake configuration time, not at cmake generation time.
• This would be based the basic release/debug build type configuration, not the mapping of additional build types.

(I keep coming back to the idea of encapsulating this stuff in a helper function. Such a function might provide a user variable to change behaviour when needed in a user project.)

[Neumann-A]

However, some find modules (FindJPEG.cmake before CMake 3.12), and possibly user projects, rely on _LIBRARY being a singe path, e.g. using get_filename_component on this variable.

In general I never found a valid case of get_filename_component of something find_library returned. Most of those are just hacks and generally not necessary.
In general I expect downstream to fix those cases and switch to use proper targets instead.

and rely on a single find_library(_LIBRARY NAMES ... NAMES_PER_DIR) instead,

This breaks multi configuration generators.
Multi config generators need both variables to be set at configure time. It even breaks the generated *Targets.cmake if the knowledge of both variants are not supplied at configure time. (e.g. missing genex in interface_link_libraries)

[Neumann-A]

select_library_configurations should only be called after find_package(). Either the module itself already called it then it is unnecessary or the module used <PKG>_LIBRARY to maybe set IMPORTED_LOCATION of a target. In this case we cannot modify <PKG>_LIBRARY before find_package()

[dg0yt]

This section is about (older) modules (versions) that do not call select_library_configurations but do use find_library(<PKG>_LIBRARY ...). So the "Either" part is out of scope.

I try to reflect the argument against calling select_library_configurations and setting cache variable <PKG>_LIBRARY before find_package() in this case. IMPORTED_LOCATION is documented to be a Full path, not a list. This is in line with the problem for get_filename_component(... <PKG>_LIBRARY ...). However, <PKG>_LIBRARY has no multi-config alternative, but IMPORTED_LOCATION has.

I don't think this can generally be resolved. But maybe the scope of support for older versions of CMake in user projects should be explicitly limited to

• that CMake version being at least 3.4,
• the config files used by the project supporting that CMake version,
• the user project, and the CMake Find modules used by the project,
  • either not using config-unaware CMake features (e.g.
    target property IMPORTED_LOCATION, get_filename_component(... <PKG>_LIBRARY ...))
  • or using single-config generators.

(This is not for the wrapper documentation but for the CMake requirements.)

[dg0yt]

This breaks multi configuration generators.
Multi config generators need both variables to be set at configure time. It even breaks the generated *Targets.cmake if the knowledge of both variants are not supplied at configure time. (e.g. missing genex in interface_link_libraries)

That's why it could be only an opt-in alternative. But to implement a global choice, it needs a single function implementing that choice.

[JackBoosY]

Any progress here?

[dg0yt]

Well, I could do an update, based on the experiences collected.

[JackBoosY]

Consider to finish this PR now?

[JackBoosY]

Ping for response.

[JackBoosY]

Please temporary close this PR if you don't have spare time to finish it.

[JackBoosY]

Closing this PR since it seems that no progress is being made. Please reopen if work is still being done.

[dg0yt]

I cannot reopen. Stop closing my drafts.

[ras0219-msft]

This is incredibly useful information and it's a shame it has been sitting for so long! Thank you so much for writing it, @dg0yt.

Since our new docs are in Microsoft/vcpkg-docs, I've taken liberty to migrate the work over to a branch MicrosoftDocs/vcpkg-docs@main...ras0219-msft:vcpkg-docs:pr-19692. This branch currently contains two commits:

1. ras0219-msft/vcpkg-docs@18098d3, which is a (near) direct squash of this PR so far
2. MicrosoftDocs/vcpkg-docs@3a31546 is a bunch of editorializing I made on top that I believe makes it ready to merge.

I've attempted to apply all the critical comments in this PR, though I've probably missed some things. Most importantly, I've rewritten the recommendation on how to bake in features:

# portfile.cmake
vcpkg_check_features(OUT_FEATURE_OPTIONS FEATURE_OPTIONS
    PREFIX feature
    FEATURES
        ZLIB WITH_ZLIB
        TIFF WITH_TIFF
)
configure_file(
    "${CMAKE_CURRENT_LIST_DIR}/vcpkg-cmake-wrapper.cmake.in"
    "${CURRENT_PACKAGES_DIR}/share/<pkg>/vcpkg-cmake-wrapper.cmake"
    @ONLY
)

# vcpkg-cmake-wrapper.cmake.in
if("@feature_WITH_ZLIB@" STREQUAL "ON") # feature_WITH_ZLIB
    find_package(ZLIB)
    list(APPEND <Pkg>_LIBRARIES ${ZLIB_LIBRARIES})
    if(TARGET <Pkg>::Tgt)
        set_property(TARGET <Pkg>::Tgt APPEND PROPERTY INTERFACE_LINK_LIBRARIES ZLIB::ZLIB)
    endif()
endif()

This resolves both the problem of IN_LIST requiring a policy and needing to reset Z_VCPKG_FEATURES after each subordinate call to find_package() (which could trigger a different wrapper that itself sets Z_VCPKG_FEATURES!).

@dg0yt How would you like to proceed with the work you've done here? My suggestions are:

1. You can grab the first or both commits from my branch and open a PR against vcpkg-docs.
2. Tell me to handle it and I'll open a PR in the new repo and get it merged.

[dg0yt]

I'm hesitating to put time into this at the moment.

TBH I would prefer to discourage the current form of wrappers and get to an implementation based on vcpkg find modules which call into external find modules.

• The wrappers can't see the package interface variables.
• They can't tell module mode from config mode. (Some look for CONFIG, NO_MODULE or MODULE, but each keyword of the full find_package signature activates config mode.)
• They depend on undocumented CMake behaviour (overriding built-in commands).
• The find_package macro has problems with policy scopes: find_package macro breaks CMake policy propagation #28960.

[dan-shaw]

Closing this PR for now, as our docs have moved. Please open a new PR there if you are still working on this. Thanks!