Update FindVulkan.cmake to cross-compile for Windows/Windows on ARM64

[fgiancane8]

Backport patches from upstream CMake to support Windows cross-compilation use cases (x86_64 to ARM64 and vice-versa).

Upstream patches backported:
5e1440302a FindVulkan: Add support for cross-compiling between Windows x64/ARM64
f9a09f76f3 FindVulkan: Drop support for 32-bit SDK on Windows
b40740f28a FindVulkan: Do not search bin directories for libraries
947adbba91 FindVulkan: Use ENV{VULKAN_SDK} only if it exists

See #1489 for more context about this change.

General Checklist:

Please ensure the following points are checked:

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

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

• I have updated the header Copyright to reflect the current year (CI build will fail if Copyright is out of date)
• My changes build on Windows, Linux, macOS and Android. Otherwise I have documented any exceptions

[CLAassistant]

All committers have signed the CLA.

[SRSaunders]

This is failing on iOS because the standard version of FindVulkan.cmake is missing the following code. In contrast, this code is present in the downstream project version:

if(IOS)
    get_filename_component(Vulkan_Target_SDK "$ENV{VULKAN_SDK}/.." REALPATH)
    list(APPEND CMAKE_FRAMEWORK_PATH "${Vulkan_Target_SDK}/iOS/lib")
    set (CMAKE_FIND_ROOT_PATH_MODE_PROGRAM BOTH)
    set (CMAKE_FIND_ROOT_PATH_MODE_LIBRARY BOTH)
    set (CMAKE_FIND_ROOT_PATH_MODE_INCLUDE BOTH)
endif()

Possible solutions:

1. Retain a project-specific FindVulkan.cmake, but fast forward to current upstream and merge in the above code.
2. Get the following code snippet added to the upstream CMake FindVulkan module code. This may be a slow process and likely will lead to user error with Vulkan-Samples until the cmake update becomes more widely deployed.
3. Patch the project's main CMakeLists.txt file with the above code just prior to calling find_package(Vulkan) for the first time. This will ensure that CMake's Vulkan cache variables for iOS are set properly right from the start.

Option 3 is likely the best IMHO and would retain your main objective for this PR. The updated CMakeLists.txt code would look like this:

...
project(vulkan_samples)

if(VKB_GENERATE_ANTORA_SITE)
    add_subdirectory(antora)
else ()

# set up the path for Vulkan loader and MoltenVK library frameworks on iOS
if(IOS)
	get_filename_component(Vulkan_Target_SDK "$ENV{VULKAN_SDK}/.." REALPATH)
	list(APPEND CMAKE_FRAMEWORK_PATH "${Vulkan_Target_SDK}/iOS/lib")
	set(CMAKE_FIND_ROOT_PATH_MODE_PROGRAM BOTH)
	set(CMAKE_FIND_ROOT_PATH_MODE_LIBRARY BOTH)
	set(CMAKE_FIND_ROOT_PATH_MODE_INCLUDE BOTH)
endif()

# search for Vulkan SDK
find_package(Vulkan)

if(Vulkan_FOUND)
...

[gpx1000]

Thanks for testing @SRSaunders You're exactly right. While we are not quite to the point where we can do a complete removal of findVulkan.cmake in favor of a package maintained config variation; which would be the ultimate correct solution. I think, if possible we should promote these changes back to kitware's gitlab version; then remove the copy here. In the meantime, we should at a minimum accept the upstream changes to our copy of findVulkan.cmake.

[SRSaunders]

