-
Notifications
You must be signed in to change notification settings - Fork 6.6k
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
[xmp-toolkit-sdk] Add XMP Toolkit to vcpkg #43566
base: master
Are you sure you want to change the base?
Conversation
@microsoft-github-policy-service agree |
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 need to be more confident that the third party vendored dependencies are removed to land this.
I would like to see an example use to ensure things are installed correctly given that this is 'unconventional' (doesn't install normal CMake or pkgconfig bindings)
// It avoids the need to patch multiple files. | ||
// If/when Adobe XMP Toolkit SDK become more package manager friendly, this header file should be removed. | ||
|
||
#include "@CURRENT_INSTALLED_DIR@/@TRIPLET@/include/zlib.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.
I think you could do vcpkg_replace_string(each file here [[#include "third-party/zlib/zlib.h"]] [[#include <zlib.h>]])
and achieve this effect without having to add random extra source files.
set(pdf_handler_mini_pdfl_dir "windows") | ||
set(pdf_handler_resource_dir "win") | ||
|
||
# The XMPFilesPlugins folder provides the support code and resources you need to build handlers for custom file formats. |
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 point us at an example that needs to work that uses this stuff so we can prove that it's installed correctly? No change to this code requested.
if("${VCPKG_LIBRARY_LINKAGE}" STREQUAL "static") | ||
set(build_static "On") | ||
if("${VCPKG_CRT_LINKAGE}" STREQUAL "static") | ||
set(expat_debug_lib "libexpatdMT.lib") | ||
set(expat_release_lib "libexpatMT.lib") | ||
else() | ||
set(expat_debug_lib "libexpatdMD.lib") | ||
set(expat_release_lib "libexpatMD.lib") | ||
endif() | ||
else() | ||
set(build_static "Off") | ||
set(expat_debug_lib "libexpatd.lib") | ||
set(expat_release_lib "libexpat.lib") | ||
endif() | ||
|
||
# Redirect build to use expat library from vcpkg | ||
configure_file(${CURRENT_PORT_DIR}/expat.h ${SOURCE_PATH}/third-party/expat/lib/expat.h @ONLY) | ||
string(APPEND VCPKG_LINKER_FLAGS_DEBUG " ${CURRENT_INSTALLED_DIR}/${TRIPLET}/debug/lib/${expat_debug_lib} ") | ||
string(APPEND VCPKG_LINKER_FLAGS_RELEASE " ${CURRENT_INSTALLED_DIR}/${TRIPLET}/lib/${expat_release_lib} ") | ||
|
||
# Redirect build to use zlib library from vcpkg | ||
configure_file(${CURRENT_PORT_DIR}/zlib.h ${SOURCE_PATH}/third-party/zlib/zlib.h @ONLY) | ||
string(APPEND VCPKG_LINKER_FLAGS_DEBUG " ${CURRENT_INSTALLED_DIR}/${TRIPLET}/debug/lib/zlibd.lib ") | ||
string(APPEND VCPKG_LINKER_FLAGS_RELEASE " ${CURRENT_INSTALLED_DIR}/${TRIPLET}/lib/zlib.lib ") |
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.
These need to use CMake configs or pkgconfig rather than assuming how this layout happens to work right now.
endif() | ||
|
||
# Redirect build to use expat library from vcpkg | ||
configure_file(${CURRENT_PORT_DIR}/expat.h ${SOURCE_PATH}/third-party/expat/lib/expat.h @ONLY) |
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 make sure this was done correctly and there were no more vendored dependency assumptions I tried adding:
file(REMOVE_RECURSE "${SOURCE_PATH}/third-party")
file(MAKE_DIRECTORY "${SOURCE_PATH}/third-party")
and that failed because there are still many internal references to these vendored libs.
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.
40f87ef contains everything I tried
Closes #909, closes #42887
find_package
calls are REQUIRED, are satisfied byvcpkg.json
's declared dependencies, or disabled with CMAKE_DISABLE_FIND_PACKAGE_Xxx.vcpkg.json
matches what upstream says.vcpkg.json
matches what upstream says../vcpkg x-add-version --all
and committing the result.