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

fix(build): fix build on FreeBSD #1639

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

fix(build): fix build on FreeBSD #1639

wants to merge 5 commits into from

Conversation

rene0
Copy link

@rene0 rene0 commented Aug 4, 2024

Checklist

Description

The build of ooni-probe currently fails on FreeBSD because the compiler cannot find some required symbols (C.SOCK_STREAM, C.AF_INET, C.AF_INET6 and C.AF_UNSPEC). This causes the package net/ooni-probe-cli to be unavailable.

Fix this by including sys/socket.h when building on FreeBSD.

See also these FreeBSD bug reports (the second report also applies to Go 1.21 and Go 1.22):

I took a bit of a shortcut for the fix in the first URL by omitting the #ifdef as that patch would only exist in the FreeBSD port anyway. Both URLs also discuss if this is a problem in OONI or Go itself.

@rene0 rene0 requested review from hellais and DecFox as code owners August 4, 2024 19:47
@bassosimone
Copy link
Contributor

bassosimone commented Aug 7, 2024

Thank you for your diff!

I've taken a look at IEEE Std 1003.1, and it says that the sys/socket.h file should define SOCK_STREAM, AF_INET, etc. About netdb.h, it reads: "inclusion of the <netdb.h> header may also make visible all symbols from [...] sys/socket.h".

Because it says may, for correctness, I think this repository should always include sys/socket.h when it is not not compiling for Windows. FreeBSD seems to have very clean headers and surfaced our optimistic include policy 😄.

Regarding this:

Both URLs also discuss if this is a problem in OONI or Go itself.

This issue is definitely an issue with this codebase and not an issue with Go itself.

Cc: @DecFox

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