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

Add missing DHT_bootstrap to CMakeLists.txt. #37

Merged
merged 1 commit into from
Aug 17, 2016
Merged

Conversation

iphydf
Copy link
Member

@iphydf iphydf commented Aug 17, 2016

  • This PR also adds a DEBUG cmake option that enables -DDEBUG.
  • We also remove -Wall, because there are too many warnings, and nobody really
    looks at them at the moment. We'll see about fixing them soon. We'll also want
    to enable -Werror at some point.
  • We also factor out nproc calls into the env scripts so we can use sysctl on OSX where nproc doesn't exist.
  • Finally, this PR enables -O3 to make sure toxcore still works correctly
    under heavy compiler optimisations.

This change is Reviewable

@nurupo
Copy link
Member

nurupo commented Aug 17, 2016

Given that tox-bootstrapd has got --foreground option, we might consider deprecating DHT_bootstrap.tox-bootstrapd should have same features DHT_bootstrap has and even more, at the down side of using a config file instead of command line arguments.


Review status: 0 of 5 files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.


CMakeLists.txt, line 28 [r1] (raw file):

option(DEBUG "Enable assertions and other debugging facilities" OFF)
if(DEBUG)
  add_definitions(-DDEBUG=1)

DEBUG is rather a common word. Are we sure that nothing is using it besides Tox libraries? e.g. no third party header files Tox libraries include use DEBUG and no compiler sets DEBUG for debug builds automatically? If we are not sure, TOX_DEBUG might be a safer bet. TOX_DEBUG would also match the naming of TOX_LOGGER. (No idea why LOGGER_LEVEL and LOGGER_OUTPUT_FILE are not prefixed with TOX_ though...)


CMakeLists.txt, line 120 [r1] (raw file):

  target_link_libraries(toxnetwork ${RT_LIBRARIES})
  set(toxcore_PKGCONFIG_LIBS ${toxcore_PKGCONFIG_LIBS} "-lrt")
endif()

Since this block adds linking libraries to toxnetwork, why not put this block right under the add_library(toxnetwork ${LIBTYPE}, instead of having it under toxdht.


Comments from Reviewable

@nurupo
Copy link
Member

nurupo commented Aug 17, 2016

Reviewed 4 of 5 files at r1.
Review status: 4 of 5 files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.


Comments from Reviewable

@iphydf iphydf force-pushed the cmake branch 2 times, most recently from d136840 to 93a857f Compare August 17, 2016 21:11
- This PR also adds a DEBUG cmake option that enables -DTOX_DEBUG.
- We also remove `-Wall`, because there are too many warnings, and nobody really
  looks at them at the moment. We'll see about fixing them soon. We'll also want
  to enable `-Werror` at some point.
- Finally, this PR enables `-O3` to make sure toxcore still works correctly
  under heavy compiler optimisations.
@iphydf
Copy link
Member Author

iphydf commented Aug 17, 2016

I'd like to do that in a separate PR. If you like, you can make a PR to do it.


Review status: 3 of 11 files reviewed at latest revision, 2 unresolved discussions.


CMakeLists.txt, line 28 [r1] (raw file):

Previously, nurupo wrote…

DEBUG is rather a common word. Are we sure that nothing is using it besides Tox libraries? e.g. no third party header files Tox libraries include use DEBUG and no compiler sets DEBUG for debug builds automatically? If we are not sure, TOX_DEBUG might be a safer bet. TOX_DEBUG would also match the naming of TOX_LOGGER. (No idea why LOGGER_LEVEL and LOGGER_OUTPUT_FILE are not prefixed with TOX_ though...)

Done.

CMakeLists.txt, line 120 [r1] (raw file):

Previously, nurupo wrote…

Since this block adds linking libraries to toxnetwork, why not put this block right under the add_library(toxnetwork ${LIBTYPE}, instead of having it under toxdht.

Done.

Comments from Reviewable

@nurupo
Copy link
Member

nurupo commented Aug 17, 2016

I was looking for a change in configure.ac that would do s/DEBUG/TOX_DEBUG/, but apparently we never had an option for enabling DEBUG in autotools build system? Did I miss it or it's really not there?


Reviewed 8 of 8 files at r2.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@iphydf
Copy link
Member Author

iphydf commented Aug 17, 2016

It doesn't exist. I looked for it as well.


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


Comments from Reviewable

@nurupo
Copy link
Member

nurupo commented Aug 17, 2016

:lgtm:


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


Comments from Reviewable

@GrayHatter
Copy link

:lgtm:

!! See comments in #36 near, but unrelated to changes made here !!


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


Comments from Reviewable

@iphydf iphydf merged commit db22522 into TokTok:master Aug 17, 2016
@iphydf iphydf deleted the cmake branch August 17, 2016 22:52
@iphydf iphydf modified the milestone: v0.0.1 Nov 6, 2016
zugz pushed a commit to zugz/c-toxcore that referenced this pull request Jan 2, 2018
This pull request was closed.
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.

3 participants