roc-streaming / roc-toolkit

Real-time audio streaming over the network.
https://roc-streaming.org
Mozilla Public License 2.0
1.03k stars 205 forks source link

Added version check to CI checks #545

Closed dheerajd5 closed 9 months ago

dheerajd5 commented 10 months ago

This PR adds a version-check to CI checks. It takes the version fetched from git and from the file version.h and compares it. https://github.com/roc-streaming/roc-toolkit/issues/497

gavv commented 10 months ago

Two small comments:

gavv commented 10 months ago

@dheerajd5 As you mentioned in the issue, android failure is not related to your pr. I restarted that job.

dheerajd5 commented 10 months ago

Two small comments:

  • we shouldn't run this check unless we're building a tag naned as v* - because we usually update header before creating a tag
  • ideally we shouldn't checkout full history unless we're doing this check, to avoid slowing down regular builds

Hey, So I looked into conditional execution in Actions, to check if we're building a tag with v, we can probably check the latest commit message, and see if the string matches a v format. And for the second point, we can pass an if condition to see what kind of check we're going to run and based of that if it is necessary to pull the full history. I will try to implement the solution to the second point.

Edit: Removed the note asking about to how rebase without coauthoring the commit.

dheerajd5 commented 10 months ago

I was looking into the first point, I'm not really sure how to go about it do I check the latest commit and message and try to find a v* matching string?

gavv commented 10 months ago

I think we should extract this from linux-checks into separate job. Only that job will do deep checkout and fetch tags, and other jobs won't be affected. E.g. we can call it release-checks.

The new job then can be enabled conditionally only for properly named tags. Here is how you can do it: https://github.com/roc-streaming/roc-go/blob/6212890ef56528624350571ce13f308e0a3ae11c/.github/workflows/build.yaml#L219C53-L219C53

dheerajd5 commented 10 months ago

There are also ways to conditionally execute steps in a job, that's what I've done. Although the solution is a bit hard coded, the workflow only executes the step, fetch tag, if the script that is going to be run is version-check.

But in this case, there was no way that I was aware of where I could conditionally run it based on the tag name.

I'll go ahead with the solution you suggested.

dheerajd5 commented 10 months ago

Hey, so I implemented the changes, but it's not passing a few unrelated tests. I don't really have an idea why, Namely, the macos13 series, Here's the workflow run https://github.com/dheerajd5/roc-toolkit/actions/runs/6157211700 and here's the commit page, (https://github.com/dheerajd5/roc-toolkit/commit/64f2fd63291d9b06a4870e4b0d7b27d5465297e3)

gavv commented 10 months ago

Hey, so I implemented the changes, but it's not passing a few unrelated tests. I don't really have an idea why, Namely, the macos13 series

Could you please fetch fresh develop and rebase on it? Should be fixed now.

dheerajd5 commented 9 months ago

Hey so I did what you told, it has passed all the checks now. It hasn't run the check I've written cause I didn't upload this with a version tag.

gavv commented 9 months ago

Thanks. Tested it in a fork, there were problems.

Follow-up commit with fixes: 0266785edcb87d7349a4329b7de026c4c7c15c18

Changes:

dheerajd5 commented 9 months ago

Ahh, sorry I couldn't submit a more complete code, but enjoyed working, thanks guys @gavv @adrianopol

gavv commented 9 months ago

You're welcome!