-
Notifications
You must be signed in to change notification settings - Fork 37.7k
travis: Build with --enable-werror under OS X #10923
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
Conversation
Looks good to me, though I'm confused, in #10866 you indicated that these checks should fail, but travis passed here? |
@TheBlueMatt Yes, that is a bit weird. I verified this change locally using these four tests before submitting the PR: $ CC=clang CXX=clang++ ./configure --enable-werror && make clean && make check; echo $?
…
# fails as expected:
Makefile:5576: recipe for target 'libbitcoin_server_a-net_processing.o' failed
2
$ CC=clang CXX=clang++ ./configure && make clean && make check; echo $?
…
# passes as expected:
0
$ CC=gcc CXX=g++ ./configure --enable-werror && make clean && make check; echo $?
…
# passes as expected:
0
$ CC=gcc CXX=g++ ./configure && make clean && make check; echo $?
…
# passes as expected:
0
$ clang++ --version | head -1
clang version 3.8.0-2ubuntu4 (tags/RELEASE_380/final) Locally everything works as expected. Does the Travis CI build run with |
No, |
Great! Lets enable it by default at least on the osx build. Its deliberately heavily limited so no reason not to (also maybe do it by default, @theuni?). |
Yea, I like this! My only concern is that the annotations are infectious, so this may cause us to have to add them while doing large refactors where one annotation already exists. That's a good problem to have though, IMO. utACK after #10866 goes in. |
Looks like the travis failure was from building fontconfig, not the expected annotation-missing failure. Is it unrelated? |
@TheBlueMatt Yes it appears to fail before hitting expected annotation-missing failure. I'm not sure about the root cause here. Can we as a first step re-trigger the Travis CI build? |
@practicalswift OK, it has been kicked, I dont know how building fontconfig would have failed because of this, but hopefully its deterministic if it is an issue with this PR. |
@TheBlueMatt Travis CI now complains about a locking issue but it is not the locking issue I'm encountering locally (locally I'm getting the error described in #10866). Is the locking issue reported by Travis a false positive? |
@practicalswift hmm, well I got it to work by changing the sync.h primitives to std from boost (see https://travis-ci.org/TheBlueMatt/bitcoin/jobs/265262602 and https://github.com/TheBlueMatt/bitcoin/commits/2017-08-test-10923) |
@TheBlueMatt Oh, nice! What is the best way to proceed from here? Should the changing of These are the changes: practicalswift/bitcoin@thread-safety-analysis...TheBlueMatt:2017-08-test-10923 |
@practicalswift as per meeting split the "enable werror on Travis" part from the Wthread-safety-analysis part. Maybe just leave this PR as the first and move the second back to the did-thread-safety-analysis pr?
…On August 16, 2017 4:09:42 PM EDT, practicalswift ***@***.***> wrote:
@TheBlueMatt Oh, nice! What is the best way to proceed from here? Shall
I incorporate your changes into this PR or should I cherry-pick the
individual commits?
--
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
#10923 (comment)
|
@TheBlueMatt What meeting? What am I missing? :-) |
9df8538
to
e1abd17
Compare
@TheBlueMatt PR updated. Looks good? :-) |
@ptracticalswift the IRC meeting, Werror was discussed. Change the PR title? |
@practicalswift wait, huh, I meant just do Werror here, and the thread-safety-analysis thing can go in #10866. |
e1abd17
to
a7433f0
Compare
a7433f0
to
a65e028
Compare
@TheBlueMatt Got it! I haven't attended any Bitcoin IRC channels/meetings - are there logs available somewhere? :-) |
ACK |
ACK a65e028 |
Prefixed the issue title with "travis:" otherwise it looks like all OSX builds will enable -werror, which is not the intent. |
a65e028 Build with --enable-werror under OS X (practicalswift) Pull request description: Build with `--enable-werror` under OS X. As suggested by @TheBlueMatt in #10866. This will allow catching violations using Travis CI which does a `clang` build for OS X. Tree-SHA512: 326248897e0776106983e0824e7e80eee3c6e584a1d360f429c30f3375dad83ab4c360c86ab0729bd9ede747ea0caa13cd6a7c35072ed9b845362423c9c37a64
…se of uninitialized memory 870f0cd build: Add MemorySanitizer (MSan) in Travis to detect use of uninitialized memory (practicalswift) Pull request description: Add MemorySanitizer (MSan) in Travis to detect use of uninitialized memory. First UBSan, then ASan followed by TSan... and now: yes, the wait is over -- **MSan is finally here!** :) Some historical context: * 2017: Continuous compilation with Clang Thread Safety analysis enabled (#10866, #10923) * 2018: Continuous testing with trapping on signed integer overflows (`-ftrapv`) (#12686) * 2018: Continuous testing of use of locale dependent functions (#13041) * 2018: Continuous testing of format strings (#13705) * 2018: Continuous compilation with MSVC `TreatWarningAsError` (#14151) * 2018: Continuous testing under UndefinedBehaviorSanitizer – UBSan (#14252, #14673, #17006) * 2018: Continuous testing under AddressSanitizer – ASan (#14794, #17205, #17674) * 2018: Continuous testing under ThreadSanitizer – TSan (#14829) * 2019: Continuous testing in an unsigned char environment (`-funsigned-char`) (#15134) * 2019: Continuous compile-time testing of assumptions we're making (#15391) * 2019: Continuous testing of fuzz test cases under Valgrind (#17633, #18159, #18166) * 2020: Finally... MemorySanitizer – MSAN! :) What is the next step? What tools should we add to CI to keep bugs from entering `master`? :) ACKs for top commit: MarcoFalke: ACK 870f0cd Tree-SHA512: 38327c8b75679d97d469fe42e704cacd1217447a5a603701dd8a58ee50b3be2c10248f8d68a479ed081c0c4b254589d3081c9183f991640b06ef689061f75578
…etect use of uninitialized memory 870f0cd build: Add MemorySanitizer (MSan) in Travis to detect use of uninitialized memory (practicalswift) Pull request description: Add MemorySanitizer (MSan) in Travis to detect use of uninitialized memory. First UBSan, then ASan followed by TSan... and now: yes, the wait is over -- **MSan is finally here!** :) Some historical context: * 2017: Continuous compilation with Clang Thread Safety analysis enabled (bitcoin#10866, bitcoin#10923) * 2018: Continuous testing with trapping on signed integer overflows (`-ftrapv`) (bitcoin#12686) * 2018: Continuous testing of use of locale dependent functions (bitcoin#13041) * 2018: Continuous testing of format strings (bitcoin#13705) * 2018: Continuous compilation with MSVC `TreatWarningAsError` (bitcoin#14151) * 2018: Continuous testing under UndefinedBehaviorSanitizer – UBSan (bitcoin#14252, bitcoin#14673, bitcoin#17006) * 2018: Continuous testing under AddressSanitizer – ASan (bitcoin#14794, bitcoin#17205, bitcoin#17674) * 2018: Continuous testing under ThreadSanitizer – TSan (bitcoin#14829) * 2019: Continuous testing in an unsigned char environment (`-funsigned-char`) (bitcoin#15134) * 2019: Continuous compile-time testing of assumptions we're making (bitcoin#15391) * 2019: Continuous testing of fuzz test cases under Valgrind (bitcoin#17633, bitcoin#18159, bitcoin#18166) * 2020: Finally... MemorySanitizer – MSAN! :) What is the next step? What tools should we add to CI to keep bugs from entering `master`? :) ACKs for top commit: MarcoFalke: ACK 870f0cd Tree-SHA512: 38327c8b75679d97d469fe42e704cacd1217447a5a603701dd8a58ee50b3be2c10248f8d68a479ed081c0c4b254589d3081c9183f991640b06ef689061f75578
Build with
--enable-werror
under OS X.As suggested by @TheBlueMatt in #10866. This will allow catching violations using Travis CI which does a
clang
build for OS X.