Thanks @gpx1000. Just to clarify, Option 1 (merge upstream changes into the project's FindVulkan.cmake) and Option 3 (remove FindVulkan.cmake and patch CMakeLists.txt) are mutually exclusive - we should do one or the other, but not both. Option 2 could happen in parallel regardless and let that process take its course.

[gpx1000]

I'm not a fan of option 3; it invites a lot of questions as to why it's required and encourages end users to think the top level CMakeLists.txt is too complex to learn anything from (a feedback item we're already receiving). As much as possible, I want to encourage the "best practice" of using find_library(vulkan) being the universal only thing that is needed by end users beyond needing the findVulkan.cmake file if necessary.
When the changes are promoted upstream, we can remove the findVulkan.cmake file here. But that's just my opinion other reviewers might have a different thought.

[SaschaWillems]

Note that there is a discussion on removing the FindVulkan.cmake from CMake: https://gitlab.kitware.com/cmake/cmake/-/issues/25617

[SRSaunders]

Thanks @SaschaWillems. From the discussion link it seems that Option 1, i.e. merging upstream changes and retaining the project-specific FindVulkan.cmake, is the right way to go. While it appears that Kitware is still accepting changes to the FindVulkan module, the longer term view is that downstream projects should handle it themselves.

[fgiancane8]

Hello and thanks for all the feedbacks! So I started already the discussion with upstream CMake and sounds like the accepted go-to is to at some point remove FindVulkan.cmake altogether following what is recommended in the linked thread by @SaschaWillems .

In the meanwhile what I can do is:

• start the discussion with upstream CMake
• Merge/adapt the downstream changes here so that we're covered on iOS as well
• Verify that this repo can live with upstream FindVulkan.cmake so to accept this PR
• Wait for changes in upstream CMake for future strategy plans

Let me know what you all folks think about.

[fgiancane8]

Hello and thanks for all the feedbacks! So I started already the discussion with upstream CMake and sounds like the accepted go-to is to at some point remove FindVulkan.cmake altogether following what is recommended in the linked thread by @SaschaWillems .

In the meanwhile what I can do is:

• start the discussion with upstream CMake
• Merge/adapt the downstream changes here so that we're covered on iOS as well
• Verify that this repo can live with upstream FindVulkan.cmake so to accept this PR
• Wait for changes in upstream CMake for future strategy plans

Let me know what you all folks think about.

Reached out to upstream CMake folks to further clarify. This issue was linked to my previous Pull Request for FindVulkan.

[fgiancane8]

Thanks @SaschaWillems. From the discussion link it seems that Option 1, i.e. merging upstream changes and retaining the project-specific FindVulkan.cmake, is the right way to go. While it appears that Kitware is still accepting changes to the FindVulkan module, the longer term view is that downstream projects should handle it themselves.

So my understanding on this is that the split would be a requirement since Vulkan on Linux is managed by distro packagers, while on Windows/iOS/macOS is packaged with LunarG SDK. Android is managed by Android SDK itself. So it would make sense to keep the upstream version as much updated as possible so that:

• we can remove it from here;
• LunarG can take the work from thereafter and to maintain for the mentioned OSes;
• Linux can have its own packaging systems handling the issues;
• no need to maintain a downstream module here anymore.

This is what I feel would be the best outcome but let's discuss, I'd like to reach for a conclusion that works for all these major platforms without duplicating too much between vendored/downstream/upstream.

[fgiancane8]

Note that there is a discussion on removing the FindVulkan.cmake from CMake: https://gitlab.kitware.com/cmake/cmake/-/issues/25617

Seems like it's not on priority list and will take time to materialise. The thread got revived today with similar issues. So as of now, it's basically mostly on upstream CMake and this repo.

[fgiancane8]

FYI: https://gitlab.kitware.com/cmake/cmake/-/issues/27661

Upstream CMake provided a recommended way to build on iOS. They recommended us to have an iOS toolchain file and amend the way we search for the Vulkan SDK. I'll try and post another commit to see if that fixes the issue.

[gpx1000]

Thanks for tracking this down and working with kitware on this.

The toolchain file idea is not a bad one. We might want to think if there's a way to do additional toolchain files as downstream projects from us (i.e. lots of people copy paste directly out of samples) might use toolchain files for project specific needs and this might inhibit their use if we take up the project's sole toolchain slot. This concern might be addressed via documentation and highlighting the toolchain requirement. But, it is worth considering if we want this for best practice recommendations which samples are meant to be.

[fgiancane8]

Thanks for tracking this down and working with kitware on this.

No worries at all!

The toolchain file idea is not a bad one. We might want to think if there's a way to do additional toolchain files as downstream projects from us (i.e. lots of people copy paste directly out of samples) might use toolchain files for project specific needs and this might inhibit their use if we take up the project's sole toolchain slot. This concern might be addressed via documentation and highlighting the toolchain requirement. But, it is worth considering if we want this for best practice recommendations which samples are meant to be.

@gpx1000 ,
thanks for the comments! Which platform(s) do we think may need to be tweaked with toolchain files ? As per the conversation linked onto the upstream CMake my understanding is that, as of now, only Apple systems (maybe even only iOS?) required it. But good point. In my experience as a Windows/Linux user, I was never required to use more than 1 toolchain file. Let's see if folks over here have other experience as well.

[gpx1000]

Well, all platforms technically can use Vulkan Samples, so everything from a rPi / wrist watch to desktop or server rack or XR development rig including the Apple headset, watch, phone, TV, car etc could all be potential platform end points. Each are popular places to have a toolchain.

It might be exceedingly rare to the point of not even the SDK has a pre-made build for such platform. But, each does have a potential role as a target end platform. Samples are useful to validate that Vulkan is working on newer unreleased systems as well; so it's hard to predict where it might be used and we try to opt for practices in general that don't make assumptions about what end users might require. If we can make a decision that doesn't restrict users that's what we typically try to do.

[fgiancane8]

Well, all platforms technically can use Vulkan Samples, so everything from a rPi / wrist watch to desktop or server rack or XR development rig including the Apple headset, watch, phone, TV, car etc could all be potential platform end points. Each are popular places to have a toolchain.

Makes sense. So in my mind:

• Rpi is still Linux so whatever works there, should work on RPi as well.
• The whole Apple ecosystem should be covered. If my change is only adding iOS, I think all other platforms you mentioned should be added as well.
• For everything else that's not of "public knowledge" we should probably relay this task to other contributors, on per-need basis?

It might be exceedingly rare to the point of not even the SDK has a pre-made build for such platform. But, each does have a potential role as a target end platform. Samples are useful to validate that Vulkan is working on newer unreleased systems as well; so it's hard to predict where it might be used and we try to opt for practices in general that don't make assumptions about what end users might require. If we can make a decision that doesn't restrict users that's what we typically try to do.

[fgiancane8]

By the way, I see we do not yet build for Windows on ARM64. Since this change is exactly trying to validate that, maybe that target should be added as well.

[gpx1000]

Well we can keep this solely targeted to iOS. It's just important to remember the scope of potential platforms when we make decisions like using a toolchain file vs not. Yes, we can add windows ARM64 build to the CI, but CI in windows already takes a really long time and it's something I've been struggling to find ways to make faster already. I'm hesitant to add more to the CI for windows until I can get the build time reduced on windows.
I'd also rather keep this as targeted as possible to the only real issue of this PR which is making sure that the kitware upstream version of findvulkan.cmake covers everything we currently do so we can remove the one we use. That way when we do get requests to add other supported platforms, we can do so at the kitware level moving forward and having only one "best practices" findvulkan.cmake for the ecosystem.

[fgiancane8]

Well we can keep this solely targeted to iOS. It's just important to remember the scope of potential platforms when we make decisions like using a toolchain file vs not. Yes, we can add windows ARM64 build to the CI, but CI in windows already takes a really long time and it's something I've been struggling to find ways to make faster already. I'm hesitant to add more to the CI for windows until I can get the build time reduced on windows. I'd also rather keep this as targeted as possible to the only real issue of this PR which is making sure that the kitware upstream version of findvulkan.cmake covers everything we currently do so we can remove the one we use. That way when we do get requests to add other supported platforms, we can do so at the kitware level moving forward and having only one "best practices" findvulkan.cmake for the ecosystem.

Sure, was just sharing couple of ideas to brainstorm on the subject. Let's see if these changes are enough to better align with upstream CMake. We can take further action from there.

Thanks!

[fgiancane8]

Good news, build for iOS correctly detected 🚀
Bad news:

CMake Error at app/CMakeLists.txt:140 (target_sources):

Vulkan-Samples/app/CMakeLists.txt

Lines 138 to 148 in 19a5668

	# Vulkan cache variables already defined by main project CMakeLists and updated on Apple platforms by global_options.cmake
	if(Vulkan_LIBRARY AND ${Vulkan_VERSION} VERSION_GREATER_EQUAL 1.3.278)
	target_sources(${PROJECT_NAME} PRIVATE
	${Vulkan_Target_SDK}/iOS/share/vulkan
	)
	set_source_files_properties(
	${Vulkan_Target_SDK}/iOS/share/vulkan
	PROPERTIES
	MACOSX_PACKAGE_LOCATION Resources
	)
	endif ()

[SRSaunders]

iOS is a bit more complicated than just finding the pre-installed Vulkan runtime libraries and dynamically linking to them at runtime which is all you need on most desktop platforms. iOS and other device-oriented Apple platforms (e.g. iPad, AppleTV) require packaging all library dependencies (i.e. MoltenVK.framework and optionally Vulkan.framework and VkLayer_khronos_validation.framework) into an app bundle for installing onto a physical device or device simulator.

Because of this Vulkan-Samples needs to know specific library locations so Xcode can build the app bundle and load onto the chosen device or simulator for execution and debugging. The locations are identified by CMake variables set by find_package(Vulkan) which uses project's FindVulkan.cmake as the underlying module. The cmake variables are: Vulkan_Target_SDK, Vulkan_MoltenVK_FOUND, Vulkan_MoltenVK_LIBRARY, Vulkan_LIBRARY, and Vulkan_Layer_VALIDATION. There is also some special logic in the project's cmake build system regarding code signing which is required on Apple platforms for security, and code signatures require an Apple Development Team ID which you sign up for as a developer. All this special logic is found in two main places within the project: a) Vulkan-Samples/app/CMakeLists.txt and b) Vulkan-Samples/bldsys/cmake/global_options.cmake.

I guess this is a long-winded way of saying that any replacement for the FindVulkan.cmake module needs to take these variables into account and define them, or if not possible, change the iOS logic in the above two files to accommodate. The latter is possible but not that much fun as it would mandate a bunch of retesting. And since I was the last person to update most of this iOS-specific logic it would likely fall to me or @gpx1000.

With that said, if the only error is an undefined Vulkan_Target_SDK variable, the solution may be pretty simple. However, the other four variables are critical and must be defined by your FindVulkan.cmake replacement. Can you tell me if this is true or not?

Note that my original Option 3, which @gpx1000 did not like due to adding complexity to the main CMakeLists file, solved the problem without any change to the logic in the two files named above. As an alternative, I could move the same logic from Option 3 into global_options.cmake instead, since that file calls find_package(Vulkan QUIET OPTIONAL_COMPONENTS MoltenVK) a second time to get the MoltenVK library specifics. The only downside to this is the first call to find_package(Vulkan) in the main CMakeLists file would not find anything in the iOS case, and only the second call in global_options.cmake (which is currently marked as QUIET) would find the iOS stuff. This would mean that the cmake console log would not show the found Vulkan libraries for iOS build scenarios and may confuse end users. I could possibly fix this by removing the QUIET attribute from the second call but this would show the two FindVulkan calls in the log - not a huge issue but something I avoided in the past with the current setup.

