-
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
[icu] add host depedency #16577
[icu] add host depedency #16577
Conversation
@Neumann-A Do you have any other questions? |
@JackBoosY: all tools should be moved into |
Ok, it now don't use the |
Shoud we wait for PR #16627? |
Then we can use |
I don't think that the |
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 looks really great and I'm excited to merge!
I agree with @Neumann-A that it's probably better to use vcpkg_internal_get_cmake_vars()
instead of hardcoding -arch x86_64
.
ports/icu/portfile.cmake
Outdated
# Install executables from ${CURRENT_BUILDTREES_DIR}/${RELEASE_TRIPLET}/bin to /tools/icu/bin | ||
file(GLOB ICU_TOOLS | ||
${CURRENT_BUILDTREES_DIR}/${RELEASE_TRIPLET}/bin/*${VCPKG_HOST_EXECUTABLE_SUFFIX} | ||
${CURRENT_PACKAGES_DIR}/lib/icu*${ICU_VERSION_MAJOR}.dll |
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.
It would probably be better to use vcpkg_copy_tool_dependencies()
instead of manually globbing the DLLs. This function may be updated to support .so
and .dylib
in the future.
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.
Additionally, we could probably replace the original purge of the bin directory above (line ~340ish) with vcpkg_copy_tools()
. Even if vcpkg_copy_tools()
isn't applicable, it would be better to move those files to the tools directory instead of deleting and re-copying from the buildtrees.
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.
Additionally, we could probably replace the original purge of the bin directory above (line ~340ish) with vcpkg_copy_tools()
The problem with vcpkg_copy_tools()
is, that it copies to ${CURRENT_PACKAGES_DIR}/tools/${PORT}
, but the files must be copied to ${CURRENT_PACKAGES_DIR}/tools/${PORT}/bin
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.
you could use set(PORT "${PORT}/bin")
but you could also add an option like SUBDIR
or DESTINATION
to vcpkg_copy_tools
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 have added a DESTINATION
parameter to vcpkg_copy_tools
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 problem with vcpkg_copy_tool_dependencies
is, that it does not copy the tool dependencies ...
On my mashine it copies nothing and cmake can't start the applications because of missing dlls
Refolve conflicts/updating version files
Will rerun the osx pipeline test later. |
When building
|
I think all the ci errors are unrelated to this PR |
@autoantwort I've got the failure log, will restart pipeline test now. |
Maybe #17593 fixes that |
Ok seems that a simple rerun solved the problem :) |
that is because gdk-pixbuf will not be restored in the rerun and so libheif does not pick it up and installs successfully. |
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.
double documentation! o noes
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.
Seems that the DESTINATION option was also added in another PR
Most random ci errors were resolved by a rerun, now this PR only depends on #17610 |
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
Thanks @autoantwort :) |
[icu] add host depedency (microsoft#16577)
Describe the pull request
Same as #16433, but now with host support in vcpkg as base