📝 l0rinc opened a pull request: "doc: Add `nproc` support for Mac through `coreutils`"
(https://github.com/bitcoin/bitcoin/pull/30936)
See: https://www.gnu.org/software/coreutils/manual/html_node/nproc-invocation.html
Revival of a failed attempt in https://github.com/bitcoin/bitcoin/pull/30619
You can test on your mac via:
```bash
% echo $(nproc)
command not found: nproc
```
```bash
% brew install coreutils
...
% echo $(nproc)
10
```
(https://github.com/bitcoin/bitcoin/pull/30936)
See: https://www.gnu.org/software/coreutils/manual/html_node/nproc-invocation.html
Revival of a failed attempt in https://github.com/bitcoin/bitcoin/pull/30619
You can test on your mac via:
```bash
% echo $(nproc)
command not found: nproc
```
```bash
% brew install coreutils
...
% echo $(nproc)
10
```
💬 l0rinc commented on pull request "doc: Change `nproc` in docs to `getconf _NPROCESSORS_ONLN`":
(https://github.com/bitcoin/bitcoin/pull/30619#issuecomment-2363312549)
Yeah, I hated how verbose that was.
But did a search again and it seems `coreutils` adds `nproc` support, so pushed a PR adding that in the OSx docs: https://github.com/bitcoin/bitcoin/pull/30936/files
(https://github.com/bitcoin/bitcoin/pull/30619#issuecomment-2363312549)
Yeah, I hated how verbose that was.
But did a search again and it seems `coreutils` adds `nproc` support, so pushed a PR adding that in the OSx docs: https://github.com/bitcoin/bitcoin/pull/30936/files
💬 maflcko commented on pull request "test: Prove+document ConstevalFormatString/tinyformat parity":
(https://github.com/bitcoin/bitcoin/pull/30933#discussion_r1768317863)
> I'll be happy to remove them once the linter is no longer run on CI.
That seems like extra churn and merge conflicts for no good reason. It should be trivial to avoid adding lines to this file. For example a simple `#define tfm_fmt tfm::format` in the test (and using it) should avoid merge conflicts with future changes, or at least make them one-line conflicts at most.
(https://github.com/bitcoin/bitcoin/pull/30933#discussion_r1768317863)
> I'll be happy to remove them once the linter is no longer run on CI.
That seems like extra churn and merge conflicts for no good reason. It should be trivial to avoid adding lines to this file. For example a simple `#define tfm_fmt tfm::format` in the test (and using it) should avoid merge conflicts with future changes, or at least make them one-line conflicts at most.
💬 maflcko commented on pull request "test: Prove+document ConstevalFormatString/tinyformat parity":
(https://github.com/bitcoin/bitcoin/pull/30933#discussion_r1768321462)
This should also ensure that all cases are covered and wouldn't require to change the tests vectors.
If you don't want to take it, that is fine. However, it would be good to at least terminate in the default case instead of silently passing. Otherwise it is unclear that all cases are covered.
(https://github.com/bitcoin/bitcoin/pull/30933#discussion_r1768321462)
This should also ensure that all cases are covered and wouldn't require to change the tests vectors.
If you don't want to take it, that is fine. However, it would be good to at least terminate in the default case instead of silently passing. Otherwise it is unclear that all cases are covered.
💬 fanquake commented on pull request "doc: Add `nproc` support for Mac through `coreutils`":
(https://github.com/bitcoin/bitcoin/pull/30936#issuecomment-2363327675)
Cnocept nack. This is something a user can do on their own system if they want to use nproc generally. However it's not a requirement for building bitcoind.
(https://github.com/bitcoin/bitcoin/pull/30936#issuecomment-2363327675)
Cnocept nack. This is something a user can do on their own system if they want to use nproc generally. However it's not a requirement for building bitcoind.
💬 fanquake commented on pull request "build: drop obj/ subdirectory for generated build.h":
(https://github.com/bitcoin/bitcoin/pull/30856#issuecomment-2363333121)
Guix build (x86_64):
```bash
f65f7839cb8ce4b194ab55a698b9840dd335d790a5499d3303cf79885405bedb guix-build-7025942687fd/output/aarch64-linux-gnu/SHA256SUMS.part
7953c080586b3500c73edd93731b8b9bcfd79ac4f5fe7953d51dc54643899255 guix-build-7025942687fd/output/aarch64-linux-gnu/bitcoin-7025942687fd-aarch64-linux-gnu-debug.tar.gz
c82339ac1813727df996deda765087d430080a62d5fff7e16f46727be703a68c guix-build-7025942687fd/output/aarch64-linux-gnu/bitcoin-7025942687fd-aarch64-linux-gnu.tar.gz
ffe2b02
...
(https://github.com/bitcoin/bitcoin/pull/30856#issuecomment-2363333121)
Guix build (x86_64):
```bash
f65f7839cb8ce4b194ab55a698b9840dd335d790a5499d3303cf79885405bedb guix-build-7025942687fd/output/aarch64-linux-gnu/SHA256SUMS.part
7953c080586b3500c73edd93731b8b9bcfd79ac4f5fe7953d51dc54643899255 guix-build-7025942687fd/output/aarch64-linux-gnu/bitcoin-7025942687fd-aarch64-linux-gnu-debug.tar.gz
c82339ac1813727df996deda765087d430080a62d5fff7e16f46727be703a68c guix-build-7025942687fd/output/aarch64-linux-gnu/bitcoin-7025942687fd-aarch64-linux-gnu.tar.gz
ffe2b02
...
💬 maflcko commented on pull request "Embed default ASMap as binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#discussion_r1768328158)
> missing header guard
That was somewhat intentional, I guess. A header guard is really only useful for module headers to avoid compile errors if the module is included several times transitively/recursively.
However, for data-holding headers, the include should only ever happen in a single place in the whole program. Otherwise it seems too easy to bloat the binary. So a compile error in that case is actually intentional and useful.
(https://github.com/bitcoin/bitcoin/pull/28792#discussion_r1768328158)
> missing header guard
That was somewhat intentional, I guess. A header guard is really only useful for module headers to avoid compile errors if the module is included several times transitively/recursively.
However, for data-holding headers, the include should only ever happen in a single place in the whole program. Otherwise it seems too easy to bloat the binary. So a compile error in that case is actually intentional and useful.
🚀 fanquake merged a pull request: "build: drop obj/ subdirectory for generated build.h"
(https://github.com/bitcoin/bitcoin/pull/30856)
(https://github.com/bitcoin/bitcoin/pull/30856)
💬 hodlinator commented on pull request "test: Prove+document ConstevalFormatString/tinyformat parity":
(https://github.com/bitcoin/bitcoin/pull/30933#discussion_r1768337929)
Current approach:
* Linter is deleted by another PR - merge conflict has obvious resolution.
Suggested approach:
* Adding `#define`'s to temporarily work around a linter that is about to be removed, only to clean it up in another change seems like extra churn.
Regardless:
* Possible need to call other `tfm::*format*` function based on #30928.
Maybe I'm missing something.
I'm not in a hurry to get this merged before other PRs in this area. Happy move this to draft for now so other
...
(https://github.com/bitcoin/bitcoin/pull/30933#discussion_r1768337929)
Current approach:
* Linter is deleted by another PR - merge conflict has obvious resolution.
Suggested approach:
* Adding `#define`'s to temporarily work around a linter that is about to be removed, only to clean it up in another change seems like extra churn.
Regardless:
* Possible need to call other `tfm::*format*` function based on #30928.
Maybe I'm missing something.
I'm not in a hurry to get this merged before other PRs in this area. Happy move this to draft for now so other
...
📝 hodlinator converted_to_draft a pull request: "test: Prove+document ConstevalFormatString/tinyformat parity"
(https://github.com/bitcoin/bitcoin/pull/30933)
Makes unequivocally clear for which type of format strings we do have parity between the two, and which we (currently) don't.
Broken out from #30546 based on https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1755013263 and https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1756495304.
(https://github.com/bitcoin/bitcoin/pull/30933)
Makes unequivocally clear for which type of format strings we do have parity between the two, and which we (currently) don't.
Broken out from #30546 based on https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1755013263 and https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1756495304.
💬 maflcko commented on pull request "doc: Add `nproc` support for Mac through `coreutils`":
(https://github.com/bitcoin/bitcoin/pull/30936#issuecomment-2363354174)
No objection to adding this, but just installing it and then leaving the docs in this file as `# Use "-j N" here for N parallel jobs.` doesn't seem too useful for most users, assuming that devs already have it installed anyway (or an alternative).
(https://github.com/bitcoin/bitcoin/pull/30936#issuecomment-2363354174)
No objection to adding this, but just installing it and then leaving the docs in this file as `# Use "-j N" here for N parallel jobs.` doesn't seem too useful for most users, assuming that devs already have it installed anyway (or an alternative).
💬 fanquake commented on pull request "doc: Add `nproc` support for Mac through `coreutils`":
(https://github.com/bitcoin/bitcoin/pull/30936#issuecomment-2363362007)
I think the only place this might make sense would be in the developer documentation. I don't think we should be adding additional things as base dependencies for os specific instructions, just so someone can call a utility. It's also less relevant, given devs may already be using generators that are handling this by default, i.e Ninja.
(https://github.com/bitcoin/bitcoin/pull/30936#issuecomment-2363362007)
I think the only place this might make sense would be in the developer documentation. I don't think we should be adding additional things as base dependencies for os specific instructions, just so someone can call a utility. It's also less relevant, given devs may already be using generators that are handling this by default, i.e Ninja.
💬 fanquake commented on pull request "doc: cmake: prepend "build" to functional/test_runner.py":
(https://github.com/bitcoin/bitcoin/pull/30859#discussion_r1768352268)
2 lines above, this already says `Assuming the build directory is named build,`
(https://github.com/bitcoin/bitcoin/pull/30859#discussion_r1768352268)
2 lines above, this already says `Assuming the build directory is named build,`
💬 fanquake commented on pull request "doc: cmake: prepend "build" to functional/test_runner.py":
(https://github.com/bitcoin/bitcoin/pull/30859#discussion_r1768350617)
Same here. I don't think we need to duplicate `(assuming build is your build directory)` 10 lines apart. Especially when all the build commands here used `build` as the build dir.
(https://github.com/bitcoin/bitcoin/pull/30859#discussion_r1768350617)
Same here. I don't think we need to duplicate `(assuming build is your build directory)` 10 lines apart. Especially when all the build commands here used `build` as the build dir.
💬 fanquake commented on pull request "doc: cmake: prepend "build" to functional/test_runner.py":
(https://github.com/bitcoin/bitcoin/pull/30859#discussion_r1768350553)
You're adding `(assuming build is your build directory)` here, but 3 lines up it already says `Assuming the build directory is named build,`.
(https://github.com/bitcoin/bitcoin/pull/30859#discussion_r1768350553)
You're adding `(assuming build is your build directory)` here, but 3 lines up it already says `Assuming the build directory is named build,`.
💬 fanquake commented on pull request "doc: cmake: prepend "build" to functional/test_runner.py":
(https://github.com/bitcoin/bitcoin/pull/30859#discussion_r1768351361)
You can't modify the subtree here.
(https://github.com/bitcoin/bitcoin/pull/30859#discussion_r1768351361)
You can't modify the subtree here.
💬 l0rinc commented on pull request "Drop -dbcache limit":
(https://github.com/bitcoin/bitcoin/pull/28358#issuecomment-2363371383)
Unsurprisingly (should we document this?), setting `-prune` makes the `-dbcache` value less important:
<details>
<summary>benchmark</summary>
```bash
// 6d7f24595b08b8d1eba53e648533bcf87c30b48f
hyperfine \
--runs 2 \
--export-json /mnt/ibd_dbcache_prune.json \
--parameter-list DBCACHE 10240,16384,20480,30720,40960 \
--prepare 'rm -rf /mnt/BitcoinData/*' \
'./build/src/bitcoind -datadir=/mnt/BitcoinData -printtoconsole=0 -stopatheight=860000 -prune=550 -dbcache={DBCACHE}'
```
</
...
(https://github.com/bitcoin/bitcoin/pull/28358#issuecomment-2363371383)
Unsurprisingly (should we document this?), setting `-prune` makes the `-dbcache` value less important:
<details>
<summary>benchmark</summary>
```bash
// 6d7f24595b08b8d1eba53e648533bcf87c30b48f
hyperfine \
--runs 2 \
--export-json /mnt/ibd_dbcache_prune.json \
--parameter-list DBCACHE 10240,16384,20480,30720,40960 \
--prepare 'rm -rf /mnt/BitcoinData/*' \
'./build/src/bitcoind -datadir=/mnt/BitcoinData -printtoconsole=0 -stopatheight=860000 -prune=550 -dbcache={DBCACHE}'
```
</
...
💬 hodlinator commented on pull request "test: Prove+document ConstevalFormatString/tinyformat parity":
(https://github.com/bitcoin/bitcoin/pull/30933#discussion_r1768357254)
Done in 2955b1a1f3eec934960e880963a09b359d828721.
(https://github.com/bitcoin/bitcoin/pull/30933#discussion_r1768357254)
Done in 2955b1a1f3eec934960e880963a09b359d828721.
💬 l0rinc commented on pull request "doc: Add `nproc` support for Mac through `coreutils`":
(https://github.com/bitcoin/bitcoin/pull/30936#issuecomment-2363381454)
Thanks for the comments, would it make sense to add this to the mac developer notes instead, or should we remove the `-j N` docs, since people are migrating to Ninja anyway?
This stupid `nproc` problem comes up often for macs, was hoping we can finally get rid of it...
(https://github.com/bitcoin/bitcoin/pull/30936#issuecomment-2363381454)
Thanks for the comments, would it make sense to add this to the mac developer notes instead, or should we remove the `-j N` docs, since people are migrating to Ninja anyway?
This stupid `nproc` problem comes up often for macs, was hoping we can finally get rid of it...
📝 fanquake unlocked a pull request: "refactor, kernel: Remove gArgs accesses from dbwrapper and txdb"
(https://github.com/bitcoin/bitcoin/pull/25862)
Code in the libbitcoin_kernel library should not be calling `ArgsManager` methods or trying to read options from the command line. Instead it should just get options values from simple structs and function arguments that are passed in externally. This PR removes `gArgs` accesses from `dbwrapper` and `txdb` modules by defining appropriate options structs, and is a followup to PR's #25290 #25487 #25527 which remove other `ArgsManager` calls from kernel modules.
This PR does not change behavior
...
(https://github.com/bitcoin/bitcoin/pull/25862)
Code in the libbitcoin_kernel library should not be calling `ArgsManager` methods or trying to read options from the command line. Instead it should just get options values from simple structs and function arguments that are passed in externally. This PR removes `gArgs` accesses from `dbwrapper` and `txdb` modules by defining appropriate options structs, and is a followup to PR's #25290 #25487 #25527 which remove other `ArgsManager` calls from kernel modules.
This PR does not change behavior
...