CMakeLists: fix import of external jansson and utf8cpp #7982

2024-04-25T08:17:50Z

gerion0 commented

2024-04-25 08:17:50 +00:00

(Migrated from github.com)

No description provided.

biodranik (Migrated from github.com) reviewed 2024-04-25 08:31:01 +00:00

					
				@ -322,1 +322,4 @@

				  find_package(PkgConfig)

				  pkg_check_modules(jansson REQUIRED IMPORTED_TARGET jansson)

				  add_library(jansson::jansson ALIAS PkgConfig::jansson)

biodranik (Migrated from github.com) commented

2024-04-25 08:31:01 +00:00

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-

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-

biodranik (Migrated from github.com) reviewed 2024-04-25 08:33:30 +00:00

CMakeLists.txt

biodranik (Migrated from github.com) commented

2024-04-25 08:33:30 +00:00

Why is this line needed only for utf8cpp, and not for other libs? How does it work?

gerion0 (Migrated from github.com) reviewed 2024-04-25 08:37:39 +00:00

CMakeLists.txt

					
				@ -322,1 +322,4 @@

				  find_package(PkgConfig)

				  pkg_check_modules(jansson REQUIRED IMPORTED_TARGET jansson)

				  add_library(jansson::jansson ALIAS PkgConfig::jansson)

gerion0 (Migrated from github.com) commented

2024-04-25 08:37:39 +00:00

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

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

gerion0 (Migrated from github.com) reviewed 2024-04-25 08:47:12 +00:00

CMakeLists.txt

gerion0 (Migrated from github.com) commented

2024-04-25 08:47:12 +00:00

This is dependent on the library and which target name it provides (with a namespace or without one). Actually, this is version dependent for utf8cpp:

utf8cpp version 4.0.5 seems to provide utf8cpp::utf8cpp on its own
utf8cpp version 3.2.5 does not

Gentoo provides utf8cpp version 3.2.1. It might work without this commit for version 4.0.5. If you are not willing to merge it, I would provide it within the Gentoo tooling, since it is distribution specific then.

This is dependent on the library and which target name it provides (with a namespace or without one). Actually, this is version dependent for utf8cpp: - utf8cpp version 4.0.5 seems to provide `utf8cpp::utf8cpp` [on its own](https://github.com/nemtrif/utfcpp/blob/v4.0.5/utf8cppConfig.cmake.in#L7) - utf8cpp version 3.2.5 [does not](https://github.com/nemtrif/utfcpp/blob/v3.2.5/utf8cppConfig.cmake.in#L7) Gentoo provides utf8cpp version 3.2.1. It might work without this commit for version 4.0.5. If you are not willing to merge it, I would provide it within the Gentoo tooling, since it is distribution specific then.

biodranik (Migrated from github.com) reviewed 2024-04-25 08:49:44 +00:00

CMakeLists.txt

					
				@ -322,1 +322,4 @@

				  find_package(PkgConfig)

				  pkg_check_modules(jansson REQUIRED IMPORTED_TARGET jansson)

				  add_library(jansson::jansson ALIAS PkgConfig::jansson)

biodranik (Migrated from github.com) commented

2024-04-25 08:49:44 +00:00

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.

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.

biodranik (Migrated from github.com) reviewed 2024-04-25 08:51:07 +00:00

CMakeLists.txt

biodranik (Migrated from github.com) commented

2024-04-25 08:51:07 +00:00

This is the hell that I was afraid of when using system packages instead of bundled ones. Upcoming changes to the code will require utfcpp 4+, older version won't work.

gerion0 (Migrated from github.com) reviewed 2024-04-25 08:56:53 +00:00

CMakeLists.txt

					
				@ -322,1 +322,4 @@

				  find_package(PkgConfig)

				  pkg_check_modules(jansson REQUIRED IMPORTED_TARGET jansson)

				  add_library(jansson::jansson ALIAS PkgConfig::jansson)

gerion0 (Migrated from github.com) commented

2024-04-25 08:56:53 +00:00

Do you mean the test pipeline here? Does it build with or without WITH_SYSTEM_PROVIDED_3PARTY?

Do you mean the test pipeline here? Does it build with or without `WITH_SYSTEM_PROVIDED_3PARTY`?

gerion0 (Migrated from github.com) reviewed 2024-04-25 09:02:54 +00:00

CMakeLists.txt

gerion0 (Migrated from github.com) commented

2024-04-25 09:02:54 +00:00

Not really. This is the reason, why distribution specific patches exist. Just make OrganicMaps work for the version you also provide as a 3party module (hopefully the newest one) and the distribution maintainer must take care in case the distribution provides only an older version.

In this case, I was not aware of the differences until writing the above comment and Gentoo clearly provides a too old version, so I would say, this should be a distribution specific patch. However, I cannot test against utf8cpp 4.0.5.

Not really. This is the reason, why distribution specific patches exist. Just make OrganicMaps work for the version you also provide as a 3party module (hopefully the newest one) and the distribution maintainer must take care in case the distribution provides only an older version. In this case, I was not aware of the differences until writing the above comment and Gentoo clearly provides a too old version, so I would say, this should be a distribution specific patch. However, I cannot test against utf8cpp 4.0.5.

Ferenc- (Migrated from github.com) reviewed 2024-04-25 13:40:56 +00:00

CMakeLists.txt

					
				@ -322,1 +322,4 @@

				  find_package(PkgConfig)

				  pkg_check_modules(jansson REQUIRED IMPORTED_TARGET jansson)

				  add_library(jansson::jansson ALIAS PkgConfig::jansson)

Ferenc- (Migrated from github.com) commented

2024-04-25 13:40:56 +00:00

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.

> 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.

Ferenc- (Migrated from github.com) requested changes 2024-04-25 13:50:33 +00:00

Ferenc- (Migrated from github.com) left a comment

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.

> 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.

gerion0 (Migrated from github.com) reviewed 2024-04-25 20:10:36 +00:00

CMakeLists.txt

gerion0 (Migrated from github.com) commented

2024-04-25 20:10:36 +00:00

I have tested compiling against utf8cpp 4.0.5 and it builds find without this patch. Therefore, I already dropped it.