refactor: move util/pcp and util/netif to common/ #31011

pull ryanofsky wants to merge 2 commits into bitcoin:master from ryanofsky:pr/checkcheck changing 9 files +17 −14
  1. ryanofsky commented at 1:19 pm on October 1, 2024: contributor

    Move util/pcp.cpp and util/netif.cpp to common/ because they depend on netaddress.cpp which is part of the common library. This was causing check-deps.sh script to fail as reported by fanquake in #30415 (comment).

    Also make CI fail when the check-deps.sh script fails. Previously it would output errors but not cause the job to fail (which was not intended).

  2. DrahtBot commented at 1:20 pm on October 1, 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 Sjors, laanwj, tdb3, achow101

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

  3. DrahtBot added the label Refactoring on Oct 1, 2024
  4. ryanofsky force-pushed on Oct 1, 2024
  5. DrahtBot commented at 1:27 pm on October 1, 2024: contributor

    🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/30915636360

    Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

    • Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

    • A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.

    • An intermittent issue.

    Leave a comment here, if you need help tracking down a confusing failure.

  6. DrahtBot added the label CI failed on Oct 1, 2024
  7. common: move pcp.cpp and netif.cpp files from util to common library since they depend on netaddress.cpp
    Prevents check-deps.sh errors reported by fanquake
    https://github.com/bitcoin/bitcoin/pull/30415#issuecomment-2385475097
    d51edecddc
  8. ci: make CI job fail when check-deps.sh script fails
    Previously the check-deps.sh would write information about unexpected
    dependencies to stderr, but return exit code 0, so the error would be ignored
    by CI. Now it will return code 1 and cause CI to fail if unexpected
    dependencies are detected.
    fd38711217
  9. ryanofsky force-pushed on Oct 1, 2024
  10. Sjors commented at 2:44 pm on October 1, 2024: member

    utACK fd38711217cafbd62e8abd22d2b43f85fede8cde

    I suppose the alternative would be to move netaddress.h to the util library.

    The libraries.md document is a bit vague on what belongs to util vs common. It just says “low level”. Since util is also used by libbitcoin_kernel it could make sense to move things out of it that the kernel doesn’t need, but #28690 introduces a whole new kernel_util for that purpose.

  11. ryanofsky commented at 3:24 pm on October 1, 2024: contributor

    utACK fd38711

    I suppose the alternative would be to move netaddress.h to the util library.

    I think that might work, but I didn’t look into it because I don’t know if netaddress.h depends on other things in common that would require more dependencies to be moved. Also the pcp/netif files were added recently in #30043, while netaddress.h is older so I assume it is more likely netaddress is in the place where people want it to be.

    The libraries.md document is a bit vague on what belongs to util vs common. It just says “low level”. Since util is also used by libbitcoin_kernel it could make sense to move things out of it that the kernel doesn’t need, but #28690 introduces a whole new kernel_util for that purpose.

    Both are just collections of library functions shared by various executables. I think it util is a good place for things that are general purpose and can be broadly useful, and common is a better place for things that are hacky or more specific to the bitcoin core codebase. Also if something is used by kernel code it must go in util, not common.

    I tend to think the current implementation of #28690 adding a kernel_util library just adds complexity and is not a good idea. I would prefer it if kernel just used the regular util library, and provided a few extra utilities outside developers could use or not use, instead of feeling the need to strip out everything that isn’t used internally.

  12. Sjors commented at 4:37 pm on October 1, 2024: member

    I think it util is a good place for things that are general purpose and can be broadly useful, and common is a better place for things that are hacky or more specific to the bitcoin core codebase.

    The PCP code is not Bitcoin specific. Some things in netaddress.h are though, and tearing that file apart doesn’t seem worth the effort for now.

  13. laanwj commented at 6:17 pm on October 1, 2024: member

    Untested ACK fd38711217cafbd62e8abd22d2b43f85fede8cde

    The libraries.md document is a bit vague on what belongs to util vs common. It just says “low level”

    Yes, i truly had no idea what to do here. Glad the CI catches it after this.

  14. in src/common/pcp.cpp:7 in d51edecddc outdated
    1@@ -2,16 +2,16 @@
    2 // Distributed under the MIT software license, see the accompanying
    3 // file COPYING or https://www.opensource.org/licenses/mit-license.php.
    4 
    5-#include <util/pcp.h>
    6+#include <common/pcp.h>
    7 
    8+#include <common/netif.h>
    


    tdb3 commented at 0:34 am on October 2, 2024:

    femto nit. Ignore if not already updating the file for something else.

    0- #include <common/pcp.h>
    1- #include <common/netif.h>
    2+ #include <common/netif.h>
    3+ #include <common/pcp.h>
    

    laanwj commented at 8:27 am on October 2, 2024:
    even better, can remove the include of netif.h here, it’s not actually used in pcp.cpp

    maflcko commented at 8:48 am on October 2, 2024:

    For reference, according to the iwyu run by CI, as part of the tidy task:

     0[13:49:05.974] /ci_container_base/src/common/pcp.cpp should add these lines:
     1[13:49:05.974] #include <algorithm>            // for __equal_fn, equal
     2[13:49:05.974] #include <compare>              // for operator<, strong_ordering
     3[13:49:05.974] #include <cstring>              // for size_t, memcpy
     4[13:49:05.974] #include <functional>           // for function
     5[13:49:05.974] #include <map>                  // for map, operator==, _Rb_tree_const_iterator
     6[13:49:05.974] #include <memory>               // for unique_ptr
     7[13:49:05.974] #include <optional>             // for optional, nullopt, nullopt_t
     8[13:49:05.974] #include <span>                 // for span
     9[13:49:05.974] #include <utility>              // for pair
    10[13:49:05.974] #include <vector>               // for vector
    11[13:49:05.974] #include "compat/compat.h"      // for sockaddr_storage, WSAGetLastError, in_addr, socklen_t, in6_addr, sockaddr_in, AF_INET, IPPROTO_UDP, SOCK_DGRAM, MSG_D...
    12[13:49:05.974] #include "tinyformat.h"         // for format, strprintf
    13[13:49:05.974] #include "util/string.h"        // for string, basic_string, HasPrefix
    14[13:49:05.974] 
    15[13:49:05.974] /ci_container_base/src/common/pcp.cpp should remove these lines:
    16[13:49:05.974] - #include <common/netif.h>  // lines 7-7
    17[13:49:05.974] - #include <random.h>  // lines 12-12
    18[13:49:05.974] - #include <util/readwritefile.h>  // lines 15-15
    

    (Just sharing, no need to pick this up here. A future change that enforces iwyu on the codebase will fix it either way)

  15. tdb3 approved
  16. tdb3 commented at 1:03 am on October 2, 2024: contributor

    ACK fd38711217cafbd62e8abd22d2b43f85fede8cde

    Ran a sanity check: Cherry-pick’ed fd38711217cafbd62e8abd22d2b43f85fede8cde into master (intentionally without d51edecddcb7fa52349c8aa5ef88b01f72be44c7), ran check-deps.sh, and verified (with echo $?) that the script exited with 1 when the errors occurred (vs 0 on master without the cherry-pick’ed commit).

  17. maflcko removed the label CI failed on Oct 2, 2024
  18. achow101 commented at 10:31 pm on October 2, 2024: member
    ACK fd38711217cafbd62e8abd22d2b43f85fede8cde
  19. achow101 merged this on Oct 2, 2024
  20. achow101 closed this on Oct 2, 2024

  21. hebasto commented at 10:12 am on October 4, 2024: member
    Post-merge ACK fd38711217cafbd62e8abd22d2b43f85fede8cde.

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-10-08 16:12 UTC

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