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).
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For detailed information about the code coverage, see the test coverage report.
See the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
🚧 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.
Prevents check-deps.sh errors reported by fanquake
https://github.com/bitcoin/bitcoin/pull/30415#issuecomment-2385475097
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.
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.
utACK fd38711
I suppose the alternative would be to move
netaddress.hto 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.mddocument is a bit vague on what belongs toutilvscommon. It just says “low level”. Since util is also used bylibbitcoin_kernelit could make sense to move things out of it that the kernel doesn’t need, but #28690 introduces a whole newkernel_utilfor 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.
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.
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.
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>
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>
netif.h here, it’s not actually used in pcp.cpp
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)
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).
