Fix some problems with the Protobuf CMake file #4262

fruffy · 2023-11-26T17:30:17Z

A P4C extension I was working on needed gRPC. However, the way we currently build Protobuf does not combine well with the gRPC FetchContent module. This PR adds a couple fixes for the Protobuf FetchContent script.

Build protoc from scratch.
Ensure that we always build with position-independent code locally but not across the entire compiler.
Update how we set variables, align it to be the same as how the Protobuf CMake sets it.

fruffy · 2023-11-27T17:02:23Z

CMakeLists.txt

@@ -128,7 +128,7 @@ if (BUILD_STATIC_RELEASE)
  # Set the static variable
  set(P4C_STATIC_BUILD STATIC)
  # Do not bring in dynamic libstdcc and libgcc
-  set(CMAKE_EXE_LINKER_FLAGS "-static -static-libgcc -static-libstdc++ -Wl,-z,muldefs")
+  set(CMAKE_EXE_LINKER_FLAGS "${CMAKE_EXE_LINKER_FLAGS} -static-libgcc -static-libstdc++ -Wl,-z,muldefs")


Linking protoc when using -static causes a series of problems. It seems to be a bad idea to use -static in general, so let's just fix this call.

But this option is under BUILD_STATIC_RELEASE. It seems wrong to me to have this build non-static binary. If BUILD_STATIC_RELEASE is broken, we should just remove it. Arguably it is broken on most linux distros as glibc should not be linked statically (and often there is no corresponding .a). Therefore, BUILD_STATIC_RELASE would be useful only on distros like Alpine (which uses MUSL instead of GLIBC).

I suggest you just don't enable BUILD_STATIC_RELEASE. If you have need for semi-static (i.e. glibc dynamic, rest static) build, that should be handled by a new CMake option in a new PR.

We a CI test that causes problems, so it is a little tricky to make this work. -static not only causes problems for Protobuf but also the libgc build.

Let me experiment a bit with this. I am able to build locally with clang+lld. So maybe that is one way to ensure we can at least generate some form of static release.

With bf-p4c, we did not even use the -static iirc.

Yep, that is why I suggested actually removing static as a possibility. If we add semi-static instead, it would not be hard if anyone needed full-static to set it from it (basically just setting CMAKE_EXE_LINKER_FLAGS to -static). We probably have no way of knowing if static build is used by anyone (unless they show up here).

So what is the best course of action here? Throw a warning when BUILD_STATIC_RELEASE is enabled? Or rename BUILD_STATIC_RELEASE?

I would suggest renaming it. This seems the cleanest to me and least risk of breaking someones workflow. Maybe something like BUILD_SEMI_STATIC_RELEASE, although it does not explain what it means to be semi-static. But I cannot think of a short way to express "static except for basic OS dependencies like glibc".

fruffy · 2023-11-27T17:03:04Z

backends/dpdk/CMakeLists.txt

