Move CI checks to Makefile

[robinkrahl]

Executing all CI checks locally is currently non-trivial. What do you think about moving the cargo build, cargo clippy and cargo fmt invocations to the Makefile to make it easier to check changes before pushing them? For reference, I just did the same for nitrokey-rs.

[d-e-s-o]

I think that makes sense. I've also been doing that on some of my projects. My approach was a bit different, though, in that I kept the Makefile simple and focused on just the test invocations of interest, not something like installing a specific toolchain or libraries (installation in a CI container differs from how I install something locally).

So for nitrocli's CI specifically, I'd probably just move the relevant part of test into a test target and then invoke that. Locally, the assumption is that libraries and the toolchain to work with are already installed. I have no strong opinion as to whether something like cargo fmt or cargo clippy needs to be forwarded from a CI script to a Makefile (but I agree it should be checked locally as well). For me these "core" commands and don't necessarily have to have a 1:1 mapping between local and CI (duplication is probably fine to keep things simple). I'd then typically have a default target (e.g., all) in the Makefile that somewhat closely resembles the steps that CI would do as well. I treat that step more as a smoke test than a guaranteed-to-be-equivalent-to-CI run, though.

.PHONY: all
all: test fmt lint

.PHONY: test
test: CARGO ?= cargo
test:
      $(CARGO) build --workspace --bins --tests --verbose
      $(CARGO) build --workspace --bins --tests --verbose --release
      $(CARGO) test --workspace --verbose
 
.PHONY: fmt
fmt: CARGO ?= cargo
fmt:
   $(CARGO) fmt --all -- --check

.PHONY: lint
lint:
     reuse lint

(or so; for illustration only)

[robinkrahl]

The only reason why I decided to move the toolchain installation to the Makefile is that I only want to have one place that defines the MSRV. (Maybe that could also be solved by using a text file that is read by the CI script and the Makefile.) As a developer, I don’t care about these targets.

I’d like to have clippy in the Makefile too because I don’t want to type out --workspace --all-targets --all-features. Also, I’d prefer having a separate build target because that can be used during development even if production Nitrokey devices are connected. To avoid accidentally running the tests, I’d rather not have them in the default target.

[d-e-s-o]

The only reason why I decided to move the toolchain installation to the Makefile is that I only want to have one place that defines the MSRV. (Maybe that could also be solved by using a text file that is read by the CI script and the Makefile.) As a developer, I don’t care about these targets.

Okay, but I am not sure I'd want the MSRV in the Makefile to begin with. I would not be installing an age old Rust toolchain locally, to be honest. Not saying we can't or shouldn't do it, but to me that's also part of the reason we have a CI (and if I recall correctly it's pretty fast at this point).

I’d like to have clippy in the Makefile too because I don’t want to type out --workspace --all-targets --all-features.

Yeah, it was just an illustration.

Also, I’d prefer having a separate build target because that can be used during development even if production Nitrokey devices are connected. To avoid accidentally running the tests, I’d rather not have them in the default target.

Wouldn't you use cargo check in almost all cases? I question that make build is something I'd ever turn to, as the intention (to me) is for all things in the Makefile to be about comprehensive more or less final checking (such as done before creating a pull request). In all likelihood the only thing I will I remember over time is that make provides final testing capabilities when a Makefile is present in a project, but I won't remember individual targets for each and every project that I have. cargo is too much embedded into the Rust workflow for me to switch to using make on some projects and not others.

Again, to me it's not about 100% mirroring what the CI does. It's about having sufficient confidence to open a pull request. Could it still turn out red? Yes, although with a low likelihood.

Anyway, I don't feel strongly about this direction. If you want to include MSRV knowledge in the Makefile and would use that, I won't give you a hard time (as long as it's not a requirement to perform testing on an old toolchain locally). If a 100% mirroring is what you feel is right, same here. I was erring on the side of keeping things simpler.

[robinkrahl]

Wouldn't you use cargo check in almost all cases?

I was just referring to your example that does not have an option to build or check without running the tests. Of course checking is more suited for development. (Just keep in mind that some libnitrokey linking issues don’t turn up with check.)