📝 davidgumberg opened a pull request: "Lint: support running individual rust linters and improve subtree exclusion"
(https://github.com/bitcoin/bitcoin/pull/29965)
This PR improves subtree exclusion by rewriting lint checks to reuse common exclusion logic. Most lint checks previously did not exclude `crypto/ctaes`, the locale linter did not exclude `src/crc32`
Makes the include guards lint check print all missing include guards before exiting.
Adds support for running individual linters by passing `--lint=LINT_CHECK_TO_RUN` to the lint runner. If using cargo run arguments can be passed e.g.:
```bash
cd test/lint/test_runner && cargo run -- --lint
...
(https://github.com/bitcoin/bitcoin/pull/29965)
This PR improves subtree exclusion by rewriting lint checks to reuse common exclusion logic. Most lint checks previously did not exclude `crypto/ctaes`, the locale linter did not exclude `src/crc32`
Makes the include guards lint check print all missing include guards before exiting.
Adds support for running individual linters by passing `--lint=LINT_CHECK_TO_RUN` to the lint runner. If using cargo run arguments can be passed e.g.:
```bash
cd test/lint/test_runner && cargo run -- --lint
...
💬 davidgumberg commented on pull request "lint: Refactor lint checks to reuse exclusion logic; Support running individual lint checks in lint runner":
(https://github.com/bitcoin/bitcoin/pull/29744#issuecomment-2078380040)
@maflcko @fjahr Thank you for the feedback, sorry for the confusion on this PR. I've opened https://github.com/bitcoin/bitcoin/pull/29965
(https://github.com/bitcoin/bitcoin/pull/29744#issuecomment-2078380040)
@maflcko @fjahr Thank you for the feedback, sorry for the confusion on this PR. I've opened https://github.com/bitcoin/bitcoin/pull/29965
💬 ryanofsky commented on pull request "Disable util::Result copying and assignment":
(https://github.com/bitcoin/bitcoin/pull/29906#issuecomment-2078452007)
Rebased 88aa093356ff142efe60df989323eb1ed1aa9e18 -> 0cbcb66257db65f0a68d82d17a2d1a83d8654b38 ([`pr/saferesult.5`](https://github.com/ryanofsky/bitcoin/commits/pr/saferesult.5) -> [`pr/saferesult.6`](https://github.com/ryanofsky/bitcoin/commits/pr/saferesult.6), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/saferesult.5-rebase..pr/saferesult.6)) dropping Result::Update() method in this PR, and last 2 uses of it.
re: https://github.com/bitcoin/bitcoin/pull/29906#issuecomment-2078213
...
(https://github.com/bitcoin/bitcoin/pull/29906#issuecomment-2078452007)
Rebased 88aa093356ff142efe60df989323eb1ed1aa9e18 -> 0cbcb66257db65f0a68d82d17a2d1a83d8654b38 ([`pr/saferesult.5`](https://github.com/ryanofsky/bitcoin/commits/pr/saferesult.5) -> [`pr/saferesult.6`](https://github.com/ryanofsky/bitcoin/commits/pr/saferesult.6), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/saferesult.5-rebase..pr/saferesult.6)) dropping Result::Update() method in this PR, and last 2 uses of it.
re: https://github.com/bitcoin/bitcoin/pull/29906#issuecomment-2078213
...
💬 ChrisCho-H commented on pull request "script: throw disabled err for op_ver and its variants":
(https://github.com/bitcoin/bitcoin/pull/28169#issuecomment-2078502542)
> Throw `DISABLED_OPCODE` error for op_ver and its variants as it's known but just disabled opcodes. Currently, interpreter throws `BAD_OPCODE` which contains the message of `"Opcode missing or not understood"`, which isn't accurate and confusing.
Apologize for poor description, I just re-write to make it simple and clear.
I agree that this one is practically doesn't affect any, as only a few will push OP_VER opcodes on stack.
It's such a minor fix, and when I just found out that OP_VER thr
...
(https://github.com/bitcoin/bitcoin/pull/28169#issuecomment-2078502542)
> Throw `DISABLED_OPCODE` error for op_ver and its variants as it's known but just disabled opcodes. Currently, interpreter throws `BAD_OPCODE` which contains the message of `"Opcode missing or not understood"`, which isn't accurate and confusing.
Apologize for poor description, I just re-write to make it simple and clear.
I agree that this one is practically doesn't affect any, as only a few will push OP_VER opcodes on stack.
It's such a minor fix, and when I just found out that OP_VER thr
...
💬 fanquake commented on pull request "index: race fix, lock cs_main while 'm_synced' is subject to change":
(https://github.com/bitcoin/bitcoin/pull/29867#issuecomment-2078569791)
Seems like there's at least something which could/should be backported here? Maybe only the other change, given the newly broken code has only existed in master?
(https://github.com/bitcoin/bitcoin/pull/29867#issuecomment-2078569791)
Seems like there's at least something which could/should be backported here? Maybe only the other change, given the newly broken code has only existed in master?
✅ maflcko closed a pull request: "script: throw disabled err for op_ver and its variants"
(https://github.com/bitcoin/bitcoin/pull/28169)
(https://github.com/bitcoin/bitcoin/pull/28169)
💬 maflcko commented on pull request "script: throw disabled err for op_ver and its variants":
(https://github.com/bitcoin/bitcoin/pull/28169#issuecomment-2078663017)
> It's ok to close this PR if that is better.
Ok, I'll close it for now. When there is need, it can be re-opened later or picked up again. Thanks for working on this for such a long time.
(https://github.com/bitcoin/bitcoin/pull/28169#issuecomment-2078663017)
> It's ok to close this PR if that is better.
Ok, I'll close it for now. When there is need, it can be re-opened later or picked up again. Thanks for working on this for such a long time.
📝 maflcko opened a pull request: "test: Add two more urlDecode tests"
(https://github.com/bitcoin/bitcoin/pull/29967)
Trivial follow-up after https://github.com/bitcoin/bitcoin/pull/29904#discussion_r1579216072
(https://github.com/bitcoin/bitcoin/pull/29967)
Trivial follow-up after https://github.com/bitcoin/bitcoin/pull/29904#discussion_r1579216072
💬 maflcko commented on pull request "Lint: support running individual rust linters and improve subtree exclusion":
(https://github.com/bitcoin/bitcoin/pull/29965#issuecomment-2078730201)
The Win64 GHA test failure looks unrelated. If you want, you can split up 54b2115bb848e1aad8a86aae858de63a851235ce into its own pull request, as it is an independent improvement. But up to you.
(https://github.com/bitcoin/bitcoin/pull/29965#issuecomment-2078730201)
The Win64 GHA test failure looks unrelated. If you want, you can split up 54b2115bb848e1aad8a86aae858de63a851235ce into its own pull request, as it is an independent improvement. But up to you.
💬 maflcko commented on pull request "refactor: Use our own implementation of urlDecode":
(https://github.com/bitcoin/bitcoin/pull/29904#discussion_r1580556442)
https://github.com/bitcoin/bitcoin/pull/29967/files
(https://github.com/bitcoin/bitcoin/pull/29904#discussion_r1580556442)
https://github.com/bitcoin/bitcoin/pull/29967/files
💬 laanwj commented on pull request "test: Add two more urlDecode tests":
(https://github.com/bitcoin/bitcoin/pull/29967#issuecomment-2078798420)
Code review ACK https://github.com/bitcoin/bitcoin/pull/29967/commits/fa55972a758865a6bd0114afe72e51877896d495
(https://github.com/bitcoin/bitcoin/pull/29967#issuecomment-2078798420)
Code review ACK https://github.com/bitcoin/bitcoin/pull/29967/commits/fa55972a758865a6bd0114afe72e51877896d495
💬 fanquake commented on pull request "ZMQ: Support UNIX domain sockets":
(https://github.com/bitcoin/bitcoin/pull/27679#discussion_r1580606777)
FYI https://github.com/bitcoin/bitcoin/runs/24257306970:
```bash
init.cpp: In function ‘bool AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)’:
init.cpp:1319:20: error: unused variable ‘unix’ [-Werror=unused-variable]
1319 | const bool unix{port_option.second};
| ^~~~
```
(https://github.com/bitcoin/bitcoin/pull/27679#discussion_r1580606777)
FYI https://github.com/bitcoin/bitcoin/runs/24257306970:
```bash
init.cpp: In function ‘bool AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)’:
init.cpp:1319:20: error: unused variable ‘unix’ [-Werror=unused-variable]
1319 | const bool unix{port_option.second};
| ^~~~
```
👍 stickies-v approved a pull request: "test: Add two more urlDecode tests"
(https://github.com/bitcoin/bitcoin/pull/29967#pullrequestreview-2024343300)
ACK fa55972a758865a6bd0114afe72e51877896d495
(https://github.com/bitcoin/bitcoin/pull/29967#pullrequestreview-2024343300)
ACK fa55972a758865a6bd0114afe72e51877896d495
💬 fjahr commented on pull request "test: Add two more urlDecode tests":
(https://github.com/bitcoin/bitcoin/pull/29967#issuecomment-2078824038)
ACK fa55972a758865a6bd0114afe72e51877896d495
(https://github.com/bitcoin/bitcoin/pull/29967#issuecomment-2078824038)
ACK fa55972a758865a6bd0114afe72e51877896d495
💬 fanquake commented on pull request "refactor: remove remaining unused code from cpp-subprocess":
(https://github.com/bitcoin/bitcoin/pull/29961#issuecomment-2078841172)
> Note that there are some API functions of the Popen class that we don't use, e.g. wait(), pid(), poll(), kill(), but they sound IMHO common enough to be useful in the future, so not sure how deep we should go there.
Deleting as much as possible seems fine. If code turns out to be needed in future (although I wouldn't think we are going to expand this interface), it can be retrieved from the git history.
(https://github.com/bitcoin/bitcoin/pull/29961#issuecomment-2078841172)
> Note that there are some API functions of the Popen class that we don't use, e.g. wait(), pid(), poll(), kill(), but they sound IMHO common enough to be useful in the future, so not sure how deep we should go there.
Deleting as much as possible seems fine. If code turns out to be needed in future (although I wouldn't think we are going to expand this interface), it can be retrieved from the git history.
💬 maflcko commented on pull request "depends: Do not consider `CC` environment variable for detecting system":
(https://github.com/bitcoin/bitcoin/pull/29963#issuecomment-2078866286)
> The log shows no cross-compiling, but it is cross-compiling to `i686-pc-linux-gnu`.
Can you link to the exact lines in the log that show "no cross-compiling".
(https://github.com/bitcoin/bitcoin/pull/29963#issuecomment-2078866286)
> The log shows no cross-compiling, but it is cross-compiling to `i686-pc-linux-gnu`.
Can you link to the exact lines in the log that show "no cross-compiling".
📝 maflcko opened a pull request: "refactor: Avoid unused-variable warning in init.cpp"
(https://github.com/bitcoin/bitcoin/pull/29968)
Fixes https://github.com/bitcoin/bitcoin/pull/27679#discussion_r1580606777
(https://github.com/bitcoin/bitcoin/pull/29968)
Fixes https://github.com/bitcoin/bitcoin/pull/27679#discussion_r1580606777
💬 maflcko commented on pull request "ZMQ: Support UNIX domain sockets":
(https://github.com/bitcoin/bitcoin/pull/27679#discussion_r1580659104)
Fixed in #29968
(https://github.com/bitcoin/bitcoin/pull/27679#discussion_r1580659104)
Fixed in #29968
💬 laanwj commented on pull request "build: add `-Wundef`":
(https://github.com/bitcoin/bitcoin/pull/29876#discussion_r1580663336)
i don't know, i don't think what i imagined is actually doable: make sure the entire build system->program interface consists of 0/1 defines (mandatory) not error-prone `#define`/`#undef`. There's just too many. So resolving this, retracting my comment.
(https://github.com/bitcoin/bitcoin/pull/29876#discussion_r1580663336)
i don't know, i don't think what i imagined is actually doable: make sure the entire build system->program interface consists of 0/1 defines (mandatory) not error-prone `#define`/`#undef`. There's just too many. So resolving this, retracting my comment.
👍 laanwj approved a pull request: "build: add `-Wundef`"
(https://github.com/bitcoin/bitcoin/pull/29876#pullrequestreview-2024427057)
ACK cc09c165eb0bbb4b41a9132734bee55969e9d9a8
(https://github.com/bitcoin/bitcoin/pull/29876#pullrequestreview-2024427057)
ACK cc09c165eb0bbb4b41a9132734bee55969e9d9a8