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

.github/workflows/build: Workflow to test project build #5

Merged
merged 1 commit into from
Nov 25, 2020

Conversation

pfalcon
Copy link
Collaborator

@pfalcon pfalcon commented Nov 12, 2020

This Github Worflow builds each freertos-pkcs11-psa repository commit/PR
against https://github.com/aws/amazon-freertos repository (into which
it's included as a submodule). Required "grafting" is performed to
ensure that the revision-under-test of freertos-pkcs11-psa is built
(instead of the current submodule revision as recorded in amazon-freertos).

With such setup (main repo with submodules), it can be anticipated that
some changes may need to be coordinated across repositories (i.e. applied
at the same time for successful build), so there's a support to build
against arbitrary amazon-freertos revision (including a PR branch), not
just master HEAD (default). To achieve that, a freertos-pkcs11-psa PR
should include file "amazon-freertos.rev" in the top directory, containing
revision or branch name to build against (e.g "pull/NNN/head" for PRs).

The build is performed for cypress/CY8CKIT_064S0S2_4343W, which is
currently the only platform in amazon-freertos which uses
freertos-pkcs11-psa. It's anticipated that more build platforms will
be added, as they appear in amazon-freertos. (For example, it would
be nice to add QEMU-emulated platform, to not just build a sample,
but also run it.)

Signed-off-by: Paul Sokolovsky [email protected]

@pfalcon
Copy link
Collaborator Author

pfalcon commented Nov 12, 2020

cc @Sherryzhang2

mkdir -p ~/bin
mkdir -p ~/opt
cd ..
wget -q https://armkeil.blob.core.windows.net/developer/Files/downloads/gnu-rm/9-2019q4/RC2.1/gcc-arm-none-eabi-9-2019-q4-major-x86_64-linux.tar.bz2
Copy link
Contributor

Choose a reason for hiding this comment

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

At some point we should look at using a docker container, but this seems fine for now.

# needed files are put into the working tree, so we just ignore error.
git revert 287ed79eb6137443133d2a7200bc5591c02a8973 || true

if [ -d .venv ]; then
Copy link
Contributor

Choose a reason for hiding this comment

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

just curious if any reason to use a virtual env?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@pfalcon
Copy link
Collaborator Author

pfalcon commented Nov 12, 2020

Sorry, got distracted with posting additional comments:

This can be seen at action at https://github.com/pfalcon/freertos-pkcs11-psa/pulls, specifically:

Support to build against an upstream PR was added based on the experience with Zephyr RTOS development, which also uses submodules and similar pattern to test changes spanning across submodules.

One area I can invite feedback is naming. It's hard to get one perfect from first time ;-). Currently, this builds for very specific platform, but that's rather an "implementation detail", we'd extend that as soon as we can. So, I used generic names ("build.yml", "build.sh"). OTOH, if we'll add running QEMU there, then it won't be just "build" any longer. But then I didn't want to use too-generic name like "ci.yml", because well, there can be different CI scripts.

Ability to reproduce locally was an important goal (based on number of CI loops we have deployed in Linaro, where it's, well, not ideal). So, github-specific "workflow" does as little as resonably possible, and the rest is done by a shell script which can be easily run locally (with "calling conventions" explicitly described in comment). As mentioned by @galak, going Docker way would be another way, but for simple cases (like this turned out to be, so far), I find just basic shell script and development-style "separation" in it (e.g. Python virtualenv's) to be more lightweight.

@pfalcon
Copy link
Collaborator Author

pfalcon commented Nov 19, 2020

@davwan01, @Sherryzhang2: Can you please have a look at this PR? If there're no big issues, I'd like to merge this next week.

@pfalcon
Copy link
Collaborator Author

pfalcon commented Nov 23, 2020

@galak: If this is good enough to go, can you please approve it?

# 2. The gnuarmemb toolchain is already in the PATH.
#
# This script is supposed to be usable both as part of Github Workflow and
# for local testing/debugging, please keep it such.
Copy link
Contributor

Choose a reason for hiding this comment

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

If for the local test usage, the build.sh script should only be run in the path above the freertos-pkcs11-psa, right? If so, how about adding some usage guide or note for this script?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for reviewing. Well, fairly speaking, intention to keep it locally-usable was a kind of background feature, based on experience with other CI setups, that debugging issues in the real CI may be too time-consuming/cumbersome, and it's helpful to be able to run it locally easily enough.

I now see I called for extra attention to that with that "please keep it such" phrase, which more along the lines of "don't rely on Github Actions environment too much, keep it runnable on other systems too".

Well, I'll look how long instructions for local running would be, and add them to code comments or commit message (if too long to be inline).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@Sherryzhang2, so, I updated a little bit (but just a little bit) a section above in the file. It now reads:

# This script assumes:
# 1. The gnuarmemb toolchain is already in the PATH.
# 2. A particular revision of the freertos-pkcs11-psa repo was cloned under
# "freertos-pkcs11-psa" subdirectory of the current directory.

And well, those are pretty much the instructions on how to run it locally. Again, local running isn't supposed to be a core feature, it just should be possible at all, for (hopefully rare) debugging purposes. So, I don't think it makes sense to give overdetailed instructions regarding that. If in some time you, I, or somebody else will find them confusing or some points missing, we can always extend them. Of course, if you have specific suggestions now, please share them.

This Github Worflow builds each freertos-pkcs11-psa repository commit/PR
against https://github.com/aws/amazon-freertos repository (into which
it's included as a submodule). Required "grafting" is performed to
ensure that the revision-under-test of freertos-pkcs11-psa is built
(instead of the current submodule revision as recorded in amazon-freertos).

With such setup (main repo with submodules), it can be anticipated that
some changes may need to be coordinated across repositories (i.e. applied
at the same time for successful build), so there's a support to build
against arbitrary amazon-freertos revision (including a PR branch), not
just master HEAD (default). To achieve that, a freertos-pkcs11-psa PR
should include file "amazon-freertos.rev" in the top directory, containing
revision or branch name to build against (e.g "pull/NNN/head" for PRs).

The build is performed for cypress/CY8CKIT_064S0S2_4343W, which is
currently the only platform in amazon-freertos which uses
freertos-pkcs11-psa. It's anticipated that more build platforms will
be added, as they appear in amazon-freertos. (For example, it would
be nice to add QEMU-emulated platform, to not just build a sample,
but also run it.)

Signed-off-by: Paul Sokolovsky <[email protected]>
Copy link
Contributor

@Sherryzhang2 Sherryzhang2 left a comment

Choose a reason for hiding this comment

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

Looks good to me.

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