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

Remove -static and rename static build appropriately. #4284

Merged
merged 3 commits into from
Dec 21, 2023

Conversation

fruffy
Copy link
Collaborator

@fruffy fruffy commented Dec 11, 2023

Turns out we can not really build a clean static binary on Linux because of limitations of the glibc library.

https://stackoverflow.com/questions/3430400/linux-static-linking-is-dead

Also, linking with -static causes problems when building Protobuf and bdwgc from scratch. Hence, we remove -static from our build process and rename the CMake parameters appropriately.

@vlstill
Copy link
Contributor

vlstill commented Dec 14, 2023

@fruffy, what is the status of this? The changes look reasonable to me, but it is still marked WIP.

@@ -37,3 +37,12 @@ jobs:
- name: Build (Ubuntu 20.04)
run: |
sudo -E tools/ci-build.sh
# Disabling checks until we find a better way to build a fully static binary.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a way to build fully static binaries on Linux. It basically requires using different libc implementation than glibc as glibc will do dlopen for some features even if build statically and therefore it is never fully static (SO thread). One option that I have seen successfully used in using the Alpine Linux that uses musl libc. I can even try making that work for p4c if desirable. But there is a counter-argument to that and that is that linking libc statically moves the critical compatibility interface from glibc, which is widely used and attempts to be forward-compatible to Linux kernel interface, which does not need to be so stable. I'm not sure how much concern this actually is as I don't have enough knowledge about Linux kernel interlace.

For most uses it should be sufficient to use static-sans-glibc and to build on a distro with the oldest glibc that can possibly be used. Note that using musl instead of glibc will likely bring some performance differences (and a small potential for different bugs).

@fruffy
Copy link
Collaborator Author

fruffy commented Dec 14, 2023

@fruffy, what is the status of this? The changes look reasonable to me, but it is still marked WIP.

I was thinking about a solution similar to what you described in your comment. And I also need to implement the CI check. It is a little finicky since we need to grep for a lack of specific libraries.

@vlstill
Copy link
Contributor

vlstill commented Dec 14, 2023

I was thinking about a solution similar to what you described in your comment.

I still think the static-sans-glibc is actually a more useful build type. There would be niche cases where fully-static would be desired, but that would basically require validating the whole compiler in that mode quite thoroughly.

@fruffy fruffy marked this pull request as ready for review December 14, 2023 19:56
@fruffy fruffy changed the title [Wip] Remove -static and rename static build appropriately. Remove -static and rename static build appropriately. Dec 14, 2023
@fruffy fruffy requested a review from vlstill December 15, 2023 09:13
Copy link
Contributor

@vlstill vlstill left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one comment. Otherwise, I agree with this. It should probably be also reviewed by someone of the core p4c like @mihaibudiu.

CMakeLists.txt Outdated
# 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 "-static-libgcc -static-libstdc++ -Wl,-z,muldefs")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would still make sense to include the previous value of CMAKE_EXE_LINKER_FLAGS I think. The user should have ability to add to them.

@fruffy fruffy force-pushed the fruffy/rename_static_build branch 2 times, most recently from f4146c4 to e10b3aa Compare December 15, 2023 12:46
@fruffy fruffy requested a review from vlstill December 15, 2023 15:03
@fruffy fruffy merged commit 4a880fc into main Dec 21, 2023
13 checks passed
@fruffy fruffy deleted the fruffy/rename_static_build branch December 21, 2023 07:35
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