fix linkage with freetype/icu twice on linux #1877

Можно отключить явно только для одного из пары файлов, где есть дубликаты, по аналогии с platform. Тогда почти все файлы кроме исключений будут быстро собираться с unity.

CMakeLists.txt

biodranik (Migrated from github.com) commented

2022-01-18 10:40:59 +00:00

if (LINUX_DETECTED)

```suggestion if (LINUX_DETECTED) ```

CMakeLists.txt

					
				@ -316,0 +314,4 @@

				if (LINUX_DETECTED)

				  find_package(ICU COMPONENTS uc i18n data REQUIRED)

				  find_package(Freetype REQUIRED)

				else()

biodranik (Migrated from github.com) commented

2022-01-18 10:48:49 +00:00

Наш код явно загружает data/icudt70l.dat, хотя динамически прилинкованные пакеты на линуксе подгружают эти данные из shared библиотеки. Видимо, нужно вставить ifdef для линукса, чтобы 1) не включать этот файл в бандл и 2) не загружать его явно в коде, и проверить, что это работает.

indexer/transliteration_loader.cpp
16:  char const kICUDataFile[] = "icudt70l.dat";

qt/CMakeLists.txt
132:  icudt70l.dat

Наш код явно загружает data/icudt70l.dat, хотя динамически прилинкованные пакеты на линуксе подгружают эти данные из shared библиотеки. Видимо, нужно вставить ifdef для линукса, чтобы 1) не включать этот файл в бандл и 2) не загружать его явно в коде, и проверить, что это работает. ``` indexer/transliteration_loader.cpp 16: char const kICUDataFile[] = "icudt70l.dat"; ``` ``` qt/CMakeLists.txt 132: icudt70l.dat ```

coding/CMakeLists.txt

biodranik (Migrated from github.com) commented

2022-01-18 10:42:20 +00:00

Поясни логику PUBLIC/PRIVATE, пожалуйста.

coding/transliteration.hpp

					
				@ -11,1 +10,3 @@

				{

				// From ICU library, either 3party/icu or from the system's package.

				#include <unicode/uversion.h>

biodranik (Migrated from github.com) commented

2022-01-18 10:58:39 +00:00


// From ICU library, either 3party/icu or from the system's package.
#include <unicode/uversion.h>

```suggestion // From ICU library, either 3party/icu or from the system's package. #include <unicode/uversion.h> ```

drape/CMakeLists.txt

biodranik (Migrated from github.com) commented

2022-01-18 10:59:42 +00:00

Зачем в приват? Drape целиком без этих либ ниже не соберётся, если другая либа подключит drape. Поэтому я и добавлял всё в public.

biodranik commented

2022-01-18 11:01:08 +00:00

(Migrated from github.com)

Случайно выбрал approve. Тут надо доработать.

vng (Migrated from github.com) reviewed 2022-01-18 11:52:04 +00:00

3party/icu/CMakeLists.txt

vng (Migrated from github.com) commented

2022-01-18 11:52:04 +00:00

Так ниже есть target_link_libraries(icui18n PRIVATE icuuc)

vng (Migrated from github.com) reviewed 2022-01-18 11:52:31 +00:00

3party/icu/CMakeLists.txt

vng (Migrated from github.com) commented

2022-01-18 11:52:31 +00:00

Оставил как было, в целом коммит не про это

vng (Migrated from github.com) reviewed 2022-01-18 11:55:55 +00:00

coding/CMakeLists.txt

vng (Migrated from github.com) commented

2022-01-18 11:55:55 +00:00

Если кратко, то все что можно - лучше тащить в PRIVATE для более явного чтения зависимостей. Конечно, это зависит от многого ..

target_link_libraries(x PRIVATE y)
target_link_libraries(z x)
z не берет y

target_link_libraries(x PUBLIC y)
target_link_libraries(z x)
В z проваливается y

Если кратко, то все что можно - лучше тащить в PRIVATE для более явного чтения зависимостей. Конечно, это зависит от многого .. target_link_libraries(x PRIVATE y) target_link_libraries(z x) z не берет y target_link_libraries(x PUBLIC y) target_link_libraries(z x) В z проваливается y

vng (Migrated from github.com) reviewed 2022-01-18 11:59:06 +00:00

CMakeLists.txt

					
				@ -316,0 +314,4 @@

				if (LINUX_DETECTED)

				  find_package(ICU COMPONENTS uc i18n data REQUIRED)

				  find_package(Freetype REQUIRED)

				else()

vng (Migrated from github.com) commented

2022-01-18 11:59:06 +00:00

Я думаю это тут не мешает, ну или решать проблемы по мере поступления. Его формат скорее всего унифицирован и старые пакеты вполне могут читать эти свежие правила. ИМХО код и этот dat могут обновляться независимо.

Я думаю это тут не мешает, ну или решать проблемы по мере поступления. Его формат _скорее всего_ унифицирован и старые пакеты вполне могут читать эти свежие правила. ИМХО код и этот dat могут обновляться независимо.

vng (Migrated from github.com) reviewed 2022-01-18 12:00:42 +00:00

drape/CMakeLists.txt

vng (Migrated from github.com) commented

2022-01-18 12:00:42 +00:00

Собирается же. PUBLIC/PRIVATE я описал выше.

vng commented

2022-01-18 12:05:31 +00:00

(Migrated from github.com)

Надо вынести отдельно коммит с рефакторингом ICU, возможно, его придётся откатывать.

Тут не понял. Коммит делает минимальные изменения в ICU, чтобы быть похожим на системный при декларации линковки (ICU::uc, ICU::i18n - так декларируется классический ICU). Что вынести и что придется откатывать?

> Надо вынести отдельно коммит с рефакторингом ICU, возможно, его придётся откатывать. Тут не понял. Коммит делает минимальные изменения в ICU, чтобы быть похожим на системный при декларации линковки (ICU::uc, ICU::i18n - так декларируется классический ICU). Что вынести и что придется откатывать?

biodranik (Migrated from github.com) reviewed 2022-01-18 12:06:50 +00:00

coding/CMakeLists.txt

biodranik (Migrated from github.com) commented

2022-01-18 12:06:49 +00:00

Если кратко, то все что можно - лучше тащить в PRIVATE для более явного чтения зависимостей. Конечно, это зависит от многого ..

Нельзя. Смотри, что происходит:

Для статик либ вообще не нужны зависимости линкера (только инклуды). Они собираются сами по себе. Зависимости нужны на этапе линковки бинарей или динамических либ.
Если линкер умный, и умеет -dead_strip или --gc-sections, то он отбросит неиспользуемый код из статик либы, и не будет требовать для бинаря зависимостей, которые не используются. Но! Как только из бинаря начнут дёргаться другие методы либы, эти зависимости появятся ошибкой компиляции. Что будет непонятным и неприятным сюрпризом для разрабов. Поэтому я вписал зависимости в public.
Если линкер тупой, то он просто ругнётся сразу, что необходимых зависимостей нет.

Поэтому лучше всё оставлять в public. И убирать руками ненужный код и циклические зависимости.

> Если кратко, то все что можно - лучше тащить в PRIVATE для более явного чтения зависимостей. Конечно, это зависит от многого .. Нельзя. Смотри, что происходит: 1. Для статик либ вообще не нужны зависимости линкера (только инклуды). Они собираются сами по себе. Зависимости нужны на этапе линковки бинарей или динамических либ. 2. Если линкер умный, и умеет `-dead_strip` или `--gc-sections`, то он отбросит неиспользуемый код из статик либы, и не будет требовать для бинаря зависимостей, которые не используются. Но! Как только из бинаря начнут дёргаться другие методы либы, эти зависимости появятся ошибкой компиляции. Что будет непонятным и неприятным сюрпризом для разрабов. Поэтому я вписал зависимости в public. 3. Если линкер тупой, то он просто ругнётся сразу, что необходимых зависимостей нет. Поэтому лучше всё оставлять в public. И убирать руками ненужный код и циклические зависимости.

biodranik (Migrated from github.com) reviewed 2022-01-18 12:11:07 +00:00

CMakeLists.txt

					
				@ -316,0 +314,4 @@

				if (LINUX_DETECTED)

				  find_package(ICU COMPONENTS uc i18n data REQUIRED)

				  find_package(Freetype REQUIRED)

				else()

biodranik (Migrated from github.com) commented

2022-01-18 12:11:07 +00:00

Я не видел в описании, что старый код может работать с новыми данными. Да и как он будет работать, если данные уже загружаются в динамической либе? Тут будет ub.
Более того, на системах с разными кодировками и byte order будут печальные сюрпризы.
https://unicode-org.github.io/icu/userguide/icu_data/#sharing-icu-data-between-platforms

Я не видел в описании, что старый код может работать с новыми данными. Да и как он будет работать, если данные уже загружаются в динамической либе? Тут будет ub. Более того, на системах с разными кодировками и byte order будут печальные сюрпризы. https://unicode-org.github.io/icu/userguide/icu_data/#sharing-icu-data-between-platforms

biodranik (Migrated from github.com) reviewed 2022-01-18 12:11:58 +00:00

drape/CMakeLists.txt

biodranik (Migrated from github.com) commented

2022-01-18 12:11:58 +00:00

Собирается потому, что эти приватные либы подтягиваются сейчас из public зависимостей других либ. Если ты везде пропишешь их в private, будут ошибки линковки (очевидно).

biodranik commented

2022-01-18 12:12:59 +00:00

(Migrated from github.com)

Тут не понял. Коммит делает минимальные изменения в ICU, чтобы быть похожим на системный при декларации линковки (ICU::uc, ICU::i18n - так декларируется классический ICU). Что вынести и что придется откатывать?

Разбиение на две либы
Патч/ifdef загрузки .dat файла
Алиазинг и зависимости.

> Тут не понял. Коммит делает минимальные изменения в ICU, чтобы быть похожим на системный при декларации линковки (ICU::uc, ICU::i18n - так декларируется классический ICU). Что вынести и что придется откатывать? 1. Разбиение на две либы 2. Патч/ifdef загрузки .dat файла 3. Алиазинг и зависимости.

vng commented

2022-01-18 12:14:39 +00:00

(Migrated from github.com)

Это все связные изменения, одно без другого не имеет смысла. Как мы возможно откатим 1, оставив 2 и 3?

vng (Migrated from github.com) reviewed 2022-01-18 12:24:28 +00:00

coding/CMakeLists.txt

vng (Migrated from github.com) commented

2022-01-18 12:24:27 +00:00

Так а чему противоречит твои пункты? Можно или нельзя - это всего лишь наши соглашения. Можно писать все в PRIVATE и тогда для каждой либы явно ручками описывать ее зависимости. Так часто делают, чтобы явно видеть в одном файле все зависимости библиотеки. Можно делать PUBLIC и тогда у тебя неявно будут линковаться все либы по иерархии.

Итоговый Makefile в обоих случаях будет один и тот же. cmake уже давно декларативный язык

Так а чему противоречит твои пункты? Можно или нельзя - это всего лишь наши соглашения. Можно писать все в PRIVATE и тогда для каждой либы явно ручками описывать ее зависимости. Так часто делают, чтобы явно видеть в одном файле все зависимости библиотеки. Можно делать PUBLIC и тогда у тебя неявно будут линковаться все либы по иерархии. Итоговый Makefile в обоих случаях будет один и тот же. cmake уже давно декларативный язык

vng (Migrated from github.com) reviewed 2022-01-18 12:25:26 +00:00

coding/CMakeLists.txt

vng (Migrated from github.com) commented

2022-01-18 12:25:26 +00:00

Мне не принципиально, могу убрать PRIVATE.

vng (Migrated from github.com) reviewed 2022-01-18 12:26:59 +00:00

CMakeLists.txt

					
				@ -316,0 +314,4 @@

				if (LINUX_DETECTED)

				  find_package(ICU COMPONENTS uc i18n data REQUIRED)

				  find_package(Freetype REQUIRED)

				else()

vng (Migrated from github.com) commented

2022-01-18 12:26:59 +00:00

Мы же сейчас шарим этот несчастный файл. Мы можем оставить это на потом, когда будет реальная проблема, иначе тут changeset многократно усложнится.

vng (Migrated from github.com) reviewed 2022-01-18 12:27:31 +00:00

drape/CMakeLists.txt

vng (Migrated from github.com) commented

2022-01-18 12:27:31 +00:00

Мне не принципиально, могу убрать PRIVATE.

biodranik (Migrated from github.com) reviewed 2022-01-18 15:24:16 +00:00

coding/CMakeLists.txt

biodranik (Migrated from github.com) commented

2022-01-18 15:24:15 +00:00

Разница в том, что

Глупые линкеры на некоторых платформах будут падать с ошибкой, даже если методы либы не используются.
Когда разработчик решит заюзать новую функцию из уже подлинкованной либы, она может зафейлиться с ошибкой.

Я не вижу смысла в правках private только для нескольких либ.

Если сильно хочется, попробуй сделать private сразу во всех либах. Но тогда придётся явно прописывать полотенца зависимостей в каждой либе. Минус — много ручной работы. Идея с public была этого избежать.
Плюс private — линковка бинарника может быть чуть быстрее, линковаться будут только явно нужные либы.

Разница в том, что 1. Глупые линкеры на некоторых платформах будут падать с ошибкой, даже если методы либы не используются. 2. Когда разработчик решит заюзать новую функцию из уже подлинкованной либы, она может зафейлиться с ошибкой. Я не вижу смысла в правках private только для нескольких либ. Если сильно хочется, попробуй сделать private сразу во всех либах. Но тогда придётся явно прописывать полотенца зависимостей в каждой либе. Минус — много ручной работы. Идея с public была этого избежать. Плюс private — линковка бинарника может быть чуть быстрее, линковаться будут только явно нужные либы.

biodranik (Migrated from github.com) reviewed 2022-01-18 15:51:21 +00:00

CMakeLists.txt

					
				@ -316,0 +314,4 @@

				if (LINUX_DETECTED)

				  find_package(ICU COMPONENTS uc i18n data REQUIRED)

				  find_package(Freetype REQUIRED)

				else()

biodranik (Migrated from github.com) commented

2022-01-18 15:51:21 +00:00

А ты запускал и проверял транслитерацию?

biodranik (Migrated from github.com) reviewed 2022-01-18 15:52:09 +00:00

CMakeLists.txt

					
				@ -316,0 +314,4 @@

				if (LINUX_DETECTED)

				  find_package(ICU COMPONENTS uc i18n data REQUIRED)

				  find_package(Freetype REQUIRED)

				else()

biodranik (Migrated from github.com) commented

2022-01-18 15:52:09 +00:00

Это очевидный косяк и ub, зачем потом тратить время на разгребание ошибок от энтузиастов собирать и ранить код на линуксах? И они своё время будут тратить.

vng (Migrated from github.com) reviewed 2022-01-18 15:59:08 +00:00

CMakeLists.txt

					
				@ -316,0 +314,4 @@

				if (LINUX_DETECTED)

				  find_package(ICU COMPONENTS uc i18n data REQUIRED)

				  find_package(Freetype REQUIRED)

				else()

vng (Migrated from github.com) commented

2022-01-18 15:59:08 +00:00

Правка касается только Linux, все остальное остается как и было. Транслитерация у меня на Маке работает
Откуда ты взял UB? В нашем коде инициализации явно вызывается u_setDataDirectory на папку с dat файлом. Без этого вызова ICU сам ничего (скорее всего?) грузить не будет. Можно попросить чела проверить как она работает на линуксе, я думаю там все ок.

- Правка касается только Linux, все остальное остается как и было. Транслитерация у меня на Маке работает - Откуда ты взял UB? В нашем коде инициализации явно вызывается u_setDataDirectory на папку с dat файлом. Без этого вызова ICU сам ничего (скорее всего?) грузить не будет. Можно попросить чела проверить как она работает на линуксе, я думаю там все ок.

biodranik (Migrated from github.com) reviewed 2022-01-18 16:27:39 +00:00

CMakeLists.txt

					
				@ -316,0 +314,4 @@

				if (LINUX_DETECTED)

				  find_package(ICU COMPONENTS uc i18n data REQUIRED)

				  find_package(Freetype REQUIRED)

				else()

biodranik (Migrated from github.com) commented

2022-01-18 16:27:38 +00:00

Расскажи, как проверить, давай глянем сначала. UB будет из-за попытки загрузить новые ресурсы старой версией. Чем старее версия, как я понимаю, тем больше шансов UB. А на многих линуксах эта либа старая, в Ubuntu20 например версия 66.

vng (Migrated from github.com) reviewed 2022-01-18 16:34:47 +00:00

CMakeLists.txt

					
				@ -316,0 +314,4 @@

				if (LINUX_DETECTED)

				  find_package(ICU COMPONENTS uc i18n data REQUIRED)

				  find_package(Freetype REQUIRED)

				else()

vng (Migrated from github.com) commented

2022-01-18 16:34:47 +00:00

На CI успешно выполнились Transliteration_CompareSamples

vng (Migrated from github.com) reviewed 2022-01-18 16:50:18 +00:00

CMakeLists.txt

					
				@ -316,0 +314,4 @@

				if (LINUX_DETECTED)

				  find_package(ICU COMPONENTS uc i18n data REQUIRED)

				  find_package(Freetype REQUIRED)

				else()

vng (Migrated from github.com) commented

2022-01-18 16:50:18 +00:00

Ну и вообще этот транслитератор начинает вызываться сразу при инициализации поиска на старте (мутит японские категории) и отрисовке. Так что если прога запускается - значит все ок.

biodranik (Migrated from github.com) reviewed 2022-01-19 09:22:54 +00:00

CMakeLists.txt

					
				@ -316,0 +314,4 @@

				if (LINUX_DETECTED)

				  find_package(ICU COMPONENTS uc i18n data REQUIRED)

				  find_package(Freetype REQUIRED)

				else()

biodranik (Migrated from github.com) commented

2022-01-19 09:22:54 +00:00

Будем ждать, пока случится не ок?

vng (Migrated from github.com) reviewed 2022-01-19 13:39:13 +00:00

CMakeLists.txt

					
				@ -316,0 +314,4 @@

				if (LINUX_DETECTED)

				  find_package(ICU COMPONENTS uc i18n data REQUIRED)

				  find_package(Freetype REQUIRED)

				else()

vng (Migrated from github.com) commented

2022-01-19 13:39:13 +00:00

Я не вижу зазорным тут решать проблемы по мере появления, сейчас линукс вообще не работает. Больше у меня аргументов нет, я примеры работы показал :) Наверное, можно изгольнуться и:

искать при сборке папку icu (что-то типа ICU_DIR)
искать там нужный dat файл
бандлить его в qt/CMakeList

Если честно, у меня на это нет фуреку уже. Если ты видишь как это красиво сделать и протестить - давай.

Я не вижу зазорным тут решать проблемы по мере появления, сейчас линукс вообще не работает. Больше у меня аргументов нет, я примеры работы показал :) Наверное, можно изгольнуться и: - искать при сборке папку icu (что-то типа ICU_DIR) - искать там нужный dat файл - бандлить его в qt/CMakeList Если честно, у меня на это нет фуреку уже. Если ты видишь как это красиво сделать и протестить - давай.

biodranik (Migrated from github.com) reviewed 2022-01-19 14:09:24 +00:00

CMakeLists.txt

					
				@ -316,0 +314,4 @@

				if (LINUX_DETECTED)

				  find_package(ICU COMPONENTS uc i18n data REQUIRED)

				  find_package(Freetype REQUIRED)

				else()

biodranik (Migrated from github.com) commented

2022-01-19 14:09:24 +00:00

Судя по тому, что я видел, надо по ifdef OMIM_OS_LINUX просто не пытаться загружать ничего из нашего dat файла (и написать коммент, почему, что icu data is loaded automatically from the shared library on Linux.

If the ICU data directory string is empty, then ICU will not attempt to load data from the file system. It is then only possible to load data from the linked-in shared library or via udata_setCommonData() and udata_setAppData(). This is inflexible but provides the highest performance.

Судя по тому, что я видел, надо по ifdef OMIM_OS_LINUX просто не пытаться загружать ничего из нашего dat файла (и написать коммент, почему, что icu data is loaded automatically from the shared library on Linux. > If the ICU data directory string is empty, then ICU will not attempt to load data from the file system. It is then only possible to load data from the linked-in shared library or via udata_setCommonData() and udata_setAppData(). This is inflexible but provides the highest performance.

biodranik (Migrated from github.com) reviewed 2022-01-19 14:09:57 +00:00

CMakeLists.txt

					
				@ -316,0 +314,4 @@

				if (LINUX_DETECTED)

				  find_package(ICU COMPONENTS uc i18n data REQUIRED)

				  find_package(Freetype REQUIRED)

				else()

biodranik (Migrated from github.com) commented

2022-01-19 14:09:57 +00:00

И под Linux не добавлять .dat в ресурсы и не шиппить, конечно же. Сэкономим место.

vng (Migrated from github.com) reviewed 2022-01-19 15:10:46 +00:00

CMakeLists.txt

					
				@ -316,0 +314,4 @@

				if (LINUX_DETECTED)

				  find_package(ICU COMPONENTS uc i18n data REQUIRED)

				  find_package(Freetype REQUIRED)

				else()

vng (Migrated from github.com) commented

2022-01-19 15:10:46 +00:00

Я сейчас попробую, но думаю транслитерация просто не будет работать.

vng (Migrated from github.com) reviewed 2022-01-19 16:02:13 +00:00

CMakeLists.txt

					
				@ -316,0 +314,4 @@

				if (LINUX_DETECTED)

				  find_package(ICU COMPONENTS uc i18n data REQUIRED)

				  find_package(Freetype REQUIRED)

				else()

vng (Migrated from github.com) commented

2022-01-19 16:02:13 +00:00

Хм, все работает, по крайней мере тесты :)

biodranik (Migrated from github.com) approved these changes 2022-01-19 17:12:01 +00:00

biodranik (Migrated from github.com) left a comment

Почти идеально. Пройдись по оставшимся комментам, пожалуйста, и можно мержить.

P.S. Саджесты можно мержить прямо с интерфейса гитхаба.

Почти идеально. Пройдись по оставшимся комментам, пожалуйста, и можно мержить. P.S. Саджесты можно мержить прямо с интерфейса гитхаба.

3party/icu/CMakeLists.txt

					
				@ -173,0 +180,4 @@

				target_compile_definitions(icuuc PUBLIC UCONFIG_USE_LOCAL)

				set_target_properties(icuuc PROPERTIES UNITY_BUILD OFF)

biodranik (Migrated from github.com) commented

2022-01-19 16:59:33 +00:00

Лучше если эта команда будет перед add_library выше, создающей alias. Из документации cmake не ясно, как оно работает, но сказано, что alias таргеты immutable. Возможно, последующие изменения оригинального таргета не подхватятся. Поэтому сначала лучше настраивать либу, и в самом конце сделать alias.

3party/icu/CMakeLists.txt

biodranik (Migrated from github.com) commented

2022-01-19 17:01:00 +00:00

Под шумок можно и ускорить, раз уже ковыряешь юнити в этой строке )

