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
CMakeLists: fix import of external jansson and utf8cpp #7982
base: master
Are you sure you want to change the base?
Conversation
@@ -320,10 +320,14 @@ if (WITH_SYSTEM_PROVIDED_3PARTY) | |||
set(GFLAGS_USE_TARGET_NAMESPACE ON) | |||
find_package(gflags REQUIRED) | |||
|
|||
find_package(PkgConfig) | |||
pkg_check_modules(jansson REQUIRED IMPORTED_TARGET jansson) | |||
add_library(jansson::jansson ALIAS PkgConfig::jansson) |
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.
How did it work on other systems without explicit PkgConfig specification? Will it continue to work? Why it doesn't find the library without explicit PkgConfig?
CC @Ferenc-
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.
Normally, jansson is vendored as a 3party and configured with CMake:
https://github.com/organicmaps/organicmaps/blob/master/3party/CMakeLists.txt#L35
This will continue to work. A jansson installation of a distribution normally contains (just) a pkgconfig file (it's pretty common). See for example the debian package: https://packages.debian.org/bookworm/amd64/libjansson-dev/filelist
Why it doesn't find the library without explicit PkgConfig?
Appearently, CMake's find_package
does not search for pkgconfig files. It needs the FindPkgConfig
module for that: https://cmake.org/cmake/help/latest/module/FindPkgConfig.html
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.
Does it mean that on Debian/Ubuntu the current master works with PkgConfig for utf8 even without explicitly writing it in cmake? If yes, why? Could it be that cmake detects some pkgconfig vars/files/configs on Debian/Ubuntu, but doesn't do it on Gentoo for some reason? Maybe some env vars are missing?
cmake has --trace
parameter that may be helpful to discover issues with packages.
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.
Do you mean the test pipeline here? Does it build with or without WITH_SYSTEM_PROVIDED_3PARTY
?
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.
How did it work on other systems without explicit PkgConfig specification?
It's not past tense, it is present. It is still working with WITH_SYSTEM_PROVIDED_3PARTY
perfectly fine, if one installs the latest release of jansson on a system. The find_package
is looking for janssonConfig.cmake
first, and that is by default installed to /usr/local/lib/cmake/jansson/
if you install from source.
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.
Upstream jansson provides a pkgconfig file instead of a cmake file.
Just because your distro doesn't have a cmake file for jansson, that doesn't mean that upstream doesn't provide it. I believe there is evidence to the contrary.
These are distros that don't provide a cmake file but a pkgconfig file:
I tried building myself with these commands (as recommended in the readme):
I got these files:
There is a cmake folder within libjansson and it seems to be possible to build with cmake. I'm assuming in this case, it installs the cmake file, but no distro that I checked does that and it is not recommended by upstream. |
I do not understand why "build 3p from source" approach should be used if you can literally "build 3p of the necessary, usually most recent version from organic maps source" automatically. Do we need a more flexible way to specify which libs should be taken from the system, and which libs should be built from OM source? Of course, I would prefer to keep things simple, and always build everything from OM repo. It helps to test 3ps that are used on mobiles and also on other desktop machines. And we can always use the bleeding edge versions of 3ps with the latest features and fixes. Without wasting time of maintainers to provide patches for each system. |
That's a respectable research you did on distro packages. I just don't immediately see why. My comment was merely about the commit message, which has a statement about |
I don't think it was even discussed which approach
I don't think so. I think the discussion is mainly about the nuances of how specifically |
Upstream jansson offers compiling with autotools and CMake. If it is compiled with autotools (the recommended way) it does only provide a pkgconfig file and not a cmake file. This is also the way most distributions provide jansson, so this commit changes the behavior to use pkgconfig, too. Signed-off-by: Gerion Entrup <[email protected]>
Ah, I understood it in a way that you meant the patch as a whole. I reformulated the commit message and hope it is better formulated now.
Interestingly, this results in a compilation error for me (while an autotools build works).
I agree to that. |
- adjust patches. They depend on organicmaps/organicmaps#7982 organicmaps/organicmaps#6310 - fix dependencies. OrganicMaps has more external libs now. Moreover all dynamically linked libraries were checked and added as deps. - don't install test targets Signed-off-by: Gerion Entrup <[email protected]>
- adjust patches. They depend on organicmaps/organicmaps#7982 organicmaps/organicmaps#6310 - fix dependencies. OrganicMaps has more external libs now. Moreover all dynamically linked libraries were checked and added as deps. - don't install test targets Signed-off-by: Gerion Entrup <[email protected]>
- adjust patches. They depend on organicmaps/organicmaps#7982 organicmaps/organicmaps#6310 - fix dependencies. OrganicMaps has more external libs now. Moreover all dynamically linked libraries were checked and added as deps. - don't install test targets Signed-off-by: Gerion Entrup <[email protected]>
- adjust patches. They depend on organicmaps/organicmaps#7982 organicmaps/organicmaps#6310 - fix dependencies. OrganicMaps has more external libs now. Moreover all dynamically linked libraries were checked and added as deps. - don't install test targets Signed-off-by: Gerion Entrup <[email protected]>
- adjust patches. They depend on organicmaps/organicmaps#7982 organicmaps/organicmaps#6310 - fix dependencies. OrganicMaps has more external libs now. Moreover all dynamically linked libraries were checked and added as deps. - don't install test targets Signed-off-by: Gerion Entrup <[email protected]>
No description provided.