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

Added missing includes: <netinet/in.h> and <sys/socket.h> #473

Merged
merged 1 commit into from Apr 12, 2017

Conversation

Projects
None yet
8 participants
@yurivict
Member

yurivict commented Feb 13, 2017

Found these missing includes while compiling on FreeBSD 11.


This change is Reviewable

@SkyzohKey SkyzohKey requested a review from iphydf Feb 13, 2017

@iphydf iphydf removed their request for review Feb 15, 2017

@iphydf

This comment has been minimized.

Show comment
Hide comment
@iphydf
Member

iphydf commented Feb 15, 2017

@Diadlo

This comment has been minimized.

Show comment
Hide comment
@Diadlo

Diadlo Feb 15, 2017

Try to remove all this includes and add them into network.h with freebsd ifdef

Diadlo commented Feb 15, 2017

Try to remove all this includes and add them into network.h with freebsd ifdef

@yurivict

This comment has been minimized.

Show comment
Hide comment
@yurivict

yurivict Feb 15, 2017

Member

I moved them into natwork.h
But it looks like ifdef FreeBSD should also be dropped, because linux should need the same includes. They probably get in accidentally on linux through some other path.

Member

yurivict commented Feb 15, 2017

I moved them into natwork.h
But it looks like ifdef FreeBSD should also be dropped, because linux should need the same includes. They probably get in accidentally on linux through some other path.

@Diadlo

This comment has been minimized.

Show comment
Hide comment
@Diadlo

Diadlo Feb 16, 2017

@yurivict Do we really need to include it directly on linux? If you think so, feel free to replace on UNIX ifdef (but not drop)

Diadlo commented Feb 16, 2017

@yurivict Do we really need to include it directly on linux? If you think so, feel free to replace on UNIX ifdef (but not drop)

@yurivict

This comment has been minimized.

Show comment
Hide comment
@yurivict

yurivict Feb 16, 2017

Member

I updated the patch.

  • Moved all network-related includes together into network.h
  • Updated the comment there to say "UNIX includes"

On all UNIXes basic network includes should be the same. It's better to keep them in one place - network.h. Before on Linux some of them were included through some other includes despite manpages saying they should be included.

Member

yurivict commented Feb 16, 2017

I updated the patch.

  • Moved all network-related includes together into network.h
  • Updated the comment there to say "UNIX includes"

On all UNIXes basic network includes should be the same. It's better to keep them in one place - network.h. Before on Linux some of them were included through some other includes despite manpages saying they should be included.

@Diadlo

This comment has been minimized.

Show comment
Hide comment
@Diadlo

Diadlo Feb 16, 2017

@yurivict In real, this includes should be in network.cpp. But network.h should provide platform independent interface (work in progress)
:lgtm_strong:


Reviewed 3 of 3 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.


Comments from Reviewable

Diadlo commented Feb 16, 2017

@yurivict In real, this includes should be in network.cpp. But network.h should provide platform independent interface (work in progress)
:lgtm_strong:


Reviewed 3 of 3 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.


Comments from Reviewable

@iphydf iphydf modified the milestone: v0.1.7 Feb 26, 2017

@nurupo

This comment has been minimized.

Show comment
Hide comment
@nurupo

nurupo Mar 6, 2017

Member

Can confirm, toxcore doesn't build on FreeBSD 11.0 and this PR fixes the issue.

FreeBSD doesn't have IPV6_ADD_MEMBERSHIP and IPV6_DROP_MEMBERSHIP defined, IPV6_JOIN_GROUP and IPV6_LEAVE_GROUP should be used instead. There is already a code block in network.c that handles this:

#ifndef IPV6_ADD_MEMBERSHIP
#ifdef  IPV6_JOIN_GROUP
#define IPV6_ADD_MEMBERSHIP IPV6_JOIN_GROUP
#define IPV6_DROP_MEMBERSHIP IPV6_LEAVE_GROUP
#endif
#endif

The issue is that netinet/in.h needs to be included before this code block, as that's where the IPV6_JOIN_GROUP is defined.

You also need to include sys/socket.h in network.h for AF_INET6 and AF_INET to be defined, which are used in DHT.c.

Member

nurupo commented Mar 6, 2017

Can confirm, toxcore doesn't build on FreeBSD 11.0 and this PR fixes the issue.

FreeBSD doesn't have IPV6_ADD_MEMBERSHIP and IPV6_DROP_MEMBERSHIP defined, IPV6_JOIN_GROUP and IPV6_LEAVE_GROUP should be used instead. There is already a code block in network.c that handles this:

#ifndef IPV6_ADD_MEMBERSHIP
#ifdef  IPV6_JOIN_GROUP
#define IPV6_ADD_MEMBERSHIP IPV6_JOIN_GROUP
#define IPV6_DROP_MEMBERSHIP IPV6_LEAVE_GROUP
#endif
#endif

The issue is that netinet/in.h needs to be included before this code block, as that's where the IPV6_JOIN_GROUP is defined.

You also need to include sys/socket.h in network.h for AF_INET6 and AF_INET to be defined, which are used in DHT.c.

@nurupo

This comment has been minimized.

Show comment
Hide comment
@nurupo

nurupo Mar 6, 2017

Member

@Diadlo do you think we can merge this PR now or we still need to wait for the network refactor? It looks like there is still work to be done in network refactoring since this is an issue

You also need to include sys/socket.h in network.h for AF_INET6 and AF_INET to be defined, which are used in DHT.c.

Ideally DHT.c would use platform independent network interface, rather than using AF_INET6 and AF_INET.

Member

nurupo commented Mar 6, 2017

@Diadlo do you think we can merge this PR now or we still need to wait for the network refactor? It looks like there is still work to be done in network refactoring since this is an issue

You also need to include sys/socket.h in network.h for AF_INET6 and AF_INET to be defined, which are used in DHT.c.

Ideally DHT.c would use platform independent network interface, rather than using AF_INET6 and AF_INET.

@Diadlo

This comment has been minimized.

Show comment
Hide comment
@Diadlo

Diadlo Mar 6, 2017

@nurupo I'm currently working on replacing AF_INET on platform independent TOX_AF_INET. It will be in the next network refactoring PR. So you can merge this PR (in my opinion)

Diadlo commented Mar 6, 2017

@nurupo I'm currently working on replacing AF_INET on platform independent TOX_AF_INET. It will be in the next network refactoring PR. So you can merge this PR (in my opinion)

@nurupo

This comment has been minimized.

Show comment
Hide comment
@nurupo

nurupo Mar 6, 2017

Member

Then you'd need to refactoring this merge in that next refactoring PR.

Member

nurupo commented Mar 6, 2017

Then you'd need to refactoring this merge in that next refactoring PR.

@Diadlo

This comment has been minimized.

Show comment
Hide comment
@Diadlo

Diadlo Mar 6, 2017

Yes, but currently we haven't base for the writing right platform-independent code

Diadlo commented Mar 6, 2017

Yes, but currently we haven't base for the writing right platform-independent code

@nurupo nurupo referenced this pull request Mar 14, 2017

Merged

Add FreeBSD Travis #505

@yurivict yurivict referenced this pull request Mar 15, 2017

Closed

cmake build fails #4175

@iphydf

This comment has been minimized.

Show comment
Hide comment
@iphydf

iphydf Mar 26, 2017

Member

@yurivict can you rebase on master, and also enable the checkbox that lets collaborators push to the PR branch.

Member

iphydf commented Mar 26, 2017

@yurivict can you rebase on master, and also enable the checkbox that lets collaborators push to the PR branch.

@iphydf iphydf modified the milestones: v0.1.7, v0.1.8 Mar 26, 2017

@yurivict

This comment has been minimized.

Show comment
Hide comment
@yurivict

yurivict Mar 26, 2017

Member

I rebased and enabled the checkbox.

Member

yurivict commented Mar 26, 2017

I rebased and enabled the checkbox.

@thierry-FreeBSD

This comment has been minimized.

Show comment
Hide comment
@thierry-FreeBSD

thierry-FreeBSD Mar 27, 2017

In 0.1.7 netinet/in.h is still missing in toxcore/network.c

--- toxcore/network.c.orig      2017-03-26 13:42:48 UTC
+++ toxcore/network.c
@@ -43,6 +43,10 @@
 #include <mach/mach.h>
 #endif
 
+#ifdef __FreeBSD__
+#include <netinet/in.h>
+#endif
+
 #ifndef IPV6_ADD_MEMBERSHIP
 #ifdef  IPV6_JOIN_GROUP
 #define IPV6_ADD_MEMBERSHIP IPV6_JOIN_GROUP

thierry-FreeBSD commented Mar 27, 2017

In 0.1.7 netinet/in.h is still missing in toxcore/network.c

--- toxcore/network.c.orig      2017-03-26 13:42:48 UTC
+++ toxcore/network.c
@@ -43,6 +43,10 @@
 #include <mach/mach.h>
 #endif
 
+#ifdef __FreeBSD__
+#include <netinet/in.h>
+#endif
+
 #ifndef IPV6_ADD_MEMBERSHIP
 #ifdef  IPV6_JOIN_GROUP
 #define IPV6_ADD_MEMBERSHIP IPV6_JOIN_GROUP

@yurivict

This comment has been minimized.

Show comment
Hide comment
@yurivict

yurivict Mar 27, 2017

Member

#ifdef __FreeBSD__ shouldn't be needed. It doesn't complain on linux only because of the accidental inclusion somewhere else.

Member

yurivict commented Mar 27, 2017

#ifdef __FreeBSD__ shouldn't be needed. It doesn't complain on linux only because of the accidental inclusion somewhere else.

Added missing includes: <netinet/in.h> and <sys/socket.h>
Found these missing includes while compiling on FreeBSD 11.

@robinlinden robinlinden merged commit f751fcf into TokTok:master Apr 12, 2017

4 checks passed

code-review/reviewable 1/1 LGTMs
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.4%) to 74.3%
Details
@thierry-FreeBSD

This comment has been minimized.

Show comment
Hide comment
@thierry-FreeBSD

thierry-FreeBSD Dec 27, 2017

v0.1.11 is still unbuildable on FreeBSD:

FAILED: CMakeFiles/toxcore-sut.dir/testing/hstox/driver.c.o 
/usr/local/libexec/ccache/cc  -I/usr/local/include -I/usr/local/include/opus -I/usr/local/include/opencv -O2 -pipe  -fstack-protector -fno-strict-aliasing -std=c99 -pedantic -Wall -Wextra -Weverything -Wno-cast-align -Wno-conversion -Wno-covered-switch-default -Wno-disabled-macro-expansion -Wno-documentation-deprecated-sync -Wno-format-nonliteral -Wno-missing-field-initializers -Wno-missing-prototypes -Wno-padded -Wno-parentheses -Wno-return-type -Wno-sign-compare -Wno-sign-conversion -Wno-tautological-constant-out-of-range-compare -Wno-thread-safety-analysis -Wno-type-limits -Wno-undef -Wno-unreachable-code -Wno-unused-macros -Wno-unused-parameter -Wno-vla -Wno-assign-enum -Wno-bad-function-cast -Wno-double-promotion -Wno-gnu-zero-variadic-macro-arguments -Wno-packed -Wno-reserved-id-macro -Wno-shadow -Wno-shorten-64-to-32 -Wno-unreachable-code-return -Wno-used-but-marked-unused -isystem /usr/local/include -isystem /usr/local/include/opus -isystem /usr/local/include -isystem /usr/local/include -isystem /usr/local/include -isystem /usr/local/include -isystem /usr/local/include -isystem /usr/local/include -isystem /usr/local/include -O2 -pipe  -fstack-protector -fno-strict-aliasing -MD -MT CMakeFiles/toxcore-sut.dir/testing/hstox/driver.c.o -MF CMakeFiles/toxcore-sut.dir/testing/hstox/driver.c.o.d -o CMakeFiles/toxcore-sut.dir/testing/hstox/driver.c.o   -c testing/hstox/driver.c
testing/hstox/driver.c:71:49: error: use of undeclared identifier 'S_IRUSR'
    int fd = open(filename, O_WRONLY | O_CREAT, S_IRUSR | S_IWUSR);
                                                ^
testing/hstox/driver.c:71:59: error: use of undeclared identifier 'S_IWUSR'
    int fd = open(filename, O_WRONLY | O_CREAT, S_IRUSR | S_IWUSR);
                                                          ^
testing/hstox/driver.c:226:24: error: variable has incomplete type 'struct sockaddr_in'
    struct sockaddr_in servaddr;
                       ^
testing/hstox/driver.c:226:12: note: forward declaration of 'struct sockaddr_in'
    struct sockaddr_in servaddr;
           ^
testing/hstox/driver.c:228:38: error: use of undeclared identifier 'INADDR_ANY'
    servaddr.sin_addr.s_addr = htons(INADDR_ANY);
                                     ^
testing/hstox/driver.c:228:38: error: use of undeclared identifier 'INADDR_ANY'
testing/hstox/driver.c:228:38: error: use of undeclared identifier 'INADDR_ANY'
testing/hstox/driver.c:228:38: error: use of undeclared identifier 'INADDR_ANY'
7 errors generated.

thierry-FreeBSD commented Dec 27, 2017

v0.1.11 is still unbuildable on FreeBSD:

FAILED: CMakeFiles/toxcore-sut.dir/testing/hstox/driver.c.o 
/usr/local/libexec/ccache/cc  -I/usr/local/include -I/usr/local/include/opus -I/usr/local/include/opencv -O2 -pipe  -fstack-protector -fno-strict-aliasing -std=c99 -pedantic -Wall -Wextra -Weverything -Wno-cast-align -Wno-conversion -Wno-covered-switch-default -Wno-disabled-macro-expansion -Wno-documentation-deprecated-sync -Wno-format-nonliteral -Wno-missing-field-initializers -Wno-missing-prototypes -Wno-padded -Wno-parentheses -Wno-return-type -Wno-sign-compare -Wno-sign-conversion -Wno-tautological-constant-out-of-range-compare -Wno-thread-safety-analysis -Wno-type-limits -Wno-undef -Wno-unreachable-code -Wno-unused-macros -Wno-unused-parameter -Wno-vla -Wno-assign-enum -Wno-bad-function-cast -Wno-double-promotion -Wno-gnu-zero-variadic-macro-arguments -Wno-packed -Wno-reserved-id-macro -Wno-shadow -Wno-shorten-64-to-32 -Wno-unreachable-code-return -Wno-used-but-marked-unused -isystem /usr/local/include -isystem /usr/local/include/opus -isystem /usr/local/include -isystem /usr/local/include -isystem /usr/local/include -isystem /usr/local/include -isystem /usr/local/include -isystem /usr/local/include -isystem /usr/local/include -O2 -pipe  -fstack-protector -fno-strict-aliasing -MD -MT CMakeFiles/toxcore-sut.dir/testing/hstox/driver.c.o -MF CMakeFiles/toxcore-sut.dir/testing/hstox/driver.c.o.d -o CMakeFiles/toxcore-sut.dir/testing/hstox/driver.c.o   -c testing/hstox/driver.c
testing/hstox/driver.c:71:49: error: use of undeclared identifier 'S_IRUSR'
    int fd = open(filename, O_WRONLY | O_CREAT, S_IRUSR | S_IWUSR);
                                                ^
