travis: Build with –enable-werror under OS X #10923

pull practicalswift wants to merge 1 commits into bitcoin:master from practicalswift:thread-safety-analysis changing 1 files +1 −1
  1. practicalswift commented at 7:34 am on July 25, 2017: contributor

    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.

  2. fanquake added the label Build system on Jul 25, 2017
  3. practicalswift renamed this:
    Use -Wthread-safety-analysis if available (+ -Werror=thread-safety-analysis if --enable-werror)
    Use -Wthread-safety-analysis if available (+ -Werror=[…] if --enable-werror)
    on Jul 25, 2017
  4. TheBlueMatt commented at 6:52 pm on July 25, 2017: member
    Looks good to me, though I’m confused, in #10866 you indicated that these checks should fail, but travis passed here?
  5. practicalswift commented at 10:54 pm on July 25, 2017: contributor

    @TheBlueMatt Yes, that is a bit weird. I verified this change locally using these four tests before submitting the PR:

     0$ CC=clang CXX=clang++ ./configure --enable-werror && make clean && make check; echo $?
 1…
 2# fails as expected:
 3Makefile:5576: recipe for target 'libbitcoin_server_a-net_processing.o' failed
 42
 5$ CC=clang CXX=clang++ ./configure && make clean && make check; echo $?
 6…
 7# passes as expected:
 80
 9$ CC=gcc CXX=g++ ./configure --enable-werror && make clean && make check; echo $?
10…
11# passes as expected:
120
13$ CC=gcc CXX=g++ ./configure && make clean && make check; echo $?
14…
15# passes as expected:
160
17$ clang++ --version | head -1
18clang version 3.8.0-2ubuntu4 (tags/RELEASE_380/final)

    Locally everything works as expected.

    Does the Travis CI build run with --enable-werror? I cannot find that in .travis.yml.

  6. MarcoFalke commented at 12:28 pm on July 30, 2017: member
    No, --enable-werror is currently not set on travis.
  7. TheBlueMatt commented at 2:23 pm on July 30, 2017: member
    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?).
  8. theuni commented at 3:43 pm on July 31, 2017: member

    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.

  9. TheBlueMatt commented at 3:00 pm on August 1, 2017: member
    Looks like the travis failure was from building fontconfig, not the expected annotation-missing failure. Is it unrelated?
  10. practicalswift commented at 3:07 pm on August 1, 2017: contributor
    @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?
  11. TheBlueMatt commented at 3:10 pm on August 1, 2017: member
    @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.
  12. practicalswift commented at 1:39 pm on August 15, 2017: contributor
    @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?
  13. TheBlueMatt commented at 7:07 pm on August 16, 2017: member
    @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)
  14. practicalswift commented at 8:09 pm on August 16, 2017: contributor

    @TheBlueMatt Oh, nice! What is the best way to proceed from here?

    Should the changing of sync.h primitives from boost to std be put in a separate PR of yours, or should I simply incorporate your changes into this PR?

    These are the changes: https://github.com/practicalswift/bitcoin/compare/thread-safety-analysis...TheBlueMatt:2017-08-test-10923

  15. TheBlueMatt commented at 1:47 am on August 18, 2017: member

    @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 notifications@github.com 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: https://github.com/bitcoin/bitcoin/pull/10923#issuecomment-322884443

  16. practicalswift commented at 7:54 am on August 18, 2017: contributor
    @TheBlueMatt What meeting? What am I missing? :-)
  17. practicalswift force-pushed on Aug 18, 2017
  18. practicalswift renamed this:
    Use -Wthread-safety-analysis if available (+ -Werror=[…] if --enable-werror)
    Use -Wthread-safety-analysis if available
    on Aug 18, 2017
  19. practicalswift commented at 2:06 pm on August 18, 2017: contributor
    @TheBlueMatt PR updated. Looks good? :-)
  20. TheBlueMatt commented at 9:40 pm on August 18, 2017: member
    @ptracticalswift the IRC meeting, Werror was discussed. Change the PR title?
  21. TheBlueMatt commented at 9:41 pm on August 18, 2017: member
    @practicalswift wait, huh, I meant just do Werror here, and the thread-safety-analysis thing can go in #10866.
  22. practicalswift force-pushed on Aug 19, 2017
  23. Build with --enable-werror under OS X a65e02803c
  24. practicalswift force-pushed on Aug 19, 2017
  25. practicalswift renamed this:
    Use -Wthread-safety-analysis if available
    Build with --enable-werror under OS X
    on Aug 19, 2017
  26. practicalswift commented at 4:30 pm on August 19, 2017: contributor
    @TheBlueMatt Got it! I haven’t attended any Bitcoin IRC channels/meetings - are there logs available somewhere? :-)
  27. fanquake commented at 5:01 am on August 20, 2017: member
    @practicalswift https://botbot.me/freenode/bitcoin-core-dev/
  28. TheBlueMatt commented at 0:15 am on August 21, 2017: member
    ACK
  29. jonasschnelli commented at 6:55 am on August 21, 2017: contributor
    ACK a65e02803cc76e9250f67a34c610234c263d4ab2
  30. laanwj renamed this:
    Build with --enable-werror under OS X
    travis: Build with --enable-werror under OS X
    on Aug 23, 2017
  31. laanwj commented at 10:00 am on August 23, 2017: member
    Prefixed the issue title with “travis:” otherwise it looks like all OSX builds will enable -werror, which is not the intent.
  32. laanwj merged this on Aug 23, 2017
  33. laanwj closed this on Aug 23, 2017
  34. laanwj referenced this in commit 2c9f5ecf3f on Aug 23, 2017
  35. MarcoFalke referenced this in commit 7027c67cac on Jul 2, 2020
  36. practicalswift deleted the branch on Apr 10, 2021
  37. vijaydasmp referenced this in commit dfa262c93f on Oct 4, 2021
  38. DrahtBot locked this on Aug 18, 2022


practicalswift TheBlueMatt MarcoFalke theuni fanquake jonasschnelli laanwj

Labels
Build system

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-07-01 10:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me