Using std::filesystem is problematic:
fs namespace wrapper for it. So having two ways to achieve the same is confusing.fs wrapper is dangerous and buggy, because it disables known bugs by deleting problematic functions.Fix all issues by removing use of it and adding a linter to avoid using it again in the future.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For detailed information about the code coverage, see the test coverage report.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | TheCharlatan, fanquake |
| Concept NACK | luke-jr |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
No conflicts as of last run.
Why to introduce Rust as additional dependency just for the linter code?
Because I don’t think it is a good use of developer and reviewer time to write not type-safe code, only to later (after review or merge) discover that it crashes with a TypeError. (Just one example: #27921 (comment), obviously the problem is a general one and it is left as an exercise to the reader to create a full list of past issues, including the ones that were more severe.)
Yes, I know that mypy exists, but no one uses it, it is incomplete, and it comes with many other issues.
Moreover, the linter code already has a bunch of random dependencies, so adding one more shouldn’t be an issue? Finally, it is entirely optional, and already run by CI, so no one is forced or even encouraged to run it. (Personally I can’t recall the last time I’ve run any linter outside of testing one after writing the code for it.)
Because I don’t think it is a good use of developer and reviewer time to write not type-safe code, only to later (after review or merge) discover that it crashes with a
TypeError.
Why not just use C++ then? (I’m not a Rust expert, maybe there are good reasons)
I can’t recall the last time I’ve run any linter outside of testing one after writing the code for it.
I try always to remember running linters locally before submitting a new PR (sometimes I fail and forget, need to admit).
Why not just use C++ then? (I’m not a Rust expert, maybe there are good reasons)
I tried that, but vanilla C++ doesn’t have a nice interface for processes. There’s only libc, or third party libs. So it seemed easier to just use rust with the added bonus that the written code closely resembles the look and feel of python. If you write something elegant in C++, I am happy to push it here, tough.
I’d love for Rust to be “backdoored” into the project, but I am not sure if adding yet another linting infrastructure is the right way to do this.
What does this check do that a check with a clang-tidy plugin could not? I have pushed a dirty prototype (likely incomplete) for such a plugin check here: https://github.com/TheCharlatan/bitcoin-tidy-experiments/commit/5c1f1e1289758df4a0407cddcfd1f8b043c53be9. It checks for declarations of std::filesystem::path, and calls to its string method. It also showcases some of the pre-processor checks that it can do. In this case, it checks if <filesystem> is included, and whether std::filesystem can be found anywhere in the source file text.
The tradeoff is obviously the readability of the code and that there might be some declaration edge cases that are not caught by them. A simple git-grep will always be a decent catch-all, but might miss certain declarations, or be unable to distinguish between comment and code. What do you think?
What do you think?
I’d say we should pick what reviewers like the best. However, as you say the clang-tidy plugin has many downsides:
I think it should be decided on a case-by-case basis whether a simple git grep-based linter is preferred, or a fully engineered clang-tidy plugin. Here, I think a git grep seems a better fit.
git grep-based linters obviously assume some kind of formatting, but I think this is fine. Otherwise we’d have to rewrite the locale linter to clang-tidy, because someone could write std :: to_string to trick the linter? (Same for the other linters, such as the assertions or includes linter). Maybe for critical stuff we could consider adding both, one using git grep and another using clang-tidy, but I don’t think we have any “critical” linters yet.
Standing Concept NACK to Rust while it has security issues (unbootstrappable)
“It’s just for CI” - but we shouldn’t be relying on CI alone.
unbootstrappable
Simply claiming this doesn’t seem helpful. What is the compile error you encountered? (Ideally with steps to reproduce)
ACK fa8d99d8932df08320f1bfb4e8f1d4b4169466fd (first commit)
I have no issues with Rust, or using it in the CI; we already do in other CIs in the Bitcoin Core project. Saying that, I’m not sure if we want to introduce a 4th way to do linting in this repo (yet). One potential alternative, if we go further down the clang-tidy route, is the <filesystem> linter here: https://github.com/fanquake/bitcoin/commits/integrate_thecharlatan_filesystem, put together by @TheCharlatan.
4th way
Currently the following is used:
./ci/lint/06_script.sh, and in some lint script that weren’t converted to pythonPersonally I don’t see a problem of adding a new dependency. Once and if this was merged, my plans were to at least remove the Bash driver, and fix the outstanding issues, e.g. to collect all failures before aborting (https://github.com/bitcoin/bitcoin/pull/28103#discussion_r1268140307). So personally, at least we’d end up with one consistent driver eventually, instead of several that behave differently. But if reviewers are happy with the current lint driver, I am happy to close this pull.
clang-tidy
I am a fan of clang-tidy, but I don’t think it is the better solution in this case, see #28076 (comment). In real-world scenarios it only has downsides when solving this problem at hand. Or did I miss something? It may be good to explain with an example why clang-tidy is better.
Currently the following is used:
Did you exclude clang-tidy here, or is it part of the list in the quote later in your comment. We already use it for linting, and my plan is to migrate more checks too it. For example, we can replace our python based assert linter (and more) with the equivalent clang-tidy check (LLVM 17 though).
I think the CI clang-tidy task is completely separate from the CI lint task. Just to repeat some points: It requires a full build system, a compile_commands.json, and must do a full build (slow). For the specifics, it may use more complicated and more verbose code, and it apparently ignores commented code?
For example, we can replace our python based assert linter (and more) with the equivalent clang-tidy check
Yes, this is possible, but comes with the above downsides. Or did I miss something? It may be good to explain with an example why clang-tidy is better in a specific case.
I think the CI clang-tidy task is completely separate from the CI lint task.
I guess in my mind they are the same thing. clang-tidy is just a more sane form of linting infrastructure, compared to a lot of what we currently use.
It requires a full build system,
Can you elaborate on why this is a problem? Any rust code also requires a full build system (cargo) and npm-like dependency management (crates). In the clang-tidy case, the build system is packages we’d already be installing to compile our code, or are dependencies that we already use for various other task, i.e LLVM (memory saniitizer, include-what-you-use, any non-GCC compile) etc.
and it apparently ignores commented code
Could you elaborate on this, I think I’m missing the context.
Yes, this is possible, but comes with the above downsides. Or did I miss something? It may be good to explain with an example why clang-tidy is better in a specific case.
I thik in general, I’d rather the project lean into code linting/tidy infrastructure that is more-widely maintained /used by other projects, rather than ad-hoc Python code calling out to git-grep, to scan for certain symbols that might appear between the brackets of an assert().
In general I agree with consolidating our infrastructure as much as possible, and would be entirely happy if we ended up in a world where that infrastructure was LLVM/clang-tidy + Rust & Cargo.
Can you elaborate on why this is a problem?
It just makes developer UX worse. The linters are already horrible, with some contributors having quit (at least parts of the code base) because of them. If someone pushes code and then has to wait more than 20 minutes (a full tidy CI run on 4 CPUs, 30+ minutes on a dirty ccache) to figure out they’ll have to replace std::to_string with ToString, it is certainly more annoying than having to wait 30 milliseconds:
0$ time git grep 'std::to_string' > /dev/null
1
2real 0m0.032s
rather than ad-hoc Python code calling out to git-grep, to scan for certain symbols that might appear between the brackets of an assert().
Ok, parsing arbitrary C++ code would be a valid reason to move the assert check to clang-tidy. However, I don’t see a reason for this assert check to exist in the first place. Our assert/Assert/Assume do have side effects, since it is not possible to compile if they hadn’t. The section in the dev notes should just be removed along with the linter. (But maybe removal or conversion of this check to clang-tidy can be discussed and done in a separate pull?)
Simply claiming this doesn’t seem helpful. What is the compile error you encountered? (Ideally with steps to reproduce)
The several issues bootstrapping Rust are well-known and documented elsewhere. Fixing it is not within the scope of this project. It simply shouldn’t be relied on until then.
Guix is shipping rust, according to https://packages.guix.gnu.org/packages/rust-cargo, so it can be bootstrapped on all systems that can be used to compile release binaries. I am not aware of any requirement nor efforts that the linters must be as bootstrappable as the release binaries, but at least the ones in this pull are, so that alone seems sufficient. On top of that, NixOS ships rust: https://github.com/NixOS/nixpkgs/tree/nixos-23.05/pkgs/development/compilers/rust, there is also mrustc for amd64 and gcc-rust (experimental), and maybe others that I’ve missed.
In any case, if there is a requirement that the linters are bootstrappable on some architectures, at a minimum it should be documented. Currently, they are literally hard-coding the requirement to linux.x86_64, see https://github.com/bitcoin/bitcoin/blob/b565485c24c0feacae559a7f6f7b83d7516ca58d/ci/lint/04_install.sh#L45 . (This should probably be fixed either way). Also, I wonder how we want to review and check for bootstrappability of the linters. Currently they are hardcoded to (mostly) download random binary packages and executables from random places in the internet. I don’t have any objection to changing that, but I presume coming up with fully bootstappable linters will be quite an effort. Also, given that the linters are downloading random binaries from random places in the internet, there is a chance that some of them already depend on rust or may even be compiled with rustc. Moreover, they may already not be bootstrappable for other reasons. When is the last time someone checked this, if at all? For example, pip packages can be wheels, which can be native binaries compiled from rust code. The same holds for any other downloaded package.
Finally, the wrapper/driver code is simply calling git grep, meaning anyone can just directly run the git-grep command directly on any system that ships with git, with no need for bootstrapping rustc, or even compiling any lint code.
Do you want to split that out, and continue the lint discussion
Seems better to keep the discussion in one place for now.
Seems better to keep the discussion in one place for now.
Ok, but the discussion of introducing Rust into the codebase (in whatever form) is clearly contentious, and shouldn’t be a blocker for merging obvious bug fixes.
I don’t think anything here is a bug fix (currently it should be a refactor). Using fs:: is safer, as explained in the pull request description.
There shouldn’t be any rush to merge this pull request before someone reviewed the code, and I am happy to rebase it every now and then (when no review happened yet) to see if there are any real (unlikely) bugs introduced.
I would just like to remove the
confusing and potentially dangerous,
code. I don’t see how that is predicated on, or needs to wait for a Rust based linter. Happy to ACK the first commit.
All code in this repo uses <util/fs.h>, except for a few lines. This is
confusing and potentially dangerous, if the safe <util/fs.h> wrappers
are not used.
Here is what happened when I tried to use this according to the added documentation. Any suggestion?
0jon|master =:~/bitcoin/bitcoin$ cd ./test/lint/test_runner/ && cargo fmt && cargo clippy && cargo run
1zsh: command not found: cargo
2
3jon|master =:~/bitcoin/bitcoin/test/lint/test_runner$ brew install cargo
4Running `brew update --auto-update`...
5==> Auto-updated Homebrew!
6==> Updated Homebrew from 75e8376816 to 8df40f4a41.
7No changes to formulae or casks.
8
9Warning: No available formula with the name "cargo". Did you mean cargo-c, argo, cairo, carton, cdargs or charge?
10cargo is part of the rust formula:
11 brew install rust
12
13jon|master =:~/bitcoin/bitcoin/test/lint/test_runner$ brew install rust
14==> Downloading https://ghcr.io/v2/homebrew/core/rust/manifests/1.72.1
15################################################################################################################################################################################## 100.0%
16==> Fetching dependencies for rust: libssh2 and libgit2@1.6
17==> Downloading https://ghcr.io/v2/homebrew/core/libssh2/manifests/1.11.0_1
18################################################################################################################################################################################## 100.0%
19==> Fetching libssh2
20==> Downloading https://ghcr.io/v2/homebrew/core/libssh2/blobs/sha256:41e860bcf96b8e86bb5f2c321fb1ca14b620adce510cec881eeac2f432e00e5e
21################################################################################################################################################################################## 100.0%
22==> Downloading https://ghcr.io/v2/homebrew/core/libgit2/1.6/manifests/1.6.4
23################################################################################################################################################################################## 100.0%
24==> Fetching libgit2@1.6
25==> Downloading https://ghcr.io/v2/homebrew/core/libgit2/1.6/blobs/sha256:d2b76777e0b8bf572537ff560539d6ad082c737851a148d71635ab899dbe6ead
26################################################################################################################################################################################## 100.0%
27==> Fetching rust
28==> Downloading https://ghcr.io/v2/homebrew/core/rust/blobs/sha256:be9922a4b56016f18d209067f5a4d148d2aad4db3061092f848744aff41e337d
29################################################################################################################################################################################## 100.0%
30==> Installing dependencies for rust: libssh2 and libgit2@1.6
31==> Installing rust dependency: libssh2
32==> Downloading https://ghcr.io/v2/homebrew/core/libssh2/manifests/1.11.0_1
33Already downloaded: /Users/jon/Library/Caches/Homebrew/downloads/48ca0c7785b21630a4817c59b72205609ccb0575e7abc64d64af2e61a60b5b0a--libssh2-1.11.0_1.bottle_manifest.json
34==> Pouring libssh2--1.11.0_1.arm64_ventura.bottle.tar.gz
35🍺 /opt/homebrew/Cellar/libssh2/1.11.0_1: 197 files, 1.2MB
36==> Installing rust dependency: libgit2@1.6
37==> Downloading https://ghcr.io/v2/homebrew/core/libgit2/1.6/manifests/1.6.4
38Already downloaded: /Users/jon/Library/Caches/Homebrew/downloads/e26ed1c3bc32b8b069e105866bbd146413a04ff18eee86eadc48fa356651736b--libgit2@1.6-1.6.4.bottle_manifest.json
39==> Pouring libgit2@1.6--1.6.4.arm64_ventura.bottle.tar.gz
40🍺 /opt/homebrew/Cellar/libgit2@1.6/1.6.4: 104 files, 4.5MB
41==> Installing rust
42==> Pouring rust--1.72.1.arm64_ventura.bottle.tar.gz
43==> Caveats
44zsh completions have been installed to:
45 /opt/homebrew/share/zsh/site-functions
46==> Summary
47🍺 /opt/homebrew/Cellar/rust/1.72.1: 38,925 files, 842.8MB
48==> Running `brew cleanup rust`...
49Disable this behaviour by setting HOMEBREW_NO_INSTALL_CLEANUP.
50Hide these hints with HOMEBREW_NO_ENV_HINTS (see `man brew`).
51==> Caveats
52==> rust
53zsh completions have been installed to:
54 /opt/homebrew/share/zsh/site-functions
55
56jon|master =:~/bitcoin/bitcoin/test/lint/test_runner$ cargo fmt && cargo clippy && cargo run
57error: no such command: `fmt`
58
59 Did you mean `fix`?
60
61 View all installed commands with `cargo --list`
62
63jon|master =:~/bitcoin/bitcoin/test/lint/test_runner$ cargo --list
64Installed Commands:
65 add Add dependencies to a Cargo.toml manifest file
66 b alias: build
67 bench Execute all benchmarks of a local package
68 build Compile a local package and all of its dependencies
69 c alias: check
70 check Check a local package and all of its dependencies for errors
71 clean Remove artifacts that cargo has generated in the past
72 clippy Checks a package to catch common mistakes and improve your Rust code.
73 config Inspect configuration values
74 d alias: doc
75 doc Build a package's documentation
76 fetch Fetch dependencies of a package from the network
77 fix Automatically fix lint warnings reported by rustc
78 generate-lockfile Generate the lockfile for a package
79 git-checkout This command has been removed
80 help Displays help for a cargo subcommand
81 init Create a new cargo package in an existing directory
82 install Install a Rust binary. Default location is $HOME/.cargo/bin
83 locate-project Print a JSON representation of a Cargo.toml file's location
84 login Save an api token from the registry locally. If token is not specified, it will be read from stdin.
85 logout Remove an API token from the registry locally
86 metadata Output the resolved dependencies of a package, the concrete used versions including overrides, in machine-readable format
87 new Create a new cargo package at <path>
88 owner Manage the owners of a crate on the registry
89 package Assemble the local package into a distributable tarball
90 pkgid Print a fully qualified package specification
91 publish Upload a package to the registry
92 r alias: run
93 read-manifest Print a JSON representation of a Cargo.toml manifest.
94 remove Remove dependencies from a Cargo.toml manifest file
95 report Generate and display various kinds of reports
96 rm alias: remove
97 run Run a binary or example of the local package
98 rustc Compile a package, and pass extra options to the compiler
99 rustdoc Build a package's documentation, using specified custom flags.
100 search Search packages in crates.io
101 t alias: test
102 test Execute all unit and integration tests and build examples of a local package
103 tree Display a tree visualization of a dependency graph
104 uninstall Remove a Rust binary
105 update Update dependencies as recorded in the local lock file
106 vendor Vendor all dependencies for a project locally
107 verify-project Check correctness of crate manifest
108 version Show version information
109 yank Remove a pushed crate from the index
110
111jon|master =:~/bitcoin/bitcoin/test/lint/test_runner$ cargo clippy && cargo run
112dyld[51046]: Symbol not found: __ZN4llvm12writeArchiveENS_9StringRefENS_8ArrayRefINS_16NewArchiveMemberEEEbNS_6object7Archive4KindEbbNSt3__110unique_ptrINS_12MemoryBufferENS7_14default_deleteIS9_EEEE
113 Referenced from: <21A05006-4BB0-3CF9-B95B-E327A38E2C39> /opt/homebrew/Cellar/rust/1.72.1/lib/librustc_driver-b4744cc222a0204b.dylib
114 Expected in: <E1E1E0C9-E06E-39DC-87F4-F3E473AE4DED> /opt/homebrew/Cellar/llvm/17.0.4/lib/libLLVM.dylib
115zsh: abort cargo clippy
116
117jon|master =:~/bitcoin/bitcoin/test/lint/test_runner$ cargo run
118error: process didn't exit successfully: `rustc -vV` (signal: 6, SIGABRT: process abort signal)
119--- stderr
120dyld[51064]: Symbol not found: __ZN4llvm12writeArchiveENS_9StringRefENS_8ArrayRefINS_16NewArchiveMemberEEEbNS_6object7Archive4KindEbbNSt3__110unique_ptrINS_12MemoryBufferENS7_14default_deleteIS9_EEEE
121 Referenced from: <21A05006-4BB0-3CF9-B95B-E327A38E2C39> /opt/homebrew/Cellar/rust/1.72.1/lib/librustc_driver-b4744cc222a0204b.dylib
122 Expected in: <E1E1E0C9-E06E-39DC-87F4-F3E473AE4DED> /opt/homebrew/Cellar/llvm/17.0.4/lib/libLLVM.dylib
I run (most of) the linters locally with a bash alias before pushing a change.
I’d suggest fixing your brew / cargo / rust installation. It looks broken, i.e:
error: process didn't exit successfully: `rustc -vV`
Just running the compiler doesn’t work.
cargo fmt and clippy part is just for writing new code and not strictly needed for just running code. You should be good to skip over it for now. However, if you want to write code, it may be better to look into getting it to work.
Thanks. Resolved (on ARM64 macOS 13.6 Ventura) as follows:
0$ brew remove rust
1$ curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs | sh
2$ rustc --version
3rustc 1.73.0 (cc66ad468 2023-10-03)
4$ cargo --list
(running cargo --list now includes the fmt command)
0$ cd ./test/lint/test_runner/ && cargo fmt && cargo clippy && cargo run
1 Checking test_runner v0.1.0 (/Users/jon/bitcoin/bitcoin/test/lint/test_runner)
2 Finished dev [unoptimized + debuginfo] target(s) in 0.16s
3 Compiling test_runner v0.1.0 (/Users/jon/bitcoin/bitcoin/test/lint/test_runner)
4 Finished dev [unoptimized + debuginfo] target(s) in 1.37s
5 Running `target/debug/test_runner`
Concept NACK
The integration of Rust into Core, further increases the technical debt. Each person’s ability to run tests locally is essential, and relying on CIs to manage Rust code is not a viable solution. In addition, introducing Rust can encourage the addition of other languages, risking fragmenting and further complicating the code base.
relying on CIs
This pull request does not change how much CI is relied upon. Previously, CI was running on all patches, and it is still after this pull request. Previously, it was possible to run any CI task locally, and it is still possible, in the same way. I am running the CI locally regularly and it obviously helps if others do the same. If you have issues running anything locally, it would be good to open an issue, so that it can be fixed. When filing an issue, adding context which documentation you followed and adding exact steps to reproduce is helpful.