-
Notifications
You must be signed in to change notification settings - Fork 74
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for Protobuf 22+ which can require either C++14 or C++17 #76
Comments
I can not figure out how this is supposed to work with CMake. As I understand it setting the C++ standard in CMake should be a minimum and CMake should automatically figure out that an included library needs a newer version and then do the right thing. But it seems it doesn't do that. I have tried a different approach in the trial-cmake branch, but in that case it doesn't work either. You should be able to add I can not test all of this on macOS except through Github action which is a pain. If you can shed any light on this and propose a solution I'd be greatful. There must be some way we can say: The minimum C++ version for our software is C++11 and CMake will combine this with the minimum version of all libraries used and do the right thing. |
I don't know best way to handle. At least the shared library seems to work if you switch to Protobuf's CMake files rather than the outdated ones provided by CMake, but I haven't figured out static library. I may need to build some static libs for Protobuf and/or Abseil first. For shared, I tried following and CMake did pick diff --git a/CMakeLists.txt b/CMakeLists.txt
index 147d012..dadad15 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -4,7 +4,8 @@ project(osmpbf VERSION 1.5.0)
include(GNUInstallDirs)
-find_package(Protobuf REQUIRED)
+set(protobuf_MODULE_COMPATIBLE ON CACHE BOOL "")
+find_package(Protobuf CONFIG REQUIRED)
add_subdirectory(osmpbf)
diff --git a/osmpbf/CMakeLists.txt b/osmpbf/CMakeLists.txt
index 41f3c2a..b64d726 100644
--- a/osmpbf/CMakeLists.txt
+++ b/osmpbf/CMakeLists.txt
@@ -1,9 +1,9 @@
protobuf_generate_cpp(CPPS HS fileformat.proto osmformat.proto)
-add_library(osmpbf STATIC ${CPPS})
-target_include_directories(osmpbf SYSTEM PUBLIC ${Protobuf_INCLUDE_DIRS})
-set_property(TARGET osmpbf PROPERTY CXX_STANDARD 11)
-install(TARGETS osmpbf ARCHIVE DESTINATION ${CMAKE_INSTALL_LIBDIR})
+#add_library(osmpbf STATIC ${CPPS})
+#target_include_directories(osmpbf SYSTEM PUBLIC ${Protobuf_INCLUDE_DIRS})
+#set_property(TARGET osmpbf PROPERTY CXX_STANDARD 11)
+#install(TARGETS osmpbf ARCHIVE DESTINATION ${CMAKE_INSTALL_LIBDIR})
add_library(osmpbf_shared SHARED ${CPPS})
set_property(TARGET osmpbf_shared PROPERTY CXX_STANDARD 11)
diff --git a/tools/CMakeLists.txt b/tools/CMakeLists.txt
index c7e7002..ce10303 100644
--- a/tools/CMakeLists.txt
+++ b/tools/CMakeLists.txt
@@ -9,7 +9,7 @@ add_definitions(-D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64)
add_executable(osmpbf-outline osmpbf-outline.cpp)
target_include_directories(osmpbf-outline SYSTEM PRIVATE ${ZLIB_INCLUDE_DIR})
-target_link_libraries(osmpbf-outline PRIVATE osmpbf ZLIB::ZLIB protobuf::libprotobuf)
+target_link_libraries(osmpbf-outline PRIVATE osmpbf_shared ZLIB::ZLIB protobuf::libprotobuf)
set_property(TARGET osmpbf-outline PROPERTY CXX_STANDARD 11)
install(TARGETS osmpbf-outline RUNTIME DESTINATION ${CMAKE_INSTALL_BINDIR}) |
Though, above example only works if Protobuf package includes CMake files, so may need to do some fallback onto find_package(Protobuf CONFIG)
if(NOT Protobuf_FOUND)
find_package(Protobuf MODULE REQUIRED)
endif() EDIT: I guess setting up link to protobuf lib would allow picking up C++ standard, but can't comment on functionality. Most repositories (Linux distros and Homebrew) don't provide static libs for Protobuf and its dependencies. diff --git a/CMakeLists.txt b/CMakeLists.txt
index 147d012..52bd709 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -4,7 +4,11 @@ project(osmpbf VERSION 1.5.0)
include(GNUInstallDirs)
-find_package(Protobuf REQUIRED)
+set(protobuf_MODULE_COMPATIBLE ON CACHE BOOL "")
+find_package(Protobuf CONFIG)
+if(NOT Protobuf_FOUND)
+ find_package(Protobuf MODULE REQUIRED)
+endif()
add_subdirectory(osmpbf)
diff --git a/osmpbf/CMakeLists.txt b/osmpbf/CMakeLists.txt
index 41f3c2a..2478f06 100644
--- a/osmpbf/CMakeLists.txt
+++ b/osmpbf/CMakeLists.txt
@@ -1,6 +1,7 @@
protobuf_generate_cpp(CPPS HS fileformat.proto osmformat.proto)
add_library(osmpbf STATIC ${CPPS})
+target_link_libraries(osmpbf PRIVATE protobuf::libprotobuf)
target_include_directories(osmpbf SYSTEM PUBLIC ${Protobuf_INCLUDE_DIRS})
set_property(TARGET osmpbf PROPERTY CXX_STANDARD 11)
install(TARGETS osmpbf ARCHIVE DESTINATION ${CMAKE_INSTALL_LIBDIR}) |
I tried various things and I think I came up with a solution that should work everwhere. Instead of explicitly trying |
@cho-m Did you get a chance to check that version? |
@joto the homebrew build for protobuf is actually compiled with the latest abseil (which requires cpp cxx 17), any chance of updating to cxx17? this is our cmake config change, |
Sorry, just got around to this again. The CMake should be fine in that branch. Homebrew has also used I've confirmed successful compilation with both:
|
Thanks @cho-m, I have merged that code now. Closing this as fixed. I'll tag a release soon. |
Thanks @joto!! We shipped out c++17 change via Homebrew/homebrew-core#165865 |
Since Protobuf 22, the minimum C++ standard is now C++14. If using recent Protobuf linked to Abseil, this may go up to C++17.
Looking at OSM code:
set(CMAKE_CXX_STANDARD 11)
- https://github.com/openstreetmap/OSM-binary/blob/v1.5.0/CMakeLists.txt#L7set_property(TARGET ... PROPERTY CXX_STANDARD 11)
- https://github.com/openstreetmap/OSM-binary/blob/master/osmpbf/CMakeLists.txt#L5C1-L5C53These both seem to force C++11 resulting in error:
There may be a way to use CMake meta features as Protobuf CMake files specify
cxx_std_14
and Abseil CMake files specifycxx_std_17
.Alternatively, a way of letting user to just override
CMAKE_CXX_STANDARD
or an option to set value would help. Right now,-DCMAKE_CXX_STANDARD=17
doesn't work due to C++11 changes.The text was updated successfully, but these errors were encountered: