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

Include netinet/in.h everywhere except Windows #949

Merged
merged 1 commit into from May 27, 2023
Merged

Conversation

bentley
Copy link
Contributor

@bentley bentley commented May 24, 2023

libkiwix fails to build on OpenBSD:

c++ -Isrc/libkiwix.so.0.0.p -Isrc -I../libkiwix-12.0.0/src -Iinclude -I../libkiwix-12.0.0/include -Istatic -I/usr/local/include -I/usr/local/include/p11-kit-1 -fcolor-diagnostics -D_FILE_OFFSET_BITS=64 -Wall -Winvalid-pch -Werror -std=c++11 -O2 -pipe -fPIC -MD -MQ src/libkiwix.so.0.0.p/server_internalServer.cpp.o -MF src/libkiwix.so.0.0.p/server_internalServer.cpp.o.d -o src/libkiwix.so.0.0.p/server_internalServer.cpp.o -c ../libkiwix-12.0.0/src/server/internalServer.cpp
../libkiwix-12.0.0/src/server/internalServer.cpp:434:22: error: variable has incomplete type 'struct sockaddr_in'
  struct sockaddr_in sockAddr;
                     ^
../libkiwix-12.0.0/src/server/internalServer.cpp:434:10: note: forward declaration of 'sockaddr_in'
  struct sockaddr_in sockAddr;
         ^

This can be resolved by adding OpenBSD to an existing FreeBSD check. But OpenBSD and FreeBSD are doing nothing wrong here—according to POSIX, the correct header to include for sockaddr_in is netinet/in.h, meaning they are compliant with the spec. Other POSIX systems may hit the same issue.

Linux seems to be more relaxed by declaring sockaddr_in via some other header as well. But it does no harm to include netinet/in.h on Linux, and doing so is better than continuing to add operating systems to the #ifdef.

@kelson42 kelson42 added this to the 12.1.0 milestone May 24, 2023
@veloman-yunkan
Copy link
Collaborator

Something is wrong with the CI workflows - all of them except CI/macOS failed because git clone was invoked with https://github.com/kiwix/libkiwix rather than https://github.com/bentley/libkiwix. I remember seeing that issue before.

According to POSIX, sockaddr_in is declared in netinet/in.h.
Some POSIX systems (notably OpenBSD and FreeBSD) declare it in
only this header, so including it is required. Others, like Linux,
are are more lax in exposing symbols to the namespace, providing
sockaddr_in via additional headers, but it does no harm to include
the standard header on such systems.
@kelson42
Copy link
Collaborator

@veloman-yunkan W had a problem indeed with the CI. i have just merged the fix and have rebased this PR.

@codecov
Copy link

codecov bot commented May 27, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (58890a3) 38.88% compared to head (df164ae) 38.88%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #949   +/-   ##
=======================================
  Coverage   38.88%   38.88%           
=======================================
  Files          56       56           
  Lines        3973     3973           
  Branches     2186     2186           
=======================================
  Hits         1545     1545           
  Misses       1097     1097           
  Partials     1331     1331           
Impacted Files Coverage Δ
src/server/internalServer.cpp 33.60% <ø> (ø)

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@kelson42 kelson42 merged commit b45cfd7 into kiwix:main May 27, 2023
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants