Devendor some thridparty deps #7836

2024-04-06T13:49:16Z

Osyotr commented

2024-04-06 13:49:16 +00:00

(Migrated from github.com)

No description provided.

👍 1

biodranik (Migrated from github.com) reviewed 2024-04-06 14:06:22 +00:00

biodranik (Migrated from github.com) left a comment

Thanks!

@Ferenc- can you please help with testing these changes?

Thanks! @Ferenc- can you please help with testing these changes?

3party/CMakeLists.txt

					
				@ -36,3 +34,4 @@

				  target_include_directories(jansson INTERFACE "${PROJECT_BINARY_DIR}/3party/jansson/jansson/include")

				  add_library(jansson::jansson ALIAS jansson)

				  # Add gflags library.

biodranik (Migrated from github.com) commented

2024-04-06 14:04:14 +00:00

This change would also be great to test and merge separately.

3party/CMakeLists.txt

biodranik (Migrated from github.com) commented

2024-04-06 14:00:26 +00:00

Why this change was required? Can it be extracted into a separate PR to test and polish properly?

CMakeLists.txt

biodranik (Migrated from github.com) commented

2024-04-06 13:56:07 +00:00

Should it be set only for Debug and RelWithDebInfo configs?

CMakeLists.txt

biodranik (Migrated from github.com) commented

2024-04-06 13:57:35 +00:00

Can this block be moved to 3party?

CMakeLists.txt

biodranik (Migrated from github.com) commented

2024-04-06 14:06:01 +00:00

Why doesn't it compile from sources?

CMakeLists.txt

biodranik (Migrated from github.com) commented

2024-04-06 14:01:46 +00:00

Is it really packaged on different systems?..

biodranik (Migrated from github.com) commented

2024-04-06 14:04:39 +00:00

What is wrong with building these packages from sources?..

