Skip to content

scripted-diff: Use LogInfo over LogPrintf [WIP, NOMERGE, DRAFT] #29641

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft

scripted-diff: Use LogInfo over LogPrintf [WIP, NOMERGE, DRAFT] #29641

wants to merge 3 commits into from
+209 −215

Conversation

maflcko
Copy link
Member

@maflcko maflcko commented Mar 12, 2024 •
edited
Loading

LogPrintf has many issues:

  • It does not mention the log severity (info).
  • It is a deprecated alias for LogInfo, according to the dev notes.
  • It wastes review cycles, because reviewers sometimes point out that it is deprecated.

@DrahtBot
Copy link
Contributor

DrahtBot commented Mar 12, 2024 •
edited
Loading

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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/29641.

Reviews

See the guideline for information on the review process.

Type Reviewers
Concept ACK TheCharlatan, l0rinc
Stale ACK kevkevinpal

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:

  • #33192 (refactor: unify container presence checks (without PR conflicts) by l0rinc)
  • #33191 (net: Provide block templates to peers on request by ajtowns)
  • #33034 (wallet: Store transactions in a separate sqlite table by achow101)
  • #33033 (wallet, sqlite: Encapsulate SQLite statements in a RAII class by achow101)
  • #33026 (test, refactor: Embedded ASmap selected preparatory work by fjahr)
  • #32748 (fees: prevent redundant estimates flushes by ismaelsadeeq)
  • #32685 (wallet: Allow read-only database access for info and dump commands by PeterWrighten)
  • #32394 (net: make m_nodes_mutex non-recursive by vasild)
  • #32326 (net: improve the interface around FindNode() and avoid a recursive mutex lock by vasild)
  • #32317 (kernel: Separate UTXO set access from validation functions by TheCharlatan)
  • #32297 (bitcoin-cli: Add -ipcconnect option by ryanofsky)
  • #28792 (Embed default ASMap as binary dump header file by fjahr)
  • #28584 (Fuzz: extend CConnman tests by vasild)

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.

@kevkevinpal
Copy link
Contributor

concept ACK fae5751

@kevkevinpal
Copy link
Contributor

I noticed that there are helper functions such as the following using the LogPrintf naming scheme

  • WalletLogPrintf in src/wallet/wallet.h
  • LogPrintfCategoryWithThreadNames, LogPrintfCategory, LogPrintfCategoryWithoutThreadNames, LogPrintfWithoutThreadNames, LogPrintfWithThreadNames in ./src/bench/logging.cpp

Using this grep grep -nri "\<LogPrintLevel\>" ./src --binary-files=without-match
I also noticed that we are using LogPrintLevel when we could be using LogInfo, LogWarning, LogError, LogDebug and LogTrace

these might want to be addressed in a separate PR though

This was referenced Mar 13, 2024
Draft
Open
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Closed
Closed
Closed
Open
Open
Merged
Closed
Closed
Closed
@DrahtBot DrahtBot mentioned this pull request Mar 13, 2024
Merged
This was referenced Mar 13, 2024
Open
Merged
@DrahtBot
Copy link
Contributor

🚧 At least one of the CI tasks failed.
Task ARM, unit tests, no functional tests: https://github.com/bitcoin/bitcoin/runs/45922957541
LLM reason (✨ experimental): The CI failure is caused by a compilation error due to the use of a non-declared function 'RemovePidFileeeeeeeeeeeee' and the compiler being set to treat all warnings as errors.

Hints

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.

This was referenced Jul 14, 2025
Merged
Merged
This was referenced Jul 24, 2025
Draft
Open
Draft
Open
@fanquake fanquake mentioned this pull request Jul 25, 2025
Open
This was referenced Jul 30, 2025
Closed
Merged
Merged
MarcoFalke added 2 commits August 21, 2025 10:06
doc: Fix typo in init log
d4530b9
scripted-diff: LogPrintf -> LogInfo
2fd6c66 
-BEGIN VERIFY SCRIPT-
 sed -i 's/\<LogPrintf\>/LogInfo/g' $( git grep -l '\<LogPrintf\>' -- ./contrib/ ./src/ ./test/ ':(exclude)src/logging.h' )
-END VERIFY SCRIPT-
This was referenced Aug 21, 2025
Open
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fanquake fanquake fanquake left review comments

+2 more reviewers

@l0rinc l0rinc l0rinc requested changes

@Theschorpioen Theschorpioen Theschorpioen approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
Refactoring
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@maflcko @DrahtBot @kevkevinpal @ryanofsky @hebasto @TheCharlatan @jonatack @fanquake @l0rinc @Theschorpioen