Conversation
cf53d8e to
a3c34ae
Compare
|
The only potential issue I see with the *_EXEC env vars for |
|
Copy-pasting the discussion from discord (https://discord.com/channels/753336465005608961/753367451319926827/1161651733173391433): @evanlinjin: @danielabrozzoni :
Are there any other benefits I'm not seeing? @realeinherjar: With Nix would be an easy nix build test.WASM, nix build test nix build clippy @danielabrozzoni : @realeinherjar: @danielabrozzoni
rustup works fine? You can do cargo +1.57.0 for a specific rust version.
There should be no problem installing wasm-pack locally right? I think supporting unix-only is fine. @notmandatory @realeinherjar: @realeinherjar:
That's easy in CI, not necessarily so easy when CI fails and you need to debug on your pc |
f2d5e02 to
5e78044
Compare
74a5ecd to
1b69859
Compare
12ab93d to
55e6b2b
Compare
This comment was marked as resolved.
This comment was marked as resolved.
c535ecc to
f19efec
Compare
4438ca0 to
27d04b5
Compare
| rustTarget = pkgs.rust-bin.stable.latest.default; | ||
| # we pin clippy instead of using "stable" so that our CI doesn't break | ||
| # at each new cargo release | ||
| rustClippyTarget = pkgs.rust-bin.stable."1.67.0".default; |
There was a problem hiding this comment.
thought: Since everything is pinned, splitting between "latest and clippy" seems unnecessary and can have a cost of number of toolchains that need to be downloaded.
There was a problem hiding this comment.
I don't think so. We have 3 different Rust versions:
- latest
- clippy, pinned to 1.67.0
- MSRV, pinned to 1.57.0
| # just build bdk for now | ||
| cargoExtraArgs = "--locked --target wasm32-unknown-unknown -p bdk --no-default-features --features bitcoin/no-std,miniscript/no-std,bdk_chain/hashbrown,dev-getrandom-wasm"; | ||
| # env vars | ||
| CC = "${stdenv.cc.nativePrefix}cc"; |
There was a problem hiding this comment.
suggestion: Please verify if this even works, because IIRC stdenv.mkDerivation overrides CC and CXX anyway. If you want these changed, you should override stdenv https://github.com/ipetkov/crane/blob/master/docs/API.md#optional-attributes-15 . Again, IIRC.
dpc
left a comment
There was a problem hiding this comment.
Nice job. Left some thoughts and suggestions, but nothing major.
|
review walk through video here: https://www.youtube.com/watch?v=Mtfo97KLfWA&t=24s |
Just to clarify on the question asked in the video: While Nix on it's own can not guarantee byte-for-byte reproducibility, in practice that's usually the case. At least in Fedimint we do get byte-identical produced binaries between building machines with the same OS&architecture. I would expect this to be the case for BDK as well. See also trustix and https://reproducible.nixos.org/ . |
|
I just noticed that this PR misses two nice optimizations. Not a blocker for this PR, but nice to have in some future:
|
Re First, Second, with Nix it's almost effortless to just start Lastly, in Fedimint we have a custom random port allocator library, so we can ran whole distinct test suites in parallel (via GNU parallel) just to make the machine sweat 100% and CI finish faster. It has an additional benefit of introducing a lot of timing jitter, which is more likely to surface any lingering race conditions. I would suspect that BDK is somewhat more lightweight in a lot of ways than Fedimint, but it's probably good to know that there's a lot of room for improvements in case your CI/test suites feel slow. |
|
Oh. Additional benefit of |
danielabrozzoni
left a comment
There was a problem hiding this comment.
I just took a quick look, I need to do a more in-depth review, will get to that later.
I would open a different PR for the doc improvements, typos, and formatting. These changes can be reviewed and merged quickly, while the nix ci will take (I suppose) a bit longer, so there's no reason to keep them in the same PR.
Once the typos/docs fixes are in another PR, I would squash the commits of this PR. The purpose of having multiple commits is to simplify review; instead, if your PR is composed of too many commits that keep modifying each other, review is actually more difficult. If you want to read more: https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#committing-patches
ci: update Flake.lock automatically ci: test electrsd with optional download ci: bitcoind no download option for Nix build ci: esplora pkg same setup as fedimint fix(readme): add explainer about external deps ci: add cachix cache ci(nix): add rust audit capabilities ci: WASM checks ci(nix): add code coverage ci(nix): add rust nightly docs ci(nix): pre-commit-checks with typos fixed chore: add Nix instructions ci: move to DeterminateSystems/magic-nix-cache-action ci(nix): pin crane ci: remove `--keep-failed` in CI ci(nix): refactor checks ci: use cargo ci profile for build deps refactor(nix): libsDarwin inline check ci: update flake.lock
|
We can close this in favor of #1257. |
Description
This would make all the tests possible to run locally.
It enhances developer experience and facilitates onboarding of new contributors.
Additions:
rustsec/advisory-db.cargo {check,build,test}in the whole workspace.Superseds
.github/workflows/audit.yml.NIX.mdCloses bitcoindevkit/bdk_wallet#129.
Superseds #1122 and #1156.
Pinneds Dependencies:
bitcoind: pinned to0.25.0electrs: pinned toBlockstream's esplorausing the Fedimint deployment, check jkitman/nixpkgs@61ccef8 and https://github.com/fedimint/fedimint/blob/master/flake.nix#L5TODO:
llvmdeps.opensslerrors.WASM.MSRV.Add a custom
CargoMSRV.lock? (Or maybe aCargoMSRV.tomlwith the pinned MSRV dependencies and thencargo generate-lockfilewith Rust MSRV?)Depends on solving fix: build
crane-utilsusing a different toolchain ipetkov/crane#422cont_integration.ymltoflake.nix(all thecargo update -p)From the Fedimint suggestion we'll use
crane.This would allow caching of a lot of things
Then the user would call
nix buid -L .#MSRV --keep-failedand so on.This would also eliminate all the multiple
runs-onincont_integration.ymlto a single one where each step would be anameandrunthenix buildcommand.DeterminateSystems/magic-nix-cache-actionto cache stuff in CI.CONTRIBUTINGwith instructions. Use fedimint instructions for inspiration.nix develop:default: Rust latestMSRVWASMnix flake check:rustfmttest--all-features--no-default-featurestest--all-features--no-default-features--target wasm32-unknown-unknown)test-p bdk --no-default-features --features bitcoin/no-std,miniscript/no-std,bdk_chain/hashbrown,dev-getrandom-wasm-p esplora --target wasm32-unknown-unknown --no-default-features --features bitcoin/no-std,miniscript/no-std,bdk_chain/hashbrown,asynccachix/pre-commit-hooks.nixcommitizenrustfmtAdd a CI to update(is this possible?)Cargo.lockflake.lock.github/workflows/audit.ymlnumtide/devshellNix Commands
nix flake show: show all possible commands from the flake.nix develop: creates adevShellwith all the things you need to develop installed.Also handles ENV vars.
Currently is
bash,git,ripgrep,bitcoind(pinned),electrs(pinned),openssl,pkg-config,libiconv, and latest stable Rust with all goodies.It also handles specific MacOS deps: Apple XCode SDKs (for
bitcoindandelectrsdcrates).Open to suggestions on what to include.
nix develop .#MSRV: same but with Rust MSRV.nix flake check:rustfmt,nixpkgs-fmt(.nixfiles).clippyin all workspace (pinned to 1.67.0) with--all-features --all-targets -- -D warnings.cargo checkin all workspace (latest stable Rust).rustsec/advisory-db..#checks.${system}.{CHECK}nix build -L .: runscargo buildin all workspace with latest stable Rust.nix build -L .#stable: runscargo buildin all workspace with latest stable Rust.nix build -L .#MSRV: runscargo buildin all workspace with MSRV stable Rust.PLACEHOLDER: ...nix build -L .#checks.${system}.{CHECK}: runs a specificCHECK. In my casesystem = aarch64-darwinthen it isnix build .#checks.aarch64-darwin.CHECK.pre-commit-check: checks for typos, conventional commits,nixpkgs-fmt(.nixfiles).clippy: runsclippyin all workspace (pinned to 1.68.0) with--all-features --all-targets -- -D warnings.fmt: runscargo fmtwith--all -- --check --config format_code_in_doc_comments=truein all workspace with latest Rust.audit: checks dependencies for security advisory usingrustsec/advisory-db.latest:cargo buildin whole workspace using latest Rust.latestAll:cargo test --all-featuresin whole workspace using latest Rust.latestNoDefault:cargo test --no-default-featuresin whole workspace using latest Rust.latestNoStdBdk:cargo test -p bdk --no-default-features --features bitcoin/no-std,miniscript/no-std,bdk_chain/hashbrownusing latest Rust.latestNoStdChain:cargo test -p bdk_chain --no-default-features --features bitcoin/no-std,miniscript/no-std,hashbrownusing latest Rust.latestNoStdEsplora:cargo test -p bdk_esplora --no-default-features --features bitcoin/no-std,miniscript/no-std,bdk_chain/hashbrownusing latest Rust.MSRV:cargo buildin whole workspace using MSRV Rust.MRSVAll:cargo test --all-featuresin whole workspace using MSRV Rust.MSRVNoDefault:cargo test --no-default-featuresin whole workspace using MSRV Rust.MSRVNoStdBdk:cargo test -p bdk --no-default-features --features bitcoin/no-std,miniscript/no-std,bdk_chain/hashbrownusing MSRV Rust.MRSVNoStdChain:cargo test -p bdk_chain --no-default-features --features bitcoin/no-std,miniscript/no-std,hashbrownusing MSRV Rust.MSRVNoStdEsplora:cargo test -p bdk_esplora --no-default-features --features bitcoin/no-std,miniscript/no-std,bdk_chain/hashbrownusing MSRV Rust.Notes to the reviewers
We are adding automatic pre-commit checks with checks for typos, gpg-signed commits, conventional commits,
nixpkgs-fmt(.nixfiles).This is done automatically if the user has Nix and
devshellinstalled.If not, it will be checked on CI, or with a
nix flake check.I fixed a bunch of typos and added the
.typos.tomlto whitelist somethings like hashes, addresses, etc that were being flagged as false positives.I am adding in the building cache/tests a crate name and version.
This does not interact with the name or versioning in any of bdk's crates
Cargo.tomlTo avoid this nasty warning in Nix:
We are using
legacyPackagesinstead ofpackagesin thecibuild calls becauselegacyPackagesallows nested sets, e.g.:It makes a nice grouping of all CI stuff under the same call:
nix build -L .#ci.latest.{CHECK}Removed the
ci/dir with the Dockerfiles for hardware signer testing.We are not using them, removed in bdk v1.0.0-alpha.0 #793,
more specifically in 3f5a78a.
Related Move hardware signer into its own crate #872.
NOTE:
speculoscan be run under NixWe are moving from
mozilla/grcovtotaiki-e/cargo-llvm-cov.Why? 3 reasons:
grcovis a big thing, it does coverage for a lot of things C/C++, JS, Java, and Rust.cargo-llvm-covuses Rust's native coverage tools via LLVM.crane(craneLib.cargoLlvmCov) supports nativelycargo-llvm-covwhich will be much easier to make it work and maintainfedimintalso usecargo-llvm-covwithcraneso it makes easier to collaborate in improvements and issues.Potential issues:
We had to remove
Cargo.lockfrom the.gitignore. Nix (crane) needs it for deterministic stuff.From the
craneFAQ:Also Rust changed their
Cargo.lockcommit policy a couple months ago:Mismatch versions between the executables under the
*_EXECenv vars forbitcoind/electrsdcrates and the version the crate thinks to have.Centralizes CI maintainability to people that have Nix experience.
We are removing the auto-download feature of
bitcoindandelectrsdin thebitcoind_rpcandesploracrates. I added an explanation in the repo and crates'README.mds. This also simplifies a little bit the MSRV pinning of deps.Changelog notice
Checklists
All Submissions:
cargo fmtandcargo clippybefore committingNew Features:
Bugfixes: