-
Notifications
You must be signed in to change notification settings - Fork 6.4k
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
[usd] Bump to v24.08 #40225
base: master
Are you sure you want to change the base?
[usd] Bump to v24.08 #40225
Conversation
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 early comments, not a full review.
- find_package(Python3 "@Python3_VERSION@" EXACT COMPONENTS Development REQUIRED) | ||
+ find_dependency(Python3 "@Python3_VERSION@" EXACT COMPONENTS Development REQUIRED) |
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.
find_dependency
shall not be used with REQUIRED
.
TBH I would suggest to stop adding such replacements in vcpkg (unless there is another reason for corection).
It is to be fixed upstream. In vcpkg, find_package
must not fail if all dependencies are done correctly.
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.
Good catch, I'll update the patch shortly in the meantime I have opened a PR upstream for it PixarAnimationStudios/OpenUSD#3205
-find_package(TBB REQUIRED COMPONENTS tbb) | ||
+find_package(TBB CONFIG REQUIRED) |
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.
-find_package(TBB REQUIRED COMPONENTS tbb) | |
+find_package(TBB CONFIG REQUIRED) | |
-find_package(TBB REQUIRED COMPONENTS tbb) | |
+find_package(TBB CONFIG REQUIRED) | |
+set(TBB_tbb_LIBRARY TBB::tbb) |
and get rid of the following changes.
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 would prefer that we use CMake namespace target all the way, let me try to follow up with an upstream PR to propose this
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.
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.
My suggestion is for keeping the patch in vcpkg small and still sufficiently robust. It is not what I would propose for upstream.
ports/usd/002-vcpkg_find_tbb.patch
Outdated
|
||
+include(CMakeFindDependencyMacro) | ||
+ | ||
+find_dependency(TBB CONFIG REQUIRED) |
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.
+find_dependency(TBB CONFIG REQUIRED) | |
+find_dependency(TBB CONFIG) |
(All use of find_dependency
depends on this patch. Hm.)
-# Use the import targets set by Imath's package config | ||
-if (Imath_FOUND) | ||
- set(__OIIO_IMATH_LIBS "Imath::Imath") |
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 must work in vcpkg.
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.
Can you clarify what you meant ?
I think instead of removing I could use a if(0) ... endif()
.
But what I am really after is to not use Imath::Imath
and use OpenImageIO::OpenImageIO
instead: here OpenUSD is micro-managing the indirect dependency.
ports/usd/007-vcpkg_find_vma.patch
Outdated
#include "pxr/base/tf/diagnostic.h" | ||
|
||
diff --git a/pxr/imaging/hgiVulkan/vk_mem_alloc.cpp b/pxr/imaging/hgiVulkan/vk_mem_alloc.cpp | ||
deleted file mode 100644 |
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.
Delete files with portfile commands instead.
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 for the suggestion.
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.
Any opinions on using patch over portfile command to transform #include "pxr/imaging/hgiVulkan/vk_mem_alloc.h"
in #include <vk_mem_alloc.h>
?
ports/usd/portfile.cmake
Outdated
HEAD_REF master | ||
REF "v${VERSION}" | ||
SHA512 7d4404980579c4de3c155386184ca9d2eb96756ef6e090611bae7b4c21ad942c649f73a39b74ad84d0151ce6b9236c4b6c0c555e8e36fdd86304079e1c2e5cbe | ||
HEAD_REF dev |
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.
HEAD_REF dev | |
HEAD_REF release |
The default branch of openusd is named release.
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 for the review, can you clarify the purpose of HEAD_REF
is it to points to the latest bleeding edge dev commit (here dev
for OpenUSD), or the latest stable repository commit (here release
for OpenUSD)
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.
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 for the early review, here is a partial review on your feedbacks @dg0yt
- find_package(Python3 "@Python3_VERSION@" EXACT COMPONENTS Development REQUIRED) | ||
+ find_dependency(Python3 "@Python3_VERSION@" EXACT COMPONENTS Development REQUIRED) |
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.
Good catch, I'll update the patch shortly in the meantime I have opened a PR upstream for it PixarAnimationStudios/OpenUSD#3205
-find_package(TBB REQUIRED COMPONENTS tbb) | ||
+find_package(TBB CONFIG REQUIRED) |
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 would prefer that we use CMake namespace target all the way, let me try to follow up with an upstream PR to propose this
2b8d78e
to
dd944d0
Compare
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
usd:arm64-uwp=skip | ||
usd:x64-uwp=skip | ||
usd:x64-windows=skip | ||
usd:x64-windows-static=skip |
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.
usd:x64-windows-static=skip should be kept.
# Proper support for a true static usd build is left as a future port improvement. | ||
vcpkg_check_linkage(ONLY_DYNAMIC_LIBRARY) |
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.
Proper support for USD as a static library should probably involve setting the USD monolithic build correctly.
This is not supported before this PR and thus left as a future improvement.
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.
Proper support for USD as a static library should probably involve setting the USD monolithic build correctly
If monolitic means "all parts of USD source", okay.
If monolithic means "one binary including external deps", then "monolithic" is not what vcpkg wants. With CMake config it is easy enough to export the transitive usage requirements. Even if it means adding some conditional find_dependency
.
-# Use the import targets set by Imath's package config | ||
-if (Imath_FOUND) | ||
- set(__OIIO_IMATH_LIBS "Imath::Imath") |
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.
Can you clarify what you meant ?
I think instead of removing I could use a if(0) ... endif()
.
But what I am really after is to not use Imath::Imath
and use OpenImageIO::OpenImageIO
instead: here OpenUSD is micro-managing the indirect dependency.
ports/usd/007-vcpkg_find_vma.patch
Outdated
#include "pxr/base/tf/diagnostic.h" | ||
|
||
diff --git a/pxr/imaging/hgiVulkan/vk_mem_alloc.cpp b/pxr/imaging/hgiVulkan/vk_mem_alloc.cpp | ||
deleted file mode 100644 |
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 for the suggestion.
ports/usd/007-vcpkg_find_vma.patch
Outdated
#include "pxr/base/tf/diagnostic.h" | ||
|
||
diff --git a/pxr/imaging/hgiVulkan/vk_mem_alloc.cpp b/pxr/imaging/hgiVulkan/vk_mem_alloc.cpp | ||
deleted file mode 100644 |
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.
Any opinions on using patch over portfile command to transform #include "pxr/imaging/hgiVulkan/vk_mem_alloc.h"
in #include <vk_mem_alloc.h>
?
@dg0yt sorry for taking this long, I think I have addressed all your suggestions and comments. |
Also OpenUSD v24.11 has just been released and I would like to work toward making |
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.
(Community feedback)
You could go straight to the latest version if it isn't too much work to update the patches.
Patch should be kept small. This might be different from what one submit upstream.
-find_package(TBB REQUIRED COMPONENTS tbb) | ||
+find_package(TBB CONFIG REQUIRED) |
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.
My suggestion is for keeping the patch in vcpkg small and still sufficiently robust. It is not what I would propose for upstream.
# Proper support for a true static usd build is left as a future port improvement. | ||
vcpkg_check_linkage(ONLY_DYNAMIC_LIBRARY) |
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.
Proper support for USD as a static library should probably involve setting the USD monolithic build correctly
If monolitic means "all parts of USD source", okay.
If monolithic means "one binary including external deps", then "monolithic" is not what vcpkg wants. With CMake config it is easy enough to export the transitive usage requirements. Even if it means adding some conditional find_dependency
.
if(NOT VCPKG_BUILD_TYPE OR VCPKG_BUILD_TYPE STREQUAL "debug") | ||
vcpkg_copy_tools( | ||
TOOL_NAMES ${tools} | ||
SEARCH_DIR "${CURRENT_PACKAGES_DIR}/debug/bin" | ||
DESTINATION "${CURRENT_PACKAGES_DIR}/debug/tools/${PORT}" | ||
) | ||
endif() |
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 guess there is a reason why vcpkg_copy_tools
normally omits debug tools.
If the port is really meant to install the debug tools, it might be good to add a comment.
And btw it is enough to check if(NOT VCPKG_BUILD_TYPE)
in a portfile.
@@ -1,22 +1,12 @@ | |||
{ | |||
"name": "usd", | |||
"version": "24.5", | |||
"version-string": "24.08", |
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.
"version-string": "24.08", | |
"version": "24.8", |
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.
we use version because it allows for >=, and we transform VERSION to the desired tag.
REF "v${USD_VERSION}" | ||
SHA512 e510f6421caba5e74c6efe5b56b17e9c9741ece0cfd5020148ca89b3ac32bd8781ab00dfc7a134163c85af3f4f01f2529a9baa5a9df9b0c80cbca003e6d199e2 | ||
HEAD_REF master | ||
REF "v${VERSION}" |
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.
REF "v${VERSION}" | |
REF "v${USD_VERSION}" |
This PR updates
usd
port to v24.08.This versions has no more dependency on
boost
(when USD Python is disabled, which is already set like this in the current port).Moreover this version is now compatible with oneTBB so we should now be able to test it on the CI.
./vcpkg x-add-version --all
and committing the result.