@@ -28,7 +28,7 @@ add_custom_target (dpdk_runtime_dir
 # placed in the build directory inside `control-plane` directory
 add_custom_command(
    OUTPUT ${DPDK_P4RUNTIME_INFO_GEN_SRCS} ${DPDK_P4RUNTIME_INFO_GEN_HDRS}
-  COMMAND ${PROTOBUF_PROTOC_EXECUTABLE}
+  COMMAND ${Protobuf_PROTOC_EXECUTABLE}


The canonical Protobuf variable prefix is now Protobuf_ instead of PROTOBUF_.

https://cmake.org/cmake/help/latest/module/FindProtobuf.html

fruffy · 2023-11-27T17:05:06Z

cmake/Protobuf.cmake

    set(protobuf_BUILD_TESTS OFF CACHE BOOL "Build tests.")
-    set(protobuf_BUILD_PROTOC_BINARIES OFF CACHE BOOL "Build libprotoc and protoc compiler.")
+    set(protobuf_BUILD_PROTOC_BINARIES ON CACHE BOOL "Build libprotoc and protoc compiler.")


We build protoc from scratch, because the downloaded, prebuilt binary can interact badly with protoc plugins such as grpc_cpp_plugin.

vlstill

The change in P4C_STATIC_BUILD is wrong in my opinion. It might be broken, but changing its behavior to build non-static binary would be extremely confusing.

Otherwise there seem to be some unnecessary code and I don't understand why you are using two different variables for PROTOC binary (in different places in the cmake).

vlstill · 2023-12-05T12:05:27Z

CMakeLists.txt

@@ -128,7 +128,7 @@ if (BUILD_STATIC_RELEASE)
  # Set the static variable
  set(P4C_STATIC_BUILD STATIC)
  # Do not bring in dynamic libstdcc and libgcc
-  set(CMAKE_EXE_LINKER_FLAGS "-static -static-libgcc -static-libstdc++ -Wl,-z,muldefs")
+  set(CMAKE_EXE_LINKER_FLAGS "${CMAKE_EXE_LINKER_FLAGS} -static-libgcc -static-libstdc++ -Wl,-z,muldefs")


But this option is under BUILD_STATIC_RELEASE. It seems wrong to me to have this build non-static binary. If BUILD_STATIC_RELEASE is broken, we should just remove it. Arguably it is broken on most linux distros as glibc should not be linked statically (and often there is no corresponding .a). Therefore, BUILD_STATIC_RELASE would be useful only on distros like Alpine (which uses MUSL instead of GLIBC).

I suggest you just don't enable BUILD_STATIC_RELEASE. If you have need for semi-static (i.e. glibc dynamic, rest static) build, that should be handled by a new CMake option in a new PR.

cmake/Protobuf.cmake

control-plane/CMakeLists.txt

vlstill

I think the changes now make sense. But I strongly suggest the renaming of static build is a separate PR. It is not directly related to Protobuf and it would be good if it is more visible in the commit messages for the benefit of downstream tools etc.

vlstill · 2023-12-11T12:44:13Z

.github/workflows/ci-static-build-test.yml

+        # Disabling checks until we find a better way to build a fully static binary.
+        ldd ./build/p4c-bm2-psa 2>&1 # | grep -E -qi "(not a dynamic executable)|(statically linked)"
+        ldd ./build/p4c-bm2-ss 2>&1 # | grep -E -qi "(not a dynamic executable)|(statically linked)"
+        ldd ./build/p4c-dpdk 2>&1 # | grep -E -qi "(not a dynamic executable)|(statically linked)"
+        ldd ./build/p4c-ebpf 2>&1 # | grep -E -qi "(not a dynamic executable)|(statically linked)"
+        ldd ./build/p4c-pna-p4tc 2>&1 # | grep -E -qi "(not a dynamic executable)|(statically linked)"
+        ldd ./build/p4c-ubpf 2>&1 # | grep -E -qi "(not a dynamic executable)|(statically linked)"
+        ldd ./build/p4test 2>&1 # | grep -E -qi "(not a dynamic executable)|(statically linked)"
+        ldd ./build/p4testgen 2>&1 # | grep -E -qi "(not a dynamic executable)|(statically linked)"


We could check that there is no boost, no libstdc++, etc. It would be a partial check only, but it would be at least something.

… Build protoc from scratch.

fruffy force-pushed the fruffy/protobuf_fixes branch 6 times, most recently from 3b2b974 to 93dd245 Compare November 27, 2023 16:12

fruffy marked this pull request as ready for review November 27, 2023 17:05

fruffy commented Nov 27, 2023

View reviewed changes

fruffy force-pushed the fruffy/protobuf_fixes branch 3 times, most recently from 00ae8b3 to 1b7d9cb Compare December 1, 2023 17:14

fruffy changed the title ~~Fix some problems with the Protobuf cmake file~~ Fix some problems with the Protobuf CMake file Dec 4, 2023

fruffy force-pushed the fruffy/protobuf_fixes branch from 1b7d9cb to 2dbbf7a Compare December 4, 2023 18:43

fruffy requested review from vlstill and pkotikal December 4, 2023 19:08

vlstill requested changes Dec 5, 2023

View reviewed changes

fruffy force-pushed the fruffy/protobuf_fixes branch 6 times, most recently from f675f43 to 7458d4a Compare December 8, 2023 15:41

vlstill reviewed Dec 11, 2023

View reviewed changes

fruffy force-pushed the fruffy/protobuf_fixes branch from 7458d4a to f520ef6 Compare December 11, 2023 13:26

fruffy force-pushed the fruffy/protobuf_fixes branch from f520ef6 to 0da31ee Compare December 21, 2023 07:40

fruffy requested a review from vlstill December 21, 2023 16:18

vlstill approved these changes Jan 2, 2024

View reviewed changes

fruffy force-pushed the fruffy/protobuf_fixes branch from 0da31ee to d27dce7 Compare January 3, 2024 11:22

fruffy added 2 commits January 3, 2024 18:28

Make sure to export the FindProtobuf config after using FetchContent.…

73350e1

… Build protoc from scratch.

Review comments.

8c28a57

fruffy force-pushed the fruffy/protobuf_fixes branch from d27dce7 to 8c28a57 Compare January 3, 2024 17:28

fruffy merged commit aaa5c7d into main Jan 3, 2024
13 checks passed

fruffy deleted the fruffy/protobuf_fixes branch January 3, 2024 22:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix some problems with the Protobuf CMake file #4262

Fix some problems with the Protobuf CMake file #4262

fruffy commented Nov 26, 2023 •

edited

Loading

fruffy Nov 27, 2023

vlstill Dec 5, 2023

fruffy Dec 5, 2023

vlstill Dec 5, 2023

fruffy Dec 5, 2023

vlstill Dec 6, 2023

fruffy Nov 27, 2023

fruffy Nov 27, 2023 •

edited

Loading

vlstill left a comment

vlstill Dec 5, 2023

vlstill left a comment

vlstill Dec 11, 2023

Fix some problems with the Protobuf CMake file #4262

Fix some problems with the Protobuf CMake file #4262

Conversation

fruffy commented Nov 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fruffy Nov 27, 2023 • edited Loading

Choose a reason for hiding this comment

vlstill left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vlstill left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fruffy commented Nov 26, 2023 •

edited

Loading

fruffy Nov 27, 2023 •

edited

Loading