[SRSaunders]

Going back to the beginning, and perhaps I am missing something here, but given the following:

from @fgiancane8:

By the way, I see we do not yet build for Windows on ARM64. Since this change is exactly trying to validate that, maybe that target should be added as well.

from @gpx1000:

Well we can keep this solely targeted to iOS. It's just important to remember the scope of potential platforms when we make decisions like using a toolchain file vs not. Yes, we can add windows ARM64 build to the CI, but CI in windows already takes a really long time and it's something I've been struggling to find ways to make faster already. I'm hesitant to add more to the CI for windows until I can get the build time reduced on windows.

Why are we trying to change FindVulkan.cmake if Windows ARM64 was the reason for this PR, but will not be added to the project as an official target with CI, at least for now? In spite of the discussion regarding technical alternatives above, iOS currently works just fine with no changes to the project's current FindVulkan.cmake.

Is the request in this PR therefore to provide "unofficial" support for Windows ARM64? i.e. allow it to work without adding it as a CI target? Before jumping into cmake changes, I would like to understand the reason and scope.

[fgiancane8]

iOS is a bit more complicated than just finding the pre-installed Vulkan runtime libraries and dynamically linking to them at runtime which is all you need on most desktop platforms. iOS and other device-oriented Apple platforms (e.g. iPad, AppleTV) require packaging all library dependencies (i.e. MoltenVK.framework and optionally Vulkan.framework and VkLayer_khronos_validation.framework) into an app bundle for installing onto a physical device or device simulator.

Because of this Vulkan-Samples needs to know specific library locations so Xcode can build the app bundle and load onto the chosen device or simulator for execution and debugging. The locations are identified by CMake variables set by find_package(Vulkan) which uses project's FindVulkan.cmake as the underlying module. The cmake variables are: Vulkan_Target_SDK, Vulkan_MoltenVK_FOUND, Vulkan_MoltenVK_LIBRARY, Vulkan_LIBRARY, and Vulkan_Layer_VALIDATION. There is also some special logic in the project's cmake build system regarding code signing which is required on Apple platforms for security, and code signatures require an Apple Development Team ID which you sign up for as a developer. All this special logic is found in two main places within the project: a) Vulkan-Samples/app/CMakeLists.txt and b) Vulkan-Samples/bldsys/cmake/global_options.cmake.

Checked upstream CMake, and only Vulakn_Layer_VALIDATION is missing. I have to check with the upstream maintainers. That variable is not documented in the public comments of our downstream version as well, so probably no one ported it back. I can do it.

I guess this is a long-winded way of saying that any replacement for the FindVulkan.cmake module needs to take these variables into account and define them, or if not possible, change the iOS logic in the above two files to accommodate. The latter is possible but not that much fun as it would mandate a bunch of retesting. And since I was the last person to update most of this iOS-specific logic it would likely fall to me or @gpx1000.

With that said, if the only error is an undefined Vulkan_Target_SDK variable, the solution may be pretty simple. However, the other four variables are critical and must be defined by your FindVulkan.cmake replacement. Can you tell me if this is true or not?

So I posted an update on this. Looks like the SDK is properly detected, but it fails at the app/CMakeLists.txt because of the packaging as resource command over there. I don't really know what's going on there so an explanation would be very appreciated as well! (I am no iOS/macOS developer, only know the ways of Linux and Windows).

Note that my original Option 3, which @gpx1000 did not like due to adding complexity to the main CMakeLists file, solved the problem without any change to the logic in the two files named above. As an alternative, I could move the same logic from Option 3 into global_options.cmake instead, since that file calls find_package(Vulkan QUIET OPTIONAL_COMPONENTS MoltenVK) a second time to get the MoltenVK library specifics. The only downside to this is the first call to find_package(Vulkan) in the main CMakeLists file would not find anything in the iOS case, and only the second call in global_options.cmake (which is currently marked as QUIET) would find the iOS stuff. This would mean that the cmake console log would not show the found Vulkan libraries for iOS build scenarios and may confuse end users. I could possibly fix this by removing the QUIET attribute from the second call but this would show the two FindVulkan calls in the log - not a huge issue but something I avoided in the past with the current setup.

Thanks for the super detailed explanation, really appreciated it!

[fgiancane8]

Going back to the beginning, and perhaps I am missing something here, but given the following:

from @fgiancane8:

By the way, I see we do not yet build for Windows on ARM64. Since this change is exactly trying to validate that, maybe that target should be added as well.

from @gpx1000:

Well we can keep this solely targeted to iOS. It's just important to remember the scope of potential platforms when we make decisions like using a toolchain file vs not. Yes, we can add windows ARM64 build to the CI, but CI in windows already takes a really long time and it's something I've been struggling to find ways to make faster already. I'm hesitant to add more to the CI for windows until I can get the build time reduced on windows.

Why are we trying to change FindVulkan.cmake if Windows ARM64 was the reason for this PR, but will not be added to the project as an official target with CI, at least for now? In spite of the discussion regarding technical alternatives above, iOS currently works just fine with no changes to the project's current FindVulkan.cmake.

So the reason for this PR was to fix issues when cross-compiling Vulkan apps to and from ARM64. As of now either Intel machine to ARM64 machine or ARM64 machines to Intel builds are broken. I posted a fix upstream and since there is the whole discussion of unifying the modules, plus CMake will eventually remove their module in favour of LunarG's one (whenever it happens), I was trying to be nice and have a working solution for all Vulkan platforms in place.

Is the request in this PR therefore to provide "unofficial" support for Windows ARM64? i.e. allow it to work without adding it as a CI target? Before jumping into cmake changes, I would like to understand the reason and scope.

Not at all. We can discuss WoA on a separate issue/PR if needed. The goal here was to enable people to cross compile on Windows for both its supported architectures (Intel/ARM64) since LunarG is shipping SDKs supporting both targets.

Also a note on the "unofficial": as far as I can tell, LunarG SDK on Windows officially supports Windows on ARM64, shouldn't we consider that enough to mark the target as official? Not saying we need to add CI and strong testing today, maybe tier 2? I would expect people to at some point start playing with Vulkan Samples on those machines as well. What's your thoughts on this?

[SRSaunders]

So the reason for this PR was to fix issues when cross-compiling Vulkan apps to and from ARM64. As of now either Intel machine to ARM64 machine or ARM64 machines to Intel builds are broken.

Thanks for your clear restatement of the issue. Are Windows ARM64 native builds working with the current project cmake files? Is it only the Windows cross compilation scenarios that aren't? I know from my own efforts that macOS arm64 (Apple Silicon) native builds do work without issue, and cross compiling from macOS x86_64 hosts to iOS arm64 targets also works fine.

as far as I can tell, LunarG SDK on Windows officially supports Windows on ARM64, shouldn't we consider that enough to mark the target as official?

Regarding expanding Vulkan-Samples official supported platforms list, that is above my pay grade 😄. I am a contributor with expertise in Apple platforms, but not a maintainer here. Please ask @gpx1000 about whether Windows on ARM64 and Win x86_64 to ARM64 cross-compilation builds (and vice versa) are within the project's scope. As an aside I don't have the hardware for testing those scenarios so can't really contribute on that side of things.

To keep things moving on the technical side for possibly removing the project FindVulkan.cmake, can you tell me specifically what you did to generate the cmake error at app/CMakeLists.txt:140 (target_sources):? Did you try Option 1 in my original response and merge upstream changes into the project FindVulkan.cmake? Or did you remove the project's FindVulkan.cmake and apply Option 3 to the main CMakeLists.txt? Or did you simply remove the project's FindVulkan.cmake with no other changes and observe this error?

Okay, I see that you posted new commits. I will have a look at those first...

[SRSaunders]

@fgiancane8 the error caused by an undefined Vulkan_Target_SDK.

In Vulkan-Samples/app/CMakeLists.txt add the following line:

    # Vulkan cache variables already defined by main project CMakeLists and updated on Apple platforms by global_options.cmake
    if(Vulkan_LIBRARY AND ${Vulkan_VERSION} VERSION_GREATER_EQUAL 1.3.278)
--->    get_filename_component(Vulkan_Target_SDK "$ENV{VULKAN_SDK}/.." REALPATH)
        target_sources(${PROJECT_NAME} PRIVATE
                ${Vulkan_Target_SDK}/iOS/share/vulkan
        )
        set_source_files_properties(
                ${Vulkan_Target_SDK}/iOS/share/vulkan
                PROPERTIES
                MACOSX_PACKAGE_LOCATION Resources
        )
    endif ()

Your solution with the toolchain file finds the iOS Vulkan frameworks because of updating CMAKE_FIND_ROOT_PATH with the iOS SDK path information. However, this particular ios.cmake toolchain file is only useful for CI builds, it cannot be used for actual deployment to iOS or the iOS Simulator since it is missing some important attributes. Building for a real iOS target requires the following where a user has to fill in XXXX and YYYY with their own signing information:

cmake -G Xcode -Bbuild/ios -DCMAKE_BUILD_TYPE=Release -DCMAKE_SYSTEM_NAME=iOS -DCMAKE_OSX_SYSROOT=iphoneos -DCMAKE_OSX_DEPLOYMENT_TARGET=16.3 -DCMAKE_XCODE_ATTRIBUTE_ONLY_ACTIVE_ARCH=YES -DCMAKE_OSX_ARCHITECTURES=arm64 -DCMAKE_IOS_INSTALL_COMBINED=NO -DCMAKE_XCODE_ATTRIBUTE_DEVELOPMENT_TEAM="XXXX" -DMACOSX_BUNDLE_GUI_IDENTIFIER="com.YYYY.vulkansamples"

See the following link for docs on how to build for iOS: https://github.com/KhronosGroup/Vulkan-Samples/blob/main/docs/build.adoc#ios

I suppose we could make 3 toolchain file for iOS:

1. ios-ci.cmake for non-changing, unsigned CI builds as you have done
2. ios.cmake for real iOS builds where you set ios-specific non-changing information in the toolchain file, and any variable stuff on the command line.
3. ios-sim.cmake for iOS Simulator builds where you set sim-specific non-changing information in the toolchain file, and any variable stuff on the command line.

However, this toolchain stuff feels a bit of overkill for the problem at hand. Only two small things really have to change to make this PR (i.e. remove project FindVulkan.cmake) work:

1. Append $ENV{VULKAN_SDK} to CMAKE_FIND_ROOT_PATH in the main CMakeLists.txt file just prior to find_package(Vulkan), i.e. list(APPEND CMAKE_FIND_ROOT_PATH "$ENV{VULKAN_SDK}"). I am not sure whether to make this conditional for iOS only, or whether this could apply generally. Let me think about that.
2. Update Vulkan-Samples/app/CMakeLists.txt to define Vulkan_Target_SDK as I have shown above.

I have tried this and it works fine with no toolchain files. Nothing else has to change, including CI and docs. The only downside is that the project FindVulkan.cmake also defines layer cmake variables: Vulkan_Layer_API_DUMP, Vulkan_Layer_SHADER_OBJECT, Vulkan_Layer_SYNC2, and Vulkan_Layer_VALIDATION. However, the only one I see used is Vulkan_Layer_VALIDATION and that is not very useful on a real iOS target. I don't think we would miss it.

[fgiancane8]

So the reason for this PR was to fix issues when cross-compiling Vulkan apps to and from ARM64. As of now either Intel machine to ARM64 machine or ARM64 machines to Intel builds are broken.

Thanks for your clear restatement of the issue. Are Windows ARM64 native builds working with the current project cmake files? Is it only the Windows cross compilation scenarios that aren't? I know from my own efforts that macOS arm64 (Apple Silicon) native builds do work without issue, and cross compiling from macOS x86_64 hosts to iOS arm64 targets also works fine.

No problem at all. So I haven't tested it extensively. We had couple of issues with other projects using Vulkan on Windows on ARM64. I fixed them on CMake upstream initially (as there were some relics of x86 era SDK and no proper management of ARM64). As far as I can tell, with my changes on upstream CMake all the combinations of Windows flavours and Vulkan work.

as far as I can tell, LunarG SDK on Windows officially supports Windows on ARM64, shouldn't we consider that enough to mark the target as official?

Regarding expanding Vulkan-Samples official supported platforms list, that is above my pay grade 😄. I am a contributor with expertise in Apple platforms, but not a maintainer here. Please ask @gpx1000 about whether Windows on ARM64 and Win x86_64 to ARM64 cross-compilation builds (and vice versa) are within the project's scope. As an aside I don't have the hardware for testing those scenarios so can't really contribute on that side of things.

Haha of course nothing binding here, was talking about community effort altogether. Not expecting to leave all the burden on you (or other solo efforts as well!)

To keep things moving on the technical side for possibly removing the project FindVulkan.cmake, can you tell me specifically what you did to generate the cmake error at app/CMakeLists.txt:140 (target_sources):? Did you try Option 1 in my original response and merge upstream changes into the project FindVulkan.cmake? Or did you remove the project's FindVulkan.cmake and apply Option 3 to the main CMakeLists.txt? Or did you simply remove the project's FindVulkan.cmake with no other changes and observe this error?

Okay, I see that you posted new commits. I will have a look at those first...

I let the CI here do its own tests. You can find the full log failures here: https://github.com/KhronosGroup/Vulkan-Samples/actions/runs/22683125940/job/65768554240

Just for reference:
https://github.com/fgiancane8/Vulkan-Samples/blob/265d31356e127a5c071034f325b79137cbee71ff/.github/workflows/build.yml#L194-L198

I would consider the proper/official/recommended command line to build on iOS targets.

[fgiancane8]

Sorry guys, I'm trying to catch up. It looks like everything you're doing is reasonable. It might need to wait for the Samples phone call on Monday to get a lot of us maintainers able to review in depth. However, to give feedback now, I do like the direction (I think), I still need to catch up myself and make sure I fully understand it.

Thanks @gpx1000 ! Take your time for the review,there's no rush. Have a good one and for further clarifications just ping me/us.

[fgiancane8]

@SRSaunders
Backported upstream CMake changes into my PR and renamed accordingly.
I tested it for Windows/Windows on ARM64 case on my machine and it is working as expected.
Have a look at it, and let me know/comment on the change.

P.S. if this is ever going to be merged, please squash the commit logs with the PR text otherwise we'll get a lot of unpleasant "revert xxxx" not super cool :)

[SRSaunders]

Backported upstream CMake changes into my PR and renamed accordingly. I tested it for Windows/Windows on ARM64 case on my machine and it is working as expected. Have a look at it, and let me know/comment on the change.

Thanks @fgiancane8, I will review this weekend.

In the mean time I have submitted my PR with the iOS build changes. I cited you in the copyright notice for the new ios.cmake toolchain file. Thanks for your help on this.

[fgiancane8]

Backported upstream CMake changes into my PR and renamed accordingly. I tested it for Windows/Windows on ARM64 case on my machine and it is working as expected. Have a look at it, and let me know/comment on the change.

Thanks @fgiancane8, I will review this weekend.

In the mean time I have submitted my PR with the iOS build changes. I cited you in the copyright notice for the new ios.cmake toolchain file. Thanks for your help on this.

Thanks @SRSaunders enjoy the weekend and let me know if there is something to amend.

I could not post the downstream changes to CMake but will do it either this weekend or on Monday.

Talk to you later!

[fgiancane8]

@SRSaunders ,
I took my time to branch and prepare commits to implement the proper detection of Layer libraries on CMake. Please find my changes here on this branch: https://gitlab.kitware.com/fgiancan/cmake/-/tree/fgiancan-backport-vklayers-variables?ref_type=heads

On Windows, those Layers are (correctly) marked as not found as there is no .lib file and they are only provided as DLLs. Can you check that these changes work only in the iOS case? I marked the find_library commands as optional so it should not fail on platforms where these are not available as static libraries (which per my understanding is basically every other platforms besides iOS).

Let me know if there is something else to be addressed, otherwise I think that after this gets reviewed and is merged, we are on feature parity between upstream and downstream FindVulkan.cmake (as of now).