coding/transliteration.cpp

					
				@ -58,8 +60,12 @@ void Transliteration::Init(std::string const & icuDataDir)

				    return;

biodranik (Migrated from github.com) commented

2022-01-19 17:02:15 +00:00

  // an ICU data file. On Linux, data file is loaded automatically from the shared library.

```suggestion // an ICU data file. On Linux, data file is loaded automatically from the shared library. ```

coding/transliteration.cpp

biodranik (Migrated from github.com) commented

2022-01-19 17:03:48 +00:00

У функции будет warning unused variable на линуксе. Надо бы исправить.

qt/CMakeLists.txt

					
				@ -148,3 +152,3 @@

				  )

				elseif(PLATFORM_LINUX)

				elseif (PLATFORM_LINUX)

				  execute_process(

biodranik (Migrated from github.com) commented

2022-01-19 17:11:14 +00:00

  # On Linux, ICU data is loaded from the shared library.
  copy_resources(icudt70l.dat)

```suggestion # On Linux, ICU data is loaded from the shared library. copy_resources(icudt70l.dat) ```

biodranik (Migrated from github.com) reviewed 2022-01-19 17:12:26 +00:00

CMakeLists.txt

					
				@ -316,0 +314,4 @@

				if (LINUX_DETECTED)

				  find_package(ICU COMPONENTS uc i18n data REQUIRED)

				  find_package(Freetype REQUIRED)

				else()