[Zephyr] Add opt-in PR workflow #1348
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
quic-areg
force-pushed
the
zephyr-pr-wf
branch
3 times, most recently
from
660d085 to
4c70622
Compare
quic-areg
changed the title
quic-areg
force-pushed
the
zephyr-pr-wf
branch
2 times, most recently
from
2617a05 to
771ab69
Compare
quic-areg
force-pushed
the
zephyr-pr-wf
branch
2 times, most recently
from
3b6d9e5 to
1d2ae7a
Compare
Shankar Easwaran (quic-seaswara)
left a comment
Shankar Easwaran (quic-seaswara)
left a comment
There was a problem hiding this comment.
Very cool.
Have few comments.
| with: | ||
| name: pr-eld-toolset | ||
| run-id: ${{ inputs.artifact_run_id }} | ||
| github-token: ${{ github.token }} |
There was a problem hiding this comment.
Remove the artifact after download to conserve space ?
There was a problem hiding this comment.
retention days is set to 1
| extra_args: CONFIG_PICOLIBC_USE_TOOLCHAIN=y | ||
| test_scope: tests/kernel | ||
| eld_source: artifact | ||
| artifact_run_id: ${{ needs.resolve.outputs.run_id }} |
There was a problem hiding this comment.
can we run all the architectures supported by eld ?
There was a problem hiding this comment.
yes i am going to add them in a follow-up
| uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a #v7.0.1 | ||
| with: | ||
| name: pr-eld-toolset | ||
| path: pr-${{ env.PR_NUMBER }}/install-pr-toolset.tar.xz |
There was a problem hiding this comment.
Have you considered running this on the self hosted machine, that will allow the artifacts to be passed between the workflows without uploading to github ?
There was a problem hiding this comment.
I think even on a self-hosted machine the artifact should be passed through github. Not every CI build will have an associated zephyr build, so I think the self-hosted runner would accumulate too many unused artifacts.
| retention-days: 2 | ||
| path: | | ||
| zephyr/build/zephyr/zephyr.elf | ||
| zephyr/build/zephyr/zephyr.map |
There was a problem hiding this comment.
Do we use these artifacts ?
There was a problem hiding this comment.
I have never used them myself. I can remove it separately.
Jonathon Penix (jonathonpenix)
left a comment
Jonathon Penix (jonathonpenix)
left a comment
There was a problem hiding this comment.
A few scattered thoughts, but none are at all blocking/etc.
Overall, looks good to me!
| # Both branches normalize the payload to ./inst so the splice is identical. | ||
| - name: Fetch nightly ELD | ||
| if: inputs.eld_source == 'nightly' | ||
| uses: ./llvm-project/llvm/tools/eld/.github/workflows/FetchNightlyToolset |
There was a problem hiding this comment.
This isn't at all an important/blocking comment, but since I've been tinkering with similar stuff in CPULLVM:
How similar are your nightly and PR installs/tarballs?
We're doing similar stuff in CPULLVM right now and essentially what I landed on was giving our Zephyr workflow run id and package name input parameters and we'll have an action that'll fetch a toolchain based on those two things (which, that action is basically a clone of/riff on eld's FetchNightlyToolset).
Our packaging is basically identical between our nightlies and PRs, so I think that ends up being a pretty generic "interface" so the Zephyr workflow doesn't have to care too much about where the toolchain it'll use is coming from (PR vs nightly, different branches with different package naming schemes, different hosts).
Maybe something similar would work here? Might be more trouble than its worth (and I might be misunderstanding something) but it might let you hoist these PR vs Nightly ifs into their respective workflows, and let this workflow assume a common format.
| - { board: qemu_riscv32_xip, arch: riscv32, extra_args: CONFIG_PICOLIBC_USE_TOOLCHAIN=y } | ||
| - { board: qemu_riscv64, arch: riscv64, extra_args: CONFIG_PICOLIBC_USE_TOOLCHAIN=y } | ||
| # cpullvm ARM picolibc is missing a _nopic version, so its objects | ||
| # have GOT relocations that fail when .got is discarded. |
There was a problem hiding this comment.
Two misc. comments not at all related to this PR but that I'm just now thinking about seeing this and don't remember if it was mentioned elsewhere:
- We did add a variant for this, but it isn't in the 22.x release (only mainline right now). Not sure how you all feel about fetching nightly toolchains instead but that'd be one option. Or, it should be there in the 23.x release once that is out.
- We recently turned on qemu_cortex_a9 testing too since we have some users doing 32bit armv7a stuff. A compatible multilib is only available in CPULLVM mainline right now though. There are a fair number of failures (including in kernel tests IIRC), but haven't looked into them at all (no idea if they are compiler vs lib vs linker issues).
…workflow Factor the Zephyr build+twister body into zephyr-run.yml, which zephyr-nightly.yml now calls. Adds zephyr-pr.yml. a manual workflow_dispatch that downloads the PR's own ld.eld artifact from a successful ci.yml run and runs tests/kernel on qemu_cortex_a53. ci.yml packages the built ld.eld + libLW.so.4 + libc++ so zephyr-pr.yml can consume it. Signed-off-by: quic-areg <aregmi@qti.qualcomm.com>
4 participants
Factor the Zephyr build+twister body into zephyr-run.yml, which zephyr-nightly.yml now calls. Adds zephyr-pr.yml, a manual workflow_dispatch that downloads the PR's own ld.eld artifact from a successful ci.yml run and runs tests/kernel on qemu_cortex_a53.
ci.yml packages the built ld.eld + libLW.so.4 + libc++ so zephyr-pr.yml can consume it.