ci: Doc: Move all config comments right next to the option they explain #36052

pull maflcko wants to merge 3 commits into bitcoin:master from maflcko:2608-ci-doc-comments changing 26 files +100 −98

maflcko commented at 11:19 AM on August 21, 2026: member

This is a CI doc-style-cleanup. BITCOIN_CONFIG is a single manually-formatted-and-quoted large string. This is fine, but shellcheck doesn't like when trailing comments are added: SC2155 -- Declare and assign separately.

Fix this style by using printf to format the single string. The second commit then moves the comments right into (or next to) the line that it concerns.
DrahtBot renamed this:
~~ci: Doc: Move all config comments right next to the option they explain~~
ci: Doc: Move all config comments right next to the option they explain
on Aug 21, 2026
DrahtBot added the label Tests on Aug 21, 2026
DrahtBot commented at 11:19 AM on August 21, 2026: contributor


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/36052.



Reviews

See the guideline and AI policy for information on the review process.

Type Reviewers

ACK hebasto, willcl-ark

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.



Conflicts

Reviewers, this pull request conflicts with the following ones:
- #36033 ([wip,nomerge,rfc] build: Require C++23 compiler by maflcko)
- #35957 (ci: Enable Boost.MultiIndex invariant-checking mode by hebasto)
- #31349 (ci: detect outbound internet traffic generated while running tests by vasild)
- #29700 (kernel, refactor: return error status on all fatal errors by ryanofsky)
- #26022 (Add util::ResultPtr class by ryanofsky)
- #25722 (refactor: Use util::Result class for wallet loading by ryanofsky)
- #25665 (refactor: Add util::Result failure types and ability to merge result values by ryanofsky)
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.
hebasto approved
hebasto commented at 10:50 AM on August 22, 2026: member

ACK fa03706fdef52e85b36b758c5867519acfc32a70.
maflcko requested review from fanquake on Aug 24, 2026
DrahtBot added the label Needs rebase on Sep 9, 2026

Type	Reviewers
ACK	hebasto, willcl-ark

ci: refactor: Use printf %q quoting for BITCOIN_CONFIG

This refactor turns a long manually formatted and quoted string into one
formatted and quoted by printf.

fa0e84ec53

ci: Doc: Move all config comments right next to the option they explain fa18f9aeac
maflcko force-pushed on Sep 21, 2026
DrahtBot removed the label Needs rebase on Sep 21, 2026
maflcko commented at 3:18 PM on September 21, 2026: member

rebased (trivial)
ci: [refactor] Use eval-based string-to-argv conversion for FUZZ_TESTS_ARGS
Same approach is used for other argv strings.
fa90a8a013
maflcko commented at 9:57 AM on September 23, 2026: member

(added a small commit to remove one shellcheck disable)
maflcko requested review from willcl-ark on Sep 30, 2026
willcl-ark commented at 10:47 AM on September 30, 2026: member

Hmmm, so we switch "manually quoted strings with docstrings before" for "automatically-quoted strings with doctrings inline, using backticks".

I'm ~0 on this? I never found having the docstring above confusing or anything. If anything the new backticks slightly hurt readability? And using command substitution for a comment seems weird, but also fine.

Have people been fighting against the quoting in the linter/CI?

That said I think printf %q is probably worth the change. So happy to ack here.
maflcko commented at 10:58 AM on September 30, 2026: member

I never found having the docstring above confusing or anything.

Ok, maybe it is just me. I find the comments that are ~6 or ~10 lines above the line they refer to a bit confusing to read and sometimes makes me wonder if it is outdated.

But happy to close this pull, if the other CI people would prefer to leave this as-is.
hebasto approved
hebasto commented at 11:14 AM on September 30, 2026: member

re-ACK fa90a8a01318493d39bca924dbd51c72f0232e4c.

I never found having the docstring above confusing or anything.

Ok, maybe it is just me. I find the comments that are ~6 or ~10 lines above the line they refer to a bit confusing to read and sometimes makes me wonder if it is outdated.

Same for me.
willcl-ark approved
willcl-ark commented at 12:02 PM on September 30, 2026: member

ACK fa90a8a01318493d39bca924dbd51c72f0232e4c

No objection to move the docstrings closer to their sources, and printf %q seems like an improvement.
in ci/test/00_setup_env_native_tsan.sh:19 in fa90a8a013
```
  16 | @@ -17,10 +17,10 @@ export DEP_OPTS="CC=clang CXX=clang++ CXXFLAGS='${LIBCXX_FLAGS}' NO_QT=1"
  17 |  export GOAL="install"
  18 |  export CI_LIMIT_STACK_SIZE=1
  19 |  # Disable fortification with -U_FORTIFY_SOURCE to work around https://github.com/bitcoin/bitcoin/issues/30586
```
maflcko commented at 7:29 AM on October 2, 2026:

self-review: Forgot to move this comment?

Also, I forgot that I already fixed this in https://github.com/llvm/llvm-project/commit/f801f9d0103dd715fde0d3518d47b5fe605f093c

So maybe this can be tackled later, so that this pull stays a refactor-only doc-only chnage with two acks (rfm?)

fanquake commented at 8:51 AM on October 2, 2026:

Looks like it can just be removed entirely, if this was included in 23.1.0?
fanquake merged this on Oct 2, 2026
fanquake closed this on Oct 2, 2026
hebasto referenced this in commit a829aeaac1 on Oct 2, 2026
maflcko deleted the branch on Oct 2, 2026

Contributors

Review Requested
fanquake

Labels

Tests

Linked (view graph)

#36412 ci: drop `-U_FORTIFY_SOURCE` from TSAN job

ci: Doc: Move all config comments right next to the option they explain #36052

Code Coverage & Benchmarks

Reviews

Conflicts