Skip to content
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

Set required C++ version #78

Closed
wants to merge 1 commit into from
Closed

Conversation

AMDmi3
Copy link
Contributor

@AMDmi3 AMDmi3 commented Mar 12, 2024

6e9c007 introduced a regression. The code requires c++17 (not c++11) and it's not yet default everywhere.

6e9c007 introduced a regression. The code requires c++17 (not c++11) and it's not yet default everywhere.
@AMDmi3
Copy link
Contributor Author

AMDmi3 commented Mar 12, 2024

On a second though, the fix may be incorrect. The c++17 requirement in fact comes from abseil which is protobuf dependency. Here's the failure:

/usr/local/libexec/ccache/c++ -DPROTOBUF_USE_DLLS -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -I/work/usr/ports/astro/libosmpbf/work/OSM-binary-1.5.1/include -I/work/usr/ports/astro/libosmpbf/work/.build -isystem /usr/local/include -O2 -pipe -fstack-protector-strong -fno-strict-aliasi
ng -O2 -pipe -fstack-protector-strong -fno-strict-aliasing   -DNDEBUG -std=gnu++14 -pthread -MD -MT tools/CMakeFiles/osmpbf-outline.dir/osmpbf-outline.cpp.o -MF tools/CMakeFiles/osmpbf-outline.dir/osmpbf-outline.cpp.o.d -o tools/CMakeFiles/osmpbf-outline.dir/osmpbf-outline.cpp.o -c /
work/usr/ports/astro/libosmpbf/work/OSM-binary-1.5.1/tools/osmpbf-outline.cpp
In file included from /work/usr/ports/astro/libosmpbf/work/OSM-binary-1.5.1/tools/osmpbf-outline.cpp:24:
In file included from /work/usr/ports/astro/libosmpbf/work/OSM-binary-1.5.1/include/osmpbf/osmpbf.h:16:
In file included from /work/usr/ports/astro/libosmpbf/work/.build/osmpbf/fileformat.pb.h:24:
In file included from /usr/local/include/google/protobuf/io/coded_stream.h:134:
In file included from /usr/local/include/absl/strings/cord.h:78:
In file included from /usr/local/include/absl/container/inlined_vector.h:53:
In file included from /usr/local/include/absl/container/internal/inlined_vector.h:30:
In file included from /usr/local/include/absl/container/internal/compressed_tuple.h:40:
/usr/local/include/absl/utility/utility.h:164:12: error: no member named 'in_place_t' in namespace 'std'
using std::in_place_t;
      ~~~~~^

As you can see, there's -std=gnu++14 (not sure where it comes from though) and std::in_place_t is only available since c++17.

The patch proposed does not solve the problem, but this one does:

--- osmpbf/CMakeLists.txt.orig	2024-03-11 08:26:22 UTC
+++ osmpbf/CMakeLists.txt
@@ -1,13 +1,13 @@ add_library(osmpbf STATIC ${CPPS})
 protobuf_generate_cpp(CPPS HS fileformat.proto osmformat.proto)
 
 add_library(osmpbf STATIC ${CPPS})
-target_compile_features(osmpbf PUBLIC cxx_std_11)
+target_compile_features(osmpbf PUBLIC cxx_std_17)
 target_link_libraries(osmpbf PRIVATE protobuf::libprotobuf)
 target_include_directories(osmpbf SYSTEM PUBLIC ${Protobuf_INCLUDE_DIRS})
 install(TARGETS osmpbf ARCHIVE DESTINATION ${CMAKE_INSTALL_LIBDIR})
 
 add_library(osmpbf_shared SHARED ${CPPS})
-target_compile_features(osmpbf_shared PUBLIC cxx_std_11)
+target_compile_features(osmpbf_shared PUBLIC cxx_std_17)
 target_link_libraries(osmpbf_shared PRIVATE protobuf::libprotobuf)
 target_include_directories(osmpbf_shared SYSTEM PUBLIC ${Protobuf_INCLUDE_DIRS})
 set_target_properties(osmpbf_shared PROPERTIES OUTPUT_NAME osmpbf

@joto
Copy link
Collaborator

joto commented Mar 12, 2024

Hmpf. We don't require C++17 and we don't want to require it. So both your solutions don't work. CMake should be able to figure out which C++ version to require based on all the dependencies, that's its job. That's what we have been trying to get it to do in #76. In your case it seems CMake chooses -std=gnu++14 for some reason, but it did work for some people in #76. :-(

@joto joto closed this Apr 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants