From cbbe594d55e9c09a09d2ad61187b954aa84a0b92 Mon Sep 17 00:00:00 2001 From: jkhsjdhjs Date: Sat, 13 Jan 2024 11:31:55 +0100 Subject: [PATCH] Fix build with system Protobuf (#4321) * Fix build with system Protobuf This sets the flag `protobuf_MODULE_COMPATIBLE` in the Protobuf cmake file, so the Protobuf files expose some variables required by the build, such as `Protobuf_PROTOC_EXECUTABLE`. Furthermore, `Protobuf_LIBRARY` is defined manually as `protobuf::libprotobuf`, because it is only holds the path to the Protobuf library, not the abseil library Protobuf depends on since version 22. Fix #4316 * Use `Protobuf_INCLUDE_DIRS` with protoc Protobuf does not seem to set Protobuf_INCLUDE_DIR correctly, but we need this variable for generating code with protoc. Instead, we rely on Protobuf_INCLUDE_DIRS and generate a custom utility variable for includes. --------- Co-authored-by: Fabian Ruffy <5960321+fruffy@users.noreply.github.com> --- backends/dpdk/CMakeLists.txt | 2 +- .../testgen/targets/bmv2/CMakeLists.txt | 2 +- .../targets/bmv2/test/TestTemplate.cmake | 4 +-- cmake/Protobuf.cmake | 30 +++++++++++++------ control-plane/CMakeLists.txt | 4 +-- 5 files changed, 27 insertions(+), 15 deletions(-) diff --git a/backends/dpdk/CMakeLists.txt b/backends/dpdk/CMakeLists.txt index ffe4d26d922..fe3ffc33eea 100644 --- a/backends/dpdk/CMakeLists.txt +++ b/backends/dpdk/CMakeLists.txt @@ -41,7 +41,7 @@ add_dependencies(dpdk_runtime_dir controlplane) add_library(dpdk_runtime STATIC ${DPDK_P4RUNTIME_INFO_GEN_SRCS}) target_include_directories(dpdk_runtime - SYSTEM BEFORE PUBLIC ${Protobuf_INCLUDE_DIR} + SYSTEM BEFORE PUBLIC ${Protobuf_INCLUDE_DIRS} ) # Silence various warnings as the root issue is out of our control, example https://github.com/protocolbuffers/protobuf/issues/7140 diff --git a/backends/p4tools/modules/testgen/targets/bmv2/CMakeLists.txt b/backends/p4tools/modules/testgen/targets/bmv2/CMakeLists.txt index 381f45d05ec..801c55579e1 100644 --- a/backends/p4tools/modules/testgen/targets/bmv2/CMakeLists.txt +++ b/backends/p4tools/modules/testgen/targets/bmv2/CMakeLists.txt @@ -58,6 +58,6 @@ set( set( TESTGEN_INCLUDES ${TESTGEN_INCLUDES} - SYSTEM BEFORE PUBLIC ${Protobuf_INCLUDE_DIR} + SYSTEM BEFORE PUBLIC ${Protobuf_INCLUDE_DIRS} PARENT_SCOPE ) diff --git a/backends/p4tools/modules/testgen/targets/bmv2/test/TestTemplate.cmake b/backends/p4tools/modules/testgen/targets/bmv2/test/TestTemplate.cmake index f6e071e53ab..0e5c95c4752 100644 --- a/backends/p4tools/modules/testgen/targets/bmv2/test/TestTemplate.cmake +++ b/backends/p4tools/modules/testgen/targets/bmv2/test/TestTemplate.cmake @@ -46,7 +46,7 @@ function(validate_protobuf testfile testfolder) file(APPEND ${testfile} "for item in \${txtpbfiles[@]}\n") file(APPEND ${testfile} "do\n") file(APPEND ${testfile} "\techo \"Found \${item}\"\n") - file(APPEND ${testfile} "\t${PROTOC_BINARY} -I${Protobuf_INCLUDE_DIR} -I${CMAKE_CURRENT_LIST_DIR}/../proto -I${P4RUNTIME_STD_DIR} -I${P4C_SOURCE_DIR}/control-plane --encode=p4testgen.TestCase p4testgen.proto < \${item} > /dev/null\n") + file(APPEND ${testfile} "\t${PROTOC_BINARY} ${PROTOBUF_PROTOC_INCLUDES} -I${CMAKE_CURRENT_LIST_DIR}/../proto -I${P4RUNTIME_STD_DIR} -I${P4C_SOURCE_DIR}/control-plane --encode=p4testgen.TestCase p4testgen.proto < \${item} > /dev/null\n") file(APPEND ${testfile} "done\n") endfunction(validate_protobuf) @@ -60,7 +60,7 @@ function(validate_protobuf_ir testfile testfolder) file(APPEND ${testfile} "for item in \${txtpbfiles[@]}\n") file(APPEND ${testfile} "do\n") file(APPEND ${testfile} "\techo \"Found \${item}\"\n") - file(APPEND ${testfile} "\t${PROTOC_BINARY} -I${Protobuf_INCLUDE_DIR} -I${CMAKE_CURRENT_LIST_DIR}/../proto -I${P4RUNTIME_STD_DIR} -I${P4C_SOURCE_DIR} -I${P4C_SOURCE_DIR}/control-plane --encode=p4testgen_ir.TestCase p4testgen_ir.proto < \${item} > /dev/null\n") + file(APPEND ${testfile} "\t${PROTOC_BINARY} ${PROTOBUF_PROTOC_INCLUDES} -I${CMAKE_CURRENT_LIST_DIR}/../proto -I${P4RUNTIME_STD_DIR} -I${P4C_SOURCE_DIR} -I${P4C_SOURCE_DIR}/control-plane --encode=p4testgen_ir.TestCase p4testgen_ir.proto < \${item} > /dev/null\n") file(APPEND ${testfile} "done\n") endfunction(validate_protobuf_ir) diff --git a/cmake/Protobuf.cmake b/cmake/Protobuf.cmake index f1feeb00e9a..4c3e801cf11 100644 --- a/cmake/Protobuf.cmake +++ b/cmake/Protobuf.cmake @@ -11,19 +11,23 @@ macro(p4c_obtain_protobuf) set(SAVED_CMAKE_FIND_LIBRARY_SUFFIXES ${CMAKE_FIND_LIBRARY_SUFFIXES}) set(CMAKE_FIND_LIBRARY_SUFFIXES .a) endif() + set(protobuf_MODULE_COMPATIBLE TRUE) find_package(Protobuf CONFIG) if(NOT Protobuf_FOUND) find_package(Protobuf REQUIRED) endif() - # Protobuf sets the protoc binary to a generator expression, which are problematic. - # They are problematic because they are only evaluated at build time. - # However, we may have scripts that depend on the actual build time during the configure phase. - # Hard code a custom binary instead. + # Protobuf sets the protoc binary to a generator expression, which are problematic. They are + # problematic because they are only evaluated at build time. However, we may have scripts that + # depend on the actual build time during the configure phase. Hard code a custom binary instead. find_program(PROTOC_BINARY protoc) if(ENABLE_PROTOBUF_STATIC) set(CMAKE_FIND_LIBRARY_SUFFIXES ${SAVED_CMAKE_FIND_LIBRARY_SUFFIXES}) endif() + # While Protobuf_LIBRARY is already defined by find_package(Protobuf) because we set protobuf_MODULE_COMPATIBLE, + # it only contains the path to the protobuf shared object, not libraries Protobuf depends on, such as abseil for Protobuf >= 22. + # See https://github.com/p4lang/p4c/issues/4316 + set(Protobuf_LIBRARY "protobuf::libprotobuf") else() # Google introduced another breaking change with protobuf 22.x by adding abseil as a new # dependency. https://protobuf.dev/news/2022-08-03/#abseil-dep We do not want abseil, so we stay @@ -71,12 +75,13 @@ macro(p4c_obtain_protobuf) set(Protobuf_LIBRARY "protobuf::libprotobuf") set(Protobuf_PROTOC_LIBRARY "protobuf::libprotoc") set(Protobuf_PROTOC_EXECUTABLE $) - # Protobuf sets the protoc binary to a generator expression, which are problematic. - # They are problematic because they are only evaluated at build time. - # However, we may have scripts that depend on the actual build time during the configure phase. - # Hard code a custom binary which we can use in addition to the generator expression. + # Protobuf sets the protoc binary to a generator expression, which are problematic. They are + # problematic because they are only evaluated at build time. However, we may have scripts that + # depend on the actual build time during the configure phase. Hard code a custom binary which we + # can use in addition to the generator expression. + # TODO: Maybe we can improve these scripts somehow? set(PROTOC_BINARY ${protobuf_BINARY_DIR}/protoc) - set(Protobuf_INCLUDE_DIR "${protobuf_SOURCE_DIR}/src/") + list(APPEND Protobuf_INCLUDE_DIRS "${protobuf_SOURCE_DIR}/src/") # Reset temporary variable modifications. set(CMAKE_UNITY_BUILD ${CMAKE_UNITY_BUILD_PREV}) @@ -84,6 +89,13 @@ macro(p4c_obtain_protobuf) set(CMAKE_POSITION_INDEPENDENT_CODE ${CMAKE_POSITION_INDEPENDENT_CODE_PREV}) endif() + # Protobuf does not seem to set Protobuf_INCLUDE_DIR correctly, but we need this variable for + # generating code with protoc. Instead, we rely on Protobuf_INCLUDE_DIRS and generate a custom + # utility variable for includes. + set(PROTOBUF_PROTOC_INCLUDES) + foreach(dir ${Protobuf_INCLUDE_DIRS}) + list(APPEND PROTOBUF_PROTOC_INCLUDES "-I${dir}") + endforeach() message("Done with setting up Protobuf for P4C.") endmacro(p4c_obtain_protobuf) diff --git a/control-plane/CMakeLists.txt b/control-plane/CMakeLists.txt index 1e59f9a566f..c113513eec7 100644 --- a/control-plane/CMakeLists.txt +++ b/control-plane/CMakeLists.txt @@ -52,7 +52,7 @@ add_custom_target (mkP4configdir ${CMAKE_COMMAND} -E make_directory ${CMAKE_BINARY_DIR}/p4/config) add_custom_command(OUTPUT ${P4RUNTIME_GEN_SRCS} ${P4RUNTIME_GEN_HDRS} COMMAND ${Protobuf_PROTOC_EXECUTABLE} - -I${P4RUNTIME_STD_DIR} -I${CMAKE_CURRENT_SOURCE_DIR} -I${Protobuf_INCLUDE_DIR} + -I${P4RUNTIME_STD_DIR} -I${CMAKE_CURRENT_SOURCE_DIR} ${PROTOBUF_PROTOC_INCLUDES} --cpp_out ${CMAKE_CURRENT_BINARY_DIR} --python_out ${CMAKE_CURRENT_BINARY_DIR} --experimental_allow_proto3_optional @@ -94,7 +94,7 @@ set (CONTROLPLANE_HDRS add_library(controlplane-gen OBJECT ${P4RUNTIME_GEN_SRCS}) target_include_directories(controlplane-gen - SYSTEM BEFORE PUBLIC ${Protobuf_INCLUDE_DIR} + SYSTEM BEFORE PUBLIC ${Protobuf_INCLUDE_DIRS} ) target_link_libraries (controlplane-gen PUBLIC ${Protobuf_LIBRARY}) # Needed for the correct import of google/status.pb.cc