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

Portability Fixes #398

Merged
merged 1 commit into from
Jan 6, 2017
Merged

Portability Fixes #398

merged 1 commit into from
Jan 6, 2017

Conversation

Zer0-One
Copy link
Member

@Zer0-One Zer0-One commented Jan 6, 2017

This reverts commit 59e2a84.

  • CFLAG gnu99 was changed to c99.
  • CXXFLAG c++98 was changed to c++11.
  • CFLAG -pedantic-errors was added so that non-ISO C now throws errors.
  • _XOPEN_SOURCE feature test macro added and set to 600 to expose SUSv3 and c99 definitions in modules that required them.
  • Fixed tests (and bootstrap daemon logging) that were failing due to the altered build flags.
  • Avoid string suffix misinterpretation; explicit narrowing conversion.
  • Misc. additions to .gitignore to make sure build artifacts don't wind up in version control.

This change is Reviewable

@iphydf iphydf modified the milestone: v0.1.4 Jan 6, 2017
@iphydf
Copy link
Member

iphydf commented Jan 6, 2017

_DARWIN_C_SOURCE may be needed for osx in network.c.

@Zer0-One
Copy link
Member Author

Zer0-One commented Jan 6, 2017

@iphydf yeah, i noticed that sys/socket.h is included for macos, but that SO_NOSIGPIPE still isn't defined.

@Zer0-One
Copy link
Member Author

Zer0-One commented Jan 6, 2017

I want to take a look at an actual os x install to be sure, but I don't have one :(

@jin-eld
Copy link

jin-eld commented Jan 6, 2017

Afaik you need to add conditions to the code using #if defined(SO_NOSIGPIPE) so you do not use it at all on OSX, because it really is not available on OSX (and on BSD too I think?). I remember that from my MediaTomb times which was very portable and which we had running on pretty much every linux/unix system.

@Zer0-One
Copy link
Member Author

Zer0-One commented Jan 6, 2017

@jin-eld I think you've got it backward. SO_NOSIGPIPE is a macos and bsd thing, not available on linux, and definitely not defined in POSIX. i haven't looked at it yet, but i'd guess the issue is a missing feature-test macro, like iphy suggested.

@jin-eld
Copy link

jin-eld commented Jan 6, 2017

@Zer0-One Ooops :) Sorry... I guess it's been too long... somehow I remembered it the other way around.

@Zer0-One Zer0-One force-pushed the portability branch 2 times, most recently from 8cc3fc1 to 502261e Compare January 6, 2017 11:31
#define _DARWIN_C_SOURCE
#include <mach/clock.h>
#include <mach/mach.h>
#endif
Copy link
Member

Choose a reason for hiding this comment

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

I assume you moved this block so you don't need an extra block at the beginning. Would it work if you moved this block back to where it came from and #define _DARWIN_C_SOURCE unconditionally together with _XOPEN_SOURCE?

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@iphydf
Copy link
Member

iphydf commented Jan 6, 2017

:lgtm_strong: if you squash the commits I'll merge it. Sorting the feature test macros (darwin before xopen) would be nice, but it's ok if you can't do that now.


Review status: 0 of 27 files reviewed at latest revision, 1 unresolved discussion.


Comments from Reviewable

@iphydf iphydf self-assigned this Jan 6, 2017
This reverts commit 59e2a84, and
defines _DARWIN_C_SOURCE in toxcore/network.c
@Zer0-One
Copy link
Member Author

Zer0-One commented Jan 6, 2017

done and done


Review status: 0 of 27 files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@iphydf iphydf merged commit 8ef1f35 into TokTok:master Jan 6, 2017
@nurupo
Copy link
Member

nurupo commented Jan 12, 2017

:lgtm_strong:


Comments from Reviewable

@nurupo
Copy link
Member

nurupo commented Jan 12, 2017

CMakeLists.txt, line 59 at r2 (raw file):

  add_cxxflag(${flag})
endmacro()
  

Remove the trailing whitespace.


Comments from Reviewable

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

Successfully merging this pull request may close these issues.

4 participants