build: remove --enable-lcov-branch-coverage #30192

pull fanquake wants to merge 1 commits into bitcoin:master from fanquake:support_lcov_2 changing 3 files +11 −9
  1. fanquake commented at 2:28 pm on May 29, 2024: member

    This supports lcov 2.x in the sense that we are no-longer hardcoding version specific options, and instead will use the LCOV_OPTS variable (which is the more flexible thing to do in any case). It’s also quite likely that devs are already having to pass extra options to lcov 2.x, given it’s more stringent in terms of coverage generation and error checking. See this thread for an example: https://github.com/linux-test-project/lcov/issues/238.

    Tested on one machine (LCOV 2.0, gcc 13.2) with:

     0./autogen.sh
     1./configure --enable-lcov CXXFLAGS="-fprofile-update=prefer-atomic" LCOV_OPTS="--rc branch_coverage=1 --ignore-errors mismatch"
     2make
     3make cov
     4<snip>
     5Processing file src/netaddress.cpp
     6  lines=521 hit=480 functions=72 hit=72 branches=675 hit=499
     7Overall coverage rate:
     8  lines......: 81.8% (79362 of 97002 lines)
     9  functions......: 77.8% (10356 of 13310 functions)
    10  branches......: 49.6% (130628 of 263196 branches)
    

    and another machine (LCOV 2.1, Clang 18.1.3) with:

     0./autogen.sh
     1./configure --enable-lcov CC=clang CXX=clang++ LCOV_OPTS="--rc branch_coverage=1 --ignore-errors mismatch,inconsistent"
     2make
     3make cov
     4<snip>
     5    Processing file src/util/strencodings.cpp
     6      lines=315 hit=311 functions=38 hit=38 branches=425 hit=357
     7    Overall coverage rate:
     8      source files: 622
     9      lines.......: 79.8% (70311 of 88132 lines)
    10      functions...: 78.1% (13968 of 17881 functions)
    11      branches....: 44.5% (157551 of 354317 branches)
    12    Message summary:
    13      101 warning messages:
    14        count: 1
    15        inconsistent: 100
    16      3528 ignore messages:
    17        inconsistent: 3528
    

    Related to #28468.

  2. DrahtBot commented at 2:28 pm on May 29, 2024: contributor

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    Code Coverage

    For detailed information about the code coverage, see the test coverage report.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK theuni, hebasto
    Stale ACK maflcko

    If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

    Conflicts

    No conflicts as of last run.

  3. DrahtBot added the label Build system on May 29, 2024
  4. maflcko added the label Needs CMake port on May 29, 2024
  5. in doc/release-notes-xxxxx.md:1 in b08e070700


    maflcko commented at 2:41 pm on May 29, 2024:
    nit: filename

    fanquake commented at 2:44 pm on May 29, 2024:
    Updated
  6. maflcko commented at 2:41 pm on May 29, 2024: member
    utACK b08e07070085edf8cb5fda55d8849188fd83cfc6
  7. fanquake force-pushed on May 29, 2024
  8. hebasto approved
  9. hebasto commented at 3:11 pm on May 29, 2024: member
    ACK 462e761672d387c98e729e920a4102feaedc53cb. A similar approach was used in https://github.com/hebasto/bitcoin/pull/191.
  10. DrahtBot requested review from maflcko on May 29, 2024
  11. theuni commented at 4:16 pm on May 29, 2024: member

    I believe this only works because you’ve exported LCOV_OPTS and it makes it into the make env?

    If you use ./configure LCOV_OPTS="--rc branch_coverage=1" as you seem to be recommending:

    branch coverage can be enabled by setting LCOV_OPTS=… when configuring

    I don’t think it’d work.

    That could be fixed with a plain AC_SUBST(LCOV_OPTS), which would also document it.

  12. fanquake commented at 4:36 pm on May 29, 2024: member
    Right. I started this way, and then think I swapped to exporting because it was less annoying. Might actually just update the docs I’m adding, unless you have a preference?
  13. theuni commented at 4:43 pm on May 29, 2024: member
    I don’t like the idea of recommending undocumented vars, so I’d prefer the AC_SUBST unless you’re opposed.
  14. fanquake commented at 4:54 pm on May 29, 2024: member
    Ah yea, we won’t get it in help. I’ll update this tomorrow.
  15. build: remove --enable-lcov-branch-coverage
    This supports lcov 2.x in the sense that we are no-longer hardcoding
    version specific options, and instead will use the `LCOV_OPTS` variable
    (which is the more correct/flexible thing to do in any case). It's also
    quite likely that devs are already having to pass extra options to lcov
    2.x, given it's more stringent in terms of coverage generation and error
    checking. See this thread for an example:
    https://github.com/linux-test-project/lcov/issues/238.
    
    Added an example to the developer notes.
    
    Tested on one machine (LCOV 2.0, gcc 13.2) with:
    ```bash
    ./autogen.sh
    ./configure --enable-lcov CXXFLAGS="-fprofile-update=prefer-atomic" LCOV_OPTS="--rc branch_coverage=1 --ignore-errors mismatch"
    make
    make cov
    <snip>
    Processing file src/netaddress.cpp
      lines=521 hit=480 functions=72 hit=72 branches=675 hit=499
    Overall coverage rate:
      lines......: 81.8% (79362 of 97002 lines)
      functions......: 77.8% (10356 of 13310 functions)
      branches......: 49.6% (130628 of 263196 branches)
    ```
    
    and another machine (LCOV 2.1, Clang 18.1.3) with:
    ```bash
    ./autogen.sh
    ./configure --enable-lcov CC=clang CXX=clang++ LCOV_OPTS="--rc branch_coverage=1 --ignore-errors mismatch,inconsistent"
    make
    make cov
    <snip>
    Processing file src/util/strencodings.cpp
      lines=315 hit=311 functions=38 hit=38 branches=425 hit=357
    Overall coverage rate:
      source files: 622
      lines.......: 79.8% (70311 of 88132 lines)
      functions...: 78.1% (13968 of 17881 functions)
      branches....: 44.5% (157551 of 354317 branches)
    Message summary:
      101 warning messages:
        count: 1
        inconsistent: 100
      3528 ignore messages:
        inconsistent: 3528
    ```
    cbd4640ede
  16. fanquake force-pushed on May 30, 2024
  17. fanquake commented at 9:59 am on May 30, 2024: member
    Updated, rebased, and re-tested, also with LCOV 2.1 (built from source) (updated PR description).
  18. DrahtBot added the label CI failed on May 30, 2024
  19. theuni approved
  20. theuni commented at 4:58 pm on May 30, 2024: member
    utACK cbd4640ede92a1a5d7b7c1367eb7c00a9f476c62
  21. DrahtBot requested review from hebasto on May 30, 2024
  22. DrahtBot removed the label CI failed on May 30, 2024
  23. in doc/developer-notes.md:503 in cbd4640ede
    496@@ -497,6 +497,10 @@ make cov
    497 # unit and functional tests.
    498 ```
    499 
    500+Additional LCOV options can be specified using `LCOV_OPTS`, but may be dependant
    501+on the version of LCOV. For example, when using LCOV `2.x`, branch coverage can be
    502+enabled by setting `LCOV_OPTS="--rc branch_coverage=1"`, when configuring.
    503+
    


    hebasto commented at 3:43 pm on May 31, 2024:

    Not only “when configuring”. Providing LCOV_OPTS variable when building the cov or cov_fuzz targets also works and improves flexibility:

    0./configure --enable-lcov CXXFLAGS="-fprofile-update=prefer-atomic"
    1make
    2make cov LCOV_OPTS="--rc branch_coverage=1 --ignore-errors mismatch"
    

    fanquake commented at 3:49 pm on May 31, 2024:

    Not only “when configuring”.

    Sure. This is just one example.

  24. hebasto approved
  25. hebasto commented at 3:45 pm on May 31, 2024: member
    ACK cbd4640ede92a1a5d7b7c1367eb7c00a9f476c62, tested on Ubuntu 24.04.
  26. fanquake merged this on Jun 3, 2024
  27. fanquake closed this on Jun 3, 2024

  28. fanquake deleted the branch on Jun 3, 2024
  29. hebasto commented at 10:19 am on June 6, 2024: member
    CMake’s https://github.com/hebasto/bitcoin/pull/191 is built on top of this PR change.
  30. hebasto removed the label Build system on Jun 6, 2024
  31. hebasto removed the label Needs CMake port on Jun 6, 2024
  32. hebasto added the label Build system on Jun 6, 2024

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-09-28 22:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me