-
Notifications
You must be signed in to change notification settings - Fork 6.7k
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
[baresip-libre] Add new port #31735
[baresip-libre] Add new port #31735
Conversation
@alfredh, Thanks for your PR, I tested the usage locally failed by
CMakeLists.txt:
|
Note: I will be converting your PR to draft status. When you respond, please revert to "ready for review". That way, I can be aware that you've responded since you can't modify the tags. |
here are my files after "vcpkg install libre":
|
This is not going to work.
So the config comes from dir vcpkg_cmake_config_fixup(CONFIG_PATH lib/cmake/re PACKAGE_NAME re) Yes, the parameter is |
hi, thanks for your comment. I updated the code as suggested. |
Don't forget to mark the PR as ready for review when you are done. |
@alfredh, I tested usage again but also failed: CMakeLists:
|
It is: find_package(re CONFIG REQUIRED) |
ports/libre/usage
Outdated
The package libre is compatible with built-in CMake targets: | ||
|
||
find_package(libre CONFIG REQUIRED) | ||
target_link_libraries(main PRIVATE libre::re) |
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.
Which namespace does it use?
See example usage from testcode here: https://github.com/baresip/re/blob/main/test/CMakeLists.txt#L62
many thanks for review and testing. I have updated the "usage" text. We have added the namespace "libre" but for some reason I dont think it is working. |
Oh, re-config.cmake isn't exported CMake config. You need help with CMake ... |
... instead of review comments: |
Many thanks for your patch, I have applied it to this PR. There are some checks failing, currently around 5 failing checks, including |
ports/libre/wip.patch
Outdated
URL: @CMAKE_PROJECT_HOMEPAGE_URL@ | ||
-Libs: -L${libdir} -lre | ||
-Libs.private: -L${libdir} -lre -ldl -lssl -lcrypto -lz -lpthread | ||
+Libs: -L${libdir} -lre@PC_LIBNAME_SUFFIX@ |
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.
-+Libs: -L${libdir} -lre@PC_LIBNAME_SUFFIX@
++Libs: -L${libdir} -l@PC_LIBNAME@
(Slipped through, but only affects MSVC.)
@alfredh, I tested usage again but also failed:
|
hi, which platform are you building for? Please note that for Windows we only support .lib files and no .dll files:
Do you have the testprogram and command, so I can test it locally ? |
The |
Hi, I have tested on arm64-osx and it appears to be working:
CMake file:
usageTest.cpp: #include <re.h>
int main()
{
libre_init();
re_printf("version %s\n", sys_libre_version_get());
} |
@alfredh, Could you please test it on regular windows platform? Such as |
ports/libre/wip.patch
Outdated
+ elseif(TARGET libre::re AND (NOT BUILD_SHARED_LIBS OR NOT TARGET libre::re_shared)) | ||
+ add_library(libre::libre INTERFACE IMPORTED) | ||
+ set_target_properties(libre::libre PROPERTIES INTERFACE_LINK_LIBRARIES libre::re_shared) |
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 usage error comes from this line which should use libre::re instead of libre::re_shared.
thanks Kai, I updated the patch and pushed the changes. I also tested the usage locally with triplet
|
Tested usage successfully by
|
Hi, I have renamed the project as suggested. Is there any chance to merge this PR soon ? Br, |
Great, thank you all for help and review! |
find_package
calls are REQUIRED, are satisfied byvcpkg.json
's declared dependencies, or disabled with CMAKE_DISABLE_FIND_PACKAGE_Xxxvcpkg.json
matches what upstream says.vcpkg.json
matches what upstream says../vcpkg x-add-version --all
and committing the result.