-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Initial checkin nuttx github action CI workflow #261
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
Conversation
|
is this workflow enabled? |
|
Not yet, it just enabled and tested in my own fork. There are still some work (parallel build flock fix and check testlist update) to do before this PR merged. |
|
Does this need an Apache license header? We need to find someone with knowledge on the team to merge this. I do not have the knowledge to do that. |
|
Perhaps someone like @btashton could review and merge this. I will add him as the reviewer and also the the matching PR in apps/ |
|
Yeah this needs some work. The build job should be abstracted so you can give it a target. Then a build job is defined for each target. That way we can run against all of the configurations including non Linux configs and do them in parallel. This is normally done by breaking the core of this into it's own action that you then refer to. That can be in this repo or in another. Also reinstalling dependencies every job is expensive. We should create a container with everything already setup. |
|
Also breakings out the action code means that it can almost all live in the testing repo with just a tiny stub here |
|
That's right. I'll update with an Apache license header. And consider to use github action docker contianer to preinstall build tools. In addtion, as build job for multi targets, I'll also reconstruct testing code to handle it. It may take some time to test and verify. Thanks. |
5839f4a to
e652363
Compare
|
@btashton please help to review. Now use docker container and matrix jobs for parallel builds. |
.github/workflows/main.yml
Outdated
|
|
||
| strategy: | ||
| matrix: | ||
| boards: [sim, mips-riscv-x86, arm-01, arm-02, arm-03, arm-04, arm-05, arm-06, arm-07, arm-08, arm-09, arm-10, arm-11, arm-12, arm-13, arm-14, arm-15] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Order the list?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I'll reorder them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
.github/workflows/main.yml
Outdated
| - name: Check Pull Request | ||
| run: | | ||
| cd nuttx | ||
| git log --oneline -5 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why always fetch the first 5 commit? should we count the real patch number in PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right. I'll update it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
| - name: Fetch nuttx tags | ||
| run: | | ||
| cd nuttx | ||
| git fetch --tags |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why need fetch tag?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since tags is needed to generate the .version file, or it would build break at first : (
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be done in the checkout step https://github.com/actions/checkout#fetch-all-tags
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be done in the checkout step https://github.com/actions/checkout#fetch-all-tags
Since I'm using the with option for checkout@v2 action, the run step seems not work together. I'll double check it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have checked that the instructions in the https://github.com/actions/checkout#fetch-all-tags run two steps too.
So it doesn't make much sense to use same as it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess the cost is low for the two steps, but other examples there like submodules show two run lines which I think makes sense to just get everything done. But these are really later optimizations.
1b54753 to
7746288
Compare
This is one PR check build in my own fork: https://github.com/liuguo09/incubator-nuttx/pull/3/checks |
|
@liuguo09 - Nice! What % of board configs are being built (n of N)? |
Now support the following arch: arm/sim/mips/risc-v/x86 And haven't enabled the following archs build since lack of toolchains. If available, we could update the docker container to preinstall their toolchains and update testlist to build them.
There are two stages in PR check build.
For github action free version, there are 20 jobs upper limit. And now used 17 jobs here. Each job runs in an 2-cores cpu container. More limits about github action, refer to: |
We reserve 3 VM interntionally for two purposes: |
btashton
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great overall. I am excited to get this in and then work on getting the mac and windows configurations going.
Also I do not think it is required for this first step, but adding build artifacts to the builds would be awesome as well since one could just grab a elf and the debug symbols for a build and throw it on a board to do some other validation on hardware. https://help.github.com/en/actions/configuring-and-managing-workflows/persisting-workflow-data-using-artifacts
| name: PR Check CI | ||
|
|
||
| on: | ||
| pull_request: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we not also run on the push event? Also I think this restricts to pull requests off master which might not be what we want if there is a long running feature branch. I think is can just be this.
on: [push, pull_request]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see that it by default includes synchronize so probably fine to leave it, but I would drop the branch. Maybe we still want to keep the push but limited to branch master so there is always a build on master
something like this:
on:
pull_request
push:
branches: [master]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@liuguo09 also if you have your Dockerfile somewhere I would be happy to submit a PR against it to improve it a bit. There is some low hanging fruit that would reduce the size by quite a bit.
Yes, that's great. I meant to add the Dockerfile into nuttx testing repo. I'll add it later. Then you could update it there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see that it by default includes synchronize so probably fine to leave it, but I would drop the branch. Maybe we still want to keep the push but limited to branch master so there is always a build on master
something like this:on: pull_request push: branches: [master]
Yes, besides master branch, pull request check for feature branch would also be useful.
Now I use the pull request git log "Merge new-commit-id into old-commit-id" to filter commits in the PR and then do checkpatch.sh. But for master push, it should not work and skip. Just see the github action job steps.if could be used to judge it. I'll update it later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update on trigger pull request for all branches. As to push event, I think it may be not necessary since there is no committer push master directly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it captures the merge situation which is what I'm worried about.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is also the nightly build to make the full build although it's per day.
| - name: Fetch nuttx tags | ||
| run: | | ||
| cd nuttx | ||
| git fetch --tags |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be done in the checkout step https://github.com/actions/checkout#fetch-all-tags
|
@liuguo09 also if you have your Dockerfile somewhere I would be happy to submit a PR against it to improve it a bit. There is some low hanging fruit that would reduce the size by quite a bit. |
IMO, It's a good idea. The build artifacts stage could be added in future if agreed and it doesn't add much time. |
|
Yes, we can select some images(sim, x86, x86-64, qemu for arm/riscv/xtens..) as build artifacts and run the testsuite on this images automatically. |
I have just made PR here the Dockerfile. |
Github action CI workflow steps as below: 1. Use docker container with build essential tools preinstalled 2. nxstyle check pull request with checkpatch.sh 3. Call testing cibuild.sh to do jobs matrix check builds Signed-off-by: liuhaitao <liuhaitao@xiaomi.com>
btashton
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm happy to get this in as is and then incrementally move forward.
Github action CI workflow steps as below:
Now split full testlist into sim/mips-riscv-x86/arm01~arm15 totally 17 jobs to do parallel builds.
All the jobs could be finished in half an hour at most. We could reduce the build time later by refine the configs in each job.
Also note that the pull request check is by default whole file check.
For example, refer to the check build in my own fork:
https://github.com/liuguo09/incubator-nuttx/pull/3/checks