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

Remove libcheck from the dependencies. #766

Merged
merged 1 commit into from Feb 9, 2018
Merged

Conversation

iphydf
Copy link
Member

@iphydf iphydf commented Feb 5, 2018

We're not gaining much from this library, and it's a burden, especially
for windows development.


This change is Reviewable

@nurupo
Copy link
Member

nurupo commented Feb 6, 2018

Wow, libcheck looks kind of useless after reading this PR.
Are you sure it doesn't do more things behind the scenes?
Does this work with autotools just fine?


Review status: 0 of 28 files reviewed at latest revision, all discussions resolved, some commit checks failed.


Comments from Reviewable

@iphydf
Copy link
Member Author

iphydf commented Feb 6, 2018

What libcheck does behind the scenes:

  • If your test crashes (e.g. segfault), it tells you which assertion has been seen passing last, giving you a clue as to where the crash might have been. I.e. it's roughly doing printf debugging. gdb is a lot better at telling you where the crash happened.
  • If your test crashes (e.g. segfault), on Linux it tells you that it segfaulted and fails the test rather than aborting the whole process. On windows, it still aborts the whole process.

That's it.

@iphydf iphydf force-pushed the rm-libcheck branch 2 times, most recently from 072e21c to d21fa8f Compare February 6, 2018 15:34
@iphydf
Copy link
Member Author

iphydf commented Feb 6, 2018

I'm running a travis build with all the "if:"s commented out, so we can see if this works on all the builds.

@iphydf iphydf force-pushed the rm-libcheck branch 3 times, most recently from 4c48dc2 to f70fc79 Compare February 6, 2018 19:55
@iphydf
Copy link
Member Author

iphydf commented Feb 6, 2018

https://travis-ci.org/TokTok/c-toxcore/builds/338134142 - it works on all builds.

@iphydf iphydf force-pushed the rm-libcheck branch 2 times, most recently from ecc33ad to f8a6411 Compare February 6, 2018 20:19
@robinlinden
Copy link
Member

:lgtm_strong: (the check removal. Haven't reviewed the crypto_memcmp test commit.)


Reviewed 27 of 27 files at r2.
Review status: 27 of 32 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.


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

target_link_modules(toxnetwork toxcrypto)

message("CMAKE_THREAD_LIBS_INIT: ${CMAKE_THREAD_LIBS_INIT}")

Where did this come from?


Comments from Reviewable

@robinlinden
Copy link
Member

Reviewed 8 of 8 files at r1.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.


Comments from Reviewable

@iphydf iphydf force-pushed the rm-libcheck branch 3 times, most recently from 4f23fe8 to 3f6d1e5 Compare February 8, 2018 17:55
@iphydf iphydf force-pushed the rm-libcheck branch 2 times, most recently from 28c7b82 to 7ff713e Compare February 9, 2018 00:24
We're not gaining much from this library, and it's a burden, especially
for windows development.
@iphydf iphydf merged commit 6cd4fcd into TokTok:master Feb 9, 2018
@iphydf iphydf deleted the rm-libcheck branch February 9, 2018 21:05
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.

None yet

4 participants