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.

Sign up for GitHub

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

[vcpkg] Controlling top-level find_package #27950

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
+27 −1
Draft

[vcpkg] Controlling top-level find_package #27950

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f7f08f0
Add VCPKG_FIND_PACKAGE_<Pkg>
dg0yt Nov 22, 2022
46ed295
Merge remote-tracking branch 'origin/master' into HEAD
BillyONeal Jun 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 27 additions & 1 deletion scripts/buildsystems/vcpkg.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -782,6 +782,29 @@ macro("${VCPKG_OVERRIDE_FIND_PACKAGE_NAME}" z_vcpkg_find_package_package_name)
set(z_vcpkg_find_package_${z_vcpkg_find_package_backup_id}_ARGN "${ARGN}")
set(z_vcpkg_find_package_${z_vcpkg_find_package_backup_id}_backup_vars "")

if(z_vcpkg_find_package_backup_id EQUAL "1")
if(VCPKG_FIND_PACKAGE_${z_vcpkg_find_package_package_name})
Comment on lines +785 to +786
Copy link
Member

Choose a reason for hiding this comment

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

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

# Avoid CMake warning when both REQUIRED and CMAKE_REQUIRE_FIND_PACKAGE_<Pkg> are used
if(NOT "REQUIRED" IN_LIST z_vcpkg_find_package_${z_vcpkg_find_package_backup_id}_ARGN)
Copy link
Member

Choose a reason for hiding this comment

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

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

list(APPEND z_vcpkg_find_package_${z_vcpkg_find_package_backup_id}_backup_vars "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}}")
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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?

set(CMAKE_REQUIRE_FIND_PACKAGE_${z_vcpkg_find_package_package_name} 1)
endif()
if(VCPKG_TRACE_FIND_PACKAGE)
message(STATUS " (required by VCPKG_FIND_PACKAGE_${z_vcpkg_find_package_package_name}=${VCPKG_FIND_PACKAGE_${z_vcpkg_find_package_package_name}})")
endif()
elseif(DEFINED VCPKG_FIND_PACKAGE_${z_vcpkg_find_package_package_name})
list(APPEND z_vcpkg_find_package_${z_vcpkg_find_package_backup_id}_backup_vars "CMAKE_DISABLE_FIND_PACKAGE_${z_vcpkg_find_package_package_name}")
set(z_vcpkg_find_package_${z_vcpkg_find_package_backup_id}_backup_CMAKE_DISABLE_FIND_PACKAGE_${z_vcpkg_find_package_package_name} "${CMAKE_DISABLE_FIND_PACKAGE_${z_vcpkg_find_package_package_name}}")
set(CMAKE_DISABLE_FIND_PACKAGE_${z_vcpkg_find_package_package_name} 1)
Copy link
Member

Choose a reason for hiding this comment

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

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)

if(VCPKG_TRACE_FIND_PACKAGE)
message(STATUS " (disabled by VCPKG_FIND_PACKAGE_${z_vcpkg_find_package_package_name}=${VCPKG_FIND_PACKAGE_${z_vcpkg_find_package_package_name}})")
endif()
elseif(VCPKG_TRACE_FIND_PACKAGE)
message(STATUS " (could be controlled by VCPKG_FIND_PACKAGE_${z_vcpkg_find_package_package_name})")
endif()
endif()

# Workaround to set the ROOT_PATH until upstream CMake stops overriding
# the ROOT_PATH at apple OS initialization phase.
# See https://gitlab.kitware.com/cmake/cmake/merge_requests/3273
Expand All @@ -797,7 +820,10 @@ macro("${VCPKG_OVERRIDE_FIND_PACKAGE_NAME}" z_vcpkg_find_package_package_name)
string(TOLOWER "${z_vcpkg_find_package_package_name}" z_vcpkg_find_package_lowercase_package_name)
set(z_vcpkg_find_package_vcpkg_cmake_wrapper_path
"${_VCPKG_INSTALLED_DIR}/${VCPKG_TARGET_TRIPLET}/share/${z_vcpkg_find_package_lowercase_package_name}/vcpkg-cmake-wrapper.cmake")
if(EXISTS "${z_vcpkg_find_package_vcpkg_cmake_wrapper_path}")
if(CMAKE_DISABLE_FIND_PACKAGE_${z_vcpkg_find_package_package_name})
# Skip wrappers, fail if REQUIRED.
_find_package("${z_vcpkg_find_package_package_name}" ${z_vcpkg_find_package_${z_vcpkg_find_package_backup_id}_ARGN})
elseif(EXISTS "${z_vcpkg_find_package_vcpkg_cmake_wrapper_path}")
if(VCPKG_TRACE_FIND_PACKAGE)
string(REPEAT " " "${z_vcpkg_find_package_backup_id}" z_vcpkg_find_package_indent)
message(STATUS "${z_vcpkg_find_package_indent}using share/${z_vcpkg_find_package_lowercase_package_name}/vcpkg-cmake-wrapper.cmake")
Expand Down