scripted-diff: Use LogInfo/LogDebug over LogPrintf/LogPrint #29641

pull maflcko wants to merge 2 commits into bitcoin:master from maflcko:2403-log- changing 91 files +825 −834
  1. maflcko commented at 8:21 pm on March 12, 2024: member

    LogPrintf/LogPrint are problematic, because:

    • Their name is non-descriptive of what the function does (info logging or debug logging).
    • They are deprecated aliases, where code is using either the deprecated or non-deprecated alias, which is inconsistent and confusing.

    Fix all issues by replacing the usage.

    While the diff is large and may cause merge conflicts or backport conflicts, I don’t see the deprecated aliases being kept forever, so this will have to be done at some point. All conflicts should be trivial to solve, in any case.

  2. DrahtBot commented at 8:21 pm on March 12, 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
    Concept ACK TheCharlatan
    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:

    • #30358 (scripted-diff: Log parameter interaction not thrice by maflcko)
    • #30355 (wallet: use LogTrace for walletdb log messages at trace level by ajtowns)
    • #30347 (logging: Use LogFatal instead LogError for fatal errors by ryanofsky)
    • #30343 (wallet, logging: Replace WalletLogPrintf() with LogInfo() by ryanofsky)
    • #30342 (kernel, logging: Pass Logger instances to kernel objects by ryanofsky)
    • #30324 (optimization: Moved repeated -printpriority fetching out of AddToBlock by paplorinc)
    • #30320 (assumeutxo: Don’t load a snapshot if it’s not in the best header chain by mzumsande)
    • #30267 (assumeutxo: Check snapshot base block is not in invalid chain by fjahr)
    • #30141 (kernel: De-globalize validation caches by TheCharlatan)
    • #30065 (init: fixes file descriptor accounting by sr-gi)
    • #30064 (net: log connections failures via SOCKS5 with less severity by vasild)
    • #30043 (net: Replace libnatpmp with built-in PCP+NATPMP implementation by laanwj)
    • #29702 (fees: Remove CLIENT_VERSION serialization by maflcko)
    • #29656 (chainparams: Change nChainTx type to uint64_t by fjahr)
    • #29640 (Fix tiebreak when loading blocks from disk (and add tests for comparing chain ties) by sr-gi)
    • #29625 (Several randomness improvements by sipa)
    • #29605 (net: Favor peers from addrman over fetching seednodes by sr-gi)
    • #29480 (Drop log category in SeedStartup by hebasto)
    • #29431 (test/BIP324: disconnection scenarios during v2 handshake by stratospher)
    • #29418 (rpc: provide per message stats for global traffic via new RPC ‘getnetmsgstats’ by vasild)
    • #29415 (Broadcast own transactions only via short-lived Tor or I2P connections by vasild)
    • #29307 (util: explicitly close all AutoFiles that have been written by vasild)
    • #29256 (Improve new LogDebug/Trace/Info/Warning/Error Macros by ryanofsky)
    • #29141 (Bugfix: RPC: Check for blank rpcauth on a per-param basis by luke-jr)
    • #28792 (Embedding ASMap files as binary dump header file by fjahr)
    • #28710 (Remove the legacy wallet and BDB dependency by achow101)
    • #28676 ([WIP] Cluster mempool implementation by sdaftuar)
    • #28521 (net: additional disconnect logging by Sjors)
    • #27826 (validation: log which peer sent us a header by Sjors)
    • #27052 (test: rpc: add last block announcement time to getpeerinfo result by LarryRuane)
    • #26114 (net: Make AddrFetch connections to fixed seeds by mzumsande)
    • #25832 (tracing: network connection tracepoints by 0xB10C)
    • #25722 (refactor: Use util::Result class for wallet loading by ryanofsky)
    • #19463 (Prune locks by luke-jr)

    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.

  3. DrahtBot added the label Refactoring on Mar 12, 2024
  4. maflcko force-pushed on Mar 12, 2024
  5. DrahtBot added the label CI failed on Mar 12, 2024
  6. kevkevinpal commented at 1:55 am on March 13, 2024: contributor
    concept ACK fae5751
  7. kevkevinpal commented at 2:19 am on March 13, 2024: 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

  8. DrahtBot removed the label CI failed on Mar 13, 2024
  9. DrahtBot added the label Needs rebase on Mar 13, 2024
  10. maflcko force-pushed on Mar 13, 2024
  11. DrahtBot removed the label Needs rebase on Mar 13, 2024
  12. ryanofsky commented at 2:14 pm on March 13, 2024: contributor

    Concept -0. Not a strong opinion, but I think just mechanically replacing s/LogPrint/LogDebug/ and s/LogPrintf/LogInfo/ everywhere would not provide a major benefit, and while it may be true that “this will have to be done at some point” I think the point where it’d be nicest to do this would be after #29256 when we are able to define log sources to make LogDebug calls less verbose, and add missing category information to LogInfo calls.

    I wouldn’t object to this PR if other reviewers think it’s a worthwhile improvement and are ok with the long list of conflicts. I just think there are other improvements we should make to log prints besides this one, and it would be better to change each individual log print once instead of changing it multiple times.

  13. maflcko commented at 2:17 pm on March 13, 2024: member
    Ok, makes sense. I’ll put it in draft for now, to allow for more time, if people want to change log messages further.
  14. maflcko marked this as a draft on Mar 13, 2024
  15. DrahtBot added the label Needs rebase on Mar 14, 2024
  16. maflcko force-pushed on Mar 14, 2024
  17. DrahtBot removed the label Needs rebase on Mar 14, 2024
  18. hebasto commented at 4:42 pm on March 18, 2024: member

    LogPrintf/LogPrint are problematic…

    Some of them are just broken. For example, #29480.

  19. DrahtBot added the label Needs rebase on Mar 20, 2024
  20. maflcko force-pushed on Mar 21, 2024
  21. DrahtBot removed the label Needs rebase on Mar 21, 2024
  22. DrahtBot added the label Needs rebase on Mar 22, 2024
  23. maflcko force-pushed on Mar 24, 2024
  24. DrahtBot removed the label Needs rebase on Mar 24, 2024
  25. DrahtBot added the label Needs rebase on Mar 25, 2024
  26. maflcko force-pushed on Mar 25, 2024
  27. DrahtBot removed the label Needs rebase on Mar 25, 2024
  28. DrahtBot added the label Needs rebase on Mar 28, 2024
  29. maflcko force-pushed on Mar 28, 2024
  30. DrahtBot removed the label Needs rebase on Mar 28, 2024
  31. DrahtBot added the label Needs rebase on Apr 24, 2024
  32. maflcko force-pushed on Apr 26, 2024
  33. DrahtBot removed the label Needs rebase on Apr 26, 2024
  34. DrahtBot added the label Needs rebase on Apr 30, 2024
  35. maflcko force-pushed on May 1, 2024
  36. DrahtBot removed the label Needs rebase on May 1, 2024
  37. DrahtBot added the label Needs rebase on May 3, 2024
  38. maflcko force-pushed on May 3, 2024
  39. DrahtBot removed the label Needs rebase on May 3, 2024
  40. DrahtBot added the label Needs rebase on May 13, 2024
  41. maflcko force-pushed on May 20, 2024
  42. DrahtBot removed the label Needs rebase on May 20, 2024
  43. DrahtBot added the label CI failed on May 22, 2024
  44. DrahtBot commented at 6:50 am on May 22, 2024: contributor

    🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the documentation.

    Possibly this is 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.

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

    Debug: https://github.com/bitcoin/bitcoin/runs/25181673072

  45. maflcko force-pushed on May 22, 2024
  46. DrahtBot removed the label CI failed on May 22, 2024
  47. DrahtBot added the label Needs rebase on May 23, 2024
  48. maflcko force-pushed on May 28, 2024
  49. DrahtBot removed the label Needs rebase on May 28, 2024
  50. DrahtBot added the label Needs rebase on May 29, 2024
  51. maflcko force-pushed on May 29, 2024
  52. DrahtBot removed the label Needs rebase on May 29, 2024
  53. DrahtBot added the label Needs rebase on Jun 3, 2024
  54. maflcko force-pushed on Jun 3, 2024
  55. DrahtBot removed the label Needs rebase on Jun 3, 2024
  56. DrahtBot added the label CI failed on Jun 7, 2024
  57. DrahtBot removed the label CI failed on Jun 10, 2024
  58. DrahtBot added the label Needs rebase on Jun 10, 2024
  59. maflcko force-pushed on Jun 10, 2024
  60. DrahtBot removed the label Needs rebase on Jun 10, 2024
  61. DrahtBot added the label Needs rebase on Jun 11, 2024
  62. maflcko force-pushed on Jun 12, 2024
  63. DrahtBot removed the label Needs rebase on Jun 12, 2024
  64. DrahtBot added the label Needs rebase on Jun 14, 2024
  65. maflcko force-pushed on Jun 17, 2024
  66. DrahtBot removed the label Needs rebase on Jun 17, 2024
  67. DrahtBot added the label Needs rebase on Jun 17, 2024
  68. maflcko force-pushed on Jun 18, 2024
  69. DrahtBot removed the label Needs rebase on Jun 18, 2024
  70. DrahtBot added the label Needs rebase on Jun 20, 2024
  71. maflcko force-pushed on Jun 22, 2024
  72. DrahtBot removed the label Needs rebase on Jun 22, 2024
  73. TheCharlatan commented at 8:31 pm on June 25, 2024: contributor
    Concept ACK
  74. DrahtBot added the label Needs rebase on Jun 26, 2024
  75. maflcko force-pushed on Jun 26, 2024
  76. DrahtBot removed the label Needs rebase on Jun 26, 2024
  77. DrahtBot added the label Needs rebase on Jun 27, 2024
  78. scripted-diff: LogPrintf/LogPrint -> LogInfo/LogDebug 
    -BEGIN VERIFY SCRIPT-
 sed -i 's/\<LogPrintf\>/LogInfo/g' $( git grep -l '\<LogPrintf\>' -- ./contrib/ ./src/ ./test/ ':(exclude)src/logging.h' )
 sed -i 's/\<LogPrint\>/LogDebug/g' $( git grep -l '\<LogPrint\>'  -- ./contrib/ ./src/ ./test/ ':(exclude)src/logging.h' )
-END VERIFY SCRIPT-
    ace72d0ff9
  79. refactor: Remove unused LogPrintf/LogPrint 7c47e249b2
  80. maflcko force-pushed on Jun 28, 2024
  81. DrahtBot removed the label Needs rebase on Jun 28, 2024
  82. jonatack commented at 7:18 pm on June 28, 2024: contributor

    I suggest #29256 (comment) instead.

    It may also make sense to finish consensus and work on the simplest possible consistent user-facing API and developer API before doing a mass migration.


maflcko DrahtBot kevkevinpal ryanofsky hebasto TheCharlatan jonatack

Labels
Refactoring

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-06-29 07:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me