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

Move CI to GitHub Actions #124

Merged
merged 6 commits into from
Jul 25, 2020
Merged

Move CI to GitHub Actions #124

merged 6 commits into from
Jul 25, 2020

Conversation

teskje
Copy link
Collaborator

@teskje teskje commented Jul 24, 2020

Closes #113

The relevant functional changes to the Travis CI are:

  • We run cargo check now instead of a full cargo build. That still gives us all the error reporting and should be considerably faster.

  • I added a clippy job too. That actually found a few code smells which I also fixed in this PR.

    I have been debating if using clippy against the full MCU matrix would make sense (as an alternative to cargo check) but I figured that we'll find most issues with just the stm32f303xc feature already.

    The job is using the nightly toolchain for the clippy check. This gives us the benefit of having all the latest and greatest lints, but might lead to FPs sometimes when new lints are not well tested yet. I'd be interested in your opinion on whether this is a good idea. If we do go back to stable here, we should add a nightly matrix entry to the cargo check job, so we still keep a nightly build in our CI.

  • The different MCU features are now directly specified in the workflow YAML and not dynamically generated anymore.

    This means we will have to touch the workflow config whenever we make changes to the supported MCUs, but we already need to touch two other files in this case anyway, so I don't think that's too bad. On the other hand, this simplifies the CI configuration as everything is in a single file now. Also we get more flexibility, in case we want to test more or fewer feature combinations in the future.

Performance was quite good in my tests . All jobs finished in under 1 minute and they run mostly in parallel, so the total time is not much higher. From looking at past PRs here, the Travis CI needed 5 to 6 minutes for a PR check.

Copy link
Member

@dfrankland dfrankland left a comment

Choose a reason for hiding this comment

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

Neat!

The checkout@v2 action currently has a bug that makes it sometimes
check out the wrong commit. Since we can't have that we'll use v1 for
now, until that is fixed.
@teskje teskje changed the title Move CI to GitHub Action Move CI to GitHub Actions Jul 24, 2020
@Sh3Rm4n
Copy link
Member

Sh3Rm4n commented Jul 25, 2020

Thank you for working on this!

I was thinking about doing this myself this week, but as we can see, you were faster! 👍

We run cargo check now instead of a full cargo build. That still gives us all the error reporting and should be considerably faster.

Does make sense, as we don't need to distribute any executable binary.
I still would like to see a build of the examples, to make sure, that we don't miss any building problems, which cargo check can not catch (e.g. linking errors). Does cargo check checks the examples by default as well?

I have been debating if using clippy against the full MCU matrix would make sense (as an alternative to cargo check) but I figured that we'll find most issues with just the stm32f303xc feature already.

Hm, good question. We sacrifice a bit of coverage for clippy but as it is only a linter, and we run cargo check on everything, this shouldn't be a problem.

The job is using the nightly toolchain for the clippy check. This gives us the benefit of having all the latest and greatest lints, but might lead to FPs sometimes when new lints are not well tested yet. I'd be interested in your opinion on whether this is a good idea. If we do go back to stable here, we should add a nightly matrix entry to the cargo check job, so we still keep a nightly build in our CI.

I would stick to stable, if we don't have any concrete reason to use nightly. I don't know any specifics about the clippy development model, but rust's nightly channel is an experimentation ground and clippy could get a lint at any time, which does not make sense for our use case but could cause a failing CI.
I've no strong opinion about this, though.

The different MCU features are now directly specified in the workflow YAML and not dynamically generated anymore.

Fully agree. This is what I was going for in #96, with the intial goal to make the CI faster, but as travis showed, this attempt failed. Not in this case, though, and I am really excited about the results.
The feature / device list is still a bit volatile as we develop the crate, and we have to be careful to adjust the CI, if a new target is added but at least for the device features, these should eventually stop changing, because the stm32f3 family won't get any new device.

Copy link
Member

@Sh3Rm4n Sh3Rm4n left a comment

Choose a reason for hiding this comment

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

As I've said before, we are missing a build target at least for the examples with for at least one device (.e.g. stm32f303xc) as it is used for clippy.

Cargo.toml Show resolved Hide resolved
.github/workflows/ci.yml Show resolved Hide resolved
@teskje
Copy link
Collaborator Author

teskje commented Jul 25, 2020

I still would like to see a build of the examples, to make sure, that we don't miss any building problems, which cargo check can not catch (e.g. linking errors).

Do you expect linking to be an issue here? I'm worried that this will increase our CI times by a lot, since LLVM and linking is usually what takes the most time in a build. Other HAL crates I looked at also do check only.
But let me check the effect on CI times.

Does cargo check checks the examples by default as well?

It does, when you specify the --examples flag. Which I forgot for some reason... I'll add that!

I would stick to stable, if we don't have any concrete reason to use nightly.

You are right, there is no good reason to risk nightly here.

@Sh3Rm4n
Copy link
Member

Sh3Rm4n commented Jul 25, 2020

Do you expect linking to be an issue here? I'm worried that this will increase our CI times by a lot, since LLVM and linking is usually what takes the most time in a build. Other HAL crates I looked at also do check only.
But let me check the effect on CI times.

If the other HALs only check and it is sufficient, then I don't mind and we can leave build out :)

@teskje
Copy link
Collaborator Author

teskje commented Jul 25, 2020

I would stick to stable, if we don't have any concrete reason to use nightly.

You are right, there is no good reason to risk nightly here.

So I was just trying to implement this and actually stumbled upon one reason to run clippy on nightly after all: It simplifies the Check matrix quite a lot. Because if we don't run clippy on nightly, we should have another check job on nightly instead that is allowed to fail. This requires us to introduce two extra features in the matrix for the toolchain and the allowed-to-fail flag, just for this one job.

How about we leave the check matrix simple and clippy running on nightly and instead just make that clippy job a non-required one? This might be a good idea anyway, since clippy is also getting new lints on stable from time to time, so a stable update can make the job for a PR fail on entirely unrelated code. And if we have the clippy job non-required anyway, we can just use that same one for the nightly check too.

@Sh3Rm4n
Copy link
Member

Sh3Rm4n commented Jul 25, 2020

That sounds good to me!

@teskje teskje merged commit 15aa2cd into stm32-rs:master Jul 25, 2020
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.

Move CI to GitHub Actions
3 participants