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.

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

iOS / macOS build fixes, iOS Simulator for x86_64 hosts, VS and Xcode build/run configs #1177

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

SRSaunders
Copy link
Contributor

@SRSaunders SRSaunders commented Sep 24, 2024

Description

This PR provides build fixes and some new build features as follows:

For iOS / macOS builds only:

  1. Adds support for a new _HPP_VULKAN_LIBRARY define in vulkan_sample.h and hpp_hello_triangle.cpp. This provides the runtime infrastructure to support dynamic loading of a specified Vulkan library, e.g. a) the vulkan framework for iOS and b) SDK or development versions of MoltenVK. This follows the same approach as GLFW, with its optional define _GLFW_VULKAN_LIBRARY.
  2. Adds CMake config support for using MoltenVK standalone vs. the Vulkan loader. This is required for at least two scenarios: a) support for the iOS Simulator on x86_64 hosts (i.e. using a universal MoltenVK framework for iOS which is available in the SDK, in contrast to the Vulkan loader for iOS which is not available as a universal framework ), and b) support for using Vulkan-Samples with development builds of MoltenVK ahead of packaging into a future Vulkan SDK.
  3. Adds CMake config support for macOS builds using Xcode. This change ensures the correct Vulkan SDK environment variables are copied into Xcode's scheme and used when running or debugging from within Xcode. Previously this was a very manual and error-prone step. Without adding these env variables, the Vulkan libraries will not be found at runtime.
  4. Improves CMake config support for iOS by fixing bundle identifier logic and aligning CMake config with iOS info.plist file. This also adds library embedding optionality for the Vulkan loader and Validation layer, which is required when building using MoltenVK standalone for the iOS SImulator on x86_64, or development versions of MoltenVK.
  5. Fixes the ASTC build for iOS by supporting both iOS devices (arm64 only) and the iOS Simulator (arm64 & x86_64)

General improvements for Visual Studio Windows and Xcode macOS/iOS:

  1. Improves CMake support for VS and Xcode: Sets config for default executable, working dir, and default sample.
  2. Forces VS, Xcode, Ninja Multi-Config to build for Release when the CMake build type is set to Release. This is accomplished by removing the Debug, and MinSizeRelease build types from the CMake config type variable when the build type is set to Release. The positive effect here is that when you open Visual Studio or Xcode, the correct build type will already be set along with the executable and default sample (from 1 above). All you need to do is build and run.
  3. Updates build documentation for Visual Studio, macOS, and iOS incorporating instructions for all of the above.
  4. Reduces redundant FindVulkan.cmake messages in console log output. The result is you will see the initial Vulkan library found and dxc found messages only. Debugging and repeat messages are suppressed.

This has been pretty thoroughly tested on Windows 10 (VS and command line builds), Manjaro Linux (command line builds), macOS (Xcode and command line builds), iOS (Xcode builds), and the iOS Simulator (Xcode builds). In addition, the Vulkan loader vs. MoltenVK optionality has also been tested for macOS, iOS, and iOS Simulator targets.

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

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

@SRSaunders
Copy link
Contributor Author

Interesting, it seems the CI does not provide a Vulkan SDK for macOS Debug or Release builds, kind of unexpected. Also, it appears the CI does not specify a development team for iOS builds, again unexpected.

Let me look deeper into this and I will come back with a proposal or solution.

@SaschaWillems
Copy link
Collaborator

SaschaWillems commented Sep 24, 2024

Can you elaborate? We do have CI with SDK for macOS iOS and both fetch the SDK.

@SaschaWillems
Copy link
Collaborator

Maybe we could simplify things if you'd create an issue for changes like this first, so we can discuss these at our sample's team meeting. We talk a lot about build setups and also Apple platforms, but not always in public. Getting some notification upfront in the form of an issue would probably help us coordinate a bit more efficient ;)

docs/build.adoc Outdated Show resolved Hide resolved
@SaschaWillems
Copy link
Collaborator

SaschaWillems commented Sep 24, 2024

Adds support for a new _HPP_VULKAN_LIBRARY define in vulkan_sample.h and hpp_hello_triangle.cpp. This provides the runtime infrastructure to support dynamic loading of a specified Vulkan library, such as SDK or development versions of MoltenVK. This follows the same approach as GLFW, with its optional define _GLFW_VULKAN_LIBRARY.

Adds CMake config support for using MoltenVK standalone vs. the Vulkan loader. This is required for at least two scenarios: a) support for using Vulkan-Samples with development builds of MoltenVK ahead of packaging into a future Vulkan SDK, and b) support for the iOS Simulator on x86_64 hosts (i.e. using a universal MoltenVK framework for iOS which is available in the SDK, in contrast to the Vulkan loader for iOS which is not available as a universal framework ).

I'm not 100% convinced that this is something we want. What is the use-case here aside from running a self-built version of MoltenVK?

I don't want the CMake setup become too complicated. It's already hard to maintain (and test) all of this, and I don't think we need to support all possible use-cases, esp. niche ones.

@SaschaWillems SaschaWillems added the build This is relevant to the build system label Sep 24, 2024
app/CMakeLists.txt Outdated Show resolved Hide resolved
@SRSaunders
Copy link
Contributor Author

Maybe we could simplify things if you'd create an issue for changes like this first, so we can discuss these at our sample's team meeting. We talk a lot about build setups and also Apple platforms, but not always in public. Getting some notification upfront in the form of an issue would probably help us coordinate a bit more efficient ;)

I am not a member of your forum and can't participate in those kinds of planning discussions. And using github's issue tracker for more strategic stuff is not ideal. All I can really do is propose changes, and like most developers, we can see things better and respond if it is concrete. In the end it is up to you and your team whether to take such changes or reject them. I will not be offended if you choose not to proceed with certain ones. In the end I am just trying to help and provide a better experience for Apple developers using your project.

I will fix the CI failure issues and then you can decide what parts if any you may take. I have been around the development world a long time and sometimes work has to be adapted or thrown away. No worries.

@gpx1000
Copy link
Collaborator

gpx1000 commented Sep 24, 2024

@SaschaWillems is right in that we should optimize for reduced CMake complexity. Maybe to satisfy both needs, we could use options in CMake and set the default path to _HPP_VULKAN_LIBRARY as "vulkan.framework/vulkan" that way we'd be reducing the complexity at least in the dynamic loader. However, I ultimately think this change needs to happen in VULKAN_HPP project rather than us doing something specific per platform.

@SRSaunders
Copy link
Contributor Author

SRSaunders commented Sep 24, 2024

Can you elaborate? We do have CI with SDK for macOS iOS and both fetch the SDK.

For macOS (no SDK retrieved in setup):
macOS Debug

For iOS (have SDK in setup, but CI picks up wrong setup-env.sh and doesn't specify a development team):
iOS Debug

I believe I can work around the issue for macOS, and am still thinking about iOS.

@gpx1000
Copy link
Collaborator

gpx1000 commented Sep 24, 2024

source ~/VulkanSDK/setup-env.sh

yep, looks like iOS does indeed use the wrong script. You should be able to fix it there.

The MacOS one uses the same rules as the other Desktop OSes, it should be pulling down the VulkanSDK too.

@SRSaunders
Copy link
Contributor Author

SRSaunders commented Sep 24, 2024

yep, looks like iOS does indeed use the wrong script. You should be able to fix it there.

Agreed, it should be source ~/VulkanSDK/iOS/setup-env.sh . I will fix that with an additional commit here.

The MacOS one uses the same rules as the other Desktop OSes, it should be pulling down the VulkanSDK too.

I don't see VulkanSDK being pulled for any of the Desktop OSes. I guess the CI build is relying on Vulkan-Headers from the project. Not an emergency, and I can make small change that will avoid the CI issue for macOS builds.

@SRSaunders
Copy link
Contributor Author

@SaschaWillems is right in that we should optimize for reduced CMake complexity. Maybe to satisfy both needs, we could use options in CMake and set the default path to _HPP_VULKAN_LIBRARY as "vulkan.framework/vulkan" that way we'd be reducing the complexity at least in the dynamic loader. However, I ultimately think this change needs to happen in VULKAN_HPP project rather than us doing something specific per platform.

My proposed _HPP_VULKAN_LIBRARY define is not platform specific, but a general enhancement. It would be better if VULKAN_HPP and/or Volk had this concept too. GLFW already does it right in my opinion.

@SRSaunders
Copy link
Contributor Author

SRSaunders commented Sep 25, 2024

Okay. I think I have heard the following:

  1. Forcing VS and Xcode IDEs to use release builds is not a good idea. In addition, the related Release build config changes to Windows docs is also not good. I will revert both of these changes.
  2. Complexity is a concern, and the introduction of _HPP_VULKAN_LIBRARY is something to discuss. My original motivation was to support the iOS Simulator on x86_64 hosts, which the SDK's vulkan.framework cannot do since it is built only for arm64. However, the SDK also packages MoltenVK.xcframework which is built to include x86_64 and arm64 support for the iOS Simulator. So I needed a way to load the SDK's MoltenVK library vs. the vulkan library. The additional ability to support dev builds of MoltenVK from their github project was an afterthought since it essentially came for free. And I personally like this as a developer since I occasionally contribute to that project and use Vulkan Samples to regression test MoltenVK. Given this background, there are two things to consider:

a) vulkan_sample.h already uses a hard-coded #ifdef TARGET_OS_IPHONE to handle loading vulkan.framework/vulkan for iOS vs. the default vulkan library. hpp_hello_triangle.cpp should do the same but it is currently missing this logic (a defect). _HPP_VULKAN_LIBRARY generalizes this concept and could replace it in both files, i.e. removing the need for #ifdef TARGET_OS_IPHONE and replacing it with #ifdef _HPP_VULKAN_LIBRARY, or as @gpx1000 suggested above maybe just a single static vk::DynamicLoader dl(_HPP_VULKAN_LIBRARY) call. In this way Vulkan Library config for iOS Devices and the iOS Simulator would be done in cmake. This could be a complexity reducer.

b) The support for MoltenVK dev builds was a freebie (cmake changes only) and a bit of a personal need. However, this may be a step too far for this project. I could remove that part from this PR, or at minimum remove it from the docs. I will start with the latter approach (i.e. remove from docs), and wait for further feedback.

  1. I would like to keep the other general improvements for Xcode and VS configuration (minus the Release build stuff). Having pre-config for the executable, working dir, envrironment variables, and default sample is quite useful. Also the additional information in the docs about VS and Xcode builds (vs. only command line) should be useful to some.
  2. There is a surprising defect in the way cmake config for iOS interacts with CI builds for iOS. The existing cmake code for iOS checks CMAKE_OSX_SYSROOT for the value "ios", and if it doesn't match, disables code signing which is necessary for batch build testing. The only problem is that "ios" is not a valid value for sysroot, which is defined as either "iphoneos" or "iphonesimulator". This means that code signing will be disabled for ALL builds where the development team is not specified (independent of whether on physical device or simulator). The error condition and message will never be activated when building for "iphoneos", which is not great for end-user builds. I guess I had fixed that issue earlier but did not foresee the impact on CI builds. I have added some additional cmake checks and a command line flag in the CI script to fix this.

I plan to push these changes today and hopefully the CI will pass. Following that, I await additional feedback on what to keep and what to toss out.

@SRSaunders
Copy link
Contributor Author

SRSaunders commented Sep 26, 2024

I decided to proceed with the DynamicLoader _HPP_VULKAN_LIBRARY simplification for iOS, removing the #ifdef TARGET_OS_IPHONE condition from vulkan_sample.h and hpp_hello_triangle.cpp. A trivial change and it simplifies things a bit. Thanks to @gpx1000 for the motivation.

However, clang-format is haunting me again. I run it before commiting, everything is clean, and yet it still complains during CI. I may have to move to Windows or Linux and try to force-push again. Not sure what is going on.

clang-format UPDATE 1: I moved to Linux and ran clang-format with no issues. Then I went to Windows and undid the commit and the changes. Re-edited the changes inside Visual Studio. Downloaded LLVM and ran clang-format over there. Force-pushed from Windows and the same thing happened. Even the CI clang-format diffs message doesn't seem to make sense. I've spent more time trying to fix clang-format than making and testing the actual code change. I give up - perhaps someone who has more experience with clang-format could help or tell me what is wrong with commit e1ffff3.

clang-format UPDATE 2: Well, I finally solved it by removing a clang-format change. I suspect there is a settings problem or defect in clang-format-diff.py when there is a spacing-only change on a line. I had to undo the spacing-only change made by clang-format in an earlier commit (417b4a5) and force push again. It's kind of frustrating when the tools don't work properly.

@SRSaunders SRSaunders force-pushed the ios-build-fixes branch 2 times, most recently from e1ffff3 to ec38a37 Compare September 27, 2024 01:38
@SRSaunders SRSaunders changed the title iOS / macOS build fixes, MoltenVK standalone support, VS and Xcode build/run configs iOS / macOS build fixes, iOS Simulator for x84_64 hosts, VS and Xcode build/run configs Sep 30, 2024
@SRSaunders SRSaunders changed the title iOS / macOS build fixes, iOS Simulator for x84_64 hosts, VS and Xcode build/run configs iOS / macOS build fixes, iOS Simulator for x86_64 hosts, VS and Xcode build/run configs Oct 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build This is relevant to the build system Mac
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants