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

Check all features and targets in CI when linting #1824

Merged
merged 12 commits into from
Jul 24, 2024

Conversation

MabezDev
Copy link
Member

@MabezDev MabezDev commented Jul 17, 2024

Thank you for your contribution!

We appreciate the time and effort you've put into this pull request.
To help us review it efficiently, please ensure you've gone through the following checklist:

Submission Checklist 📝

  • I have updated existing examples or added new ones (if applicable).
  • I have used cargo xtask fmt-packages command to ensure that all changed code is formatted correctly.
  • My changes were added to the CHANGELOG.md in the proper section.
  • My changes are in accordance to the esp-rs API guidelines

Extra:

Pull Request Details 📖

Description

What this doesn't solve just yet:

  • log/defmt, checking, I've gone with defmt for now, but ideally I'd like to check both but this would require double the CI time.
  • all the packages, this mostly tackles esp-hal and esp-wifi, but I'd like 100% coverage I think this covers everything now, at the very least each package is built for each target.

Closes #1644
Closes #1693

@MabezDev MabezDev added the skip-changelog No changelog modification needed label Jul 17, 2024
@MabezDev MabezDev force-pushed the check-all-features branch from 67dfdb6 to d0af72d Compare July 17, 2024 16:57
@MabezDev
Copy link
Member Author

Reminder to myself: this PR has some breaking changes (enum renaming mostly) that needs a changelog entry

esp-wifi/src/wifi/mod.rs Outdated Show resolved Hide resolved
@MabezDev MabezDev changed the title Check more features and targets in CI when linting Check all features and targets in CI when linting Jul 17, 2024
xtask/src/main.rs Outdated Show resolved Hide resolved
@MabezDev
Copy link
Member Author

This makes the clippy job take 16minutes, but I don't really see a way to reduce this. If we want everything checked, then it takes time.

@bjoernQ
Copy link
Contributor

bjoernQ commented Jul 18, 2024

This makes the clippy job take 16minutes, but I don't really see a way to reduce this. If we want everything checked, then it takes time.

I think there is no way around that

@MabezDev MabezDev force-pushed the check-all-features branch 2 times, most recently from 204cd30 to 989dbf8 Compare July 18, 2024 14:34
@MabezDev
Copy link
Member Author

On second review, I don't think this PR has any breaking changes, but please check my work :D

@MabezDev MabezDev marked this pull request as ready for review July 18, 2024 14:50
Copy link
Contributor

@bjoernQ bjoernQ left a comment

Choose a reason for hiding this comment

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

Thanks!

@bjoernQ
Copy link
Contributor

bjoernQ commented Jul 19, 2024

Haven't seen anything looking sus

There is one warning:

warning: constant `ESP32P4_TOML` is never used
  --> esp-metadata\src\lib.rs:10:7
   |
10 | const ESP32P4_TOML: &str = include_str!("../devices/esp32p4.toml");
   |       ^^^^^^^^^^^^
   |
   = note: `#[warn(dead_code)]` on by default

Copy link
Contributor

@JurajSadel JurajSadel left a comment

Choose a reason for hiding this comment

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

LGTM, left a nitpick suggestion, feel free to ignore. Thanks!

esp-hal/src/uart.rs Show resolved Hide resolved
@MabezDev
Copy link
Member Author

I think I might be able to trim some old CI jobs now as they should be covered by clippy.

And provided we are still building the examples we should still detect linker issues.

Let me try.

@MabezDev MabezDev force-pushed the check-all-features branch from 147b703 to 7c27ff9 Compare July 19, 2024 12:33
@MabezDev
Copy link
Member Author

Hmm, that worked. Now I'm wondering if I can split the clippy job into the esp-hal matrix, which might make the clippy job quicker as most of the crate should be built? Let's see :D

@MabezDev MabezDev force-pushed the check-all-features branch 4 times, most recently from 1be6508 to 514805f Compare July 19, 2024 13:30
@MabezDev
Copy link
Member Author

MabezDev commented Jul 19, 2024

It helped a little bit I think, it shaved a few minutes of some of the jobs at least.

This is now ready for a final review. Before merging we need to remove the required checks on the jobs that no longer exist, and subsequent PRs should be rebased.

Edit: this also nicely closes #1644 as we're now down to 18 jobs.

Copy link
Member

@SergioGasquez SergioGasquez left a comment

Choose a reason for hiding this comment

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

LGTM! I like the improvements, thanks!

.github/workflows/ci.yml Outdated Show resolved Hide resolved
@MabezDev MabezDev force-pushed the check-all-features branch from eaab685 to fff707e Compare July 24, 2024 10:41
@MabezDev MabezDev enabled auto-merge July 24, 2024 11:27
@MabezDev MabezDev force-pushed the check-all-features branch from fff707e to 5d9c44d Compare July 24, 2024 11:28
@MabezDev MabezDev added this pull request to the merge queue Jul 24, 2024
Merged via the queue into esp-rs:main with commit e8b0a37 Jul 24, 2024
18 checks passed
@MabezDev MabezDev deleted the check-all-features branch July 24, 2024 12:16
@MabezDev MabezDev mentioned this pull request Jul 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
skip-changelog No changelog modification needed
Projects
None yet
4 participants