[fgiancane8]

Please find here the upstream MR to add Vulkan Layers discovery capability: https://gitlab.kitware.com/cmake/cmake/-/merge_requests/11808

This would in turn mitigate what was missing on the IOS part as @SRSaunders mentioned. Feedbacks/comments on the upstream MR are welcomed from folks here as well.

On a side note, with CMake < 4.3 on MacBooks, finding properly Frameworks for iOS is broken: we may still need to keep the downstream version of FindVulkan around for people not updating to latest CMake, maybe adding a deprecation note somewhere.

I was thinking of conditionally including the bundled version of the module only if the detected CMake version is less than 4.3 or whatever upstream version ends up with feature parity with us in their module (possibly 4.4). I'll keep you posted and report once the MR is discussed and reworked or merged.

[gpx1000]

Guys, this works well for me. Thanks so much for the work on this!!

[fgiancane8]

@gpx1000 sorry I had to force-push to amend the licensing issues. I think there's no need for my changes to be BSD-3-Clause but I am still waiting for the final legal response.

[gpx1000]

Ah, no worries then. Comment here when you're ready. This has my testing approval when/if you are able to accept the license.

[fgiancane8]

It's just a matter of when, the ifs are mostly cleared :) thanks for the patience!

[SRSaunders]

@fgiancane8 thanks for pushing this forward, but notice you have not replied to my review comments above:

1. Please remove duplicated change in CMakeLists.txt already done in my companion PR Update iOS build and CI to support standardization of FindVulkan.cmake #1500.
2. Regression in Apple logic block that reintroduces the MoltenVK directory which is no longer at the SDK top level. It would be better to simply not touch this if(APPLE) block and focus on the Windows-related code only.

Before proceeding with this PR, would you be able to reply to my review comments? Thanks.

[fgiancane8]

@fgiancane8 thanks for pushing this forward, but notice you have not replied to my review comments above:

1. Please remove duplicated change in CMakeLists.txt already done in my companion PR Update iOS build and CI to support standardization of FindVulkan.cmake #1500.

2. Regression in Apple logic block that reintroduces the MoltenVK directory which is no longer at the SDK top level. It would be better to simply not touch this if(APPLE) block and focus on the Windows-related code only.

Before proceeding with this PR, would you be able to reply to my review comments? Thanks.

Sorry @SRSaunders , i missed the comments. I have no problem amending my part. I'll reduce the scope to windows only and let you take care of the rest!

[fgiancane8]

@SRSaunders , @gpx1000
This should be the final version.

[SRSaunders]

@fgiancane8 thanks for removing the Apple-specific parts.

The rest looks fine, although I cannot test the Windows ARM stuff since I don't have hardware. I do notice that some legacy Windows Vulkan SDK stuff may be removed here, such as bin32 and lib32, and searching for libs in the bin folder. Is this a concern? I am not a Windows Vulkan SDK expert so I cannot offer an opinion on this.

Lastly, and just a small nit, I wonder if you should unset(_Vulkan_arch_suffix) at the end of its usage scope, i.e. right after set(_Vulkan_hint_library_search_paths "$ENV{VULKAN_SDK}/lib${_Vulkan_arch_suffix) and before the block's final endif(). This is not a functionality difference, just a code cleanliness issue.

[fgiancane8]

@fgiancane8 thanks for removing the Apple-specific parts.

The rest looks fine, although I cannot test the Windows ARM stuff since I don't have hardware. I do notice that some legacy Windows Vulkan SDK stuff may be removed here, such as bin32 and lib32, and searching for libs in the bin folder. Is this a concern? I am not a Windows Vulkan SDK expert so I cannot offer an opinion on this.

Thanks! This is what I've posted upstream and was extensively tested. It should be good to go. Lib32 and Bin32 are already removed. (at least this is what I see from the diff).

Lastly, and just a small nit, I wonder if you should unset(_Vulkan_arch_suffix) at the end of its usage scope, i.e. right after set(_Vulkan_hint_library_search_paths "$ENV{VULKAN_SDK}/lib${_Vulkan_arch_suffix) and before the block's final endif(). This is not a functionality difference, just a code cleanliness issue.

Let me double-check. The plan was to unset the variable just after its usage. It may be an unwanted result of removing the apple code.

[fgiancane8]

Aesthetic changes to unset.

[SRSaunders]

@fgiancane8 this looks good to me. I think all you need to do now is accept the CLA.

[fgiancane8]

CLA accepted. Thank you all for the time and the reviews!