- Add empty bitcoinsuite-core crate. We keep it empty as we don't have any lints yet, which will be added next.
- Add CMake files to enable Chronik via BUILD_BITCOIN_CHRONIK CMake flag
- Find Rust via Corrosion
- Add check-bitcoinsuite target to run cargo test on all the rust crates starting with bitcoinsuite-.
Details
- Reviewers
Fabien - Group Reviewers
Restricted Project - Commits
- rABC5615fb72f897: [Chronik] Add minimal bitcoinsuite-core Rust crate + CMake to `cargo test` it
- Install Rust 1.61 and Corrosion 0.2, see D11663
- cmake .. -GNinja -DBUILD_BITCOIN_CHRONIK=on
- ninja check-bitcoinsuite
- Optionally (will be automated next): in src/rust: None of cargo check, cargo doc and cargo clippy raise warnings
Diff Detail
- Repository
- rABC Bitcoin ABC
- Branch
- chronik-add-rust
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 19480 Build 38693: Build Diff build-without-wallet · build-clang-tidy · build-diff · build-clang · lint-circular-dependencies · build-debug Build 38692: arc lint + arc unit
Event Timeline
There are several copyright headers missing.
What test is the target running ? None for the moment ?
Also the linter should be added to our linters running from arc lint if possible.
src/CMakeLists.txt | ||
---|---|---|
686 ↗ | (On Diff #34138) | I don't expect to build rust. You probably want to call this director chronik instead. That also removes the need for the above comment |
src/rust/.gitignore | ||
2 ↗ | (On Diff #34138) | We usually use out of tree build, is this possible ? So there is no file generated in the source tree |
src/rust/CMakeLists.txt | ||
1 ↗ | (On Diff #34138) | Copyright is missing |
3 ↗ | (On Diff #34138) | Are we checking rust ? |
8 ↗ | (On Diff #34138) | Same question, why forcing in tree build ? |
There are several copyright headers missing.
Will fix, my bad.
What test is the target running ? None for the moment ?
Yes, it just runs the test suite, which finds no tests and succeeds. I wanted to keep things small and simple.
Also the linter should be added to our linters running from arc lint if possible.
There's some question marks on how to lint exactly, especially with Phabricator integration, I'd prefer to do it in a later diff?
I don't expect to build rust. You probably want to call this director chronik instead. That also removes the need for the above comment
I thought this too yet went for rust, but this might be a bigger discussion. Several things:
- What if we add non-Chronik Rust code, like adding our Rust BIP70 server (@Mengerian showed interest in this), or SLP Post Office, or a SWaP server? Then we could have this (idiomatic) folder structure in src/rust, each being one Rust crate:
- bip70-server
- bitcoinsuite-core
- bitcoinsuite-slp
- ...
- chronik-http
- chronik-indexer
- chronik-rocksdb
- ...
- chronik-rocksdb
- post-office-pool
- post-office-server
- swap-server
- ...
- I expect this folder to evolve over time into a more and more sophisticated library, which might add crates for tx construction, signing etc. at some point, basically a set of general-purpose Rust crates. Cargo makes it very easy to reuse these, especially if we extract the src/rust folder automatically to GitHub like we do for secp256k1, then people can reference any crate in their dependencies, like via bitcoinsuite-core = { git = "https://github.com/Bitcoin-ABC/ecash-rust" } (see here). In that case, it makes more sense to just call the folder rust and keep all things written in Rust in there.
- Cargo has one workspace file (called Cargo.toml) and one lockfile (called Cargo.lock). The latter makes it such that all crates have a consistent set of (outside) dependencies, so there should be only one workspace and lockfile. We could have src/chronik, src/bitcoinsuite, src/post-office etc., but then where to put the workspace & lockfiles?
- corrosion_import_crate seems to work best if there's just one workspace file, and I'd like to maintain only one bridge to bitcoind. If there's just one rust folder, all the future stuff we could add could depend on that one bridge.
What is unclear to me is how to do the build flag. Say we have BUILD_BITCOIN_CHRONIK and BUILD_BITCOIN_BIP70_SERVER. Should there also be a BUILD_BITCOIN_RUST?. Both would always imply that. I think it will be that there will be some kind of this down the line:
set(BUILD_BITCOIN_RUST, BUILD_BITCOIN_CHRONIK || BUILD_BITCOIN_BIP70_SERVER) (don't know how to do logical OR in CMake, doesn't matter)
with this in src/CMakeLists.txt
if(BUILD_BITCOIN_RUST) add_subdirectory(rust) endif()
and then in src/rust/CMakeLists.txt we configure Chronik and the BIP70 server depending on the states of BUILD_BITCOIN_CHRONIK and BUILD_BITCOIN_BIP70_SERVER.
We usually use out of tree build, is this possible ? So there is no file generated in the source tree
Yes, good catch. Will fix. There's one thing though: Using my IDE, it generates some artefacts into src/rust/target. I think it would be nice to add an ignore for that for convenience so people don't have to do crazy configs just to move their IDE's target into the build folder.
Are we checking rust ?
We're testing everything in the src/rust folder. Similar reasoning as in the directory question—although this could easily be split up by project (check-chronik, check-bitcoinsuite, check-bip70-server, check-post-office) etc. I think that would be a good idea.
I'll keep the discussion on the directory name for another time as I need to learn about the tools first.
Yes, it just runs the test suite, which finds no tests and succeeds. I wanted to keep things small and simple.
OK
There's some question marks on how to lint exactly, especially with Phabricator integration, I'd prefer to do it in a later diff?
Sure, and I can help with that
Yes, good catch. Will fix. There's one thing though: Using my IDE, it generates some artefacts into src/rust/target. I think it would be nice to add an ignore for that for convenience so people don't have to do crazy configs just to move their IDE's target into the build folder.
We don't add code/configs related to IDE in the source tree as they are different for every developer and as such will never be maintained properly. See https://github.com/Bitcoin-ABC/bitcoin-abc/blob/master/doc/developer-notes.md#ignoring-ideeditor-files
We're testing everything in the src/rust folder. Similar reasoning as in the directory question—although this could easily be split up by project (check-chronik, check-bitcoinsuite, check-bip70-server, check-post-office) etc. I think that would be a good idea.
Yes that makes more sense to check by project. You can eventually add it to some magic targets like check or check-all at a later point in time if needed.
Resolve issues raised:
- Added copyright
- Moved /src/rust to /chronik, moved workspace file and lockfile to /
- Made crate empty instead of some useless hash structs
- Removed #[warn] lints for now
- Move test artefacts into ${CMAKE_CURRENT_BINARY_DIR}/cargo/build
- Rename test target to check-bitcoinsuite (will add check-chronik, ... later)
chronik/CMakeLists.txt | ||
---|---|---|
5–26 ↗ | (On Diff #34144) | The whole file can be much simplified, and ninja clean now works as expected |
7 ↗ | (On Diff #34144) | Can you require a min version of rustc and cargo from corrosion ? I tried the target but got this: The package requires the Cargo feature called `edition2021`, but that feature is not stabilized in this version of Cargo (1.55.0 (32da73ab1 2021-08-23)). Consider trying a newer version of Cargo (this may require the nightly release). Updating to 1.61 fixed it but it's better UX to fail early with a nicer error message |
13 ↗ | (On Diff #34144) | You can avoid all this get_property calls by using generator expressions, see below |
19 ↗ | (On Diff #34144) | You need to use double quotes to make it work for paths with space, and add this dir to the clean list so the magic clean target can remove it. |