cppjansson/CMakeLists.txt

					
				@ -9,2 +9,3 @@

				target_link_libraries(${PROJECT_NAME}

				  jansson

				  PUBLIC

				    base

biodranik (Migrated from github.com) commented

2024-04-06 13:53:41 +00:00

Won't private be enough?
Should base also be public?

1. Won't private be enough? 2. Should base also be public?

ge0/CMakeLists.txt

					
				@ -12,1 +12,4 @@

				omim_add_library(${PROJECT_NAME} ${SRC})

				target_link_libraries(${PROJECT_NAME}

				  PUBLIC

				    coding

biodranik (Migrated from github.com) commented

2024-04-06 13:54:11 +00:00

Why public? How does it work? )

transit/CMakeLists.txt

biodranik (Migrated from github.com) commented

2024-04-06 13:54:25 +00:00

coding

Osyotr (Migrated from github.com) reviewed 2024-04-06 14:06:33 +00:00

transit/CMakeLists.txt

Osyotr (Migrated from github.com) commented

2024-04-06 14:06:32 +00:00

    coding

```suggestion coding ```

Osyotr (Migrated from github.com) reviewed 2024-04-06 14:10:38 +00:00

cppjansson/CMakeLists.txt

					
				@ -9,2 +9,3 @@

				target_link_libraries(${PROJECT_NAME}

				  jansson

				  PUBLIC

				    base

Osyotr (Migrated from github.com) commented

2024-04-06 14:10:37 +00:00

Both are used in public interface.

Osyotr (Migrated from github.com) reviewed 2024-04-06 14:12:56 +00:00

ge0/CMakeLists.txt

					
				@ -12,1 +12,4 @@

				omim_add_library(${PROJECT_NAME} ${SRC})

				target_link_libraries(${PROJECT_NAME}

				  PUBLIC

				    coding

Osyotr (Migrated from github.com) commented

2024-04-06 14:12:56 +00:00

My rule is "private by default unless it's used in a public header".

Osyotr (Migrated from github.com) reviewed 2024-04-06 14:16:45 +00:00

CMakeLists.txt

Osyotr (Migrated from github.com) commented

2024-04-06 14:16:45 +00:00

Yes, but you need CMake 3.24+ for that
https://cmake.org/cmake/help/latest/variable/CMAKE_FIND_PACKAGE_TARGETS_GLOBAL.html#variable:CMAKE_FIND_PACKAGE_TARGETS_GLOBAL
Though IMHO it really shouldn't be in 3party.

Yes, but you need CMake 3.24+ for that https://cmake.org/cmake/help/latest/variable/CMAKE_FIND_PACKAGE_TARGETS_GLOBAL.html#variable:CMAKE_FIND_PACKAGE_TARGETS_GLOBAL Though IMHO it really shouldn't be in 3party.

Osyotr (Migrated from github.com) reviewed 2024-04-06 14:20:47 +00:00

3party/CMakeLists.txt

Osyotr (Migrated from github.com) commented

2024-04-06 14:20:47 +00:00

Remove hardcoded "${OMIM_ROOT}/3party/utf8cpp/include" from include path
Unify usage for system-provided and vendored copy of utfcpp.

Changes in this PR somewhat depend on each other, I really wouldn't like to split them.

1) Remove hardcoded `"${OMIM_ROOT}/3party/utf8cpp/include"` from include path 2) Unify usage for system-provided and vendored copy of utfcpp. Changes in this PR somewhat depend on each other, I really wouldn't like to split them.

Osyotr (Migrated from github.com) reviewed 2024-04-06 14:23:37 +00:00

3party/CMakeLists.txt

					
				@ -36,3 +34,4 @@

				  target_include_directories(jansson INTERFACE "${PROJECT_BINARY_DIR}/3party/jansson/jansson/include")

				  add_library(jansson::jansson ALIAS jansson)

				  # Add gflags library.

Osyotr (Migrated from github.com) commented

2024-04-06 14:23:37 +00:00

Ditto wouldn't want to dive into rebase-cherry-pick-hell

Osyotr (Migrated from github.com) reviewed 2024-04-06 14:26:41 +00:00

CMakeLists.txt

Osyotr (Migrated from github.com) commented

2024-04-06 14:26:41 +00:00

Those who set WITH_SYSTEM_PROVIDED_3PARTY should make sure they have it.
https://repology.org/project/utfcpp/versions

Those who set `WITH_SYSTEM_PROVIDED_3PARTY` should make sure they have it. https://repology.org/project/utfcpp/versions

Osyotr (Migrated from github.com) reviewed 2024-04-06 14:36:03 +00:00

CMakeLists.txt

Osyotr (Migrated from github.com) commented

2024-04-06 14:36:02 +00:00

Nothing in particular, though mixing system and vendored deps is considered bad.
Note that add_subdirectory(jansson) is not called on when WITH_SYSTEM_PROVIDED_3PARTY. CMake implicitly uses -ljansson if it couldn't find target with name jansson. It happens to work on Linux because everything is in include/link directories. This is why namespaced targets are preferred (they don't allow this implicit behavior).

Nothing in particular, though mixing system and vendored deps is considered bad. Note that `add_subdirectory(jansson)` is not called on when `WITH_SYSTEM_PROVIDED_3PARTY`. CMake implicitly uses `-ljansson` if it couldn't find target with name `jansson`. It happens to work on Linux because everything is in include/link directories. This is why namespaced targets are preferred (they don't allow this implicit behavior).

Osyotr (Migrated from github.com) reviewed 2024-04-06 14:36:09 +00:00

CMakeLists.txt

Osyotr (Migrated from github.com) commented

2024-04-06 14:36:09 +00:00

ditto

Osyotr (Migrated from github.com) reviewed 2024-04-06 15:29:54 +00:00

CMakeLists.txt

Osyotr (Migrated from github.com) commented

2024-04-06 15:29:53 +00:00

Does it even make sense to have release build without debug symbols?

biodranik (Migrated from github.com) reviewed 2024-04-06 15:50:45 +00:00

CMakeLists.txt

biodranik (Migrated from github.com) commented

2024-04-06 15:50:45 +00:00

Yes, when distributing a binary.

Osyotr (Migrated from github.com) reviewed 2024-04-06 17:13:12 +00:00

CMakeLists.txt

Osyotr (Migrated from github.com) commented

2024-04-06 17:13:11 +00:00

Yes, when distributing a binary.

Windows binaries don't embed any debug information. I.e. without this flag you get .exe, with this flag you get .exe + .pdb.

> Yes, when distributing a binary. Windows binaries don't embed any debug information. I.e. without this flag you get `.exe`, with this flag you get `.exe` + `.pdb`.