-
Notifications
You must be signed in to change notification settings - Fork 7.1k
[pjproject] add new port #47427
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
base: master
Are you sure you want to change the base?
[pjproject] add new port #47427
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.
IMO source modifications should be implemented as patches as much as possible.
For getting flags and actual link libs for dependencies, you could use x_vcpkg_pkgconfig_get_modules
.
When generating pkgconfig files, the dependencies should be named in Requires.private
(or Requires
) instead of spelling out the libs.
Given the unusual port structure and the dependency network, it might make sense to add a test port to scripts/test_ports
. Examples are there, e.g. vcpkg-ci-libavif
.
ports/pjproject/usage
Outdated
pjproject provides pkg-config support for integration: | ||
|
||
find_package(PkgConfig REQUIRED) | ||
pkg_check_modules(PJ REQUIRED IMPORTED_TARGET libpjproject) | ||
target_link_libraries(main PRIVATE PkgConfig::PJ) |
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 this sufficiently covered by the vcpkg's heuristical output. (The vcpkg explicitly decided against documenting how to consume pkgconfig in cmake. It doesn't work with multi-config anyways.)
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.
Done in 683167f
ports/pjproject/vcpkg.json
Outdated
"description": "PJSIP (pjproject) is a free and open source multimedia communication library implementing standard based protocols such as SIP, SDP, RTP, STUN, TURN, and ICE. It combines signaling protocol (SIP) with rich multimedia framework and NAT traversal functionality into high level API that is portable", | ||
"homepage": "https://github.com/pjsip/pjproject", | ||
"license": "GPL-2.0-or-later", | ||
"supports": "(windows & !uwp) | mingw | linux", |
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.
"supports": "(windows & !uwp) | mingw | linux", | |
"supports": "linux | (windows & !uwp)", |
windows includes mingw.
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.
Done in f3ae18e
ports/pjproject/vcpkg.json
Outdated
{ | ||
"name": "pkgconf", | ||
"host": true | ||
}, |
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.
{ | |
"name": "pkgconf", | |
"host": true | |
}, |
Use
vcpkg_find_acquire_program(PKGCONFIG)
set(ENV{PKG_CONFIG} "${PKGCONFIG}")
in the port file 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.
Removed pkgconf
from vcpkg.json
file, but in my tests it wasn't necessary to add
vcpkg_find_acquire_program(PKGCONFIG)
set(ENV{PKG_CONFIG} "${PKGCONFIG}")
to the port file since pkgconfig is getting pulled via vcpkg-pkgconfig-get-modules
which is in vcpkg.json
Is there any other reason to put it there that I'm not aware of?
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.
vcpkg-pkgconfig-get-modules
takes care of its own dependencies.
But what we have now is that at least the unix build relies on transitve dependencies with
set(ENV{PKG_CONFIG} "${CURRENT_HOST_INSTALLED_DIR}/tools/pkgconf/pkgconf")
This is fragile.
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 clarification.
Done in cf51d74
6b2b6c2
to
a4fa053
Compare
a4fa053
to
5ecfb97
Compare
Done in the following commits |
@ShahriyarB, first, thank you for your contribution to the registry. We appreciate the effort you've put into this port. That said, the team made a few suggestions for the port. For example, we noticed the port uses a highly factorized approach with multiple helper functions, and worry this will create a higher maintenance burden over time. The addition of a test port was appreciated, but, in our experience, we've found that simpler, more linear ports tend to be easier for the community to review and maintain. We suggest inlining the logic directly within the Additionally, we noticed the port adds pkgconfig support that upstream doesn't provide. We generally prefer to stick with upstream's build system choices to avoid diverging from what the library authors intended and to reduce maintenance over time. In this case, we'd suggest sticking with upstream's CMake build system for Windows and avoiding the pkgconfig additions that upstream doesn't currently support. Thanks again! |
Thanks for the review. I can make the change to move all logic into portfile.cmake. Regarding pkg-config support on Windows, the upstream project is not based on CMake, it provides a Makefile for Unix builds and a |
Unfortunately, we are hesitant to add pkgconfig support that isn't native to upstream. That said, you are more than welcome to use overlay ports for your specific project. We encourage you to submit a PR with upstream to add pkgconfig support you need. |
It's funny that |
I just noticed that the |
Perhaps we should hold off until upstream publishes a new release and, in the meantime, continue using their That sounds reasonable to me! |
Closes #30391
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.Installs PJPROJECT (PJSIP) library with its dependencies (ffmpeg, openssl, libvpx, sdl2, opus)