Skip to content

Conversation

@bugadani
Copy link
Contributor

@bugadani bugadani commented Jul 20, 2023

This PR updates the PR template to remove checklist items that are covered by CI. The PR also ensures that warnings are rejected by CI where possible (sadly, not for examples).

  • CI now sets the correct CPU target triples where necessary, mostly to prevent weird asm macro warnings
  • Clippy checks now correctly apply config.toml settings
  • Some paths have been replaced by existing imports
  • Bracketed names in efuse descriptions are now inline code. It helps with readability besides preventing link warnings.
  • Fixed broken doc links
  • enabled eh1 feature for rustdoc so that ehal1 references don't trigger warnings
  • Fixed some spelling mistakes in RMT, which affects the public API, too
  • :( warnings in examples are not denied because -D warnings breaks some of them (by ignoring flags from config.toml)

I've tried to not use #[deny(warnings)] as it is an antipattern and has the potential of breaking future code. Unfortunately, other attempts (RUSTFLAG, cargo rustc -- -D warnings) have failed and I'm out of ideas besides grepping for warnings or using a github action if one exists. These latter ideas seemed way too much work for today and at this point I'm sufficiently frustrated.

@bugadani
Copy link
Contributor Author

Looks like we had more warnings than I thought 😅

@bugadani bugadani changed the title PR template related changes PR template related changes and The Big Warning Cleanup Jul 20, 2023
@MabezDev
Copy link
Member

How is it that I've never seen these warnings before? 🤔

@bugadani
Copy link
Contributor Author

How is it that I've never seen these warnings before? 🤔

Some of these are in dependencies so the approach here isn't entirely correct.

@bugadani
Copy link
Contributor Author

@MabezDev do you know what we should do with https://github.com/esp-rs/esp-hal/actions/runs/5611347093/jobs/10267543869?pr=666 instead of simply #[allow(...)]-ing?

@jessebraham
Copy link
Member

The other maintainers can veto me, but for the record I'm not really not big on the idea of adding #![deny(warnings)] here, makes it really annoying to add new drivers/chip support.

@MabezDev
Copy link
Member

@MabezDev do you know what we should do with https://github.com/esp-rs/esp-hal/actions/runs/5611347093/jobs/10267543869?pr=666 instead of simply #[allow(...)]-ing?

Hmm, I guess they removed the argument order syntax. I think we can just use the fmt arg env capture way, i.e "wsr.ps {tkn}".

@bugadani
Copy link
Contributor Author

The other maintainers can veto me, but for the record I'm not really not big on the idea of adding #![deny(warnings)] here, makes it really annoying to add new drivers/chip support.

I can roll that back when I'm done with the warnings, but removing really makes the "please don't introduce warnings" checklist item in the PR template ineffective. I'm not married to the lint, but either the reviewers should be more strict, or the CI should use some tooling to catch them.

@MabezDev
Copy link
Member

The other maintainers can veto me, but for the record I'm not really not big on the idea of adding #![deny(warnings)] here, makes it really annoying to add new drivers/chip support.

I don't want #![deny(warnings)] either, but I do think its worth having deny warnings in CI.

@bugadani bugadani force-pushed the pr branch 7 times, most recently from 9c1f4ab to 352a0fb Compare July 20, 2023 14:52
@bugadani
Copy link
Contributor Author

bugadani commented Jul 20, 2023

Things are starting to green up, but:

  • I'm not sure what to do about smartled, I don't know the riscv interrupt code so I don't know if I can hide the offending unused stuff behind the vectored feature, or what else I should do.
  • Warnings in examples aren't denied at this point. I'll figure out how to deal with this. I gave up on this

Also, this PR has become a small mess as it touches basically everything: CI, docs, code, examples and the PR template 😂

@bugadani bugadani force-pushed the pr branch 12 times, most recently from 86729b5 to 255fa0c Compare July 20, 2023 17:59
@bugadani bugadani force-pushed the pr branch 2 times, most recently from 55c9f7e to a5069fc Compare July 20, 2023 18:12
@bugadani bugadani force-pushed the pr branch 3 times, most recently from e73376e to 6834cc5 Compare July 20, 2023 18:20
@bugadani bugadani marked this pull request as ready for review July 20, 2023 18:37
@bugadani
Copy link
Contributor Author

@MabezDev @jessebraham this PR is ready for your attention. I thought this would be much smaller and much more straight forward, and I was terribly wrong.

The commits should mostly stand on their own, in case you want me to undo something. I am disappointed that I couldn't figure out how to check examples, but I guess those hurt less. I hope the end result is not too annoying to work with.

I've updated the PR description with more info about what's been done. Hopefully nothing got lost in the big frustration :)

@MabezDev
Copy link
Member

Thanks for looking into this! I don't think this is mergeable as it is with the use of #[deny(warnings)], so I will have a think about how to solve this.

Copy link
Member

@MabezDev MabezDev left a comment

Choose a reason for hiding this comment

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

The rest of the changes look good to me! Thanks for taking care of those. Just to solve the deny warning issue. What about a CI step which uses sed to insert the rustflag into .cargo/config.toml?

# Run clippy on all packages targeting RISC-V.
- name: clippy (esp-riscv-rt)
run: cargo +stable clippy --manifest-path=esp-riscv-rt/Cargo.toml -- --no-deps
run: cd esp-riscv-rt/ && cargo +nightly clippy -- --no-deps
Copy link
Member

Choose a reason for hiding this comment

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

Why are these tested on nightly now? AFAIK we don't use any nightly features inside esp-riscv-rt?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This might have been a consistency/copy-paste thingy. I don't quite remember which one, but either way using a single toolchain looks better and using nightly means usually more lints.

@bugadani
Copy link
Contributor Author

We could add the flag as cargo rustc -- -D warnings in place of cargo check or build (not doc, though, but that might work with an env var anyway) which is nicer than modifying config.toml, but still rather verbose.

But, I'm reluctant to use rustflags at all as they will trigger for upstream crates we don't care about.

@bjoernQ
Copy link
Contributor

bjoernQ commented Jul 21, 2023

Looks good to me - and made me finally install a spell checker plugin 🤦‍♂️

@bugadani bugadani mentioned this pull request Jul 21, 2023
8 tasks

- Update `embedded-hal-*` alpha packages to their latest versions (#640)
- Implement the `Clone` and `Copy` traits for the `Rng` driver (#650)
- Implement the `Clone` and `Copy` traits for the `Rng` driver (#650, #666)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

nobody noticed that this is in the wrong line :)

@bugadani
Copy link
Contributor Author

I'm going to split out the uncontroversial parts into other PRs that can be landed without the warning stuff.

@bugadani bugadani marked this pull request as draft July 21, 2023 09:35
@bugadani bugadani closed this Aug 10, 2023
@bugadani bugadani deleted the pr branch September 4, 2024 11:50
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.

4 participants