Devendor some thridparty deps #7836

Osyotr · 2024-04-06T13:49:16Z

No description provided.

Signed-off-by: Osyotr <Osyotr@users.noreply.github.com>

Drive-by: use add_compile_options instead of add_definitions to set -fno-omit-frame-pointer Signed-off-by: Osyotr <Osyotr@users.noreply.github.com>

GLOBAL flag for find_package has been added in CMake 3.24 Signed-off-by: Osyotr <Osyotr@users.noreply.github.com>

Signed-off-by: Osyotr <Osyotr@users.noreply.github.com>

…ED_3PARTY is set Signed-off-by: Osyotr <Osyotr@users.noreply.github.com>

Signed-off-by: Osyotr <Osyotr@users.noreply.github.com>

biodranik

Thanks!

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

biodranik · 2024-04-06T13:53:41Z

cppjansson/CMakeLists.txt

@@ -7,5 +7,7 @@ set(SRC
 )
 omim_add_library(${PROJECT_NAME} ${SRC})
 target_link_libraries(${PROJECT_NAME}
-  jansson
+  PUBLIC


Won't private be enough?

Should base also be public?

Both are used in public interface.

biodranik · 2024-04-06T13:54:11Z

ge0/CMakeLists.txt

@@ -10,5 +10,13 @@ set(SRC
 )

 omim_add_library(${PROJECT_NAME} ${SRC})
+target_link_libraries(${PROJECT_NAME}
+  PUBLIC
+    coding


Why public? How does it work? )

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

transit/CMakeLists.txt

biodranik · 2024-04-06T13:56:07Z

CMakeLists.txt

+if (MSVC)
+  add_compile_options(/utf-8)
+  add_link_options(/INCREMENTAL:NO)
+  add_link_options(/DEBUG:FULL)


Should it be set only for Debug and RelWithDebInfo configs?

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

Yes, when distributing a binary.

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.

Does it influence the build speed?

My unscientific measurements (full build with tests, Release):
With flag: 8:50
Without flag: 8:30
Anyway, I've already removed it.

biodranik · 2024-04-06T13:57:35Z

CMakeLists.txt

@@ -319,6 +319,11 @@ if (PLATFORM_LINUX)
  find_package(harfbuzz REQUIRED)
 endif()

+if (WITH_SYSTEM_PROVIDED_3PARTY)


Can this block be moved to 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.

biodranik · 2024-04-06T14:00:26Z

3party/CMakeLists.txt

+
+  add_library(utf8cpp INTERFACE)
+  add_library(utf8cpp::utf8cpp ALIAS utf8cpp)
+  target_include_directories(utf8cpp INTERFACE "${OMIM_ROOT}/3party/utfcpp/source")


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

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.

biodranik · 2024-04-06T14:01:46Z

CMakeLists.txt

@@ -322,6 +322,7 @@ endif()
 if (WITH_SYSTEM_PROVIDED_3PARTY)
  set(GFLAGS_USE_TARGET_NAMESPACE ON)
  find_package(gflags REQUIRED)
+  find_package(utf8cpp REQUIRED)


Is it really packaged on different systems?..

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

biodranik · 2024-04-06T14:04:14Z

3party/CMakeLists.txt

@@ -22,6 +22,7 @@ if (NOT WITH_SYSTEM_PROVIDED_3PARTY)
  set(EXPAT_DTD OFF)
  set(EXPAT_NS ON)
  add_subdirectory(expat/expat)
+  add_library(expat::expat ALIAS expat)


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

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

biodranik · 2024-04-06T14:04:39Z

CMakeLists.txt

@@ -324,6 +324,7 @@ if (WITH_SYSTEM_PROVIDED_3PARTY)
  find_package(gflags REQUIRED)

  find_package(expat CONFIG REQUIRED)
+  find_package(jansson CONFIG REQUIRED)


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

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

biodranik · 2024-04-06T14:06:01Z

CMakeLists.txt

@@ -320,6 +320,7 @@ if (WITH_SYSTEM_PROVIDED_3PARTY)

  find_package(expat CONFIG REQUIRED)
  find_package(jansson CONFIG REQUIRED)
+  find_package(pugixml REQUIRED)


Why doesn't it compile from sources?

transit/CMakeLists.txt

Signed-off-by: Osyotr <Osyotr@users.noreply.github.com>

Osyotr · 2024-04-06T15:01:23Z

Please use squash when merging.

Signed-off-by: Osyotr <Osyotr@users.noreply.github.com>

biodranik

Ok, let's move on with this to be able to run Windows soon. Thank you very much!

Ferenc- · 2024-04-07T21:52:42Z

Thanks!

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

So far, I haven't found any issues.

Osyotr added 11 commits April 6, 2024 16:50

Link with Qt6::Network on windows

9f0bf65

Signed-off-by: Osyotr <Osyotr@users.noreply.github.com>

Make find_program(BASH) REQUIRED to prevent build-time errors

f7850f1

Signed-off-by: Osyotr <Osyotr@users.noreply.github.com>

Link more targets explicitly to pickup transitive dependencies

ae04ac3

Signed-off-by: Osyotr <Osyotr@users.noreply.github.com>

Don't add unsupported flags on MSVC

d825eb4

Drive-by: use add_compile_options instead of add_definitions to set -fno-omit-frame-pointer Signed-off-by: Osyotr <Osyotr@users.noreply.github.com>

Move find_package(gflags) out of 3party

bef0ae8

GLOBAL flag for find_package has been added in CMake 3.24 Signed-off-by: Osyotr <Osyotr@users.noreply.github.com>

Don't hardcode paths to 3party/utfcpp

13a8601

Signed-off-by: Osyotr <Osyotr@users.noreply.github.com>

Use system expat explicitly if needed

b5a7352

Signed-off-by: Osyotr <Osyotr@users.noreply.github.com>

Use system jansson explicitly if needed

90241c0

Signed-off-by: Osyotr <Osyotr@users.noreply.github.com>

Use find_package(ZLIB) to find zlib

d49d554

Signed-off-by: Osyotr <Osyotr@users.noreply.github.com>

Don't use vendored Freetype, ICU and HarfBuzz when WITH_SYSTEM_PROVID…

616cde3

…ED_3PARTY is set Signed-off-by: Osyotr <Osyotr@users.noreply.github.com>

Find pugixml explicitly

c4dccc6

Signed-off-by: Osyotr <Osyotr@users.noreply.github.com>

Osyotr force-pushed the cmake-fixes branch from 664d5db to c4dccc6 Compare April 6, 2024 13:52

biodranik reviewed Apr 6, 2024

View reviewed changes

Osyotr commented Apr 6, 2024

View reviewed changes

transit/CMakeLists.txt Outdated Show resolved Hide resolved

Osyotr added 2 commits April 6, 2024 17:06

Fix typo in target name

3fc33f3

Signed-off-by: Osyotr <Osyotr@users.noreply.github.com>

Update utfcpp include path

21b6b7a

Signed-off-by: Osyotr <Osyotr@users.noreply.github.com>

Let CMake handle /DEBUG flag for MSVC

207e9b1

Signed-off-by: Osyotr <Osyotr@users.noreply.github.com>

biodranik approved these changes Apr 6, 2024

View reviewed changes

biodranik merged commit 76dd632 into organicmaps:master Apr 6, 2024
13 of 15 checks passed

Osyotr deleted the cmake-fixes branch April 6, 2024 23:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Devendor some thridparty deps #7836

Devendor some thridparty deps #7836

Osyotr commented Apr 6, 2024

biodranik left a comment

biodranik Apr 6, 2024

Osyotr Apr 6, 2024

biodranik Apr 6, 2024

Osyotr Apr 6, 2024

biodranik Apr 6, 2024

Osyotr Apr 6, 2024

biodranik Apr 6, 2024

Osyotr Apr 6, 2024

biodranik Apr 6, 2024

Osyotr Apr 6, 2024

biodranik Apr 6, 2024

Osyotr Apr 6, 2024

biodranik Apr 6, 2024

Osyotr Apr 6, 2024

biodranik Apr 6, 2024

Osyotr Apr 6, 2024

biodranik Apr 6, 2024

Osyotr Apr 6, 2024

biodranik Apr 6, 2024

Osyotr Apr 6, 2024

biodranik Apr 6, 2024

Osyotr Apr 6, 2024

Osyotr commented Apr 6, 2024

biodranik left a comment

Ferenc- commented Apr 7, 2024

Devendor some thridparty deps #7836

Devendor some thridparty deps #7836

Conversation

Osyotr commented Apr 6, 2024

biodranik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Osyotr commented Apr 6, 2024

biodranik left a comment

Choose a reason for hiding this comment

Ferenc- commented Apr 7, 2024