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
[tiny-process-library] Bump tiny-process-library to 2.0.4 #14732
Conversation
@traversaro, thanks for the PR! Here is CI testing results, could you please take a look? x86-windows/x64-windows/arm64-windows/
failure logs for x64-windows (1).zip x64-windows-static/x64-linux
|
Thanks for the comment @PhoebeHui ! I fixed all the problems, and now the port should work fine, I am not sure what happened to the CI jobs that staled. |
) | ||
|
||
vcpkg_configure_cmake( | ||
SOURCE_PATH ${SOURCE_PATH} | ||
PREFER_NINJA | ||
OPTIONS | ||
-DBUILD_TESTING=OFF | ||
-DCMAKE_WINDOWS_EXPORT_ALL_SYMBOLS=ON |
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.
Is the author of the library using it? if not, we should not add this in vcpkg. See https://github.com/microsoft/vcpkg/blob/master/docs/maintainers/maintainer-guide.md#do-not-add-cmake_windows_export_all_symbols
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.
tiny-process-library
is a small library with a few function and no templates, so CMAKE_WINDOWS_EXPORT_ALL_SYMBOLS
works as intended. It was already used in the patch that this PR removes. Furthermore, it was also added upstream in https://gitlab.com/eidheim/tiny-process-library/-/merge_requests/39, but it is set for an error after the add_library
call, so it does not work as intended. Passing it for a command line is just a way to solve this problem before it is solved upstream (like a patch, but more future-proof).
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.
@strega-nil, could you help confirm?
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.
Yeah, LGTM
fyi @j-rivero, if you want to include |
And thanks a lot @PhoebeHui and @strega-nil for reviewing and merging this! |
Thanks Silvio. Gazebo classic is not using dependencies from vcpkg in our CI, something I will need to fix or change at some point. |
Describe the pull request
What does your PR fix? This PR updates the tiny-process-library to the 2.0.3
Which triplets are supported/not supported? Have you updated the CI baseline? The baseline should not be affected.
Does your PR follow the maintainer guide? Yes