organicmaps/organicmaps

Fork 24

Upgrade to protobuf 24.4 #6310

Open

biodranik wants to merge 3 commits from protobuf-24.4 into master

biodranik commented

2023-10-14 02:00:06 +00:00

(Migrated from github.com)

Is based on and closes #5557

Contains additional iOS fixes and a newer protobuf version

Requres https://github.com/organicmaps/kothic/pull/27 to be merged

Is based on and closes #5557 Contains additional iOS fixes and a newer protobuf version Requres https://github.com/organicmaps/kothic/pull/27 to be merged

AndrewShkrob commented

2023-10-14 08:48:53 +00:00

Member

set(protobuf_VERSION "24.4")

```suggestion:-0+0 set(protobuf_VERSION "24.4") ```

vng commented

2023-10-14 12:24:42 +00:00

(Migrated from github.com)

Why additional indent here?

pastk commented

2023-10-15 07:12:22 +00:00

Owner

Did you update the companion kothic PR as well?
At least protobuf version in tools/kothic/requirements.txt should be updated and it contains its own src/drules_struct_pb2.py.
And kothic submodule update commit added here.

Did you update the companion kothic PR as well? At least protobuf version in `tools/kothic/requirements.txt` should be updated and it contains its own `src/drules_struct_pb2.py`. And kothic submodule update commit added here.

biodranik commented

2023-10-15 07:22:43 +00:00

(Migrated from github.com)

@AndrewShkrob I don't like updating it manually every time. Can it be automated? Is it needed at all?

`@AndrewShkrob` I don't like updating it manually every time. Can it be automated? Is it needed at all?

biodranik commented

2023-10-15 07:30:43 +00:00

(Migrated from github.com)

Created https://github.com/organicmaps/kothic/pull/27

biodranik commented

2023-10-15 07:35:32 +00:00

(Migrated from github.com)

@pastk

ERROR: an icon is present, but caption's text-offset is not set for z19 entrance
ERROR: an icon is present, but caption's text-offset is not set for z19 entrance-exit
ERROR: an icon is present, but caption's text-offset is not set for z18 entrance-main
ERROR: an icon is present, but caption's text-offset is not set for z19 entrance-main
ERROR: an icon is present, but caption's text-offset is not set for z16 leisure-beach_resort

`@pastk` ``` ERROR: an icon is present, but caption's text-offset is not set for z19 entrance ERROR: an icon is present, but caption's text-offset is not set for z19 entrance-exit ERROR: an icon is present, but caption's text-offset is not set for z18 entrance-main ERROR: an icon is present, but caption's text-offset is not set for z19 entrance-main ERROR: an icon is present, but caption's text-offset is not set for z16 leisure-beach_resort ```

pastk commented

2023-10-15 07:40:17 +00:00

Owner

#6301 is not merged yet, while https://github.com/organicmaps/kothic/pull/26 is.

https://git.omaps.dev/organicmaps/organicmaps/pulls/6301 is not merged yet, while https://github.com/organicmaps/kothic/pull/26 is.

biodranik commented

2023-10-15 07:42:51 +00:00

(Migrated from github.com)

Indent looks ok:

Indent looks ok: <img width="790" alt="image" src="/uploads/2de2d33a4eb4ce49cbdf90129473abf7/dfa4ef59-77d7-4331-abfc-fb512a77faab">

pastk commented

2023-10-15 07:43:48 +00:00

Owner

kothic is not 3party, I suggest use [tools] Update kothic (...) as a commit message

kothic is not 3party, I suggest use `[tools] Update kothic (...)` as a commit message

AndrewShkrob commented

2023-10-15 10:12:41 +00:00

Member

This line is required:

set_target_properties(libprotobuf-lite PROPERTIES
    VERSION ${protobuf_VERSION}
    OUTPUT_NAME ${LIB_PREFIX}protobuf-lite
    DEBUG_POSTFIX "${protobuf_DEBUG_POSTFIX}"
    # For -fvisibility=hidden and -fvisibility-inlines-hidden
    C_VISIBILITY_PRESET hidden
    CXX_VISIBILITY_PRESET hidden
    VISIBILITY_INLINES_HIDDEN ON
)

I think it's not a big deal to update the version each time you update the version...
But if you don't like it, you can set any dummy value here, for example, "OrganicMaps", or the commit hash of the protobuf repo.

There is also another way. The version of protobuf is stored in 3party/protobuf/protobuf/version.json:

{
    "24.x": {
        "protoc_version": "24.4",
        "lts": false,
        "date": "2023-10-03",
        "languages": {
            "cpp": "4.24.4",
            "csharp": "3.24.4",
            "java": "3.24.4",
            "javascript": "3.24.4",
            "objectivec": "3.24.4",
            "php": "3.24.4",
            "python": "4.24.4",
            "ruby": "3.24.4"
        }
    }
}

We can write a script to fetch "protoc_version", but I think it's an overcomplication.

This line is required: ```cmake set_target_properties(libprotobuf-lite PROPERTIES VERSION ${protobuf_VERSION} OUTPUT_NAME ${LIB_PREFIX}protobuf-lite DEBUG_POSTFIX "${protobuf_DEBUG_POSTFIX}" # For -fvisibility=hidden and -fvisibility-inlines-hidden C_VISIBILITY_PRESET hidden CXX_VISIBILITY_PRESET hidden VISIBILITY_INLINES_HIDDEN ON ) ``` I think it's not a big deal to update the version each time you update the version... But if you don't like it, you can set any dummy value here, for example, `"OrganicMaps"`, or the commit hash of the protobuf repo. There is also another way. The version of protobuf is stored in `3party/protobuf/protobuf/version.json`: ```json { "24.x": { "protoc_version": "24.4", "lts": false, "date": "2023-10-03", "languages": { "cpp": "4.24.4", "csharp": "3.24.4", "java": "3.24.4", "javascript": "3.24.4", "objectivec": "3.24.4", "php": "3.24.4", "python": "4.24.4", "ruby": "3.24.4" } } } ``` We can write a script to fetch `"protoc_version"`, but I think it's an overcomplication.

Jean-BaptisteC commented

2024-03-15 19:55:56 +00:00

Member

ready to merge?

pastk commented

2024-03-15 20:52:32 +00:00

Owner

It'd be great to have "designer" fixes merged separately.

As for the protobuf version upgrade - there seems to be no value? in doing it, but it leads to ~300Kb (?) binary size increase, unfortunately...

It'd be great to have "designer" fixes merged separately. As for the protobuf version upgrade - there seems to be no value? in doing it, but it leads to ~300Kb (?) binary size increase, unfortunately...

gerion0 commented

2024-04-25 08:29:16 +00:00

(Migrated from github.com)

mentioned in issue #2217

pastk commented

2024-12-02 17:25:26 +00:00

Owner

As for the protobuf version upgrade - there seems to be no value? in doing it, but it leads to ~300Kb (?) binary size increase, unfortunately...

Its interesting that in Ubuntu 24.04 LTS there are two protobuf versions
libprotobuf32t64 is Installed-Size: 3 082 kB
libprotobuf-lite32t64 is 878 kB (This package contains the runtime library needed for C++ applications whose message definitions have the "lite runtime" optimization setting.).
So maybe the lite runtime option was misconfigured in our case?
ref set(protobuf_LIB_NAME libprotobuf-lite)

Also note the peculiarity in protobuf's version numbering (https://protobuf.dev/support/version-support/):

Starting with the v20.x protoc release, we changed our versioning scheme to enable nimbler updates to language-specific parts of Protocol Buffers. In the new scheme, each language has its own major version that can be incremented independently of other languages. The minor and patch versions, however, remain coupled. This allows us to introduce breaking changes into some languages without requiring a bump of the major version in languages that do not experience a breaking change. For example, a single release might include protoc version 24.0, Java runtime version 4.24.0 and C# runtime version 3.24.0.

The first instance of this new versioning scheme was the 4.21.0 version of the Python API, which followed the preceding version, 3.20.1. Other language APIs released at the same time were released as 3.21.0.

The C++ core uses protobuf 3.3.0 still. I understand that there could be breaking changes in newer versions, but for us versions mismatch between C++ and python protobufs worked well because we use basic features only.

> As for the protobuf version upgrade - there seems to be no value? in doing it, but it leads to ~300Kb (?) binary size increase, unfortunately... Its interesting that in Ubuntu 24.04 LTS there are two protobuf versions `libprotobuf32t64` is `Installed-Size: 3 082 kB` `libprotobuf-lite32t64` is `878 kB` (`This package contains the runtime library needed for C++ applications whose message definitions have the "lite runtime" optimization setting.`). So maybe the lite runtime option was misconfigured in our case? ref `set(protobuf_LIB_NAME libprotobuf-lite)` Also note the peculiarity in protobuf's version numbering (https://protobuf.dev/support/version-support/): > Starting with the v20.x protoc release, we changed our versioning scheme to enable nimbler updates to language-specific parts of Protocol Buffers. In the new scheme, each language has its own major version that can be incremented independently of other languages. The minor and patch versions, however, remain coupled. This allows us to introduce breaking changes into some languages without requiring a bump of the major version in languages that do not experience a breaking change. For example, a single release might include protoc version 24.0, Java runtime version 4.24.0 and C# runtime version 3.24.0. > The first instance of this new versioning scheme was the 4.21.0 version of the Python API, which followed the preceding version, 3.20.1. Other language APIs released at the same time were released as 3.21.0. The C++ core uses protobuf 3.3.0 still. I understand that there could be breaking changes in newer versions, but for us versions mismatch between C++ and python protobufs worked well because we use basic features only.

rtsisyk commented

2024-12-23 16:48:09 +00:00

Owner

What is the status here? Why do we need to update Protobuf at all? Were there any CVEs?

pastk commented

2024-12-23 17:19:32 +00:00

Owner

I don't think it makes sense to update protobuf, unless the increased size issue is solved.

But it would be great to salvage non-protobuf parts of this PR (designer target fixes) and merge them separately.

I don't think it makes sense to update protobuf, unless the increased size issue is solved. But it would be great to salvage non-protobuf parts of this PR (designer target fixes) and merge them separately.

rtsisyk commented

2024-12-24 19:02:20 +00:00

Owner

I don't think it makes sense to update protobuf, unless the increased size issue is solved.

Shall we close the PR? Updating dependencies without a clear purpose may not be the best use of time.

> I don't think it makes sense to update protobuf, unless the increased size issue is solved. Shall we close the PR? Updating dependencies without a clear purpose may not be the best use of time.

gerion0 commented

2024-12-25 00:31:43 +00:00

(Migrated from github.com)

As you can see from the linked messages, Gentoo (and probably other distributions) need to bundle the older protobuf as part of OrganicMaps then.
This is not a show stopper, just a consequence.

I have not tried, if OrganicMaps builds with the system version of protobuf (currently version 27.2 on my PC) with this PR.

As you can see from the linked messages, Gentoo (and probably other distributions) [need to bundle](https://gitweb.gentoo.org/repo/user/gerislay.git/tree/gui-apps/organicmaps/organicmaps-9999.ebuild#n16) the older protobuf as part of OrganicMaps then. This is not a show stopper, just a consequence. I have not tried, if OrganicMaps builds with the system version of protobuf (currently version 27.2 on my PC) with this PR.

pastk commented

2024-12-30 16:04:06 +00:00

Owner

As you can see from the linked messages, Gentoo (and probably other distributions) need to bundle the older protobuf as part of OrganicMaps then.

Its better to just continue to build with a bundled protobuf submodule, treat it as a part of the project.

> As you can see from the linked messages, Gentoo (and probably other distributions) [need to bundle](https://gitweb.gentoo.org/repo/user/gerislay.git/tree/gui-apps/organicmaps/organicmaps-9999.ebuild#n16) the older protobuf as part of OrganicMaps then. Its better to just continue to build with a bundled protobuf submodule, treat it as a part of the project.

Osyotr commented

2025-01-22 21:52:40 +00:00

(Migrated from github.com)

The version of protobuf used by OM cannot be compiled with MSVC without additional patches.
If the decision is to keep the old version then I would appreciate if someone with write access to the protobuf fork could replace this line with #if 0 and use the patched version in this repo.
a6189acd18/src/google/protobuf/stubs/hash.h (L236)
PS: Would be nice to have a more recent comparison of binary sizes.

The version of protobuf used by OM cannot be compiled with MSVC without additional patches. If the decision is to keep the old version then I would appreciate if someone with write access to the protobuf fork could replace this line with `#if 0` and use the patched version in this repo. https://github.com/organicmaps/protobuf/blob/a6189acd18b00611c1dc7042299ad75486f08a1a/src/google/protobuf/stubs/hash.h#L236 PS: Would be nice to have a more recent comparison of binary sizes.

pastk commented

2025-02-25 05:50:55 +00:00

Owner

mentioned in issue #10341