Add Clippy to CI build

[hannobraun]

I think it makes sense to add Clippy to the CI build, as it often has valid complaints. However, it shouldn't be added blindly. First, we shouldn't add it, if it causes a million warnings right away. There should be a clean-up first, to make sure all applicable warnings are fixed. Second, if during that clean-up we come across Clippy warnings that aren't applicable, those should be disabled.

Labeling as https://github.com/hannobraun/Fornjot/labels/good%20first%20issue, as this doesn't require a lot of knowledge of Fornjot, and going through the warnings could be a good way to get to know the code base a little.

[ObiWanRohan]

Is someone working on this?
If not, I'd like to solve this

[hannobraun]

Not to my knowledge. @hendrikmaus was working on adjacent build system topics, but he said he's interested in #104.

Feel free to pick this up, if you're interested! Please note that there's some discussion about how to add more things to the CI build in #64 that is relevant here too.

[hendrikmaus]

I can recommend to use this step: https://github.com/hendrikmaus/rust-workflows/blob/master/.github/workflows/ci.yaml#L90

Clippy should not run in a matrix, linting the codebase on one OS is enough.

[hendrikmaus]

Basically, we can use the entire workflow here as is https://github.com/hendrikmaus/rust-workflows/blob/master/.github/workflows/ci.yaml

[hannobraun]

Clippy should not run in a matrix, linting the codebase on one OS is enough.

Thank you, @hendrikmaus, good point!

I still want a script with all the essentials (test, clippy, fmt, ...) that a developer can execute locally, to gain confidence that CI will pass. Going back-and-forth with the CI build can be a nuisance, especially if you're new to a project and don't know everything that the CI build checks for.

That doesn't have to be a part of this issue though. I'd be happy to call this complete, if we have warning-free Clippy build that is checked on CI.

[hendrikmaus]

Regarding local checking, I usually run multiple terminals + cargo watch to run clippy and tests on every code change. Those commands are either in my shell history or I have a Justfile or Makefile wrapping them.

One could also look into making that setup easier with https://github.com/Canop/bacon which recently released a brand new major version.

[hannobraun]

Interesting, I didn't know bacon!

Are you using rust-analyzer? Won't those two interfere? I'm using rust-analyzer in VS Code and I wouldn't want to replace it, as it's providing so much more than just cargo check.

[hendrikmaus]

I am currently using CLion and the Intellij-Rust plugin + Clippy.

[hendrikmaus]

Are you using rust-analyzer? Won't those two interfere?

I just installed code, rust-analyzer and bacon and ran this setup for a bit:

• edited source code in code
• rust-analyzer was active, rust lang plugin off (they seem to collide)
• two terminals with bacon:
  • bacon clippy
  • bacon test

Aside: I also run a local instance of sccache to improve compile times.

Works find for me on a Linux box 5.16.10-arch1-1, a AMD Ryzen 9 3900X (24) @ 3.800GHz and 32 GB RAM

[hendrikmaus]

@ObiWanRohan I can foresee that this issue will overlap with #104, because the release process will be tightly integrated with the build of the repository main branch. I would recommend to wait on this one for now.

Forget what I wrote above. The workflows for CI and CD can be changed independently.

[ObiWanRohan]

@hannobraun The all-in-one script that you mentioned could be made such that the CI would use the same script for the workflows. It would mean only having to maintain the code at one place. What do you think?

For now I'll use Clippy independently in the workflow, but the longer term goal can be the one script everywhere.

[hannobraun]

The all-in-one script that you mentioned could be made such that the CI would use the same script for the workflows. It would mean only having to maintain the code at one place. What do you think?

That was my first thought, but @hendrikmaus made a good point, that we don't need to run Clippy (or anything else that doesn't actually generate code, like cargo check) on multiple platforms.

Let's not worry about it too much. What's important in the context of this issue, is that we fix/silence existing Clippy warnings and run it on CI. Eventually, I want to have a local script that tests all the relevant stuff, but we can figure out later, whether that's shared with the actions, or if we have a bit of duplication.

Maybe we can designate one of the runners (probably best to use Linux, because that tends to be the fastest one) as the "complete" runner (cargo build, cargo test, cargo clippy, cargo fmt, cargo doc, etc.; could use the locally accessible build script), while Mac/Windows only do cargo build/cargo test. Does that make sense?

[hendrikmaus]

Yes, I think I also did it in rust-workflows like that.

[hannobraun]

Addressed in #208.