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 #1257

Closed
wants to merge 1 commit into from
Closed

Conversation

storopoli
Copy link
Contributor

@storopoli storopoli commented Jan 6, 2024

Description

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

(Updated MSRV to 1.63)

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, #1156, #1165.

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 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 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 bumping hashbrown to 0.11.2 since it is compatible with MSRV of 1.63

  3. 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; }`
  4. 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}
  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

@storopoli storopoli force-pushed the storopoli/nix branch 2 times, most recently from c795b55 to ec5ba09 Compare January 6, 2024 17:08
storopoli added a commit to storopoli/bdk that referenced this pull request Jan 6, 2024
These typos are blocking the Nix typo CI in bitcoindevkit#1257
@storopoli storopoli mentioned this pull request Jan 6, 2024
8 tasks
notmandatory added a commit that referenced this pull request Jan 6, 2024
028caa9 fix(typos): existant -> existent (Jose Storopoli)

Pull request description:

  ### Description

  These typos are blocking the Nix typo CI in #1257

  ### Notes to the reviewers

  Blocking #1257

  ### Changelog notice

  - fix: typos in documentation
  -
  ### 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:
  notmandatory:
    ACK 028caa9

Tree-SHA512: 0bd91efd0eec55fdc537824435552c968858a5b827179b3f9f3f37785db3fa92d3e6f0c73023de0c506224c81217c402d5afa8a2f768fecaf6a3c8378226d184
@storopoli storopoli force-pushed the storopoli/nix branch 2 times, most recently from 77e177c to 2a488e4 Compare January 6, 2024 20:38
@storopoli storopoli mentioned this pull request Jan 6, 2024
45 tasks
@storopoli storopoli force-pushed the storopoli/nix branch 4 times, most recently from 37f31d7 to 69df634 Compare January 6, 2024 21:54
@casey-bowman
Copy link
Contributor

casey-bowman commented Jan 9, 2024

How does one update the CargoMSRV.lock file?

After the version of bip39 in the root Cargo.toml file changed to 2.0, I encounter an error when running
nix flake -L check --keep-failed

It appears to involve the 1.2.0 version that's in CargoMSRV.lock.

I tried changing the version to 2.0.0 in the root Cargo.toml

I ran the command
nix flake update
but that didn't update the CargoMSRV.lock file.

I tried removing this lock file, and that didn't work either.

I'm relatively new to nix. Perhaps instructions on handling such version updates in Cargo.toml that involve CargoMSRV.lock would be helpful in the NIX.md file.

Here is the code I started with, rebased to include recent commits to master

@storopoli
Copy link
Contributor Author

storopoli commented Jan 9, 2024

It is not that trivial.

  1. flake.nix: in the MSRV devshell change cargoArtifacts = null; (so that it won't trigger a broken compile when you enter this shell)
  2. enter the MSRV devshell with nix develop .#MSRV
  3. rm Cargo.lock
  4. cargo update (then you might need to also pin some deps)
  5. once the thing builds you can mv Cargo.lock CargoMSRV.lock
  6. reset the change in cargoArtifacts in flake.nix
  7. reset the removed Cargo.lock
  8. try nix flake -L check --keep-failed
  9. commit the new CargoMSRV.lock

I am planning to fix this and rebase today.
This is the workflow that'll be doing.

@casey-bowman
Copy link
Contributor

Thanks, @storopoli !

@casey-bowman
Copy link
Contributor

casey-bowman commented Jan 9, 2024

Last night I ran
nix flake -L check --keep-failed
on commit 2105f99

It ran successfully, except for one set of errors in the middle. I am attaching those errors, from somewhere in the middle of the console output, and a warning at the end of the console output showing which systems were not tested.

The check omitted these incompatible systems: aarch64-darwin, aarch64-linux, x86_64-darwin

The errors were all of the form

crates-audit> error: couldn't check if the package is yanked: not found: No such crate in crates.io index: addr2line

errors.txt

Here are the details of my x86-64 system -

Operating System - NixOS 24.05 (Uakari)
Hardware Model - Purism Librem 13 v2
Processor - Intel Core i5-6200U x 4
Memory - 31.3 GB

It's amazing what ground that flake covers!

@storopoli
Copy link
Contributor Author

these are http errors in the cargo-audit check. I am running these fine in my local machine and also in GitHub CI.
Maybe your machine is offline or you are blocking crates.io?

@dpc
Copy link

dpc commented Jan 10, 2024

these are http errors in the cargo-audit check. I am running these fine in my local machine and also in GitHub CI. Maybe your machine is offline or you are blocking crates.io?

nix build etc. runs in a sandbox and network calls are forbidden there.

These errors might be normal/to be ignored. The advisory-db is passed as a locked input, and not being able to check for yanked crates is just a limitation of running it in Nix, I guess.

BTW. In Fedimint we run the audit by updating the input to the latest version and then doing the update, to always get the audit against current advisories. https://github.com/fedimint/fedimint/blob/59a88ce18c1721acec0db89c6bb97d9ea18af0d9/.github/workflows/ci-nix.yml#L169

@storopoli
Copy link
Contributor Author

BTW. In Fedimint we run the audit by updating the input to the latest version and then doing the update, to always get the audit against current advisories.

Thanks for the tip I added this as well to our CI yml file

@storopoli storopoli force-pushed the storopoli/nix branch 2 times, most recently from 4853dde to 1921e8b Compare January 15, 2024 22:47
@storopoli storopoli force-pushed the storopoli/nix branch 2 times, most recently from e5868a1 to 9800282 Compare February 4, 2024 09:04
storopoli added a commit to storopoli/bdk that referenced this pull request Feb 4, 2024
More caught on by Nix CI in bitcoindevkit#1257.
notmandatory added a commit that referenced this pull request Feb 4, 2024
8d93fad chore: typos (Jose Storopoli)

Pull request description:

  More caught on by Nix CI in #1257.

  ### 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:
  evanlinjin:
    ACK 8d93fad
  notmandatory:
    ACK 8d93fad

Tree-SHA512: 28e0316d457658266b2af1de76b114f87ce7485e386ddecd805dda1266a4e8645612c0fa6bc921c58daa4886558b32b538cccbb1644c96c3bab638dd7c42ee2b
ci: update Flake.lock automatically

ci: test electrsd with optional download

ci: bitcoind no download option for Nix build

ci: esplora pkg

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(nix): pin crane

ci: remove `--keep-failed` in CI

ci(nix): refactor checks

ci: use cargo ci profile for build deps

feat(flake): update pkgs to 23.11 and pin bitcoind to 0.25
@storopoli
Copy link
Contributor Author

Closing in favor of #1320.
Cc @yanganto.

@storopoli storopoli closed this Mar 25, 2024
@storopoli storopoli deleted the storopoli/nix branch March 25, 2024 13:33
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
4 participants