Replace fetch-binary-deps.py with pure CMake #121
Conversation
also update gitignore
replaced with cmake code
|
note -- you can see messages posted by "message(VERBOSE)" by running cmake with |
| ${AVFORMAT_INCLUDE_DIR} | ||
| ${AVUTIL_INCLUDE_DIR} | ||
| ${Vulkan_INCLUDE_DIRS} | ||
| ${SWSCALE_INCLUDE_DIR} |
There was a problem hiding this comment.
i don't think these were doing anything before -- find_library just finds a library file; but it's find_package with an appropriately written CMake find-module that will often set "lib_INCLUDE_DIR". from what i can tell, things just worked before because everything was unpacked into the build dir.
There was a problem hiding this comment.
Could be the case, always better to remove cruft like this if we can. Probably those are there from historical times when Scintillator included those dependencies in third_party as submodules.
| add_library(VulkanMemoryAllocator STATIC "VulkanMemoryAllocatorBuild.cpp") | ||
| target_include_directories(VulkanMemoryAllocator INTERFACE VulkanMemoryAllocator/src) | ||
| target_include_directories(VulkanMemoryAllocator PRIVATE "${SCIN_EXT_INSTALL_DIR}/include") | ||
| target_include_directories(VulkanMemoryAllocator PUBLIC VulkanMemoryAllocator/src) |
There was a problem hiding this comment.
PUBLIC = INTERFACE && PRIVATE
There was a problem hiding this comment.
So the memory allocator needs the vulkan SDK to compile itself. I thought PRIVATE was telling cmake to provide the path the vulkan SDK header to that library but not transitively provide that path to dependent libraries.
There was a problem hiding this comment.
you had it set to INTERFACE before, which tells cmake to only transitively provide this include directory, but not use it when compiling VulkanMemoryAllocator itself. it looks like scinsynth depends on these headers too, though -- if I set this to PRIVATE, then i see this:
In file included from ../src/scinsynth.cpp:13:
../src/vulkan/Buffer.hpp:6:10: fatal error: vk_mem_alloc.h: No such file or directory
6 | #include "vk_mem_alloc.h"
| ^~~~~~~~~~~~~~~~
compilation terminated.
There was a problem hiding this comment.
That's correct. The Vulkan 1.0 standard, which we are limited to for the moment because the MacOS Vulkan emulation only supports 1.0, allows for a very small number of allocated objects. Best practice is to do a single memory allocation and BYO memory allocator, and we're using this library for exactly that.
There was a problem hiding this comment.
makes sense! i'm just pointing out that the memory allocator lib isn't the only part of this project that requires these headers.
|
Oh, Travis is breaking right now because it assumes that the python script creates the /build directory as a byproduct of being run. |
| target_include_directories(VulkanMemoryAllocator INTERFACE VulkanMemoryAllocator/src) | ||
| target_include_directories(VulkanMemoryAllocator PRIVATE "${SCIN_EXT_INSTALL_DIR}/include") | ||
| target_include_directories(VulkanMemoryAllocator PUBLIC VulkanMemoryAllocator/src) | ||
| target_link_libraries(VulkanMemoryAllocator PUBLIC vulkan) |
There was a problem hiding this comment.
i think this dependency should be public, actually. since VulkanMemoryAllocator is static, it won't be linked with libvulkan when it's built; so anything linking against VulkanMemoryAllocator should link against vulkan too. this also makes the include directories a little more generic
|
Thanks for working on this! I wonder if the download is slow in part because I have s3 configured to serve from west coast buckets. Any way to make that more generic? |
i think i just fixed it, but i don't have time to look at it again if it's failing FYI. |
glad to help! yes, that's probably part of it. i don't know if there's a way to make it more generic, i hope so! |
|
it looks like changing the "s3-website-west-..." part of the domain name to just "s3" makes it generic, i can't tell if it's that much faster for me though. |
|
ah darn, two things that need more work at least -- (1) there's a lot of installation logic i neglected, and (2) |
v3.15 seems a little new but I'd be OK with moving up the minimum requirement to that. I'm personally a big fan of modern tooling and APIs. If that doesn't sit well with you I'd also be fine with message(STATUS), as I'm also OK with chatty build logs. |
|
Hey Brian, I've got some time to hack on Scintillator this week, so this PR is likely to get pretty stale. Anything I can do to help get this across the finish line? |
|
thanks for the ping @lnihlen ! nope, i just haven't had time to work on it lately. you can close it if you want, and i'll rebase it and reopen when i'm ready for more review. |
|
i'm going to close this until i have time to work on it again. thanks again for the help with this :) |
Purpose and motivation
Fixes #117
Implementation
i tried to include some of the functionality of the original. in particular i kept the 'delete this folder to redo
the process' behavior, and added two options in CMake to (1) clobber any existing downloads and (2) check for updates
when rerunning cmake config. anything more complex than that seemed like it would take a lot of extra work, and i
think 'delete this folder to force reinstall' is perfectly OK behavior. i made checking for updates off by
default, because it slowed down cmake reconfigure on my machine by quite a lot. this seems more like a
developer/maintainer option anyway. should i document it somewhere else?
Types of changes
Status
to run through it yourself though