-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Autodesk: build_usd.py now installs Vulkan dependencies #3603
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
base: dev
Are you sure you want to change the base?
Autodesk: build_usd.py now installs Vulkan dependencies #3603
Conversation
Filed as internal issue #USD-10914 (This is an automated message. See here for more information.) |
/AzurePipelines run |
I've tested this out and it works really well besides two issues:
|
3808f58
to
901ee01
Compare
Thanks for the feedback, I made some changes, let me know if that fixes your issues. |
I can confirm that it fixes my issues! Thanks! :) |
/AzurePipelines run |
@DDoS the new commit breaks compilation for me.
If I skip the new commit and just apply the Seems like your changes somehow broke the logic for passing |
@DarkDefender can you please share your |
@DDoS if I grep the cmake cache file I get these two:
|
@DarkDefender can you add some debug |
|
||
# In either case we have custom logic for SPIRV-Reflect | ||
find_package(SpirvReflect MODULE REQUIRED) | ||
list(APPEND VULKAN_LIBS $<BUILD_LOCAL_INTERFACE:spirv-reflect>) |
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.
If I change this back to list(APPEND VULKAN_LIBS spirv-reflect)
it seems to work.
I'm guessing that the SpirvReflect_IS_SOURCE_DEP
check is needed here.
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.
That's strange, I didn't expect this to make a difference. I'll have to try harder and reproduce this issue locally to debug. Thanks for helping out.
The debug message() gets triggered for me. |
/AzurePipelines run |
@DarkDefender Which CMake version are you using? Which build system? I really can't reproduce this issue. When I build with |
/AzurePipelines run |
@DDoS I'm using cmake 3.31.7 and ninja. Are you sure that the |
@DarkDefender The relevant
When Since it appears you're using a system package manager install of spirv-reflect, can you confirm the header does support this define? It was added three years ago: KhronosGroup/SPIRV-Reflect@f2241d5 (sdk-1.3.216.0). |
@DDoS I am using the latest git version of https://github.com/KhronosGroup/SPIRV-Reflect It seems like on my end I'm not sure why though. |
This almost feels like a CMake bug. I'm using 3.31.6, we're only one patch version off. Could you downgrade and test again? |
I tried with the cmake 3.31.6 version, no change.
@DDoS Can you see if it also triggers on your machine with the same flags to |
] | ||
RunCMake(context, force, extraArgs) | ||
CopyFiles(context, 'spirv_reflect.h', 'include') | ||
CopyDirectory(context, 'include/spirv', 'include/spirv') |
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 think we should remove this as the spirv header file is not used and is not supposed to be used.
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 header is the one included by spirv_reflect.h
. It's needed.
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.
Ah, you are right. Never mind. I somehow got the idea that the build script also installed the spirv headers. But it doesn't. Sorry for the noise.
@DDoS Seems like the So I'm guessing that there are some flags that doesn't get passed on properly in the monolithic build or something. |
@DarkDefender Thanks for digging deeper into this! I never tried a monolithic build before, I'll give it a try, and fingers crossed that will reproduce the issue. I think I already have an idea of what the problem may be. Update: I can confirm this reproduces the issue! |
/AzurePipelines run |
1 similar comment
/AzurePipelines run |
887d0aa
to
2a82301
Compare
@DarkDefender this should fix the problem. Let me know if it works now, thanks. |
Seems to build fine for me now again! :) I just have one last bit of feedback. (See my review comment) |
if (SpirvReflect_FOUND AND NOT EXISTS "${spirv-reflect_INCLUDE_DIR}/include/spirv/unified1/spirv.h") | ||
# Most probably not from the Vulkan SDK, so use a "system path." | ||
# If installed from build_usd.py this will be found in the USD install path. | ||
if (SpirvReflect_IS_SOURCE_DEP) |
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 don't think this is defined anymore so this will always be false?
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.
Fixed! Good catch, I think this somehow got lost in the cherry-pick.
2a82301
to
c1663f8
Compare
@@ -12,7 +12,7 @@ | |||
#include "pxr/imaging/hgiVulkan/sampler.h" | |||
#include "pxr/imaging/hgiVulkan/diagnostic.h" | |||
|
|||
#include <float.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.
Just one last thing I noticed.
On older gcc versions (GCC 11), this include or #include <cfloat>
is needed because it can't find the definition of FLT_MAX
otherwise. Don't know why this isn't needed in newer gcc versions, but I don't think we do any harm by explicitly including <cfloat>
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.
re-added
c1663f8
to
b8db3a3
Compare
/AzurePipelines run |
All seems to be working fine on my side now! Thank you so much for all the back and forth! :) |
/AzurePipelines run |
Description of Change(s)
build_usd.py
, build and install:build_usd.py
--use-vulkan-sdk
build_usd.py
BUILD_LOCAL_INTERFACE
to keep the hgiVulkan private dependencies out of thepxr
target export.pxr
targets. You don't need to link against as many targets.find_package
(using the USD install path) and link against the installed Vulkan dependencies.pxrConfig.cmake
.build_usd.py
differences.BUILD_LOCAL_INTERFACE
) need to be found.Link to proposal (if applicable)
Fixes Issue(s)
Checklist
I have created this PR based on the dev branch
I have followed the coding conventions
I have added unit tests that exercise this functionality (Reference:
testing guidelines)
I have verified that all unit tests pass with the proposed changes
I have submitted a signed Contributor License Agreement (Reference:
Contributor License Agreement instructions)