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

CI: Do Zephyr build tests on known good and latest versions #307

Merged
merged 1 commit into from
Sep 20, 2024

Conversation

wmamills
Copy link
Contributor

Allow Zephyr testing to either use locked known good values or the very latest versions.

The known good versions are best for PR checking as if the build fails it is almost always the PR itself that broke it.

The latest version is good for periodically checking compatibility with the very latest Zephyr changes.

For now we run both on pushes and PRs.

We also run main against the latest zephyr check weekly as a look ahead.

Fixes: #295

@wmamills wmamills marked this pull request as draft August 18, 2024 00:12
@wmamills
Copy link
Contributor Author

wmamills commented Aug 18, 2024

@arnopo
This branch does not contain the v3.7.0 fix so the "latest" builds will always fail.
To show the logic I have hard coded Zephyr v3.6.0 and SDK 0.16.5-1 so they will be noticeably different than latest.
We should merge the fix for v3.7.0 before this one and I will rebase and then set known good values of v3.7.0 and 0.16.8

@wmamills
Copy link
Contributor Author

wmamills commented Aug 29, 2024

@arnopo @tnmysh @edmooring
rebased to main.
Now that main is compatible with Zephyr v3.7.0, use that as the known good version.
Also use 0.16.8 as the known good SDK version (which will also be the latest for now)
The All checks have passed section is what we should see in a PR.
If a PR fails against Zephr known good it should not be merged.
If a PR fails the Heath Check Zephyr latest, that is probably not something the PR did. You can check via the latest main heath check or running a new one manually via the web interface.

@wmamills wmamills marked this pull request as ready for review August 29, 2024 12:55
.github/workflows/heathcheck.yml Show resolved Hide resolved
# Break the zephyr builds into their own job as the common runner was
# running out of space when runs were together
# Also, as the longest running jobs, this allows them to run in ||
zephyr_build_known_good:
Copy link
Contributor

Choose a reason for hiding this comment

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

what about stable instead of "good" ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ack.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I really don't like stable. The know good version does not need to be a release, just a recent point that is know to work. If it is a release, it does not need to be the most recent stable release.

However "know good version" is a term I have used for decades and I am not the only one:
https://github.com/GoogleChromeLabs/chrome-for-testing?tab=readme-ov-file#json-api-endpoints

I can add _version. That makes it easier to understand.


# run weekly on Sunday at 5:10 AM UTC (9:10 PM US western)
schedule:
- cron: '10 5 * * 0'
Copy link
Contributor

Choose a reason for hiding this comment

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

question: what happen if it fails. A message is sent on the mailing list, or do we have to check action result each week?

Allow Zephyr testing to either use locked known good values
or the very latest versions.

The known good versions are best for PR checking as if the build fails
it is almost always the PR itself that broke it.

The latest version is good for periodically checking compatibility with
the very latest Zephyr changes.

For now we run both on pushes and PRs.

We also run main against the latest zephyr check weekly as a look ahead.

Signed-off-by: Bill Mills <[email protected]>
@wmamills
Copy link
Contributor Author

wmamills commented Sep 9, 2024

Pushed with some language updates as describe above.

@wmamills
Copy link
Contributor Author

wmamills commented Sep 9, 2024

@arnopo why do we ignore changes to cmake/* and scripts/*?? Changes there are the most likely to cause problems for the builds.

Are you OK if I start another PR to remove those ignore paths?

@wmamills
Copy link
Contributor Author

wmamills commented Sep 9, 2024

question: what happen if it fails. A message is sent on the mailing list, or do we have to check action result each week?

The answer seem to be here:
https://docs.github.com/en/actions/monitoring-and-troubleshooting-workflows/monitoring-workflows/notifications-for-workflow-runs

I don't see anyway to send that to an email list. We can change who it goes to by disable and then enable. The last enable is the winner.

We could also add a badge on the readme.

@arnopo
Copy link
Contributor

arnopo commented Sep 9, 2024

@arnopo why do we ignore changes to cmake/* and scripts/*?? Changes there are the most likely to cause problems for the builds.

Mainly to avoid issue with checkpatch and to avoid running build on non-library code (no true for cmake)

Are you OK if I start another PR to remove those ignore paths?

yes sure!

@arnopo
Copy link
Contributor

arnopo commented Sep 9, 2024

question: what happen if it fails. A message is sent on the mailing list, or do we have to check action result each week?

The answer seem to be here: https://docs.github.com/en/actions/monitoring-and-troubleshooting-workflows/monitoring-workflows/notifications-for-workflow-runs

I don't see anyway to send that to an email list. We can change who it goes to by disable and then enable. The last enable is the winner.

We could also add a badge on the readme.

Regarding the article you pointed out and this article: https://docs.github.com/en/rest/checks/runs?apiVersion=2022-11-28,
we should be able to return another status such as neutral , or action_required

In such case we could run it for each PR but returning something else than failure with an explicit print in action log:
" If the error reported is not related to your PR, you can ignore it, it probably related to an internal Zephyr error"

@arnopo arnopo merged commit a4bce35 into OpenAMP:main Sep 20, 2024
6 checks passed
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.

CI: Pin to specific version of Zephyr SDK in default PRs
3 participants