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

[Fixes #62] Remove re-exports to avoid warnings promoted to errors #64

Merged
merged 7 commits into from
Oct 10, 2023

Conversation

michalfita
Copy link
Collaborator

@michalfita michalfita commented Sep 17, 2023

Purpose

My rustc 1.72.0 started to spit warnings at me, and CI fails on workflows with disabled minimum compiler version to be used (as 1.63.0 seems to be unaffected).

Scope

  • Easy fix: Remove reexports as I'm not a fan of them coming from C++ world prefer namespaces leaving import renames to the user.
  • Add building with the latest compiler to catch errors that may break people with newer toolchain than MRV.
  • Reduce the strain on CI and remove doubling jobs.

Alternative

The alternative is to rename types to be more peripheral type specific. I have feeling at the moment we have mixture of both simple names in namespaces and specifically named peripherals despite being in namespaces.

@michalfita michalfita added the bug Something isn't working label Sep 17, 2023
@michalfita
Copy link
Collaborator Author

I cannot remove #![deny(const_err)] at the moment as it comes from PACs generation. The problem will solve itself when #61 is merged.

- name: Install Rust (thumbv7em)
run: rustup target add thumbv7em-none-eabihf
- name: Build HAL for ${{ matrix.pac }}
run: cargo check --package atsamx7x-hal --features ${{ matrix.pac }},unproven
Copy link
Member

Choose a reason for hiding this comment

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

IIRC, not using a board feature (like samv71q21b) omits the build of some modules.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@tmplt ${{ matrix.pac }} is built from following output:

["same70j19b","same70j20b","same70j21b","same70n19b","same70n20b","same70n21b","same70q19b","same70q20b","same70q21b","sams70j19b","sams70j20b","sams70j21b","sams70n19b","sams70n20b","sams70n21b","sams70q19b","sams70q20b","sams70q21b","samv70j19b","samv70j20b","samv70n19b","samv70n20b","samv70q19b","samv70q20b","samv71j19b","samv71j20b","samv71j21b","samv71n19b","samv71n20b","samv71n21b","samv71q19b","samv71q20b","samv71q21b"]

and that's CI coming from you or your company.

Copy link
Contributor

Choose a reason for hiding this comment

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

This looks good to me, although it feels like the CI is slowly growing more and more unruly and there should be a better way to do this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

GitHub Action from Software Engineering perspective are utter nightmare. It's effectively a toy for JavaScript kids to develop their creativity, to serious tool for serious projects.

@@ -1,5 +1,5 @@
name: HAL
on: [push, pull_request]
on: [push]
Copy link
Member

Choose a reason for hiding this comment

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

Does this not disable the trigger for pull requests coming from outside the org's repo?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Possible. Need to investigate, maybe we have to filter out push request triggers for main repository.
It's a good catch. I'm so used to working on one repo w/o forks.

Copy link
Member

Choose a reason for hiding this comment

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

Does the trigger overlap cause any significant overhead? I can't recall. In either case, this feels like a common use case: perhaps there is a flag that does this for us?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

All matrix jobs are run twice, they're queued as there's plenty and if you push quickly enough you have to wait for results as GH throttles them - waste of resources, even if free of charge.

Note to myself: https://joht.github.io/johtizen/build/2022/01/20/github-actions-push-into-repository.html

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think 2d75357 does the job, for 100% certainty we'd need someone PRing from the for after we merge this.

@@ -56,6 +79,7 @@ jobs:
run: cargo check --package atsamx7x-hal --features ${{ matrix.features }},samv71q21b,unproven

board-examples:
if: github.event.pull_request.head.repo.full_name != github.repository
Copy link
Contributor

Choose a reason for hiding this comment

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

@michalfita What is the purpose of this line?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

To stop redundant builds. We have two triggers push and pull_request: the former means we build everything we push including in PRs, the latter means changes in the PR are build, including branches in forks (outside contributors). But having both stupidly doubles each job. This condition let jobs from pull_request trigger on forks to be run, but stops on our own branches. Unfortunately, this condition cannot exists at higher level, has to be per job 🤦🏼‍♂️.

- name: Install Rust (thumbv7em)
run: rustup target add thumbv7em-none-eabihf
- name: Build HAL for ${{ matrix.pac }}
run: cargo check --package atsamx7x-hal --features ${{ matrix.pac }},unproven
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks good to me, although it feels like the CI is slowly growing more and more unruly and there should be a better way to do this.

@michalfita
Copy link
Collaborator Author

This looks good to me, although it feels like the CI is slowly growing more and more unruly and there should be a better way to do this.

We could extend the matrix but the Rust Version component, but I don't have will power ATM to do this, but it would keep jobs definition flatter. Raise an issue if you prefer that way, @martinmortsell, and in future we may address this - now I'm aiming at making a release with separated PACs.

@michalfita michalfita merged commit 8a1d6d7 into development Oct 10, 2023
@michalfita michalfita deleted the issue/62/fix-glob-reexport-warnings branch October 10, 2023 11:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants