[3party] Optionally allow building with packaged protobuf #3822

pastk · 2022-11-05T22:51:16Z

Ferenc- commented

2022-11-05 22:51:16 +00:00

(Migrated from github.com)

Signed-off-by: Ferenc Géczi ferenc.gm@gmail.com

Signed-off-by: Ferenc Géczi <ferenc.gm@gmail.com>

biodranik commented

2022-11-06 07:47:28 +00:00

(Migrated from github.com)

Is protobuf updated to the latest version? Are there any issues with the latest version?

Ferenc- commented

2023-01-15 11:41:54 +00:00

(Migrated from github.com)

Is protobuf updated to the latest version? Are there any issues with the latest version?

Now, that the macro has been dropped, the version has no impact on this PR, so those questions can be the topic of the separate update PR.

> Is protobuf updated to the latest version? Are there any issues with the latest version? Now, that the macro has been dropped, the version has no impact on this PR, so those questions can be the topic of the separate update PR.

biodranik (Migrated from github.com) reviewed 2023-01-15 13:42:11 +00:00

biodranik (Migrated from github.com) left a comment

Now we can expect reports about "it doesn't build if I use system package".

indexer/CMakeLists.txt

					
				@ -151,4 +151,2 @@

				add_subdirectory(${OMIM_ROOT}/3party/protobuf ${CMAKE_CURRENT_BINARY_DIR}/protobuf)

				target_link_libraries(${PROJECT_NAME}

biodranik (Migrated from github.com) commented

2023-01-15 13:41:37 +00:00

Isn't the second param necessary?

Ferenc- commented

2023-01-15 18:28:16 +00:00

(Migrated from github.com)

Now we can expect reports about "it doesn't build if I use system package".

We can set up a CI with a reference build setup with system packages, and just point towards it.
If a user can't replicate that for whatever reason, then it is not our problem anymore.

> Now we can expect reports about "it doesn't build if I use system package". We can set up a CI with a reference build setup with system packages, and just point towards it. If a user can't replicate that for whatever reason, then it is not our problem anymore.

Ferenc- (Migrated from github.com) reviewed 2023-01-15 18:31:46 +00:00

indexer/CMakeLists.txt

					
				@ -151,4 +151,2 @@

				add_subdirectory(${OMIM_ROOT}/3party/protobuf ${CMAKE_CURRENT_BINARY_DIR}/protobuf)

				target_link_libraries(${PROJECT_NAME}

Ferenc- (Migrated from github.com) commented

2023-01-15 18:31:46 +00:00

Isn't the second param necessary?

Necessary for what?
For the build to work? Apparently not.
For some so far (for me) unclear purpose, that you might know about?
Let me know and I put it back immediately.

> Isn't the second param necessary? Necessary for what? For the build to work? Apparently not. For some so far (for me) unclear purpose, that you might know about? Let me know and I put it back immediately.

biodranik (Migrated from github.com) reviewed 2023-01-15 20:01:26 +00:00

indexer/CMakeLists.txt

					
				@ -151,4 +151,2 @@

				add_subdirectory(${OMIM_ROOT}/3party/protobuf ${CMAKE_CURRENT_BINARY_DIR}/protobuf)

				target_link_libraries(${PROJECT_NAME}

biodranik (Migrated from github.com) commented

2023-01-15 20:01:26 +00:00

Is it related to generated protoc binary that can be used to recompile changed map styles?

Ferenc- (Migrated from github.com) reviewed 2023-01-16 20:01:58 +00:00

indexer/CMakeLists.txt

					
				@ -151,4 +151,2 @@

				add_subdirectory(${OMIM_ROOT}/3party/protobuf ${CMAKE_CURRENT_BINARY_DIR}/protobuf)

				target_link_libraries(${PROJECT_NAME}

Ferenc- (Migrated from github.com) commented

2023-01-16 20:01:57 +00:00

generated protoc binary

The protoc binary does not appear to be generated by the current cmake + ninja build process, on the master branch:

find -iname \*proto\*
./indexer/protobuf
./indexer/protobuf/CMakeFiles/protobuf.dir
./OMaps.app/Contents/Resources/drules_proto_dark.bin
./OMaps.app/Contents/Resources/drules_proto_vehicle_clear.bin
./OMaps.app/Contents/Resources/drules_proto_clear.bin
./OMaps.app/Contents/Resources/drules_proto_vehicle_dark.bin
./libprotobuf.a
./drules_struct.proto

Have I missed something perhaps?

recompile changed map styles

From the STYLES.md I don't see how and where that is needed, from that it appears that only the already generated python output is used. Once protobuf is istalled with pip install protobuf.

The one place where I have found usage of protoc is the generate_proto.sh, but that just assumes, that the protoc is somewhere in the PATH.

> generated protoc binary The `protoc` binary does not appear to be generated by the current `cmake` + `ninja` build process, on the `master` branch: ````bash find -iname \*proto\* ./indexer/protobuf ./indexer/protobuf/CMakeFiles/protobuf.dir ./OMaps.app/Contents/Resources/drules_proto_dark.bin ./OMaps.app/Contents/Resources/drules_proto_vehicle_clear.bin ./OMaps.app/Contents/Resources/drules_proto_clear.bin ./OMaps.app/Contents/Resources/drules_proto_vehicle_dark.bin ./libprotobuf.a ./drules_struct.proto ```` Have I missed something perhaps? > recompile changed map styles From the `STYLES.md` I don't see how and where that is needed, from that it appears that only the already generated python output is used. Once protobuf is istalled with `pip install protobuf`. The one place where I have found usage of `protoc` is the `generate_proto.sh`, but that just assumes, that the `protoc` is somewhere in the `PATH`.

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

indexer/CMakeLists.txt

					
				@ -151,4 +151,2 @@

				add_subdirectory(${OMIM_ROOT}/3party/protobuf ${CMAKE_CURRENT_BINARY_DIR}/protobuf)

				target_link_libraries(${PROJECT_NAME}

biodranik (Migrated from github.com) commented

2023-01-20 22:26:31 +00:00

As far as I remember, protoc shoud be the same version to generate styles properly. @vng @pastk do you install some custom protoc/protobuf version or the latest one works fine with styles generation?

pastk reviewed 2023-01-21 19:39:42 +00:00

indexer/CMakeLists.txt

					
				@ -151,4 +151,2 @@

				add_subdirectory(${OMIM_ROOT}/3party/protobuf ${CMAKE_CURRENT_BINARY_DIR}/protobuf)

				target_link_libraries(${PROJECT_NAME}

pastk commented

2023-01-21 19:39:41 +00:00

As far as I remember, protoc shoud be the same version to generate styles properly. @vng @pastk do you install some custom protoc/protobuf version or the latest one works fine with styles generation?

I don't seem to have a protoc binary in my system even. For the styles compiler to work I just installed python protobuf as per the STYLES doc. ATM I'm using 3.19.4.

> As far as I remember, protoc shoud be the same version to generate styles properly. @vng @pastk do you install some custom protoc/protobuf version or the latest one works fine with styles generation? I don't seem to have a `protoc` binary in my system even. For the styles compiler to work I just installed python protobuf as per the STYLES doc. ATM I'm using `3.19.4`.

Ferenc- (Migrated from github.com) reviewed 2023-01-25 16:21:01 +00:00

indexer/CMakeLists.txt

					
				@ -151,4 +151,2 @@

				add_subdirectory(${OMIM_ROOT}/3party/protobuf ${CMAKE_CURRENT_BINARY_DIR}/protobuf)

				target_link_libraries(${PROJECT_NAME}

Ferenc- (Migrated from github.com) commented

2023-01-25 16:21:00 +00:00

How about you @vng ?

vng (Migrated from github.com) reviewed 2023-01-25 17:58:53 +00:00

indexer/CMakeLists.txt

					
				@ -151,4 +151,2 @@

				add_subdirectory(${OMIM_ROOT}/3party/protobuf ${CMAKE_CURRENT_BINARY_DIR}/protobuf)

				target_link_libraries(${PROJECT_NAME}