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 packages with nightly for RISC-V devices #1881

Merged
merged 8 commits into from
Aug 7, 2024

Conversation

SergioGasquez
Copy link
Member

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

Adds a nightly job which can fail that builds and lints the packages for RISC-V devices with nightly channel. I've added the job in the same workflow, as the total time got a bit better (~15 mins to the current ~ 17, and we are now on 21 jobs but rustfmt job is really fast), happy to move into a standalone workflow if we think its better.

Testing

CI Run: https://github.com/SergioGasquez/esp-hal/actions/runs/10157891170

@SergioGasquez SergioGasquez added the skip-changelog No changelog modification needed label Jul 30, 2024
@SergioGasquez SergioGasquez linked an issue Jul 30, 2024 that may be closed by this pull request
xtask/src/main.rs Outdated Show resolved Hide resolved
@SergioGasquez SergioGasquez force-pushed the feat/ci-nightly branch 2 times, most recently from 0a72008 to 1bf9f50 Compare July 30, 2024 08:45
@jessebraham
Copy link
Member

IMO if this is how we're going to structure the workflow then we should extract the common bits into a local action. There is a ton of duplication right now and I can almost guarantee the two jobs will fall out of sync eventually.

@MabezDev
Copy link
Member

I agree with Jesse, in fact we already have the nightly toolchain on the normal esp-hal job:

- uses: dtolnay/rust-toolchain@v1
we just need to add the appropriate sections for changing to the nightly toolchain via the xtask (maybe we can make this possible to change via env, might make this easier to accomplish?)

@SergioGasquez
Copy link
Member Author

SergioGasquez commented Jul 30, 2024

Ive just squashed the nightly and esp-hal jobs into a single one by increasing the matrix (I was working on this before reading the feedback on this PR 😓), not sure if its worth to add an action now that the steps are not duplicated

@MabezDev
Copy link
Member

not sure if its worth to add an action now that the steps are not duplicated

By doing it the way I suggested, can we still allow the nightly steps to fail? If not then we likely will need to split it into a separate action and re-use.

@SergioGasquez
Copy link
Member Author

Should be possible to fail on nightly by adding the extra matrix arg, just updated it. Example CI run: https://github.com/SergioGasquez/esp-hal/actions/runs/10164106747

@jessebraham
Copy link
Member

Also curious, we had previously discussed just running the nightly checks once per day rather than for each CI workflow run. Why have we decided to deviate from that plan?

@SergioGasquez
Copy link
Member Author

Also curious, we had previously discussed just running the nightly checks once per day rather than for each CI workflow run. Why have we decided to deviate from that plan?

AFAIK, we didn't reach a decision, so I experimented with this approach but, as mentioned in the PR description, I'm happy to move into another workflow. I don't have any hard preference, but what could happen is that we completely ignore the nightly workflow (which can also happen with having it included in the main workflow, but could be easier to check)

Also, just noticed that if we end up going this approach we should update the required checks as it now includes the toolchain in the job name.

@SergioGasquez SergioGasquez force-pushed the feat/ci-nightly branch 2 times, most recently from 5787b37 to 54145b8 Compare August 5, 2024 13:51
@SergioGasquez
Copy link
Member Author

Added a new workflow for the nightly checks and gathered the common logic into an action. Here is a run of the nightly CI: https://github.com/esp-rs/esp-hal/actions/runs/10267836036

@jessebraham jessebraham changed the title Check pacakges with nightly for RISC-V devices Check packages with nightly for RISC-V devices Aug 6, 2024
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.

LGTM, thanks!

@jessebraham
Copy link
Member

Sorry this is kind of nit-picky, but I am not a huge fan of the name check-packages for the action, as it only actually checks esp-hal and this is kind of confusing when reading through the workflow.

@SergioGasquez
Copy link
Member Author

Sorry this is kind of nit-picky, but I am not a huge fan of the name check-packages for the action, as it only actually checks esp-hal and this is kind of confusing when reading through the workflow.

Agree, the name was confusing, Im not happy with check-esp-hal either as it also checks esp-lp-hal, but its a better name for the moment, if we have any better suggestion Ill update it

Copy link
Member

@jessebraham jessebraham left a comment

Choose a reason for hiding this comment

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

Thanks for this!

@jessebraham jessebraham added this pull request to the merge queue Aug 7, 2024
Merged via the queue into esp-rs:main with commit e58b4d8 Aug 7, 2024
18 checks passed
@SergioGasquez SergioGasquez deleted the feat/ci-nightly branch August 7, 2024 13:52
AnthonyGrondin pushed a commit to AnthonyGrondin/esp-hal that referenced this pull request Aug 8, 2024
* ci: Check pacakges with nightly

* ci: Remove duplication

* perf: Nightly clippy fixes

* revert: Revert unsafe changes

* ci: Use an action to remove duplication

* ci: Update trigger conditions

* ci: Update S2 serial port

* ci: Rename action
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
Development

Successfully merging this pull request may close these issues.

Verify packages build with both stable and nightly toolchains
4 participants