[3party] Update protobuf to v4.23.4 #5557

AndrewShkrob · 2023-07-16T16:53:21Z

AndrewShkrob commented

2023-07-16 16:53:21 +00:00

I'm not sure if I should also run generate_symbols.sh. It works fine even without it.
https://github.com/organicmaps/protobuf.git could be removed after merge.
Kothic PR.

Todo:

Update xcode project

Closes #3932 I'm not sure if I should also run `generate_symbols.sh`. It works fine even without it. `https://github.com/organicmaps/protobuf.git` could be removed after merge. Kothic [PR](https://github.com/organicmaps/kothic/pull/15). Todo: - [x] Update xcode project

pastk requested changes 2023-07-19 11:54:17 +00:00

pastk left a comment

Looks good aside from the added/removed zeros issue.

pastk reviewed 2023-07-19 17:15:45 +00:00

docs/STYLES.md

					
				@ -17,13 +17,10 @@ and ["icons wanted" issues](https://github.com/organicmaps/organicmaps/issues?q=

				To work with styles first [clone the OM repository](INSTALL.md#getting-sources).

pastk commented

2023-07-19 17:15:45 +00:00

also makes sense to mention the python >=3.8 requirement here

AndrewShkrob reviewed 2023-07-19 20:20:11 +00:00

rtsisyk approved these changes 2023-08-17 06:01:15 +00:00

pastk approved these changes 2023-08-17 17:48:38 +00:00

pastk left a comment

Let's merge https://github.com/organicmaps/kothic/pull/15 first.

vng (Migrated from github.com) approved these changes 2023-08-21 17:33:41 +00:00

biodranik (Migrated from github.com) requested changes 2023-08-21 19:01:06 +00:00

biodranik (Migrated from github.com) left a comment

Does designer work?

3party/protobuf/CMakeLists.txt

					
				@ -43,0 +30,4 @@

				set_target_properties(${protobuf_LIB_NAME} PROPERTIES UNITY_BUILD OFF)

				set(ABSL_TARGETS

				  absl_strings

				  absl_time

biodranik (Migrated from github.com) commented

2023-08-21 18:50:49 +00:00

Should we update this file manually next time? What is the process? Does it make sense to document it?

3party/protobuf/CMakeLists.txt

biodranik (Migrated from github.com) commented

2023-08-21 18:51:25 +00:00

Why? Are all files failing? Maybe disabling unity only for a few files will help?

3party/protobuf/config.h

biodranik (Migrated from github.com) commented

2023-08-21 18:52:01 +00:00

How is protobuf configured now for iOS? Before, this file was used.

indexer/scales_patch.hpp

					
				@ -17,3 +17,3 @@

				{

				  std::min(s + kPatchScaleShift, static_cast<uint32_t>(GetUpperStyleScale()));

				  return std::min(s + kPatchScaleShift, static_cast<uint32_t>(GetUpperStyleScale()));

				}

biodranik (Migrated from github.com) commented

2023-08-21 19:00:28 +00:00

Should this fix go to the master separately?

xcode/drape_frontend/drape_frontend.xcodeproj/project.pbxproj

biodranik (Migrated from github.com) commented

2023-08-21 18:52:39 +00:00

Why is it needed in a protobuf PR?

biodranik (Migrated from github.com) commented

2023-08-21 18:52:45 +00:00

ditto

xcode/drape_frontend/drape_frontend.xcodeproj/project.pbxproj

					
				@ -1007,6 +1011,10 @@

							isa = XCBuildConfiguration;

							buildSettings = {

								EXECUTABLE_PREFIX = lib;

biodranik (Migrated from github.com) commented

2023-08-21 18:59:25 +00:00

ditto

xcode/map/map.xcodeproj/project.pbxproj

biodranik (Migrated from github.com) commented

2023-08-21 18:59:18 +00:00

ditto

xcode/map/map.xcodeproj/project.pbxproj

					
				@ -798,1 +802,4 @@

								EXECUTABLE_PREFIX = lib;

								HEADER_SEARCH_PATHS = (

									"$(inherited)",

									"${PROTOBUF_PATHS}",

biodranik (Migrated from github.com) commented

2023-08-21 18:59:35 +00:00

ditto

AndrewShkrob reviewed 2023-08-22 16:40:27 +00:00

indexer/scales_patch.hpp

					
				@ -17,3 +17,3 @@

				{

				  std::min(s + kPatchScaleShift, static_cast<uint32_t>(GetUpperStyleScale()));

				  return std::min(s + kPatchScaleShift, static_cast<uint32_t>(GetUpperStyleScale()));

				}

AndrewShkrob commented

2023-08-22 16:40:27 +00:00

No, it's a fix for designer tool

AndrewShkrob reviewed 2023-08-22 16:49:56 +00:00

3party/protobuf/CMakeLists.txt

					
				@ -43,0 +30,4 @@

				set_target_properties(${protobuf_LIB_NAME} PROPERTIES UNITY_BUILD OFF)

				set(ABSL_TARGETS

				  absl_strings

				  absl_time

AndrewShkrob commented

2023-08-22 16:49:55 +00:00

Documenting this can be challenging, as the documentation might become outdated with new releases. Normally, updating the version number should suffice. However, if there are alterations like the addition of a new library or changes in configuration, it will be necessary to carefully examine the modifications and adjust our configuration to match them.

AndrewShkrob reviewed 2023-08-22 16:57:44 +00:00

3party/protobuf/config.h

AndrewShkrob commented

2023-08-22 16:57:44 +00:00

And now it's not used 😄

There is no configure.ac in protobuf right now. So, I guess, it automatically configures itself during the compile-time.

And now it's not used 😄 There is no `configure.ac` in protobuf right now. So, I guess, it automatically configures itself during the compile-time.

biodranik (Migrated from github.com) reviewed 2023-08-24 21:47:14 +00:00

indexer/scales_patch.hpp

					
				@ -17,3 +17,3 @@

				{

				  std::min(s + kPatchScaleShift, static_cast<uint32_t>(GetUpperStyleScale()));

				  return std::min(s + kPatchScaleShift, static_cast<uint32_t>(GetUpperStyleScale()));

				}

biodranik (Migrated from github.com) commented

2023-08-24 21:47:13 +00:00

It's used in the search/generator and can be merged to master independently. Isn't it UB? @vng

AndrewShkrob reviewed 2023-08-24 21:59:38 +00:00

indexer/scales_patch.hpp

					
				@ -17,3 +17,3 @@

				{

				  std::min(s + kPatchScaleShift, static_cast<uint32_t>(GetUpperStyleScale()));

				  return std::min(s + kPatchScaleShift, static_cast<uint32_t>(GetUpperStyleScale()));

				}

AndrewShkrob commented

2023-08-24 21:59:38 +00:00

It's covered with #ifdef BUILD_DESIGNER

It's covered with `#ifdef BUILD_DESIGNER`

biodranik (Migrated from github.com) reviewed 2023-08-24 22:30:34 +00:00

biodranik (Migrated from github.com) left a comment

Previous XCode project was crafted manually by analyzing the compiler output and project defines. As CMake is used now to build protobuf, it may make sense to check which compiler flags/settings are provided by building it for iOS and add/fix these flags in the corresponding Xcode project.
Protobuf has many dependencies. Are they really needed in our case?
Was this PR tested on iOS, Android and Mac/Linux? Does it work everywhere?
Why version 4 was chosen instead of the latest 3?
Why it was decided to upgrade protobuf? What are the benefits of the upgrade? The mentioned issue references the ability to use system Linux libraries instead of embedding protobuf in the binary. That itself is not a significant issue to force the upgrade that potentially may break something on all platforms, especially considering that many other 3p libraries are still not loaded from the Linux system.

1. Previous XCode project was crafted manually by analyzing the compiler output and project defines. As CMake is used now to build protobuf, it may make sense to check which compiler flags/settings are provided by [building it for iOS](https://cmake.org/cmake/help/latest/manual/cmake-toolchains.7.html#cross-compiling-for-ios-tvos-or-watchos) and add/fix these flags in the corresponding Xcode project. 2. Protobuf has many dependencies. Are they really needed in our case? 3. Was this PR tested on iOS, Android and Mac/Linux? Does it work everywhere? 4. Why version 4 was chosen instead of the latest 3? 5. Why it was decided to upgrade protobuf? What are the benefits of the upgrade? The mentioned issue references the ability to use system Linux libraries instead of embedding protobuf in the binary. That itself is not a significant issue to force the upgrade that potentially may break something on all platforms, especially considering that many other 3p libraries are still not loaded from the Linux system.

3party/protobuf/CMakeLists.txt

					
				@ -43,0 +30,4 @@

				set_target_properties(${protobuf_LIB_NAME} PROPERTIES UNITY_BUILD OFF)

				set(ABSL_TARGETS

				  absl_strings

				  absl_time

biodranik (Migrated from github.com) commented

2023-08-24 22:06:13 +00:00

Am I correctly understanding that you have created this file manually to wrap protobuf sources? Why the bundled CMakeLists.txt can't be customized and used via add_subdirectory?

Am I correctly understanding that you have created this file manually to wrap protobuf sources? Why the bundled [CMakeLists.txt](https://github.com/protocolbuffers/protobuf/blob/main/CMakeLists.txt) can't be customized and used via add_subdirectory?

qt/build_style/build_statistics.cpp

					
				@ -26,4 +25,1 @@

				  QProcessEnvironment env{QProcessEnvironment::systemEnvironment()};

				  env.insert("PROTOBUF_EGG_PATH", GetProtobufEggPath());

				  // Run the script.

biodranik (Migrated from github.com) commented

2023-08-24 22:11:09 +00:00

Why EGG was used before?

xcode/common.xcconfig

					
				@ -3,6 +3,7 @@ OMIM_ROOT = $(PROJECT_DIR)/../..

				QT_PATH[arch=x86_64] = /usr/local/opt/qt@5

biodranik (Migrated from github.com) commented

2023-08-24 22:13:25 +00:00

Are abseil-cpp includes required in our case? What features do they provide for the protobuf?

vng (Migrated from github.com) reviewed 2023-08-25 01:56:04 +00:00

xcode/common.xcconfig

					
				@ -3,6 +3,7 @@ OMIM_ROOT = $(PROJECT_DIR)/../..

				QT_PATH[arch=x86_64] = /usr/local/opt/qt@5

vng (Migrated from github.com) commented

2023-08-25 01:54:01 +00:00

Probably, we can avoid putting protobuf and dependent stuff here.
Enough to add them to indexer, drape_frontend, map, some tests ..
Where drules_include.hpp is included.

Probably, we can avoid putting protobuf and dependent stuff here. Enough to add them to indexer, drape_frontend, map, some tests .. Where drules_include.hpp is included.

AndrewShkrob reviewed 2023-08-26 08:49:34 +00:00

qt/build_style/build_statistics.cpp

					
				@ -26,4 +25,1 @@

				  QProcessEnvironment env{QProcessEnvironment::systemEnvironment()};

				  env.insert("PROTOBUF_EGG_PATH", GetProtobufEggPath());

				  // Run the script.

AndrewShkrob commented

2023-08-26 08:49:34 +00:00

This mechanism was employed to assist the designer in scenarios where the Python protobuf dependency might not be present, or to guarantee that the designer is utilizing the same protobuf version as OM.

I endeavored to update it to the current version; however, EGG files have been deprecated in Python, and protobuf no longer offers them.

This mechanism was employed to assist the designer in scenarios where the Python protobuf dependency might not be present, or to guarantee that the designer is utilizing the same protobuf version as OM. I endeavored to update it to the current version; however, EGG files have been deprecated in Python, and protobuf no longer offers them.

AndrewShkrob reviewed 2023-08-26 08:53:16 +00:00

AndrewShkrob reviewed 2023-08-26 08:56:22 +00:00

3party/protobuf/CMakeLists.txt

					
				@ -43,0 +30,4 @@

				set_target_properties(${protobuf_LIB_NAME} PROPERTIES UNITY_BUILD OFF)

				set(ABSL_TARGETS

				  absl_strings

				  absl_time

AndrewShkrob commented

2023-08-26 08:56:22 +00:00

It's not possible because protobuf has conflicting target's name. It was already discussed here: organicmaps/organicmaps#5188 (comment)

It's not possible because protobuf has conflicting target's name. It was already discussed here: https://git.omaps.dev/organicmaps/organicmaps/pulls/5188#issuecomment-1556212319

rtsisyk approved these changes 2023-08-26 12:59:50 +00:00

rtsisyk left a comment

LGTM

biodranik (Migrated from github.com) reviewed 2023-08-28 00:58:21 +00:00

qt/build_style/build_statistics.cpp

					
				@ -26,4 +25,1 @@

				  QProcessEnvironment env{QProcessEnvironment::systemEnvironment()};

				  env.insert("PROTOBUF_EGG_PATH", GetProtobufEggPath());

				  // Run the script.

biodranik (Migrated from github.com) commented

2023-08-28 00:58:21 +00:00

Does it mean that Designer won't work after merging this change?

biodranik (Migrated from github.com) reviewed 2023-08-28 01:17:45 +00:00

3party/protobuf/CMakeLists.txt

					
				@ -43,0 +30,4 @@

				set_target_properties(${protobuf_LIB_NAME} PROPERTIES UNITY_BUILD OFF)

				set(ABSL_TARGETS

				  absl_strings

				  absl_time

biodranik (Migrated from github.com) commented

2023-08-28 01:17:45 +00:00

Thanks for reminding me about that comment. So that is the real reason for creating a copy of CMakeLists.txt manually.

Can FindPackage + alias approach work here instead of add_subdirectory with a manual copy of CMakeLists.txt?
Can a lower, but more recent version than 3.3.0 (without a conflicting base target) be considered instead?

I'm worried about the manual support of upgrading that may lead to errors.

Thanks for reminding me about that comment. So that is the real reason for creating a copy of CMakeLists.txt manually. 1. Can FindPackage + alias approach work here instead of add_subdirectory with a manual copy of CMakeLists.txt? 2. Can a lower, but more recent version than 3.3.0 (without a conflicting `base` target) be considered instead? I'm worried about the manual support of upgrading that may lead to errors.

biodranik (Migrated from github.com) reviewed 2023-08-28 01:20:32 +00:00

biodranik (Migrated from github.com) reviewed 2023-08-28 01:23:53 +00:00

3party/protobuf/config.h

biodranik (Migrated from github.com) commented

2023-08-28 01:23:53 +00:00

CMake generates all necessary defines from CMakeLists.txt file. It would be great to make sure that they all are properly passed to non-cmake builds (XCode).

biodranik (Migrated from github.com) reviewed 2023-08-28 01:28:30 +00:00

3party/protobuf/CMakeLists.txt

					
				@ -43,0 +30,4 @@

				set_target_properties(${protobuf_LIB_NAME} PROPERTIES UNITY_BUILD OFF)

				set(ABSL_TARGETS

				  absl_strings

				  absl_time

biodranik (Migrated from github.com) commented

2023-08-28 01:28:29 +00:00

If I correctly understood, 3.21.9 version (21.x) was building fine, right @Ferenc- ? Was it the latest one without abseil and conflicting targets? organicmaps/organicmaps#3932

If I correctly understood, 3.21.9 version (21.x) was building fine, right @Ferenc- ? Was it the latest one without abseil and conflicting targets? https://git.omaps.dev/organicmaps/organicmaps/pulls/3932

This repo is archived. You cannot comment on pull requests.

No reviewers

pastk

vng

biodranik

rtsisyk

No labels

Android Automotive (AAOS)

API

AppGallery

AppStore

Battery and Performance

External Map Datasets

F-Droid

Fonts

Frequently User Reported

No milestone

No project

No assignees

4 participants

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: organicmaps/organicmaps-tmp#5557

				`@ -17,13 +17,10 @@ and ["icons wanted" issues](https://github.com/organicmaps/organicmaps/issues?q=`

				`To work with styles first [clone the OM repository](INSTALL.md#getting-sources).`

				`@ -3,6 +3,7 @@ OMIM_ROOT = $(PROJECT_DIR)/../..`
				`QT_PATH[arch=x86_64] = /usr/local/opt/qt@5`

Rows
Columns