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

chore/docs: enable cgo as much as possible #564

Draft
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

mzz2017
Copy link
Contributor

@mzz2017 mzz2017 commented Jun 24, 2024

Background

根据 #557 所述,应尽量开启 CGO。对于跨平台移植性,使用 static link 来保证。因此检查 musl-gcc 和 zig 的可用性并调整 CC 进行 static link。

Checklist

Full Changelogs

  • [Implement ...]

Issue Reference

Closes #557

Test Result

@mzz2017 mzz2017 requested review from a team as code owners June 24, 2024 18:25
@dae-prow dae-prow bot added the chore label Jun 24, 2024
@mzz2017 mzz2017 added the documentation Improvements or additions to documentation label Jun 25, 2024
@mzz2017 mzz2017 changed the title chore: enable cgo as much as possible chore/docs: enable cgo as much as possible Jun 25, 2024
Copy link
Member

@jschwinger233 jschwinger233 left a comment

Choose a reason for hiding this comment

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

tql! GNU/make 权威指南!

@douglarek
Copy link
Contributor

Forgive my frankness, but this PR is adding unprecedented complexity to the build. For proprietary builds of various distributions, using CGO is a given fact. This PR wants to cover all possible CGO builds, which doesn't seem very necessary. I think it's unnecessary to make a big fuss over a rare DNS resolution issue; a document explanation or emphasis would suffice.

@mzz2017
Copy link
Contributor Author

mzz2017 commented Jun 27, 2024

@douglarek 从用户的角度来说,如果没有安装两个工具,行为是和之前一样的,不是强制的。只是尽量在工具存在的情况下绕过glibc进行静态编译

@mzz2017
Copy link
Contributor Author

mzz2017 commented Jun 27, 2024

@douglarek CGO_ENABLED=0 可能会带来其他潜在问题,另外由关闭 cgo 导致的 dns 解析失败的问题很难排查,考虑到用户的环境复杂度,使用 libc 而不是 pure go 实现将问题复杂度从用户转移给更为专业的打包者我认为是可接受的。
欢迎大家进行讨论,特别是用户和打包者等不同角色的参与。

@douglarek
Copy link
Contributor

douglarek commented Jun 27, 2024

另外由关闭 cgo 导致的 dns 解析失败的问题很难排查

How about outputting whether a cgo is enabled in the version information? like this:

dae version 0.7.0rc1.r5.g64ebfea
go runtime go1.22.4 linux/amd64
CGO: enabled
Copyright (c) 2022-2024 @daeuniverse
License GNU AGPLv3 <https://github.com/daeuniverse/dae/blob/main/LICENSE>

if not:

dae version 0.7.0rc1.r5.g64ebfea
go runtime go1.22.4 linux/amd64
CGO: disabled (perhaps it will cause some dns issues)
Copyright (c) 2022-2024 @daeuniverse
License GNU AGPLv3 <https://github.com/daeuniverse/dae/blob/main/LICENSE>

@mzz2017
Copy link
Contributor Author

mzz2017 commented Jun 29, 2024

@douglarek What about the following compromise:

  1. Only statically link in CI/Dockerfile instead of make manually
  2. Revert related modification in Makefile
  3. Use musl-cross for static link instead of zig cc
  4. Give a docs to explain how to statically link

@douglarek
Copy link
Contributor

douglarek commented Jun 29, 2024

Only statically link in CI/Dockerfile instead of make manually

I think this is quite good; the binary released by the project can make such guarantees. However, when it comes to users compiling on their own, they can decide for themselves. For actual distribution packaging, relying on glibc to enable cgo is a very normal operation.

archlinux-github pushed a commit to archlinux/aur that referenced this pull request Jun 29, 2024
@douglarek
Copy link
Contributor

douglarek commented Jun 29, 2024

Numb, it seems that enabling CGO on amd64 causes node domain name resolution issues.

CGO_ENABLED=1 BUILD_ARGS="-buildmode=pie -modcacherw" CFLAGS="-fno-stack-protector" make VERSION="${pkgver}"

level=info msg="Include config files: [/etc/dae/config.dae]"
level=info msg="Loading eBPF programs and maps into the kernel..."
level=info msg="The loading process takes about 120MB free memory, which will be released after loading. Insufficient memory will cause loading failure."
level=info msg="Loaded eBPF programs and maps"
level=info msg="Bind to LAN: docker0"
level=info msg="Bind to LAN: virbr0"
level=info msg="Bind to WAN: enp5s0"
level=info msg="failed to parse node: create hy2://[email protected]: lookup a.b.c: Temporary failure in name resolution"
level=info msg="Group "final" node list:"
level=info msg="        <Empty>"
level=info msg="Group "us" node list:"
level=info msg="        <Empty>"
level=info msg="Routing match set len: 26/64"

@mzz2017 mzz2017 marked this pull request as draft June 30, 2024 11:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Proposal] Enable CGO as much as possible
3 participants