Install Protobuf using FetchContent. #4056
Conversation
fruffy
force-pushed
the
fruffy/protobuf_fetchcontent
branch
6 times, most recently
from
269dd1a
to
e400d7d
Compare
fruffy
changed the title
CMakeLists.txt
Outdated
| @@ -126,6 +130,8 @@ if (BUILD_STATIC_RELEASE) | |||
| # Do not bring in dynamic libstdcc and libgcc | |||
| set(CMAKE_EXE_LINKER_FLAGS "-static -static-libgcc -static-libstdc++ -Wl,-z,muldefs") | |||
| add_definitions(-DP4C_STATIC_BUILD) | |||
| else () | |||
| set(BUILD_SHARED_LIBS ON) | |||
There was a problem hiding this comment.
Hmm, why is this used? I think this would interfere with all internal libraries we have in the build -- they would all be shared now, which is probably not what we want (or do we?). That would mean that we would need to distribute not only p4c binary, but also all its library dependencies.
There was a problem hiding this comment.
Internal libraries are already linked statically since we only build them this way, e.g.
https://github.com/p4lang/p4c/blob/main/ir/CMakeLists.txt#L66
The intention was to allow Protobuf to be linked dynamically to reduce binary size as per the discussion in #4055. But it is probably more prudent to always package it in to avoid lookup issues even though that may mean we get a huge binary.
There was a problem hiding this comment.
I see. How much does the protobuf increase binary size? But I would still suggest using static protobuf.
| ) | ||
| endif() | ||
|
|
||
| fetchcontent_makeavailable(protoc) |
There was a problem hiding this comment.
should protoc be installed?
There was a problem hiding this comment.
This just downloads the binary packages, the installation of protoc takes a bit. make install will not include protoc since it is not a CMake package.
tools/install_mac_deps.sh
Outdated
| echo 'export PATH="/usr/local/opt/bison/bin:$PATH"' >> ~/.bash_profile | ||
| echo 'export PATH="/usr/local/opt/${PROTOBUF_LIB}/bin:$PATH"' >> ~/.bash_profile | ||
| eecho 'export PATH="/usr/local/opt/grep/libexec/gnubin:$PATH"' >> ~/.bash_profile |
There was a problem hiding this comment.
I know you did not change this code, but unless there is some nonstandard eecho on macOS, this is a typo and therefore does not work.
There was a problem hiding this comment.
Will do this in a separate PR.
…er P4C libraries. Revert dynamic libs. Force static local Protobuf.
fruffy
force-pushed
the
fruffy/protobuf_fetchcontent
branch
from
bc940ad
to
f05f58b
Compare
| else() | ||
| fetchcontent_declare( | ||
| protoc | ||
| URL https://github.com/protocolbuffers/protobuf/releases/download/v${P4C_PROTOBUF_VERSION}/protoc-${P4C_PROTOBUF_VERSION}-linux-x86_64.zip |
There was a problem hiding this comment.
@fruffy This seems to break the build for architectures other than x86_64 (e.g., aarch64).
There was a problem hiding this comment.
Afaik we do not support or make guarantees for any architecture other than x86_64. But if there is a need this should be easily patchable.
There was a problem hiding this comment.
Using the option P4C_USE_PREINSTALLED_PROTOBUF=ON will use the system's Protobuf version. In that case things should compile as usual until we have a patch for these types of architectures in place.
There was a problem hiding this comment.
Thank you Fabian!
Some platforms are not connected to the internet and FetchContent would fail when building p4c. As a workaround, we can run cmake on a system with internet access and include build/_deps with the source code.
Do you have any advice on how to enable building p4c offline?
There was a problem hiding this comment.
I dont think that building without internet is a supported usecase.
There was a problem hiding this comment.
There are ways to run FetchContent fully disconnected https://cmake.org/cmake/help/latest/module/FetchContent.html#command:fetchcontent_populate
At least I have been in scenarios where I did not have internet and was able to build as usual.
Now, populating build/deps in the beginning is a different story. But that is no different that running git submodule init --update. I do not have a good suggestion there yet other than using the PREINSTALLED options.
This PR install Protocol Buffers by pulling binaries with CMake Fetchcontent instead of relying on the system's version of Protobuf. This ensures that we have better control over the version of Protobuf that P4C is built with. The goal is to avoid build errors that occur frequently because of Protobuf.
Unfortunately, no prebuilt libraries exist for
libprotobufandlibprotobuf-lite, so we build from scratch. Thankfully, there is not much to build and the impact on the installation time is negligible.Partially prompted by the discussion in #4055.
Also fixes #2110, fixes #2856, fixes #3543.