Add clippy to CI and fix warnings

[ObiWanRohan]

Add a workflow to run Clippy.

Solving issue #139

[hannobraun]

Thank you for the pull request, @ObiWanRohan!

I've left some comments with questions and change suggestions.

In addition, there are some problem and questions that need to be addressed before this can be merged:

• The new jobs are failing, complaining about a missing rust-toolchain file. I don't understand why. One was added earlier today (build: add rust-toolchain.toml configuration file #206), and this pull request seems to be up-to-date with main.
• Does it make sense to also run cargo build, not just cargo test? They're mostly redundant, but a cargo build might expose additional problems, like linker issues. Might be irrelevant right now.
• We already have existing CI jobs that would need to be removed, or merged into the new jobs.
• I'm concerned about using so many different jobs. Wouldn't starting a new runner cause overhead each time, resulting in a longer CI build? Wouldn't it be better to just have one job per platform, with the appropriate steps specified for each? That would also cut down on some redundant uses declarations.
• Can we configure Dependabot to update all those actions? It might be tedious to keep them up-to-date manually.

[ObiWanRohan]

I should've created this as a draft PR.
This was just an initial commit to have a baseline of what was happening so bear with me as I ready this PR.

About your questions:

• About the rust toolchain issue, this seems to be an issue with the action. It isn't reading the new rust-toolchain.toml file.
  The issue is being tracked here.
  For a workaround - Is there a specific reason to use Rust toolchain version 1.58.1? Or can we pin it to stable? It can be used as a static argument to the workflow.
• For the second and third points, I thought this workflow would be separate than the other workflows, and this one would run more frequently than the other build workflows.
• This workflow is only going to run on one platform, so I'm not sure if it would matter currently
• I'll check if Dependabot can do it. It would be of much help if it has the ability.

[ObiWanRohan]

Looks like Dependabot has the ability to update the actions workflows too

[hannobraun]

I should've created this as a draft PR.
This was just an initial commit to have a baseline of what was happening so bear with me as I ready this PR.

Understood. I've converted the pull request to a draft (I think you could have done this too; there's a subtle link in the upper right under Reviewers). Just convert it back, once you feel it's ready.

• About the rust toolchain issue, this seems to be an issue with the action. It isn't reading the new rust-toolchain.toml file.
  The issue is being tracked here.
  For a workaround - Is there a specific reason to use Rust toolchain version 1.58.1? Or can we pin it to stable? It can be used as a static argument to the workflow.

Ah, got it. Yeah, I think we can pin it to stable, as a workaround. I don't feel strongly anyway, about whether we should pin to stable in rust-toolchain.toml, or whether we should control the version tightly.

• For the second and third points, I thought this workflow would be separate than the other workflows, and this one would run more frequently than the other build workflows.
• This workflow is only going to run on one platform, so I'm not sure if it would matter currently

The workflow I linked runs on every push to a pull request, so just as often as this one. There's really no difference between them, except that this one is new and contains more stuff. There's the same intent behind them, to test pull requests.

There's also the Compile Binaries workflow (might get renamed soon), which only runs when main is pushed to. That one is separate.

• I'll check if Dependabot can do it. It would be of much help if it has the ability.
  Looks like Dependabot has the ability to update the actions workflows too

That's great, thanks for checking 👍

[hendrikmaus]

About the rust toolchain issue, this seems to be an issue with the action. It isn't reading the new rust-toolchain.toml file.
The issue is being tracked actions-rs/toolchain#126.
For a workaround - Is there a specific reason to use Rust toolchain version 1.58.1? Or can we pin it to stable? It can be used as a static argument to the workflow.

That is a really unfortunate issue. I did not see that one coming 🤔 I think pinning to stable is fine though.

I'm concerned about using so many different jobs. Wouldn't starting a new runner cause overhead each time, resulting in a longer CI build? Wouldn't it be better to just have one job per platform, with the appropriate steps specified for each? That would also cut down on some redundant uses declarations.

@hannobraun since jobs run in parallel, the time issue is negligible. The matrix job should be kept separate, but the rest can be combined if you are more comfortable with it. Originally, I put them into individual ones for readability in the checks section of a pull-request. At a glance, you'd immediately see what part of CI went south.

The workflow I linked runs on every push to a pull request, so just as often as this one. There's really no difference between them, except that this one is new and contains more stuff. There's the same intent behind them, to test pull requests.

The existing workflow https://github.com/hannobraun/Fornjot/blob/main/.github/workflows/fornjot.yml is superseded by this one can can be removed.

---

Glad to get some feedback out of this to improve rust-workflows :)

[hannobraun]

since jobs run in parallel, the time issue is negligible. The matrix job should be kept separate, but the rest can be combined if you are more comfortable with it. Originally, I put them into individual ones for readability in the checks section of a pull-request. At a glance, you'd immediately see what part of CI went south.

I've definitely seen that jobs where blocked because they were waiting for a runner to become available, though. So it's not always parallel. (That might have been a problem with Windows runners though? Not sure I've ever seen that with Linux.)

But honestly, I don't really care. I do care that the CI run doesn't take too long, but other than that...

You're definitely the expert here. So if you're telling me it's best practice to do it this way, and it won't affect the runtime negatively (at least most of the time), that's fine by me.

[hendrikmaus]

I also added the same rust-roolchain.toml to rust-workflows and everything builds flawlessly 🤔 hendrikmaus/rust-workflows#40

Also, the main of Fornjot works exactly as expected using this:

Source: https://github.com/hannobraun/Fornjot/runs/5251297420?check_suite_focus=true

- name: Setup Toolchain
  uses: actions-rs/toolchain@v1
  with:
    override: true
    profile: minimal
    target: ${{ matrix.target }}
    toolchain: stable

Of course the target property is specific to the matrix in that very workflow: https://github.com/hannobraun/Fornjot/blob/main/.github/workflows/cd.yml#L40-L46

[hendrikmaus]

You're definitely the expert here. So if you're telling me it's best practice to do it this way, and it won't affect the runtime negatively (at least most of the time), that's fine by me.

Thanks for the praise. I'd stick with this job-split until we notice a negative impact that is significant enough to hurt the workflow. Until then, I'd keep it like it is in favor of the user experience when glancing at the "checks" section of a pull-request.

[hendrikmaus]

I added a couple of suggestions with the stable toolchain. That way, it also works as expected in rust-workflows.

[hendrikmaus]

Aside: here is how Dependabot updates GitHub Actions with the configuration contained in this pull-request: hendrikmaus/rust-workflows#42 I am quite pleased with it.

[hannobraun]

Thanks for the praise. I'd stick with this job-split until we notice a negative impact that is significant enough to hurt the workflow. Until then, I'd keep it like it is in favor of the user experience when glancing at the "checks" section of a pull-request.

Sounds good!

[ObiWanRohan]

I also added the same rust-roolchain.toml to rust-workflows and everything builds flawlessly 🤔 hendrikmaus/rust-workflows#40

@hendrikmaus You're correct. I added the override: stable argument to the toolchain action, and the rust-toolchain.toml file was picked by rustup.

[hendrikmaus]

Ci-wise this looks fine now @ObiWanRohan - thank you.

---

The changes in the src directory shouldn't show up in this change-set though 🤔

[ObiWanRohan]

The changes in the src directory shouldn't show up in this change-set though 🤔

Those are due to the trivial fixes which I did from the Clippy suggestions and the merge from the main branch to resolve conflicts

[hendrikmaus]

Ah got it, should've looked at the change-set myself 🤦🏻

IMHO I would always submit two or more change-sets in this situation to keep the concerns separated: 1) mutate the CI pipeline 2) code changes. That is because either change can have side-effects which could make you revert partial patches. That is way cleaner if each change-set is bound to a single concern. If that makes sense :)

@hannobraun are you happy to merge the change-set like this anyway?

[hannobraun]

I also prefer to have separate concerns (like fixing Clippy warnings and making CI changes) in separate pull requests. One additional reason, on top of what @hendrikmaus mentioned, is to get changes in as soon as possible. I might merge a change today that conflicts with Clippy fixes in this PR, which would then cause unnecessary work for @ObiWanRohan.

As a general rule, everything that makes sense on its own should be merged right away (but obviously you don't need to make one pull request per minute for 10 minutes, if you're doing a bunch of small cleanups; use your judgement).

But that's not a blocker. I'd be happy to merge the PR anyway.

There are two things I'd like to see done before merging, ideally:

• Fix the existing Clippy warnings, ideally in a separate pull request. I'd prefer the check to be green when merging it.
• Remove the now redundant "Build Fornjot" workflow.

But those aren't hard blockers either. If @ObiWanRohan ran out of time and couldn't work on this for a while, for example, I'd prefer to merge the existing improvements and make further changes later, instead of letting this sit here to suffer from bit rot.

Also, as a general note, if merge conflicts need to be resolved in a pull request, I prefer the pull request to be rebased, instead of a merge commit being created. That leaves a simpler (and thus easier to review) history. This isn't a hard requirement either.

(And by the way, this isn't a full review. I haven't looked at the commits for a while; this is just some stuff I noticed.)

[therealprof]

Whoops, sorry, I just noticed this PR. I've fixed some of the lints with #232. My bad.

The now remaining lints were not quite as clear cut which why I haven't addressed them yet. Might require some more consideration...

[ObiWanRohan]

I just got back to this PR.

I agree that the Clippy fixes are probably better suited to a separate PR.
@hannobraun Should I revert the commits with the Clippy changes or is it okay if I reset my branch and do a force push?

[hannobraun]

@therealprof

Whoops, sorry, I just noticed this PR. I've fixed some of the lints with #232. My bad.

My preferred solution to that problem is to get changes merged as early as practical, exactly so that contributors like you don't have to be aware of all ongoing work 😄

@ObiWanRohan

Should I revert the commits with the Clippy changes or is it okay if I reset my branch and do a force push?

My favorite solution would be to have the Clippy fixes in a separate pull request (or even multiple, if some are uncontroversial and can be merged right away, while other require discussion), then rebase/force-push this pull request to have the CI changes as a separate pull request with a clean history.

But the whole point of my preferences is to make the workflows here efficient, and that's going to depend on the developer and the work. If you think you might want to fiddle with the CI some more, it's probably best to submit the other changes separately. If you want to do a final push to get everything over the finish line in one go, than just do that. Whatever works best for you!

[ObiWanRohan]

Does it make sense to also run cargo build, not just cargo test? They're mostly redundant, but a cargo build might expose additional problems, like linker issues. Might be irrelevant right now.
We already have existing CI jobs that would need to be removed, or merged into the new jobs.

I have removed the previous checks (LinuxLatest, MacOsLatest, and WindowsLatest), and added the build to the new workflow. Those would run in the matrix job.

[hendrikmaus]

Suggested change

	args: --all-features
	args: --all-features
	name: CI / Clippy run

This should make for a nicer name to appear in the "checks" section of a pull-request. By default, the job's name appears as clippy_check.

[ObiWanRohan]

The name currently appears as CI / Clippy Check, the name is configured here

https://github.com/ObiWanRohan/Fornjot/blob/6155a28d9ee469440edb479134b9a87c1396ba5f/.github/workflows/test.yml#L71

[hannobraun]

Looks like there are two Clippy entries in the "checks" section now. One with the nice name (Clippy Check), one just called clippy.

[hendrikmaus]

The job emits a check and the step it runs emits another. I don't know a way to solve that right now.

[hannobraun]

I don't think it's much of a problem. Just mentioned it because it seemed related to the discussion here regarding the name displayed under "checks".

[hendrikmaus]

I think this one is ready to be merged 👍🏻 Nicely done @ObiWanRohan

[hannobraun]

I agree that this looks very good. Thank you @ObiWanRohan!

I'm happy to merge as-is, with a few reservations:

• I found one tiny issue. Left a comment. I can probably apply the fix myself, after leaving this review.
• I'd prefer to have all Clippy warnings fixed or suppressed before merging, but it's also okay to fix them soon-ish after merging this check. I'll open an issue.
• GitHub tells me this branch is out-of-date with the base branch, but doesn't show me the button to let me rebase it. I'll look into that after leaving the review.

If I can cajole GitHub into letting me rebase, I'll merge this right after leaving this review. If I have to manually rebase, I'll do that later today.

Thanks again, @ObiWanRohan, and thank you @hendrikmaus for the help!

[hannobraun]

Ah, I can't apply my own review suggestion. Looks like @ObiWanRohan didn't check that box that would allow me to push changes to the branch. This would also explain why I can't rebase.

So, happy to merge, once:

1. My suggestion has been applied.
2. @hendrikmaus's suggestion also looks good. I see no reason not to apply that too.
3. The branch has been rebase on main.

[ObiWanRohan]

@hannobraun The PR is ready to merge.
There are some workflows which are shown as the 3 expected checks. I think that is something to be done with the merge settings

[hannobraun]

Thank you, @ObiWanRohan, yet again!

There are some workflows which are shown as the 3 expected checks. I think that is something to be done with the merge settings

Yes, those must be because of the branch protection settings. I should be able to take care of this on my end.

[hannobraun]

Opened #261.