add nix flake with dev shells and limited crane based checks by nothingmuch · Pull Request #455 · payjoin/rust-payjoin

nothingmuch · 2025-01-02T23:15:56Z

usage:

developer shell: nix develop for nightly, nix develop .#msrv for msrv, nix flake show will list available dev shells.

checks: nix flake check --print-build-logs will run various lints and a limited range of unit tests, due to limitations of running docker in the nix sandbox and the current state of our integration tests

nix code is formatted with alejandra, a rust based formatter

followup issues and PRs with additional checks will be added after resolving some blocking problems, e.g. cargo audit or cargo deny complaints, shell script formatting, etc. i will open draft PRs for this additional stuff shortly.

some open questions:

should this be run in CI? IMO that would be desirable, but can slow CI results
should nix flake update be run periodically in a github workflow? i'd be in favor, but then we really want CI
how should this usage be documented? i guess the README should mention it, but not sure into how much detail it should go
do we want to build docker images using nix? IME that works pretty nicely and is more reproducible and maintainable than dockerfiles with whatever surprises ubuntu has in stock
should .envrc be included by default? maybe a .envrc.default? or .gitignore should be used, so that people can set up one to their liking? i rely on direnv/envrc for rust-analyzer in emacs FWIW, i find it's much nicer to have per-project toolchains managed that way but editor support is not as ubiquitous

an .envrc file that uses the flake is provided for convenience also includes nix formatting using alejandra, and a flake check that enforces formatting of nix code

- rust-analyzer - cargo watch - cargo nextest - cargo edit

coveralls · 2025-01-02T23:19:12Z

Pull Request Test Coverage Report for Build 12600527187

Details

0 of 0 changed or added relevant lines in 0 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.04%) to 61.742%

Totals
Change from base Build 12586289971:	0.04%
Covered Lines:	2942
Relevant Lines:	4765

💛 - Coveralls

flake.nix

the checks are limited, nextest only runs the unit tests without --all-features, because the integration tests require bitcoind and redis. unfortunately redis is made available via a docker container, and running docker in the nix sandbox is not possible

- NIGHTLY and UNSTABLE were unused, presumably intended to be exported as environment variables - quoting of interpolated variable (not strictly necessary for crate names)

DanGould

Works as advertised tACK e932911 (where t is an explicitly small t), though I don't understand all of it and nix flake check --print-build-logs takes a year to run

DanGould · 2025-01-03T03:41:10Z

flake.nix

+            msrv = stable.${msrv}.default;
+            stable = stable.latest.default;
+            nightly = nightly.latest.default;


How do we document how to run these devShells? nix develop .#$VERSION was not intuitive to me

not sure if it makes sense in README or some sort of dev documentation, but either or should contain some examples and discuss direnv, the devshells, and the checks

This definitely fixes the problem of having the right toolchain for testing so I ask that that gets documented in README and just that. Everything else is neat but doesn't exactly solve a particular problem we've experienced yet afaict

DanGould · 2025-01-03T04:11:14Z

flake.nix

I regret to inform you running nix flake check --print-build-logs is not intuitive. Is it supposed to be for the nixpilled? IDK what crane does or what any of these changes do

crane parses cargo packages and produces nix package derivation for them

it works in stages:

first it builds all of the dependencies of the workspace

then it builds individual crate packages, with access to the previously built deps

finally it provides various lints and test derivations, exposed here via nix flake check, some of which reuse the outputs of steps 1 or 2

the checks can be done individually or separately, and are quite costly since they build everything both with nightly and msrv, but should be faster to run after minor code changes due to the caching of the workspace dependencies (caching also works in CI potentially)

the main advantage over shell scripts is that nix can provide a lockfile (flake.lock) for all of the various unix dependencies (anything packaged in nixpkgs), in this case shfmt and shellcheck for example, ensuring that we can use that stuff without dealing with github's bloated and brittle ubuntu images.

Seems like some interesting and convenient tooling to tinker with and make our dev environments and CI more reliable, AND most importantly giving new devs a path to easily get started contributing. But perhaps it's not the glaring priority of other broken parts preventing more integrations from implementing a maintainable payjoin stack?

DanGould · 2025-01-03T04:12:17Z

flake.nix

+            shfmt = simpleCheck rec {
+              name = "shell-checks";
+              src = pkgs.lib.sources.sourceFilesBySuffices ./. [".sh"];
+              nativeBuildInputs = [pkgs.shfmt];
+              checkPhase = ''
+                shfmt -d -s -i 4 -ci ${src}
+              '';
+            };


If this is an option it needs to be a CI check (part of ./contrib/lint.sh?) otherwise we're gonna get fat diffs with lots of formatting changes between diffs that don't run shfmt

./contrib/lint.sh would be tricky without nix since installing shfmt is less standard

just track it

DanGould · 2025-01-03T16:10:07Z

RE: "some open questions:"

should this be run in CI? IMO that would be desirable, but can slow CI results

IMO the question is must this be run in CI? Or when must this be run in CI and the answer is when building depends on it, e.g. our test/lint checks in CI depend on nix instead of .sh scripts. When we get there, yes. Until we make the swap, no.

should nix flake update be run periodically in a github workflow? i'd be in favor, but then we really want CI

What happens if the flake is not updated? If it's not a critical bug I think we can leave it manual until the test infra is nixpilled.

how should this usage be documented? i guess the README should mention it, but not sure into how much detail it should go

Readme please. A demo on how to allow and run individual devShells seems to be the main contribution devs want to know about. A 1-sentence explainer of nix flake check [--print-build-logs] perhaps with a link to your comment needs to be documented if you think it's useful for others to use. I probably won't be using it yet because it seems it would just slow down flow vs using scripts unless there was some environment discrepancy between devs.

do we want to build docker images using nix? IME that works pretty nicely and is more reproducible and maintainable than dockerfiles with whatever surprises ubuntu has in stock

when must we build docker images using nix? When doing so can be done without adding a bunch of new nix tooling and it's time to update the Dockerfiles.

should .envrc be included by default? maybe a .envrc.default? or .gitignore should be used, so that people can set up one to their liking? i rely on direnv/envrc for rust-analyzer in emacs FWIW, i find it's much nicer to have per-project toolchains managed that way but editor support is not as ubiquitous

Yes .envrc by default. Give Grug sane defaults and let the tinkerers tinker. .envrc.default is a local minimum where both Grug and Tinker still have to make edits to get things working, and there's no real working default for Tinker by definition, so give Grug the default.

This only explains how to obtain different rust toolchains using direnv or `nix develop`, since flake checks are currently limited by redis/docker issue.

DanGould

Beautiful.

nothingmuch · 2025-01-03T16:25:51Z

RE: "some open questions:"

should this be run in CI? IMO that would be desirable, but can slow CI results

IMO the question is must this be run in CI? Or when must this be run in CI and the answer is when building depends on it, e.g. our test/lint checks in CI depend on nix instead of .sh scripts. When we get there, yes. Until we make the swap, no.

The only thing is the shellcheck & shell formatting stuff. We can run only those lints without building the crane packages in CI, nix flake check defaults to all checks, but only the package building (and the dependent nextest ones) are slow.

What happens if the flake is not updated? If it's not a critical bug I think we can leave it manual until the test infra is nixpilled.

The main issue is that the specific instance of rust nightly is locked in the flake.lock.

Readme please. A demo on how to allow and run individual devShells seems to be the main contribution devs want to know about.

Pushed a simple change to README

A 1-sentence explainer of nix flake check [--print-build-logs] perhaps with a link to your comment needs to be documented if you think it's useful for others to use. I probably won't be using it yet because it seems it would just slow down flow vs using scripts unless there was some environment discrepancy between devs.

For now let's hold off on this distraction, the caveat of not running any of the valuable tests yet makes it more of a distraction IMO

do we want to build docker images using nix? IME that works pretty nicely and is more reproducible and maintainable than dockerfiles with whatever surprises ubuntu has in stock

when must we build docker images using nix? When doing so can be done without adding a bunch of new nix tooling and it's time to update the Dockerfiles.

Not sure that'll ever be a "must". The main disadvantage of the dockerfiles in their current form is that "rust:1.75-slim" is not a stable reference, and adding hashes to dockerfiles to ensure they are reproducible introduces churn & potential for errors. If we want to support running the directory in a container then knowing exactly what goes into the container, and not having it depend on when docker build was run is desirable, in particular WRT vulnerability scanning (e.g. "certbot/certbot" or "redis:latest" are imprecise versions, if these things come from nixpkgs and are comitted cryptographically in a flake.lock, that is easier to support, esp. for things like vulnerability scanning)

nothingmuch added 2 commits January 2, 2025 23:51

nix flake with nightly, stable, and msrv devshells

bab0756

an .envrc file that uses the flake is provided for convenience also includes nix formatting using alejandra, and a flake check that enforces formatting of nix code

flake: add common dev tooling to dev shells

f8b69d2

- rust-analyzer - cargo watch - cargo nextest - cargo edit

nothingmuch requested review from DanGould and spacebear21 January 2, 2025 23:16

nothingmuch commented Jan 3, 2025

View reviewed changes

flake.nix Outdated Show resolved Hide resolved

nothingmuch added 5 commits January 3, 2025 02:14

format shell scripts with shfmt

548118a

flake: check shell script formatting with shfmt

850dda1

fix shellcheck violations

058f11a

- NIGHTLY and UNSTABLE were unused, presumably intended to be exported as environment variables - quoting of interpolated variable (not strictly necessary for crate names)

flake: use shellcheck in flake checks

e932911

nothingmuch force-pushed the flake branch from 1e1b6c6 to e932911 Compare January 3, 2025 01:17

DanGould approved these changes Jan 3, 2025

View reviewed changes

Document nix devshells in README

e3e5f8d

This only explains how to obtain different rust toolchains using direnv or `nix develop`, since flake checks are currently limited by redis/docker issue.

DanGould approved these changes Jan 3, 2025

View reviewed changes

DanGould merged commit dd65265 into payjoin:master Jan 3, 2025
6 checks passed

Conversation

nothingmuch commented Jan 2, 2025

Uh oh!

coveralls commented Jan 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 12600527187

Details

💛 - Coveralls

Uh oh!

Uh oh!

DanGould left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DanGould commented Jan 3, 2025

Uh oh!

DanGould left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nothingmuch commented Jan 3, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

coveralls commented Jan 2, 2025 •

edited

Loading

DanGould left a comment •

edited

Loading