refactor: Split logging utilities from system.h

TheCharlatan commented at 10:59 am on March 10, 2023: contributor

This pull request is part of the libbitcoinkernel project #24303 https://github.com/bitcoin/bitcoin/projects/18 and more specifically its “Step 2: Decouple most non-consensus code from libbitcoinkernel”. These commits were originally authored by empact and are taken from their parent PR #25152.

Context

There is an ongoing effort to decouple the ArgsManager used for command line parsing user-provided arguments from the libbitcoinkernel library (https://github.com/bitcoin/bitcoin/pull/25290, #25487, #25527, #25862, #26177, and #27125). The ArgsManager is defined in system.h.

Changes

Next to providing better code organization, this PR removes some reliance of the tree of libbitcoinkernel header includes on system.h (and thus the ArgsManager definition) by moving some logging functions out of the system.* files.

Further commits splitting more functionality out of system.h are still in #25152 and will be submitted in separate PRs once this PR has been processed.

DrahtBot commented at 10:59 am on March 10, 2023: contributor

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	MarcoFalke
Concept ACK	fanquake
Stale ACK	davidgumberg

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#26177 (refactor / kernel: Move non-gArgs chainparams functionality to kernel by TheCharlatan)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

DrahtBot added the label Refactoring on Mar 10, 2023

TheCharlatan marked this as a draft on Mar 10, 2023

TheCharlatan force-pushed on Mar 10, 2023

TheCharlatan marked this as ready for review on Mar 10, 2023

fanquake commented at 2:20 pm on March 10, 2023: member

Concept ACK

davidgumberg commented at 2:51 pm on March 11, 2023: contributor

ACK https://github.com/bitcoin/bitcoin/pull/27238/commits/485ea8d400241506a6abe485b7b71b0cacfdc853

in src/util/exception.cpp:8 in 4aa0ec6a86 outdated

0@@ -0,0 +1,40 @@
1+// Copyright (c) 2009-2010 Satoshi Nakamoto
2+// Copyright (c) 2009-2023 The Bitcoin Core developers
3+// Distributed under the MIT software license, see the accompanying
4+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
5+
6+#include <logging.h>
7+#include <tinyformat.h>
8+#include <util/exception.h>

maflcko commented at 3:36 pm on March 13, 2023:

should be on the first line and separate section to catch missing includes in the header

maflcko approved

maflcko commented at 3:38 pm on March 13, 2023: member

review ACK 4aa0ec6a8617ac77a19e49d8cf081b7a7f6404ae 🐸

Signature:

0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: review ACK 4aa0ec6a8617ac77a19e49d8cf081b7a7f6404ae 🐸
3Q9UDjbl3uLbiI3hg8GAjCLa+KcpMXFHY4zmCz2T39dQ6BsSrY7132Xgo9wCcDbaRQQsRJs5AfEjE9uCDi9P0AA==

refactor: Extract util/exception from util/system

This is a minimal extraction of a single function, but also the only use
of std::exception in util/system.

The background of this commit is an ongoing effort to decouple the
libbitcoinkernel library from the ArgsManager defined in system.h.
Moving the function out of system.h allows including it from a separate
source file without including the ArgsManager definitions from system.h.

e7333b420e

refactor: Move error() from util/system.h to logging.h

error is a low-level function with a sole dependency on LogPrintf, which
is defined in logging.h

The background of this commit is an ongoing effort to decouple the
libbitcoinkernel library from the ArgsManager defined in system.h.
Moving the function out of system.h allows including it from a separate
source file without including the ArgsManager definitions from system.h.

aaced5633b

TheCharlatan force-pushed on Mar 13, 2023

TheCharlatan commented at 4:32 pm on March 13, 2023: contributor

Updated 4aa0ec6 -> aaced5633b81b2f08b993106a527e2a0e1d663c1 (splitSystemLogging_0 -> splitSystemLogging_1, compare) to fix header inclusion order addressing @MarcoFalke’s #27238#pullrequestreview-1337368491.

maflcko commented at 4:35 pm on March 13, 2023: member

re-ACK aaced5633b81b2f08b993106a527e2a0e1d663c1 🐍

Signature:

0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: re-ACK aaced5633b81b2f08b993106a527e2a0e1d663c1 🐍
3qOnBQIyWxM4Wdga+t5Kp/TH3mEC8A8Rq0S+G4hMBYPBENLw7mwmhC9r3o1wUw9NWbmakHCuycScGpqGf0603Bg==

fanquake merged this on Mar 14, 2023

fanquake closed this on Mar 14, 2023

sidhujag referenced this in commit 10db93172b on Mar 14, 2023

Empact commented at 5:37 pm on March 14, 2023: member

Thanks @TheCharlatan for picking this up. 👍

sidhujag referenced this in commit 1b6e781d2e on Mar 15, 2023

fanquake referenced this in commit 369d4c03b7 on Apr 3, 2023

fanquake referenced this in commit 9564f98fee on May 30, 2023

PastaPastaPasta referenced this in commit ba97f49f2f on Nov 17, 2023

bitcoin locked this on Mar 13, 2024

refactor: Split logging utilities from system.h #27238

Context

Changes

Reviews

Conflicts