FindVulkan fixes. Bump CMake minimum version.

[MarkCallow]

Description

Fixes 4 issues with FindVulkan:

1. Make it work with CMake 3.20+ as well as the currently required 3.28.
2. Set CMAKE_FIND_FRAMEWORK to FIRST, the only value for which the searches work.
3. Fix if() to prevent use of a variable that is undefined when seeking for macOS.
4. Remove an errant closing brace.

Bumps the minimum cmake version for CMakeLists.txt and app/CMakeLists.txt consistent with the CMake features used. In the case of CMakeLists.txtthe version is bumped to 3.20 for consistency with app and to support use of cmake_path which is used in the accompanying fix to FindVulkan.cmake to support versions earlier than 3.28.

Fixes 1, 3 and 4 are useful to this project. All fixes are useful to people who are copying FindVulkan into their own projects. No. 2 is especially important to those using building for iOS using vcpkg which sets CMAKE_FIND_FRAMEWORK to LAST.

Fixes #1100

General Checklist:

Please ensure the following points are checked:

• [x] My code follows the coding style
• [x] I have reviewed file licenses
• [ ] I have commented any added functions (in line with Doxygen)
• [x] I have commented any code that could be hard to understand
• [x] My changes do not add any new compiler warnings
• [x] My changes do not add any new validation layer errors or warnings
• [ ] I have used existing framework/helper functions where possible
• [x] My changes do not add any regressions
• [ ] I have tested every sample to ensure everything runs correctly
• [x] This PR describes the scope and expected impact of the changes I am making

I have left unchecked items which are not relevant to this change.

Note: The Samples CI runs a number of checks including:

• [x] I have updated the header Copyright to reflect the current year (CI build will fail if Copyright is out of date)

• [x] My changes build on Windows, Linux, macOS and Android. Otherwise I have documented any exceptions

  If this PR contains framework changes:

• [ ] I did a full batch run using the batch command line argument to make sure all samples still work properly

Sample Checklist

If your PR contains a new or modified sample, these further checks must be carried out in addition to the General Checklist:

• [ ] I have tested the sample on at least one compliant Vulkan implementation
• [ ] If the sample is vendor-specific, I have tagged it appropriately
• [ ] I have stated on what implementation the sample has been tested so that others can test on different implementations and platforms
• [ ] Any dependent assets have been merged and published in downstream modules
• [ ] For new samples, I have added a paragraph with a summary to the appropriate chapter in the readme of the folder that the sample belongs to e.g. api samples readme
• [ ] For new samples, I have added a tutorial README.md file to guide users through what they need to know to implement code using this feature. For example, see conditional_rendering
• [ ] For new samples, I have added a link to the Antora navigation so that the sample will be listed at the Vulkan documentation site

[CLAassistant]

All committers have signed the CLA.

[MarkCallow]

Just spotted a completely unnecessary unset in my changes which I have removed in https://github.com/KhronosGroup/Vulkan-Samples/pull/1101/commits/11f46534cf4cf8e8f18017e62e4861c09e18c09d. Very sorry this is post your review @SaschaWillems thus invalidating it.

[marty-johnson59]

Hi @MarkCallow , we discussed this MR on the Samples call today and have reservations about merging this w/o some changes. There appears to be 2 main items covered here: 1) bumping the cmake version number; 2) changes related to FindVulkan. The first item seems to only be required for iOS builds - but it is likely to cause problems for other platforms (especially Android) where folks may have good reasons to stay with the older version. We suggest splitting this change out into a separate PR and scoping it to only iOS platforms. We aren't quite sure about the FindVulkan changes - but we think it makes sense to sync up with LunarG on this - they have plans for future versions that might impact our directions here. @gpx1000 is the most familiar with this on the team and has offered to help - Steve, please chime in as needed. Thanks!

[MarkCallow]

The first item seems to only be required for iOS builds - but it is likely to cause problems for other platforms (especially Android) where folks may have good reasons to stay with the older version. We suggest splitting this change out into a separate PR and scoping it to only iOS platforms.

How to scope to only iOS platforms? As far as I know cmake_minimum_required must appear before the project command and I know that the IOS variable is only set after the project() command. Perhaps an explicit check of the version after project, if(IOS)with a message(FATAL_ERROR ...). Suggestions welcome.

What is the minimum CMake version for which support is desired? There is no question that the XCODE_EMBED items in app/CMakeLists.txtrequire CMake 3.20. My question above relates to this file. For CMakeLists.txt, the workaround in FindVulkan.cmake for versions earlier than 3.28 can be modified to use the deprecated get_filename_component instead of cmake_path thus removing the need to up its minimum version to 3.20.

We aren't quite sure about the FindVulkan changes - but we think it makes sense to sync up with LunarG on this - they have plans for future versions that might impact our directions here. @gpx1000 is the most familiar with this on the team and has offered to help - Steve, please chime in as needed. Thanks!

I intend to sync with LunarG but currently FindVulkan is not part of the Vulkan SDK. @gpx1000's blog on developing an iOS app with the Vulkan SDK says to copy the file from here. Thus the problems need to be fixed here now not at some future time. The fixes I have provided do not break any other usage. The code to ensure CMAKE_FIND_FRAMEWORK is FIRST within FindVulkan does not affect the value outside FindVulkan. The other fixes are vanilla bug fixes.

I see no reason to split this into 2 PRs.

[gpx1000]

How to scope to only iOS platforms? As far as I know cmake_minimum_required must appear before the project command and I know that the IOS variable is only set after the project() command. Perhaps an explicit check of the version after project, if(IOS)with a message(FATAL_ERROR ...). Suggestions welcome.

What is the minimum CMake version for which support is desired? There is no question that the XCODE_EMBED items in app/CMakeLists.txtrequire CMake 3.20. My question above relates to this file. For CMakeLists.txt, the workaround in FindVulkan.cmake for versions earlier than 3.28 can be modified to use the deprecated get_filename_component instead of cmake_path thus removing the need to up its minimum version to 3.20.

I think you're right about the idea of if(IOS and version less than 3.20 in findVulkan.cmake with an assert saying minimum cmake of 3.20 is required is best. That way we're keeping the check to the file we're recommending people copy from. It also keeps the project wide changes to where they currently are for all other platforms. This reduces the amount of testing we need to do.

I intend to sync with LunarG but currently FindVulkan is not part of the Vulkan SDK. @gpx1000's blog on developing an iOS app with the Vulkan SDK says to copy the file from here. Thus the problems need to be fixed here now not at some future time. The fixes I have provided do not break any other usage. The code to ensure CMAKE_FIND_FRAMEWORK is FIRST within FindVulkan does not affect the value outside FindVulkan. The other fixes are vanilla bug fixes.

I see no reason to split this into 2 PRs.

FindVulkan.cmake should hopefully never be included in the SDK. FindVulkan.cmake is included with cmake and that is woefully out of date; and cmake is trying to remove it. Ideally, we'd transition to vulkanconfig.cmake and include that with the SDK so it's not a project only solution. However, I think FindVulkan.cmake in the current iteration should be thought of as a template. I think if you do the path I mentioned above of leaving the version bump off and inlcude an assert in FindVulkan.cmake when it's being used by cmake less than 3.20, then one PR makes sense. Otherwise, the version bump would benefit from being in a separate PR so we can make sure it works in all platforms by itself as that is a fairly big change.

[MarkCallow]

@gpx1000 I think you misunderstood what I wrote. By modifying my FindVulkan pre-CMake 3.28 workaround to use get_filename_component I can revert the cmake_required_minimum in the root CMakeLists.txt back to 3.16.

It is app/CMakeLists.txt that needs the fatal error if IOS and CMake version < 3.20 in order to remove the version bump there. I'll update the PR tomorrow.

Otherwise, the version bump would benefit from being in a separate PR so we can make sure it works in all platforms by itself

Isn't CI checking all desired supported platforms? If so they are all already passing with the PR as is.

as that is a fairly big change.

The 2 minor bug fixes in FindVulkan aside, the change there only affects searches on iOS and the Vulkan::Vulkan target on iOS as that is the only SDK with frameworks. It will not affect other platforms or tests of the version bumps on them. But this is moot I will remove them.

Please ask the samples team to consider bumping the minimum version. The ones you have now are long in the tooth and as incident has shown people are starting to use new features without realizing they are not in the minimum required version.

[gpx1000]

@gpx1000 I think you misunderstood what I wrote. By modifying my FindVulkan pre-CMake 3.28 workaround to use get_filename_component I can revert the cmake_required_minimum in the root CMakeLists.txt back to 3.16.

It is app/CMakeLists.txt that needs the fatal error if IOS and CMake version < 3.20 in order to remove the version bump there. I'll update the PR tomorrow.

Fair enough; thanks for doing the update! I think it makes a lot of sense and communicates the limitation correctly to end users.

Isn't CI checking all desired supported platforms? If so they are all already passing with the PR as is.

Sadly no. Vulkan is supported on a LOT of platforms. We don't even have full coverage of all supported Apple desktop platforms ;-) (M2 hardware isn't covered as an example). While CI does provide a measure of strong indication things work in all platforms to be fair, there's plenty of platforms ecosystems that can't easily be covered for various reasons. It's a best effort which is meant to cover the most standard way of doing things (i.e. getting the SDK from LunarG instead of other locations/distributions and installing system level cmake etc).

Please ask the samples team to consider bumping the minimum version. The ones you have now are long in the tooth and as incident has shown people are starting to use new features without realizing they are not in the minimum required version.

Yep, we do need to look at bumping the minimum version; it is pretty old. It would make a lot of sense to bring it up to the minimums each platform ships with for cmake; ensure that everyone can get access to them; then test it to ensure it doesn't break anything etc.

[MarkCallow]

I reverted the cmake version bumps in https://github.com/KhronosGroup/Vulkan-Samples/pull/1101/commits/dd24aac589d5bb7a618bdf8a4f3354abb147eb8a.