testing/hstox/driver.c:71:59: error: use of undeclared identifier 'S_IWUSR'
    int fd = open(filename, O_WRONLY | O_CREAT, S_IRUSR | S_IWUSR);
                                                          ^
testing/hstox/driver.c:226:24: error: variable has incomplete type 'struct sockaddr_in'
    struct sockaddr_in servaddr;
                       ^
testing/hstox/driver.c:226:12: note: forward declaration of 'struct sockaddr_in'
    struct sockaddr_in servaddr;
           ^
testing/hstox/driver.c:228:38: error: use of undeclared identifier 'INADDR_ANY'
    servaddr.sin_addr.s_addr = htons(INADDR_ANY);
                                     ^
testing/hstox/driver.c:228:38: error: use of undeclared identifier 'INADDR_ANY'
testing/hstox/driver.c:228:38: error: use of undeclared identifier 'INADDR_ANY'
testing/hstox/driver.c:228:38: error: use of undeclared identifier 'INADDR_ANY'
7 errors generated.

@nurupo

This comment has been minimized.

Show comment
Hide comment
@nurupo

nurupo Dec 27, 2017

Member

Why are you trying to build the incomplete Haskell testing framework testing/hstox?

Member

nurupo commented Dec 27, 2017

Why are you trying to build the incomplete Haskell testing framework testing/hstox?

@nurupo

This comment has been minimized.

Show comment
Hide comment
@nurupo

nurupo Dec 27, 2017

Member

v0.1.11 builds fine on FreeBSD, we have a Travis CI check for that. It doesn't build everything under the sun though, just the actual library and autotests for it, not testing/hstox and others.

Member

nurupo commented Dec 27, 2017

v0.1.11 builds fine on FreeBSD, we have a Travis CI check for that. It doesn't build everything under the sun though, just the actual library and autotests for it, not testing/hstox and others.

@thierry-FreeBSD

This comment has been minimized.

Show comment
Hide comment
@thierry-FreeBSD

thierry-FreeBSD Dec 27, 2017

You're right: removing testing/hstox does it.

thierry-FreeBSD commented Dec 27, 2017

You're right: removing testing/hstox does it.

@nurupo

This comment has been minimized.

Show comment
Hide comment
@nurupo

nurupo Dec 27, 2017

Member

Oh, I'm sorry, I thought hstox wasn't enabled by default and that you were explicitly enabling it by some compile flag, but apparently hstox gets built by default if MSGPACK was found.

Member

nurupo commented Dec 27, 2017

Oh, I'm sorry, I thought hstox wasn't enabled by default and that you were explicitly enabling it by some compile flag, but apparently hstox gets built by default if MSGPACK was found.

@thierry-FreeBSD

This comment has been minimized.

Show comment
Hide comment
@thierry-FreeBSD

thierry-FreeBSD Dec 27, 2017

Yes, I patched CMakeLists.txt to remove it.

thierry-FreeBSD commented Dec 27, 2017

Yes, I patched CMakeLists.txt to remove it.

@nurupo

This comment has been minimized.

Show comment
Hide comment
@nurupo

nurupo Dec 27, 2017

Member

Yeah, we should probably make it build on FreeBSD given how it's enabled by default, or disable it by default and hide under a build flag. @robinlinden @iphydf

Member

nurupo commented Dec 27, 2017

Yeah, we should probably make it build on FreeBSD given how it's enabled by default, or disable it by default and hide under a build flag. @robinlinden @iphydf

@endoffile78

This comment has been minimized.

Show comment
Hide comment
@endoffile78

endoffile78 Dec 28, 2017

Member

@thierry-FreeBSD hstox should compile on FreeBSD when #648 is merged.

Member

endoffile78 commented Dec 28, 2017

@thierry-FreeBSD hstox should compile on FreeBSD when #648 is merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment