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

Proposal: Add MegaLinter and Cargo Clippy CI Cycle #278

Draft
wants to merge 57 commits into
base: main
Choose a base branch
from

Conversation

ScottGibb
Copy link
Contributor

@ScottGibb ScottGibb commented Jan 29, 2025

Summary

This PR is a suggestion for adding MegaLinter to the project. MegaLinter provides a lot of linters that will auto format and fix various issues with the codebase. Currently I have it setup to lint all the different file types we use in Trouble.

Why

  • Automatically check for formatting and adherence to cargo clippy suggestions
  • keep formatting and code style consistent across the project using cargo and various other standardised linters.

General workflow

Whenever a PR is raised the linters will run on the PR and check all the files are correctly formatted, links are correctly working etc. We also run cargo clippy on all our examples and crates to ensure they all meet appropriate standards. If a formatting change can be fixed, the workflow will create a PR into the existing PR with the apropriate changes. Thus the code can be reformatted on GitHub.

Why all the files changes?

Most of these files are formatting changes, which have occurred from running cargo fmt on the project.

Gotchas

Sadly Megalinters support for rust is limited (still available) and due to the structure of the project ive added a couple of extra jobs/stages in order to ensure full coverage. However in the future they will be adding better rust support.

Implementation Issues regarding workflow minutes

If GitHub workflow minutes are a potential barrier, we can run this on a self-hosted Runner like a Pi5 or otherwise. Thus having no impact on embassy GitHub minutes. I believe the workflow only takes a couple of minutes at the moment anyway.

Current Issues to sort out (before merge)

  • Apache-Nimble example: Im currently unable to run/build this example and thus I cant get clippy to run either. I believe it has something to do with this issue: Update apache-nimble-sys embassy-nrf dependency to v0.3.0 benbrittain/apache-nimble-sys#6

  • esp32 examples: Im a bit confused on how I can install the esp extensa toolchain to clippy the examples, these examples are currently unclippied:
    - esp32
    - esp32s3

  • Add MegaLinter instructions/description to main readme

  • Add PAT Token so megalinter can create PRs with format fixes

Copy link
Member

@lulf lulf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TIL megalinter :) Thanks for the PR

As with all these tools, I have a hard requirement that it should be possible to run it locally and being able to correct things (ideally automatically like cargo fmt) before creating a PR.

Also, from looking at the configuration, it seems like we could get the same benefit (apart from shell, toml validation) from adding the clippy checks to ci.sh?

Comment on lines +6 to +8
mkdir -p "$HOME"/.cargo/bin
curl -L https://github.com/embassy-rs/cargo-batch/releases/download/batch-0.6.0/cargo-batch >"$HOME"/.cargo/bin/cargo-batch
chmod +x "$HOME"/.cargo/bin/cargo-batch
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going to have to argue with megalinter about tabs vs spaces now? 😆

@ScottGibb
Copy link
Contributor Author

So you can run this from command line too!! I haven't added the command to call/install to the readme section yet, but will do when I've got access to my laptop.

The real benefit of this, over adding to the ci.sh, is we don't have to maintain any of it as the setup is done by mega linter. We just have to maintain a config file and a GHA file.

As for the clippy bits adding to ci.sh, we could potentially but I'm hoping with future improvements to mega-linter we can do it all from mega-linter.

I think there is a way to get mega linters rust clippy to run more than once with more arguments but haven't dug around deep enough yet.

@lulf
Copy link
Member

lulf commented Jan 30, 2025

So you can run this from command line too!! I haven't added the command to call/install to the readme section yet, but will do when I've got access to my laptop.

Okok, let's give it a try :) Thanks for introducing some new stuff and processes!

The real benefit of this, over adding to the ci.sh, is we don't have to maintain any of it as the setup is done by mega linter. We just have to maintain a config file and a GHA file.

I just want to run 'one thing' to do it all, so in that case please add some way for ci.sh to run the megalinter as well (like, if it detects the CLI tool). Is it possible to install that in the 'prereqs' of the GHA and then run the tool as part of ci.sh instead of using a separate job/action?

The reason is that I really don't like creating too many workflows, especially since there is a shared quota on the embassy organization, and that it can easily get out of hand if developers can't run it all locally in a simple way.

I would actually like to run all integration and example tests in ci.sh as well, but can't because it could then only run on machines that has the hardware. But at least with pure software, we can run it wherever we want!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants