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

Filter workflows to affected crates #504

Conversation

LongLiveCHIEF
Copy link

This is first in a series of PR's meant to implement packaging, deployment and release changes discussed in #481. This will be merged to the feature/ci-cd-dev branch, instead of master, so that this effort can have smaller PR's that can be reviewed individually for their scope of change, before making any alterations to the 'production' pipeline.

Summary

This PR implements a build-test workflow, which automatically determines crates affected by the change, and creates a matrix consisting of the affected crate and the nightly and stable toolchains.

Example: if a change was made to any file in both /pac/atsamd11c, and boards/circuit_playground_express. This action would run a generate-matrix action, then determine that 4 build jobs need to be run:

  • pac/atsamd11c, stable
  • pac/atsamd11c, nightly
  • boards/circuit_playground_express, stable
  • boards/circuit_playground_express, static

Other Notes

  • matrix will include a crate that has any file added/deleted/modified/renamed within it's file tree
  • matrix will not run a build if none of the files affected are a child of a crate directory

In-Scope for this PR

  • creating a filtered matrix of crates affected by changes, to be used by jobs and workflows in the pipeline

Out-of-scope for this PR

  • changes to build, test, release, or package versioning

Example runs:

@LongLiveCHIEF
Copy link
Author

for now, I went with js based action, since I'm not as proficient with python. I wanted to start getting the concept out there so we could see what other parts of the pipeline this concept would affect. We can always come back and make it python later/before we adopt this to master.

For any reviewers, please leave a note if your approval is contigent upon this being done in python prior to the new ci/cd being adopted into production

@TDHolmes
Copy link
Contributor

TDHolmes commented Oct 5, 2021

If atsamd11c was touched, shouldn't the BSC for SAMD11_bare also be triggered since it depends on it?

@jbeaurivage
Copy link
Contributor

Usage of JS

Personally I don't really mind going with js, as long as we don't end up with a disparate collection of scripts in different languages. The only caveat I would see is that to me python is more readable to someone unfamiliar with the language, for example someone trying to understand why the CI is failing.

BSP triggered by PAC

@TDHolmes has a good point, especially for PAC builds, would the dependency in the BSP and HAL crates be updated automatically - and their version bumped? Or is that a manual step?

Similarly for tier 1 bsps, is there a mechanism we can put in place to automatically sync the hal version in their dependencies, and perhaps bump the (T1) bsp versions if the dependency is updated?

@LongLiveCHIEF
Copy link
Author

If atsamd11c was touched, shouldn't the BSC for SAMD11_bare also be triggered since it depends on it?

I wasn't aware of the BSC's in the boards directory, so this got by me. However, I did propose an also_build for the [package.metadata.atsamd] in #503 just in case something like this came up.

That being said... it does feel like a code smell that there's a downstream crate that introduces build dependencies to an upstream crate.

Similarly for tier 1 bsps, is there a mechanism we can put in place to automatically sync the hal version in their dependencies, and perhaps bump the (T1) bsp versions if the dependency is updated?

Yes, in fact, the sync-pac-versions action in the release workflow does almost exactly this already (it just does it to every pac crate, instead of just affected pac/bsc crates).

@LongLiveCHIEF
Copy link
Author

The only caveat I would see is that to me python is more readable to someone unfamiliar with the language, for example someone trying to understand why the CI is failing.

Agreed... python is much easier to reason through at a glance. So far, all the maintainers have basically said js is ok with them, and if we get through all this, i'll likely be the primary maintainer, and js is a language I can do in my sleep.

My primary reason for using js here is that I wanted to quickly see what the affects of this filtering would be, for which this has definitely succeeded.

as long as we don't end up with a disparate collection of scripts in different languages.

Agreed, I think we should stick to one language (either python or js) in addition to any bash needed. That is one of my acceptance requirements for when we merge the feature/ci-cd-dev branch to master.

So, I'll either redo this in python... or remove any python through the remaining work.

I'm leaning more towards redoing this in python, as I think the users of these crates are more likely to have python experience/understand python... than they are js.

@LongLiveCHIEF
Copy link
Author

@TDHolmes has a good point, especially for PAC builds, would the dependency in the BSP and HAL crates be updated automatically - and their version bumped? Or is that a manual step?

I think with the discussion in #481, the implication is that we wouldn't automatically bump the dependencies in a crate when the dependency updates. Reasons for this:

  • There aren't any tests for most crates, and just running a build does not guarantee functionality,
  • Not updating dependencies in a dependent crate decouples the PAC/HAL from their dependents, which makes it easier to implement changes to the codebase without maintaining several versions of an interface in an active codebase.
  • We can update rules for Tier 1 crates that state they will maintain forward compatibility with HAL/PAC, up to breaking changes. (will be compatible with Y or Z changes).
  • We can create an action that automatically determines if a crate is currently meeting Tier 1 rules, and adjust the listings in README and cargo.toml files accordingly automatically on each merge to master.

@LongLiveCHIEF
Copy link
Author

Unless there are no other concerns, let's merge for the moment so I can use the output of this in the other pieces of work for this feature. I can come back and re-do in python prior to releasing into the wild

@TDHolmes
Copy link
Contributor

TDHolmes commented Oct 9, 2021 via email

@LongLiveCHIEF
Copy link
Author

Do we have a good plan for the concern I raised about more secondary dependency paths?

On Sat, Oct 9, 2021 at 7:35 AM Brian Vanderbusch @.***> wrote: Unless there are no other concerns, let's merge for the moment so I can use the output of this in the other pieces of work for this feature. I can come back and re-do in python prior to releasing into the wild — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#504 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AC5YNWR77CTK5B6FSOBU4Z3UGBHMBANCNFSM5FKAYZVQ .

I believe so, if you read my comments above. Regardless, even if there was enhancements needed, that would be in another PR. Keep in mind, this is a feature branch so I can iterate rapidly while getting input. We don't have to have every problem solved until we merge the feature/ci-cd-dev into master.

@TDHolmes
Copy link
Contributor

TDHolmes commented Oct 9, 2021 via email

@jbeaurivage jbeaurivage merged commit e9e651d into atsamd-rs:feature/ci-cd-dev Oct 9, 2021
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.

3 participants