-
Notifications
You must be signed in to change notification settings - Fork 432
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
WIP: CMake only PR. #1947
base: main
Are you sure you want to change the base?
WIP: CMake only PR. #1947
Conversation
…uilding this project. It also allows for building with XCode directly. It should make maintaining the project easier in the long run.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks so much for submitting this. Automated builds are very helpful.
The existing Makefile
builds MoltenVK by using the Xcode projects embedded in the repo content, so an automated build process would currently fetch the MoltenVK repo, then run fetchDependencies
and make
.
I assume the resulting CMake
-based build process still runs fetchDependencies
and make
, after CMake
? Otherwise how are the dependencies and projects built?
So, I'm not sure what this CMake
build process adds to the process. Can you clarify a use case that requires CMake
instead of just running fetchDependencies
and make
? What does CLion
need?
I've also made some inline review comments about the content of this PR.
@@ -0,0 +1,2 @@ | |||
// Auto-generated by MoltenVK | |||
static const char* mvkRevString = "@MVK_GIT_REV@"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to avoid cluttering the top level directory if possible. Can we find a different home for this file? Maybe under Templates
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll move it there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file should be moved to a new Templates/cmake
directory.
You actually don't need fetchDependencies if we switch to submodules. CMake is a pretty smart build system that correctly determines dependencies and only builds what's asked for when it's asked for. So if we restructure the project a bit this would be the entire process from clone to built project: You can add -G XCode to the first cmake command and it will create an XCode project complete with the dependencies linked correctly and ready to be built as needed. Otherwise, you can use -G ninja or -G Makefile for those command line build systems. Because the dependencies for MoltenVk are set to PUBLIC in the link_libraries command in our CMakeLists.txt, they will get promoted automatically to the consuming target (myexecutablename) So working with MoltenVK becomes easier for end users if they have a need/want to build moltenvk. |
Thanks for clarifying the larger integration building use case. I can see benefit in consistency of plugging into a larger
Long ago, we used submodules, but moved to a script to provide options to manage the build complexity. But I am not adverse to moving back to submodules for the dependencies, if we can cover the building effectively (and I assume move it into the main build sequence).
Yup. The focus from the start was to use Xcode projects as the primary driver for editing and building. It's critical that those of us working on MoltenVK internals every day have an effective Xcode environment. But again, I am not adverse to modernizing this and making it more command-buildable moving forward. Keep in mind there are several build scripts, including the As I mention, I am no Once this basic PR is ready to go, I'll pull it in, so we have some basics. Beyond that, I suggest we create both a tracking issue, and a branch to this repo that can absorb a larger and longer effort to move everything to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some minor things I noticed
I'd recommend making a Formatting nitpick: most files on MoltenVK use either 4-space tabs or hard tabs (or both, it's a bit messy around here), but none use 8-space tabs. If you like 8-space tabs, hard tabs have the option to be 8 spaces while also fitting in with the rest of the project. |
…e for a CMake project; but it will work for our needs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR has not been updated in the past year, and so it is structurally a bit out of date. I've suggested inline some changes that at least allow this PR to perform a MoltenVK build.
Having said that, I feel this functionality is far from complete:
- As mentioned in the comments below, the builds are building for the wrong platform, or the wrong functionality, none of which are usable.
- I expect that we will receive future requests to output dynamic frameworks, a macOS
dylib
, and possibly anXCFramework
. All this will definitely be true for anyone using the Vulkan SDK, but wanting to build their own MoltenVK. Although we don't necessarily need those pieces right now, I want to have an action plan for how that might be accomplished. - We will definitely need some user documentation added to the
README.md
document to cover:- How does the user invoke CMake?
- How is the user intended to integrate this into a build plan?
- If running standalone:
- How does the user build MoltenVK?
- Which target(s) should they build?
- Where is the build output, and what should they do with it?
I remain concerned about the overall maintainability of all of the above if this becomes part of the officially-supported MoltenVK distro.
Another option might be to move this effort to a different repo somewhere. I'm thinking sort of a MoltenVK tools repo, where this could be maintained and managed separately from MoltenVK, by those users who need CMake integration. It could use an install script to put the files in this PR into the right locations within MoltenVK (or perhaps it could sit separately, and just import/reference the MoltenVK directories).
I've moved this PR back to WIP status, while we figure this out.
add_subdirectory(glslang) | ||
set(SPIRV_TOOLS_BUILD_STATIC ON) | ||
add_subdirectory(SPIRV-Cross) | ||
add_subdirectory(Vulkan-Headers) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The contents of External
are not currently saved to repo. .gitignore
needs to be adjusted so that External/CMakeLists.txt
is added to the repo.
MoltenVK/CMakeLists.txt
Outdated
${CMAKE_CURRENT_LIST_DIR}/MoltenVK/Commands/MVKCmdDraw.h | ||
${CMAKE_CURRENT_LIST_DIR}/MoltenVK/Commands/MVKCmdPipeline.h | ||
${CMAKE_CURRENT_LIST_DIR}/MoltenVK/Commands/MVKCmdQueries.h | ||
${CMAKE_CURRENT_LIST_DIR}/MoltenVK/Commands/MVKCmdRenderPass.h |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
${CMAKE_CURRENT_LIST_DIR}/MoltenVK/Commands/MVKCmdRenderPass.h | |
${CMAKE_CURRENT_LIST_DIR}/MoltenVK/Commands/MVKCmdRendering.h |
MoltenVK/CMakeLists.txt
Outdated
${CMAKE_CURRENT_LIST_DIR}/MoltenVK/Commands/MVKCmdDraw.mm | ||
${CMAKE_CURRENT_LIST_DIR}/MoltenVK/Commands/MVKCmdPipeline.mm | ||
${CMAKE_CURRENT_LIST_DIR}/MoltenVK/Commands/MVKCmdQueries.mm | ||
${CMAKE_CURRENT_LIST_DIR}/MoltenVK/Commands/MVKCmdRenderPass.mm |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
${CMAKE_CURRENT_LIST_DIR}/MoltenVK/Commands/MVKCmdRenderPass.mm | |
${CMAKE_CURRENT_LIST_DIR}/MoltenVK/Commands/MVKCmdRendering.mm |
MoltenVK/CMakeLists.txt
Outdated
${CMAKE_CURRENT_LIST_DIR}/MoltenVK/GPUObjects/MVKVulkanAPIObject.mm | ||
${CMAKE_CURRENT_LIST_DIR}/MoltenVK/Layers/MVKExtensions.mm | ||
${CMAKE_CURRENT_LIST_DIR}/MoltenVK/Layers/MVKLayers.mm | ||
${CMAKE_CURRENT_LIST_DIR}/MoltenVK/OS/CAMetalLayer+MoltenVK.m |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
${CMAKE_CURRENT_LIST_DIR}/MoltenVK/OS/CAMetalLayer+MoltenVK.m | |
${CMAKE_CURRENT_LIST_DIR}/MoltenVK/OS/CAMetalLayer+MoltenVK.mm |
MoltenVK/CMakeLists.txt
Outdated
|
||
add_library(MoltenVK-iOS STATIC | ||
${MOLTEN_VK_SOURCES} | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The MoltenVK-iOS
target builds a macOS binary.
MoltenVK/CMakeLists.txt
Outdated
|
||
add_library(MoltenVK-tvOS STATIC | ||
${MOLTEN_VK_SOURCES} | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The MoltenVK-tvOS
target builds a macOS binary.
MoltenVK/CMakeLists.txt
Outdated
|
||
add_executable(MoltenVK-macOS | ||
${MOLTEN_VK_SOURCES} | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The MoltenVK-macOS
target builds the MoltenVKShaderConverter
tool, not a MoltenVK static library.
MoltenVK/CMakeLists.txt
Outdated
${CMAKE_CURRENT_LIST_DIR}/include | ||
) | ||
|
||
target_compile_features(MoltenVK-macOS PRIVATE cxx_std_17) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To get any of the MoltenVK-*
targets to build, I also had to add the following build settings:
CURRENT_PROJECT_VERSION=1.2.9
GCC_PREPROCESSOR_DEFINITIONS=MVK_FRAMEWORK_VERSION=${CURRENT_PROJECT_VERSION}
@@ -0,0 +1,2 @@ | |||
// Auto-generated by MoltenVK | |||
static const char* mvkRevString = "@MVK_GIT_REV@"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file should be moved to a new Templates/cmake
directory.
You might also want to add a CI build for it so people know if break it, as they may not notice if they're using the xcodeproj locally. |
What is still standing in the way of the merge? I would like to implement VK_NV_memory_decompression, but would be very reluctant to work without CMake, as I use Clion. |
I'm still working on getting the CI and ancillary projects working together. Basically me having free time is what's standing in the way of this getting merged. I use CLion and the branch as it stands is good enough to get a MoltenVK built using CLion. |
There is no need for the
You can easily use the Xcode projects for command line builds too, as shown by my example above. You can append
to the last command to set those aspects of the build. You can even set Xcode specific build options by appending them to the end of the command separated by
Alternatively if users want to build using the VulkanSDK, the SDK can provide a CMake find module that would be, in part, generated by the CMake
Submodules are more convenient for users wishing to include MoltenVK into their own projects as a submodule. If that is not a consideration you can continue with the script. It can be called from the
Mac is my primary development environment and I only use Xcode for developing KTX-Software. We use CMake to generate the projects and we build for iOS, iPadOS and macOS. The CMake-generated projects provide a completely effective Xcode environment. The CMakeLists.txt and subsidiary files are the only build files we maintain.
Most of these things are well known to CMake users. In KTX-Software we have a |
This will ultimately end up being a lot more work than adopting CMake to build MoltenVK. The sole advantage is that @billhollings will not have learn CMake. 😄 |
I'd love to see this take shape. Is the main remaining issue that the wrong targets are being built? |
Good question, im always looking forward to this PR. Because AppCode got canceled and I really want wo work in a JetBrains IDE :) |
This allows other tooling such as CLion to work with building this project. It also allows for building with XCode directly. It should make maintaining the project easier in the long run.