Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ci: move to Nix #1165

Closed

Conversation

realeinherjar
Copy link
Contributor

@realeinherjar realeinherjar commented Oct 10, 2023

Description

This would make all the tests possible to run locally.
It enhances developer experience and facilitates onboarding of new contributors.

Additions:

  • Crate auditing capabilities using rustsec/advisory-db.
  • Local and CI caching of all artifacts from running cargo {check,build,test} in the whole workspace.
    Superseds .github/workflows/audit.yml.
  • Pre-commit checks for everything that we enforce (gpg-signed/conventional commits, and some goodies like typos, so docs: fix spelling errors #1086 kind of things are not necessary anymore)
  • Instructions and rationale in NIX.md

Closes #1162.
Superseds #1122 and #1156.

Pinneds Dependencies:

TODO:

  • Fix llvm deps.
  • Fix openssl errors.
  • Fix dependencies on MacOS.
  • Fix WASM.
  • Fix MSRV.
    Add a custom CargoMSRV.lock? (Or maybe a CargoMSRV.toml with the pinned MSRV dependencies and then cargo generate-lockfile with Rust MSRV?)
    Depends on solving fix: build crane-utils using a different toolchain ipetkov/crane#422
  • move all the logic from cont_integration.yml to flake.nix (all the cargo 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-failed and so on.
    This would also eliminate all the multiple runs-on in cont_integration.yml to a single one where each step would be a name and run the nix build command.
  • Add DeterminateSystems/magic-nix-cache-action to cache stuff in CI.
  • Update CONTRIBUTING with instructions. Use fedimint instructions for inspiration.
  • nix develop:
    • default: Rust latest
    • MSRV
    • WASM
  • nix flake check:
    • clippy
    • audit
    • rustfmt
    • Rust latest test
      • --all-features
      • --no-default-features
    • MSRV test
      • --all-features
      • --no-default-features
    • WASM (--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,async
    • cachix/pre-commit-hooks.nix
      • signed commits
      • conventional commits with commitizen
      • typos
      • rustfmt
  • Add a CI to update Cargo.lock (is this possible?)
  • Add a CI to update flake.lock
  • Delete .github/workflows/audit.yml
  • Port Code Coverage to Nix
  • Nixify the Nightly Docs CI
  • Add numtide/devshell
  • Clean up commit messages (this is a mess of squash fixup and ammends 😂)

Nix Commands

  • nix flake show: show all possible commands from the flake.
  • nix develop: creates a devShell with 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 bitcoind and electrsd crates).
    Open to suggestions on what to include.
    • nix develop .#MSRV: same but with Rust MSRV.
  • nix flake check:
    • checks for typos, gpg-signed commits, conventional commits, rustfmt, nixpkgs-fmt (.nix files).
    • runs clippy in all workspace (pinned to 1.67.0) with --all-features --all-targets -- -D warnings.
    • runs cargo check in all workspace (latest stable Rust).
    • checks dependencies for security advisory using rustsec/advisory-db.
    • test everything!!! (see below the individual checks in .#checks.${system}.{CHECK}
  • nix build -L .: runs cargo build in all workspace with latest stable Rust.
    • nix build -L .#stable: runs cargo build in all workspace with latest stable Rust.
    • nix build -L .#MSRV: runs cargo build in all workspace with MSRV stable Rust.
    • PLACEHOLDER: ...
  • nix build -L .#checks.${system}.{CHECK}: runs a specific CHECK. In my case system = aarch64-darwin then it is nix build .#checks.aarch64-darwin.CHECK.
    • pre-commit-check: checks for typos, conventional commits, nixpkgs-fmt (.nix files).
    • clippy: runs clippy in all workspace (pinned to 1.68.0) with --all-features --all-targets -- -D warnings.
    • fmt: runs cargo fmt with --all -- --check --config format_code_in_doc_comments=true in all workspace with latest Rust.
    • audit: checks dependencies for security advisory using rustsec/advisory-db.
    • latest: cargo build in whole workspace using latest Rust.
    • latestAll: cargo test --all-features in whole workspace using latest Rust.
    • latestNoDefault: cargo test --no-default-features in whole workspace using latest Rust.
    • latestNoStdBdk: cargo test -p bdk --no-default-features --features bitcoin/no-std,miniscript/no-std,bdk_chain/hashbrown using latest Rust.
    • latestNoStdChain: cargo test -p bdk_chain --no-default-features --features bitcoin/no-std,miniscript/no-std,hashbrown using latest Rust.
    • latestNoStdEsplora: cargo test -p bdk_esplora --no-default-features --features bitcoin/no-std,miniscript/no-std,bdk_chain/hashbrown using latest Rust.
    • MSRV: cargo build in whole workspace using MSRV Rust.
    • MRSVAll: cargo test --all-features in whole workspace using MSRV Rust.
    • MSRVNoDefault: cargo test --no-default-features in whole workspace using MSRV Rust.
    • MSRVNoStdBdk: cargo test -p bdk --no-default-features --features bitcoin/no-std,miniscript/no-std,bdk_chain/hashbrown using MSRV Rust.
    • MRSVNoStdChain: cargo test -p bdk_chain --no-default-features --features bitcoin/no-std,miniscript/no-std,hashbrown using MSRV Rust.
    • MSRVNoStdEsplora: cargo test -p bdk_esplora --no-default-features --features bitcoin/no-std,miniscript/no-std,bdk_chain/hashbrown using MSRV Rust.

Notes to the reviewers

  1. We are adding automatic pre-commit checks with checks for typos, gpg-signed commits, conventional commits, nixpkgs-fmt (.nix files).
    This is done automatically if the user has Nix and devshell installed.
    If not, it will be checked on CI, or with a nix flake check.
    I fixed a bunch of typos and added the .typos.toml to whitelist somethings like hashes, addresses, etc that were being flagged as false positives.

  2. 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.toml

    To avoid this nasty warning in Nix:

    trace: warning: crane will use a placeholder value since `name` cannot be found in /nix/store/ja4yr1m9ba72gy3pbr2vg540zjdbzp33-source/Cargo.toml
    to silence this warning consider one of the following:
    - setting `pname = "...";` in the derivation arguments explicitly
    - setting `package.name = "..."` or `workspace.package.name = "..."` in the root Cargo.toml
    - explicitly looking up the values from a different Cargo.toml via
      `craneLib.crateNameFromCargoToml { cargoToml = ./path/to/Cargo.toml; }`
  3. We are using legacyPackages instead of packages in the ci build calls because legacyPackages allows nested sets, e.g.:

    legacyPackages = ci = {
      latest = {
        all = ...;
        noDefault = ...;
        ...
      };
      MSRV = { ... };
      WASM = { ... };
      codeCoverage = ...;
    };

    It makes a nice grouping of all CI stuff under the same call:

    nix build -L .#ci.latest.{CHECK}
  4. 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: speculos can be run under Nix

  5. We are moving from mozilla/grcov to taiki-e/cargo-llvm-cov.
    Why? 3 reasons:

    1. Mozilla's grcov is a big thing, it does coverage for a lot of things C/C++, JS, Java, and Rust.
      cargo-llvm-cov uses Rust's native coverage tools via LLVM.
    2. crane (craneLib.cargoLlvmCov) supports natively cargo-llvm-cov which will be much easier to make it work and maintain
    3. Our friends at fedimint also use cargo-llvm-cov with crane so it makes easier to collaborate in improvements and issues.

Potential issues:

  • We had to remove Cargo.lock from the .gitignore. Nix (crane) needs it for deterministic stuff.
    From the crane FAQ:

    First consider if there is a release of this project available with a lock file as it may be simpler and more consistent to use the exact dependencies published by the project itself. Projects published on crates.io always come with a lock file and nixpkgs has a fetchCrate fetcher which pulls straight from crates.io.

    Also Rust changed their Cargo.lock commit policy a couple months ago:

    For years, the Cargo team has encouraged Rust developers to commit their Cargo.lock file for packages with binaries but not libraries. We now recommend people do what is best for their project. To help people make a decision, we do include some considerations and suggest committing Cargo.lock as a starting point in their decision making. To align with that starting point, cargo new will no longer ignore Cargo.lock for libraries as of nightly-2023-08-24. Regardless of what decision projects make, we encourage regular testing against their latest dependencies.

  • Mismatch versions between the executables under the *_EXEC env vars for bitcoind/electrsd crates 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 bitcoind and electrsd in the bitcoind_rpc and esplora crates. 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:

  • I've signed all my commits
  • I followed the contribution guidelines
  • I ran cargo fmt and cargo clippy before committing

New Features:

  • I've added tests for the new feature
  • I've added docs for the new feature

Bugfixes:

  • This pull request breaks the existing API
  • I've added tests to reproduce the issue which are now passing
  • I'm linking the issue being fixed by this PR

@realeinherjar realeinherjar force-pushed the einherjar/nix branch 4 times, most recently from cf53d8e to a3c34ae Compare October 11, 2023 12:03
@RCasatta
Copy link
Member

The only potential issue I see with the *_EXEC env vars for bitcoind/electrsd crate is a potential mismatch of version between the executable and the version the crate thinks to have. I am just mentioning it so that it's known, it may be a compromise worth taking.

@danielabrozzoni
Copy link
Member

Copy-pasting the discussion from discord (https://discord.com/channels/753336465005608961/753367451319926827/1161651733173391433):

@evanlinjin:
I also don't know Nix. How difficult is it to learn? I understand that the main benefit is a reproducible environment? However, it does seem we can mostly get away with a bash script?

@danielabrozzoni :
I have the same concerns. I've been a nixos user for 3 years now, and I see the benefits for my local environment, but I'm not sure if the pros outweight the cons in the CI. The main benefit described in the PR is that it makes tests possible to run locally, which is important, but personally once I got used to the CI failures I just learnt how to reproduce those locally (either a cargo test if it's a test failure, or using cargo 1.57.0 + cargo update if it's a msrv problem, or cargo fmt/cargo clippy). The main concerns I have are:

  • most of the team don't know nix (as far as I know?), so updating the CI might take longer than usual for us, making us less productive, at least in the short term
  • rewriting and reviewing the ci update takes some time that might be better spent working on pushing bdk 1.0

Are there any other benefits I'm not seeing?

@realeinherjar:
About the bash script: Yes, but then how you would run different versions of Rust, and how would you test WASM. That can be a bash script but the dependency hell and complicated instructions on how to instantiate and switch to these different environments. With the caveat for MacOS, Linux, etc

With Nix would be an easy nix build test.WASM, nix build test nix build clippy

@danielabrozzoni :
Ohhh I totally forgot about wasm, yeah using nix there made my life easier in the past

@realeinherjar:
Also MSRV is 1.57, clippy is 1.68, stable is well stable...

@danielabrozzoni
Alright, I need to learn flakes 🤷‍♀️❄️

@evanlinjin:

but then how you would run different versions of Rust

rustup works fine? You can do cargo +1.57.0 for a specific rust version.

and how would you test WASM

There should be no problem installing wasm-pack locally right?

I think supporting unix-only is fine.

@notmandatory
I agree we don't need to hold up the alpha.2 for the current CI issue, and I think the nix stuff is at least something to research for maybe a beta target to give everyone time to review and evaluate the pros/cons

@realeinherjar:
Easy to onboard new devs. Just tell them to do nix develop and they should have an environment with everything they need, rust, rust-analyzer, all ENV variables, any pre-commits hooks etc bitcoind and electrsd setup
Steve Myers — Today at 3:22 PM
I've heard good things from new devs on the fedimint project about how easy it is to get started due to their nix setup

@realeinherjar:
Fedimint is an inspiration 🙂

@danielabrozzoni :

There should be no problem installing wasm-pack locally right?

That's easy in CI, not necessarily so easy when CI fails and you need to debug on your pc
Alright, I think we can all agree to move this to the next milestone, and I'll copy this discussion in the PR

@realeinherjar realeinherjar force-pushed the einherjar/nix branch 2 times, most recently from f2d5e02 to 5e78044 Compare October 12, 2023 11:42
@realeinherjar realeinherjar changed the title ci: move to Nix [WIP] ci: move to Nix Oct 12, 2023
@realeinherjar realeinherjar force-pushed the einherjar/nix branch 7 times, most recently from 74a5ecd to 1b69859 Compare October 12, 2023 18:01
flake.nix Outdated Show resolved Hide resolved
@realeinherjar realeinherjar force-pushed the einherjar/nix branch 3 times, most recently from 12ab93d to 55e6b2b Compare October 13, 2023 13:08
@realeinherjar

This comment was marked as resolved.

@realeinherjar realeinherjar force-pushed the einherjar/nix branch 3 times, most recently from c535ecc to f19efec Compare October 13, 2023 20:28
Cargo.toml Outdated Show resolved Hide resolved
@realeinherjar realeinherjar force-pushed the einherjar/nix branch 5 times, most recently from 4438ca0 to 27d04b5 Compare October 13, 2023 22:25
flake.nix Outdated Show resolved Hide resolved
flake.nix Show resolved Hide resolved
@dpc
Copy link

dpc commented Oct 27, 2023

BTW. typos can fix all the typos automatically. We have a command for it in a justfile.

flake.nix Show resolved Hide resolved
Copy link
Member

@danielabrozzoni danielabrozzoni left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@realeinherjar realeinherjar mentioned this pull request Nov 15, 2023
8 tasks
danielabrozzoni added a commit that referenced this pull request Nov 16, 2023
27a63ab chore: typos fixed (Einherjar)

Pull request description:

  ### Description

  Fixes the typos and remove unused speculos dockerfiles that was done in #1165.
  Moving these changes into this PR to be merged first.
  Then, we can rebase #1165 and make it only related to CI and Nix.
  (Maybe do a big squash 😄)

  ## Note to Reviewers

  About the speculos stuff, we are not using them, removed in #793,
  more specifically in 3f5a78a.

  ### Changelog notice

  - Fix typos in codebase and docs
  - Remove unused CI tests with hardware signer Dockerfiles

  ### Checklists

  #### All Submissions:

  * [x] I've signed all my commits
  * [x] I followed the [contribution guidelines](https://github.com/bitcoindevkit/bdk/blob/master/CONTRIBUTING.md)
  * [x] I ran `cargo fmt` and `cargo clippy` before committing

  #### New Features:

  * [ ] I've added tests for the new feature
  * [ ] I've added docs for the new feature

  #### Bugfixes:

  * [ ] This pull request breaks the existing API
  * [ ] I've added tests to reproduce the issue which are now passing
  * [ ] I'm linking the issue being fixed by this PR

ACKs for top commit:
  danielabrozzoni:
    ACK 27a63ab

Tree-SHA512: a01101d0741e2b0e1d1254b5cae670c5a936bb0b89332c102feb57d58d2b9ae995ed4436068b0dc5fae73dbe22431c3143d6e04cdc12eab991bd726cfd2fbe25
@realeinherjar realeinherjar force-pushed the einherjar/nix branch 2 times, most recently from 41fab99 to bb486c4 Compare November 17, 2023 12:26
realeinherjar added a commit to realeinherjar/bdk that referenced this pull request Nov 17, 2023
More typos fix before bitcoindevkit#1165
@realeinherjar realeinherjar mentioned this pull request Nov 17, 2023
8 tasks
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
@notmandatory notmandatory mentioned this pull request Dec 5, 2023
8 tasks
@storopoli storopoli mentioned this pull request Jan 6, 2024
45 tasks
@storopoli
Copy link
Contributor

We can close this in favor of #1257.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

ci: workflows failing due to "Connection timed out" errors
7 participants